When user gets the event list needed to be monitored by SET_UP_EVENT_LIST proactive command in Bug 790543, user needs to inform ICC about the location information when location status is updated. So we need to add another interface for 'Location Status Event'. See TS 11.14 clause 11.14 or TS 102.223 clause 7.5.4

Yoshi, is this a basecamp blocker?

Yes, it's needed in Brazil.

Add RILContentHelper and update bugs in writing locationStatus

renamed sendLocationStatus to sendEventDownload

Renamed accordingly, also added impl in RadioInterfaceLayer.

updated to latest IDL change.

Fixed bugs in writing octets, and revised writing MCC/MNC according to TS 24.008

supply locationInformation only when service state is normal.

Moved writing location info of TLV into a helper function.

Renamed writeLocationInfo to writeLocationInfoTlv.

Renamed accordingly.

Comment on attachment 664330 [details] [diff] [review] WIP - Part 4: xpcshell test case for writing Location Info. v2 Review of attachment 664330 [details] [diff] [review]: ----------------------------------------------------------------- g ::: dom/system/gonk/tests/test_ril_worker_icc.js @@ +226,5 @@ > + lac = (pduHelper.readHexOctet() << 8) | pduHelper.readHexOctet(); > + do_check_eq(lac, 10291); > + > + cellId = (pduHelper.readHexOctet() << 8) | > + (pduHelper.readHexOctet()); indent!

Fixed indent.

Comment on attachment 662856 [details] [diff] [review] Part 1: IDL for sendEventDownload v2 Hi, philikon: From Bug 790543, ICC will send a list of events to ME, when one of these events happened, ME need to notify ICC this event through Envelope Command. So this patch is to deal when location status is updated, ME needs to notify this event to ICC by this interface. For the specification you can refer to TS 11.14 , clause 11.4 "Location Status Event".

Comment on attachment 662857 [details] [diff] [review] Part 2: Add SendEventDownload in IccManager. v2 Hi, smaug I add another interface called 'sendStkEventDownload' in nsIDOMIccManager in part 1, the purpose is to send events to ICC. Would you help to review this patch for me? Thank you.

Comment on attachment 664329 [details] [diff] [review] Part 3: Add send Location Event in RIL. v7 Hi, philikon This is the implementations. Would you review this for me ? Thank you.

Comment on attachment 664335 [details] [diff] [review] WIP - Part 4: xpcshell test case for writing Location Info. v3 Hi, philikon This is the xpcshell test for testing writeLocationInfoTlv. Could you review this for me? Thank you.

fixed typo in comments.

try result https://tbpl.mozilla.org/?tree=Try&rev=07c7b4a4f716

forgot qref. sorry

Comment on attachment 664329 [details] [diff] [review] Part 3: Add send Location Event in RIL. v7 Review of attachment 664329 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +2285,5 @@ > + * @param eventData > + */ > + sendStkEventDownload: function sendStkEventDownload(command) { > + command.tag = BER_EVENT_DOWNLOAD_TAG; > + command.eventList = STK_EVENT_TYPE_LOCATION_STATUS; should be command.eventList = command.eventType; Thanks for kk11ff to point this out.

Addressed to kk11ff's feedback.

Comment on attachment 662856 [details] [diff] [review] Part 1: IDL for sendEventDownload v2 Review of attachment 662856 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/SimToolKit.idl @@ +293,5 @@ > + * See MozStkLocationInfo. > + * This value shall only be provided if the locationStatus indicates > + * 'STK_SERVICE_STATE_NORMAL'. > + */ > + jsval locationInfo; You know me, I'm the devil's advocate for flatter structures. So I wonder why MozStkLocationEvent and MozStkLocationInfo can't be merged into one. Does MozStkLocationInfo exist outside of an MozStkLocationEvent context? ::: dom/icc/interfaces/nsIDOMIccManager.idl @@ +202,5 @@ > + * When eventType is STK_EVENT_TYPE_LOCATION_STATUS, > + * eventData is MozStkLocationEvent. > + */ > + void sendStkEventDownload(in unsigned short eventType, > + in jsval eventData); Same stupid question again: Can't `eventType` not be part of `eventData`, e.g. `eventData.type`? Flat objects FTW! :) Also, I have a question: does the radio respond to this STK command and if so, how? Should be worth documenting in the comment here. r=me so far since this could go in, though I would prefer that the above questions were addressed first.

