PROVIDE LOCAL INFORMATION is session 6.4.15 in TS 101.267 / TS 11.14

implement below item first 1.) location info 2.) date-time and time-zone

Implement below item first: 1). IMEI 2). Location Info 3). Date-Time and Time-Zone

1). Combine all response item for PROVIDE_LOCAL_INFO into MozStkLocalInfo.

1). Combine all response item for PROVIDE_LOCAL_INFO into MozStkLocalInfo. 2). add function writeTimestamp() in GsmPDUHelper. 3). add function writeLanguageTlv() in StkProactiveCmdHelper. 4). remove unnecessary code and comment.

correct typo in comment

Comment on attachment 685506 [details] [diff] [review] Part2: Support PROVIDE_LOCAL_INFO in RIL, v2 Review of attachment 685506 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +2789,5 @@ > + > + GsmPDUHelper.writeHexOctet(COMPREHENSIONTLV_TAG_IMEI); > + GsmPDUHelper.writeHexOctet(8); > + for (let i = 0; i < imei.length/2; i++) { > + GsmPDUHelper.writeHexOctet(parseInt(imei.substr(i*2, 2), 16)); space between operators. @@ +5360,5 @@ > > /** > + * Convert a BCD number to an octet (number) > + * > + * Only take two digit with absolute value. *digits* @@ +6313,5 @@ > + let min = date.getMinutes(); > + let sec = date.getSeconds(); > + // The Time Zone indicates the different between the local time and GMT. > + // Expressed in quarters of an hours. > + let zone = date.getTimezoneOffset()/15; nit, space between operators. @@ +6317,5 @@ > + let zone = date.getTimezoneOffset()/15; > + > + let octet = this.BCDToOctet(zone); > + > + // If the time zone is -0800 GMT, 480 will be returned by date.getTimezoneOffset(). Could you add more comments to make the code easier to read? For example, what's the value returned from date.getTimezoneOffset() ? what's the definition of the timezone in spec, why you OR 0x08 when zone > 0? @@ +6328,5 @@ > + this.writeSwappedNibbleBCDString(mon); > + this.writeSwappedNibbleBCDString(day); > + this.writeSwappedNibbleBCDString(hour); > + this.writeSwappedNibbleBCDString(min); > + this.writeSwappedNibbleBCDString(sec); Do you think it's better to write those values directly without introducing another variables? for example, this.writeSwappedNibbleBCDString(date.getFullYear() - PDU_TIMESTAMP_YEAR_OFFSET); @@ +7151,5 @@ > + switch (cmdDetails.commandQualifier) { > + case STK_LOCAL_INFO_IMEI: > + response = {command: cmdDetails}; > + if(RIL.IMEI == null) { > + response.resultCode = STK_RESULT_REQUIRED_VALUES_MISSING; should not be STK_RESULT_REQUIRED_VALUES_MISSING here. @@ +7697,5 @@ > + GsmPDUHelper.writeHexOctet(2); > + > + // ISO 639-1, Alpha-2 code > + // TS 123.038, clause 6.2.1, GSM 7 bit Default Alphabet > + let code1 = PDU_NL_LOCKING_SHIFT_TABLES[0].indexOf(language[0]); PDU_NL_LOCKING_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT] @@ +7701,5 @@ > + let code1 = PDU_NL_LOCKING_SHIFT_TABLES[0].indexOf(language[0]); > + let code2 = PDU_NL_LOCKING_SHIFT_TABLES[0].indexOf(language[1]); > + > + GsmPDUHelper.writeHexOctet(code1); > + GsmPDUHelper.writeHexOctet(code2); Again, do you think it's easier writing these octets directly, without adding 'code1' and 'code2' ?

Comment on attachment 685512 [details] [diff] [review] Part 3: xpcshell tests for PROVIDE_LOCAL_INFORMATION, v1 Review of attachment 685512 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/test_ril_worker_icc.js @@ +129,5 @@ > + let helper = worker.GsmPDUHelper; > + > + // current date > + let date_input = new Date(); > + let date_output = new Date(); use camelCase. @@ +143,5 @@ > + do_check_eq(date_input.getTimezoneOffset(), date_output.getTimezoneOffset()); > + > + // 2034-01-23 12:34:56 -0800 GMT > + let time = Date.UTC(2034, 1, 23, 12, 34, 56); > + time = time - 8*60*60*1000; nit, space. @@ +578,5 @@ > + > + berTlv = berHelper.decode(local_info_2.length); > + ctlvs = berTlv.value; > + tlv = stkHelper.searchForTag(COMPREHENSIONTLV_TAG_COMMAND_DETAILS, ctlvs); > + do_check_eq(tlv.value.commandNumber, 0x02); Command Number is a feature reserved for future use. Currently all STK implementation use 1 for commandNumber. We should do the same.

1). add STK_LOCAL_INFO_IMEI. 1). add imei in MozStkLocalInfo.

(In reply to Edgar Chen [:edgar] from comment #11) > Created attachment 685940 [details] [diff] [review] > Part 2: Support PROVIDE_LOCAL_INFO in RIL, v3 Address review comment #8. Thanks for the review, Yoshi.

Address review comment #9. Thanks for the review, Yoshi.

rebase

rebase

Comment on attachment 685939 [details] [diff] [review] Part 1: IDL For PROVIDE_LOCAL_INFO, v3 Review of attachment 685939 [details] [diff] [review]: ----------------------------------------------------------------- comments should be updated.

1). update comments

1). Add define for 8th byte and 9th byte of Terminal Profile. Some bit is used for PROVIDE_LOCAL_INFO. 2). TLV size need to be multiplied by 2. 3). Adjust function name.

Comment on attachment 686486 [details] [diff] [review] Part 1: IDL For PROVIDE_LOCAL_INFO, v4 Review of attachment 686486 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/nsIDOMIccManager.idl @@ +202,5 @@ > + */ > + const unsigned short STK_LOCAL_INFO_LOCATION_INFO = 0x00; > + const unsigned short STK_LOCAL_INFO_IMEI = 0x01; > + const unsigned short STK_LOCAL_INFO_DATE_TIME_ZONE = 0x03; > + const unsigned short STK_LOCAL_INFO_LANGUAGE = 0x04; When will these values be used ?

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #19) > Comment on attachment 686486 [details] [diff] [review] > Part 1: IDL For PROVIDE_LOCAL_INFO, v4 > > Review of attachment 686486 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/icc/interfaces/nsIDOMIccManager.idl > @@ +202,5 @@ > > + */ > > + const unsigned short STK_LOCAL_INFO_LOCATION_INFO = 0x00; > > + const unsigned short STK_LOCAL_INFO_IMEI = 0x01; > > + const unsigned short STK_LOCAL_INFO_DATE_TIME_ZONE = 0x03; > > + const unsigned short STK_LOCAL_INFO_LANGUAGE = 0x04; > > When will these values be used ? These values are used for indicating which information is required. Defined in "Qualifier" field of Command Detailed. (TS 102.223 session 8.6) Thanks! :)

Comment on attachment 686403 [details] [diff] [review] Part 3: xpcshell tests for PROVIDE_LOCAL_INFO, v2 Review of attachment 686403 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good to me, but I'd like to review this with implementation.

Comment on attachment 686486 [details] [diff] [review] Part 1: IDL For PROVIDE_LOCAL_INFO, v4 Review of attachment 686486 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/SimToolKit.idl @@ +458,5 @@ > boolean hasConfirmed; > + > + /** > + * The response for proactive command PROVIDE_LOCAL_INFORMATION > + * you can refer to 'STK_CMD_PROVIDE_LOCAL_INFO' directly

1). add MozStkProvideLocalInfo to make a more high-level design. 2). Address review comment #22 Thanks for the review, Yoshi! :)

Changes according to comment #23

marionette tests for PROVIDE_LOCAL_INFO

rebase

re-write marionette tests based on patch part3 of bug 814618.

Comment on attachment 688167 [details] [diff] [review] Part 1: IDL For PROVIDE_LOCAL_INFO, v5 Review of attachment 688167 [details] [diff] [review]: ----------------------------------------------------------------- Good! thanks You also need to request for sr? for IDL change

Comment on attachment 689606 [details] [diff] [review] Part 2: Support PROVIDE_LOCAL_INFO in RIL, v5.1 Review of attachment 689606 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +8808,5 @@ > + GsmPDUHelper.writeTimestamp(date); > + }, > + > + writeLanguageTlv: function writeLanguageTlv(language) { > + GsmPDUHelper.writeHexOctet(COMPREHENSIONTLV_TAG_LANGUAGE); indent seems wrong here. should be only 2 ws.

Comment on attachment 689607 [details] [diff] [review] Part 4: marionette tests for PROVIDE_LOCAL_INDO, v2 Review of attachment 689607 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your hard work.

correct indent style. (comment #29)

add reviewer and super-reviewer into title after review+ and superreview+

try server: https://tbpl.mozilla.org/?tree=Try&rev=54899d804adf

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ffb3bd63f71 https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd9ff852c5e https://hg.mozilla.org/integration/mozilla-inbound/rev/7169b3dfe53a https://hg.mozilla.org/integration/mozilla-inbound/rev/e5e4ea3327df

https://hg.mozilla.org/mozilla-central/rev/1ffb3bd63f71 https://hg.mozilla.org/mozilla-central/rev/0bd9ff852c5e https://hg.mozilla.org/mozilla-central/rev/7169b3dfe53a https://hg.mozilla.org/mozilla-central/rev/e5e4ea3327df

Request to shira+ as this feature is required for v1.0.1

This doesn't apply cleanly to b2g18 (I made it to part 2 before I gave up).

Hi Rayn, I will rebase these patches for b2g18. Thanks.

Patch for mozilla-b2g18

Patch for mozilla-b2g18

Patch for mozilla-b2g18

https://hg.mozilla.org/releases/mozilla-b2g18/rev/62c030520097 https://hg.mozilla.org/releases/mozilla-b2g18/rev/83a300618d60 https://hg.mozilla.org/releases/mozilla-b2g18/rev/8ad5fbebbc19 https://hg.mozilla.org/releases/mozilla-b2g18/rev/aacd5474fe8b