Comment on attachment 662857 [details] [diff] [review] Part 2: Add SendEventDownload in IccManager. v2 Review of attachment 662857 [details] [diff] [review]: ----------------------------------------------------------------- I'm sure smaug won't mind if I just r+ this. It's a pretty trivial piece of code.

Comment on attachment 664409 [details] [diff] [review] Part 3: Add send Location Event in RIL. v8 Review of attachment 664409 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +2325,5 @@ > + (options.eventList ? 3 : 0) + > + (options.locationStatus ? 3 : 0) + > + (options.locationInfo ? > + (options.locationInfo.gsmCellId > 0xffff ? 11 : 9) : > + 0); Nit: it would be good to have size consts for the individual building blocks so that these numbers are a bit clearer and not so magical, e.g. (options.helpRequested ? SIZE_FOO : 0) + (options.eventList ? SIZE_BAR + SIZE_FOO : 0) +

Comment on attachment 664405 [details] [diff] [review] Part 4: xpcshell test case for writing Location Info. v4 Review of attachment 664405 [details] [diff] [review]: ----------------------------------------------------------------- I do like tests. <3

(In reply to Philipp von Weitershausen [:philikon] from comment #28) > I'm sure smaug won't mind if I just r+ this. It's a pretty trivial piece of > code. Hah looks like smaug pre-empted me ;) Yay.

(In reply to Philipp von Weitershausen [:philikon] from comment #27) Hi, philikon Thanks for your review. > Comment on attachment 662856 [details] [diff] [review] > Part 1: IDL for sendEventDownload v2 > > Review of attachment 662856 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/icc/interfaces/SimToolKit.idl > @@ +293,5 @@ > > + * See MozStkLocationInfo. > > + * This value shall only be provided if the locationStatus indicates > > + * 'STK_SERVICE_STATE_NORMAL'. > > + */ > > + jsval locationInfo; > > You know me, I'm the devil's advocate for flatter structures. So I wonder > why MozStkLocationEvent and MozStkLocationInfo can't be merged into one. > Does MozStkLocationInfo exist outside of an MozStkLocationEvent context? > Yes, I understand this. Sorry for not pointing this out in comments, in STK spec(TS 11.14), Location Information TLV will be used in several places, proactive command, terminal response, and even call control envelope command(Bug 791939). So finally I move those members of locationInfo out of MozStkLocationEvent and make it MozLocationInfo. So this object could be reused in other places. > ::: dom/icc/interfaces/nsIDOMIccManager.idl > @@ +202,5 @@ > > + * When eventType is STK_EVENT_TYPE_LOCATION_STATUS, > > + * eventData is MozStkLocationEvent. > > + */ > > + void sendStkEventDownload(in unsigned short eventType, > > + in jsval eventData); > > Same stupid question again: Can't `eventType` not be part of `eventData`, > e.g. `eventData.type`? Flat objects FTW! :) > Sounds good, I'll make it void sendStkEventDownload(in jsval eventData); with moving eventType inside eventData. > Also, I have a question: does the radio respond to this STK command and if > so, how? Should be worth documenting in the comment here. > For this envelope command, RIL(ICC) won't return any response. So I update the documentation to point this out. > r=me so far since this could go in, though I would prefer that the above > questions were addressed first. Thanks for your commens. :)

Addressed to philikon's comments: 1. move eventType into eventData, and rename eventData to event in sendStkEventDownload. 2. Add documentations to indicate ICC won't respond any data with this command. I didn't merge MozStkLocationInfo into MozStkLocationEvent, as I explained in my previous comment. philikon, does my comment make sense to you? Thanks

Updated parameters in SendStkEventDownload accordingly.

Hi, philikon I addressed your comments to const the size of TLVs, but for some TLVs, their size will vary according to content. So in this patch I just const some of them, but I am not sure if this is better. Could you review this part again? Thank you.

Comment on attachment 664831 [details] [diff] [review] Part 1: IDL for sendEventDownload v3 Review of attachment 664831 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! ::: dom/icc/interfaces/nsIDOMIccManager.idl @@ +193,5 @@ > in boolean helpRequested); > > /** > + * Send "Event Download" Envelope command to ICC. > + * ICC will not respond any data for this command. s/respond/respond with/

Comment on attachment 664868 [details] [diff] [review] Part 3: Add send Location Status Event in RIL. v9 Review of attachment 664868 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_consts.js @@ +544,5 @@ > const COMPREHENSIONTLV_TAG_URL = 0x31; > > +const COMPREHENSIONTLV_ITEM_IDENTIFIER_SIZE = 3; > +const COMPREHENSIONTLV_HELP_REQUESTED_SIZE = 2; > +const COMPREHENSIONTLV_LOCATION_STATUS_SIZE = 3; Small nit: this is not a const we need to "export", it's an implementation detail, so IMHO would be better at the top of ril_worker (e.g. like PDU_HEX_OCTET_SIZE). ::: dom/system/gonk/ril_worker.js @@ +2326,5 @@ > + (options.eventList ? 3 : 0) + > + (options.locationStatus ? > + COMPREHENSIONTLV_LOCATION_STATUS_SIZE : 0) + > + (options.locationInfo ? > + (options.locationInfo.gsmCellId > 0xffff ? 11 : 9) : 0); I'm not quite sure why these can't be const'ed, but I'll take your word for it. This is just a personal pet peeve, I don't want to bikeshed over something unimportant.

(In reply to Philipp von Weitershausen [:philikon] from comment #37) > Comment on attachment 664868 [details] [diff] [review] > Part 3: Add send Location Status Event in RIL. v9 > > Review of attachment 664868 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_consts.js > @@ +544,5 @@ > > const COMPREHENSIONTLV_TAG_URL = 0x31; > > > > +const COMPREHENSIONTLV_ITEM_IDENTIFIER_SIZE = 3; > > +const COMPREHENSIONTLV_HELP_REQUESTED_SIZE = 2; > > +const COMPREHENSIONTLV_LOCATION_STATUS_SIZE = 3; > > Small nit: this is not a const we need to "export", it's an implementation > detail, so IMHO would be better at the top of ril_worker (e.g. like > PDU_HEX_OCTET_SIZE). > > ::: dom/system/gonk/ril_worker.js > @@ +2326,5 @@ > > + (options.eventList ? 3 : 0) + > > + (options.locationStatus ? > > + COMPREHENSIONTLV_LOCATION_STATUS_SIZE : 0) + > > + (options.locationInfo ? > > + (options.locationInfo.gsmCellId > 0xffff ? 11 : 9) : 0); > > I'm not quite sure why these can't be const'ed, but I'll take your word for > it. This is just a personal pet peeve, I don't want to bikeshed over > something unimportant. Ha, if I don't have to put them in ril_const then I understand now, will const them in ril_worker. Thank you. :)

Addressed to philikon's comments in comment 36.

Addressed to philikon's comment in comment 37

Comment on attachment 665243 [details] [diff] [review] Part 1: IDL for sendEventDownload v4 Hi, Jonas From Bug 790543, ICC will send a list of events to ME, when one of these events happened, ME need to notify ICC this event through Envelope Command. So this patch is to deal when location status is updated, ME needs to notify this event to ICC by this interface. For the specification you can refer to TS 11.14 , clause 11.4 "Location Status Event". Thank you

Comment on attachment 665243 [details] [diff] [review] Part 1: IDL for sendEventDownload v4 Review of attachment 665243 [details] [diff] [review]: ----------------------------------------------------------------- I can't say that I understand a lot of the STK still :(. But this looks clean to me.

rebase

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0194ff64049 https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e9422b1be8 https://hg.mozilla.org/integration/mozilla-inbound/rev/c249961afd99 https://hg.mozilla.org/integration/mozilla-inbound/rev/48d10ef28d52

Try run for c286b232f355 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=c286b232f355 Results (out of 290 total builds): success: 271 warnings: 18 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-c286b232f355

https://hg.mozilla.org/mozilla-central/rev/d0194ff64049 https://hg.mozilla.org/mozilla-central/rev/c2e9422b1be8 https://hg.mozilla.org/mozilla-central/rev/c249961afd99 https://hg.mozilla.org/mozilla-central/rev/48d10ef28d52