On my Nexus S running master, the SMSC number visible in Settings is not a valid one. This is reproducing at least with my Free Mobile SIM card. SMSC is expected to be +33695000695, but the value exposed is 07913396050096f5.

For some reason, Buf.readString() on my Free Mobile SIM card in Nexus S returns an invalid SMSC value (07913396050096f5). Hacking a bit the way we read the GET_SMSC reply I get the proper value being read. This does not reproduce on other devices I could test with (Flame, namely).

Comment on attachment 8461441 [details] [diff] [review] Read proper SMSC value on Nexus S Hsin, do you mind having a look at this ? Maybe you can find a proper reason that explains why it does not work in the first place, and help me produce a nicer patch :)

Comment on attachment 8461441 [details] [diff] [review] Read proper SMSC value on Nexus S Review of attachment 8461441 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I don't have enough bandwidth. :( Bevis, would you mind helping this?

Comment on attachment 8461441 [details] [diff] [review] Read proper SMSC value on Nexus S Review of attachment 8461441 [details] [diff] [review]: ----------------------------------------------------------------- Sorry to give you feedback-. My reasons are 1. It seems that you are trying to interpret the response as the RP-Destination address Centre Address defined in TS 24.011. 2. However, according to the RIL in AOSP[1][2] and the corresponding usage of the SMSC in RadioInfo Activity of AOSP[3][4], the response of RIL_REQUEST_GET_SMSC_ADDRESS is more likely to be a plain text of the smsc address. If you have other Android Phone, you can double confirm this by the following steps: a. Type *#*#4636#*#* in the dialer. b. Go into "Phone Information" c. Press the "Refresh" button of the SMSC to read the SMSC address. 3. Other devices than Nexus S will get into trouble if we apply this patch. To me, it's more likely to be a Nexus S specific issue instead. [1]http://androidxref.com/4.4.4_r1/xref/hardware/ril/include/telephony/ril.h#3320 [2]http://androidxref.com/4.4.4_r1/xref/frameworks/opt/telephony/src/java/com/android/internal/telephony/RIL.java#2423 [3]http://androidxref.com/4.4.4_r1/xref/packages/apps/Settings/src/com/android/settings/RadioInfo.java#227 [4]http://androidxref.com/4.4.4_r1/xref/packages/apps/Settings/src/com/android/settings/RadioInfo.java#1056

(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #4) > Comment on attachment 8461441 [details] [diff] [review] > Read proper SMSC value on Nexus S > > Review of attachment 8461441 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry to give you feedback-. That's no problem, I asked feedback to know how to do it properly. > > My reasons are > 1. It seems that you are trying to interpret the response as the > RP-Destination address Centre Address defined in TS 24.011. Then we should implement this parsing, shouldn't we? > 2. However, according to the RIL in AOSP[1][2] and the corresponding usage > of the SMSC in RadioInfo Activity of AOSP[3][4], I would not consider AOSP as being the reference for specifications. > the response of RIL_REQUEST_GET_SMSC_ADDRESS is more likely to be a plain > text of the smsc address. *More likely* > If you have other Android Phone, you can double confirm this by the > following steps: > a. Type *#*#4636#*#* in the dialer. > b. Go into "Phone Information" > c. Press the "Refresh" button of the SMSC to read the SMSC address. > 3. Other devices than Nexus S will get into trouble if we apply this patch. > > To me, it's more likely to be a Nexus S specific issue instead. How can you judge so ? Specifically when your links are referring to KK codebase while Nexus S is ICS. I'll look into the specs for the proper encoding.

(In reply to Alexandre LISSY :gerard-majax from comment #5) > > To me, it's more likely to be a Nexus S specific issue instead. > > How can you judge so ? Specifically when your links are referring to KK > codebase while Nexus S is ICS. I'll look into the specs for the proper > encoding. Actually, the implementation is the same even in the 1st AOSP release. (I've involved in it. :p) After more comparison, it's not Nexus S issue but a bad API design: I've tried *#*#4636#*#* on both HTC One & Nexus 5 and found something interesting: 1. In HTC device, the response is readable phone number "+886932400821". 2. In Nexus 5, the response is in form of '"+886932400821",145' which is formed in +CSCA response defined TS 27.005. If we take your Nexus S (07913396050096f5) into account, that means there are 3 different format of this response. Hence, treat it as a plain text will be more flexible to present this field with various modem solution. However, it becomes difficult to the user to read and update this field. :(

(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #6) > (In reply to Alexandre LISSY :gerard-majax from comment #5) > > > To me, it's more likely to be a Nexus S specific issue instead. > > > > How can you judge so ? Specifically when your links are referring to KK > > codebase while Nexus S is ICS. I'll look into the specs for the proper > > encoding. > > Actually, the implementation is the same even in the 1st AOSP release. > (I've involved in it. :p) > > After more comparison, it's not Nexus S issue but a bad API design: > I've tried *#*#4636#*#* on both HTC One & Nexus 5 and found something > interesting: > 1. In HTC device, the response is readable phone number "+886932400821". > 2. In Nexus 5, the response is in form of '"+886932400821",145' which is > formed in +CSCA response defined TS 27.005. > If we take your Nexus S (07913396050096f5) into account, that means there > are 3 different format of this response. > > Hence, treat it as a plain text will be more flexible to present this field > with various modem solution. > However, it becomes difficult to the user to read and update this field. :( The values for the HTC One and Nexus 5 seems to be proper strings, while the Nexus S one clearly involves some BCD string. Could it be that we are just missing some specs implementation ? I already got similar things around ICCIO when hacking on HTC Desire Z, so I would not be surprised :)

(In reply to Alexandre LISSY :gerard-majax from comment #7) > The values for the HTC One and Nexus 5 seems to be proper strings, while the > Nexus S one clearly involves some BCD string. Could it be that we are just > missing some specs implementation ? I already got similar things around No, so far as I know, according to TS 27.005 and TS 24.011, SMSC was formed in either text or pdu format as what Nexus 5 & Nexus S presents, and HTC One just represents decoded one instead. :) > ICCIO when hacking on HTC Desire Z, so I would not be surprised :)

For some reason, Buf.readString() on my Free Mobile SIM card in Nexus S returns an invalid SMSC value (07913396050096f5). Hacking a bit the way we read the GET_SMSC reply I get the proper value being read. This does not reproduce on other devices I could test with (Flame, namely).

For some reason, Buf.readString() on my Free Mobile SIM card in Nexus S returns an invalid SMSC value (07913396050096f5). Hacking a bit the way we read the GET_SMSC reply I get the proper value being read. This does not reproduce on other devices I could test with (Flame, namely).

As mentioned in the meeting yesterday, I recommend to have a per-device build time configuration to let Gecko know what underlying representation format is in use on the running device, so we can ensure that Gecko always converts and returns the same format (which I assume readable number / ITU E.164 format is preferred) to Gaia apps. I haven't figured out where to put those configs, though. Certainly we're not going to hard-code it in the source.

(In reply to Samael Wang [:freesamael][:sawang] from comment #11) > As mentioned in the meeting yesterday, I recommend to have a per-device > build time configuration to let Gecko know what underlying representation > format is in use on the running device, so we can ensure that Gecko always > converts and returns the same format (which I assume readable number / ITU > E.164 format is preferred) to Gaia apps. > > I haven't figured out where to put those configs, though. Certainly we're > not going to hard-code it in the source. You may refer to [1] which is a config for only shinaro devices [2]. [1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#51 [2] https://github.com/mozilla-b2g/device-shinano-common

[Tracking Requested - why for this release]:

ril_worker.js: - Remove SMSC attribute. - Rewrite REQUEST_GET_SMSC_ADDRESS function. - Remove the usage of rilRequestError since it's removed in Bug 991582. test_ril_worker_smsc_address.js: - Add a test case for getSmscAddress.

nsIMobileMessageCallback.idl - Update the signature of NotifyGetSmscAddress and uuid. MobileMessageCallback.cpp: - Update NotifyGetSmscAddress to reflect the signature change and to adapt Promise. - Update NotifyGetSmscAddressFailed to adapt Promise. nsISmsService.idl: - Update the comment to emphasize NUMBER_PLAN_IDENTIFICATION constants reflects enumeration values. SmsService.js: - Update the invocation of notifySmscAddress as the signature changes.

MobileMessageManager / MozMobileMessageManager.webidl: - Adapt Promise in GetSmscAddress function instead of DOMRequest.

PSmsRequest.ipdl: - Update ReplyGetSmscAddress to include extra parameters. SmsChild: - Update Recv__delete__ to reflect the change of PSmsRequest. SmsParent: - Update NotifyGetSmscAddress to reflect the change of PSmsRequest.

test_set_smsc_address / test_smsc_address: - Update the test cases to reflect the interface change.

Comment on attachment 8633325 [details] [diff] [review] Part 1: Update ril_worker and xpcshell test Hi Bevis, Could you help to review this patch? BTW I've also removed the usage of rilRequestError of REQUEST_SET_SMSC_ADDRESS in this patch since all rilRequestError usages in ril_worker are cleaned up in Bug 991582.

Comment on attachment 8633326 [details] [diff] [review] Part 2: Update MobileMessageCallback and SmsService. r=btseng Hi Bevis, Could you help to review this patch?

Comment on attachment 8633327 [details] [diff] [review] Part 3: Update MozMobileMessageManager WebIDL interface and implementation. r=htsai, btseng Hi Bevis, Could you help to review this patch?

Comment on attachment 8633328 [details] [diff] [review] Part 4: Update SMS IPC implementation. r=btseng Hi Bevis, Could you help to review this patch?

Comment on attachment 8633329 [details] [diff] [review] Part 5: Update marionette test cases. r=btseng Hi Bevis, Could you help to review this patch?

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f34eeb7fca26

Comment on attachment 8633325 [details] [diff] [review] Part 1: Update ril_worker and xpcshell test Review of attachment 8633325 [details] [diff] [review]: ----------------------------------------------------------------- Per offline discussion, please help to ensure that additional user cases are supported: 1. In PDU mode, we have to identify the scenario that the value of this setting is empty. 2. In Text mode, - Empty. - What if modem doesn't reply with ",<tosca>" <tosca> for setting is optional but is required when retrieving. (It's handy for us to identify this if modem doesn't support this well.) Thanks!

Comment on attachment 8633326 [details] [diff] [review] Part 2: Update MobileMessageCallback and SmsService. r=btseng Review of attachment 8633326 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! Thanks!

Comment on attachment 8633327 [details] [diff] [review] Part 3: Update MozMobileMessageManager WebIDL interface and implementation. r=htsai, btseng Review of attachment 8633327 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Rember to invite hsinyi to review the webidl change. Please also set dependency to a new Gaia bug in which the implementation adopts DOMRequest.then() to support both the legacy one and this new API. Thanks!

Comment on attachment 8633328 [details] [diff] [review] Part 4: Update SMS IPC implementation. r=btseng Review of attachment 8633328 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks!

Comment on attachment 8633329 [details] [diff] [review] Part 5: Update marionette test cases. r=btseng Review of attachment 8633329 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks! :)

ril_worker.js: - Remove SMSC attribute. - Rewrite REQUEST_GET_SMSC_ADDRESS function. - Remove the usage of rilRequestError since it's removed in Bug 991582. test_ril_worker_smsc_address.js: - Add a test case for getSmscAddress.

ril_worker.js: - Remove SMSC attribute. - Rewrite REQUEST_GET_SMSC_ADDRESS function. - Remove the usage of rilRequestError since it's removed in Bug 991582. test_ril_worker_smsc_address.js: - Add a test case for getSmscAddress.

Comment on attachment 8635158 [details] [diff] [review] Part 1(v2): Update ril_worker and xpcshell test Hi Bevis, could you help to review the updated patch?

Comment on attachment 8633327 [details] [diff] [review] Part 3: Update MozMobileMessageManager WebIDL interface and implementation. r=htsai, btseng Hi Hsinyi, could you help to review the Web IDL interface change?

Comment on attachment 8635158 [details] [diff] [review] Part 1(v2): Update ril_worker and xpcshell test Review of attachment 8635158 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comments inline. Thanks! ::: dom/system/gonk/ril_worker.js @@ +5514,5 @@ > + const TON_UNKNOWN = (TOA_UNKNOWN & 0x70) >> 4; > + let tonValue = (tosca & 0x70) >> 4; > + let ton = TON_UNKNOWN; > + for (let i = 0; i < CALLED_PARTY_BCD_NPI.length; i++) { > + if (CALLED_PARTY_BCD_NPI[i] === tonValue) { Oops! It seems that you incorrectly mapped NPI with tonValue instead of the npi one. ::: dom/system/gonk/tests/test_ril_worker_smsc_address.js @@ +28,2 @@ > equal(this.readString(), expected); > + nits: please remove this line. @@ +97,5 @@ > + getSmsc(worker, context, SMSC_O2_TEXT, SMSC_O2, 1, 1); > + getSmsc(worker, context, SMSC_TON_UNKNOWN_TEXT, SMSC_TON_UNKNOWN, 0, 1); > + getSmsc(worker, context, SMSC_TON_UNKNOWN_TEXT_NO_TOA, SMSC_TON_UNKNOWN, 0, 1); > + getSmsc(worker, context, SMSC_TON_UNKNOWN_TEXT_INVALID_TOA, SMSC_TON_UNKNOWN, > + 0, 1); Per offline discuss, please help to add test coverage of empty sms for text mode. Thanks!

Comment on attachment 8635158 [details] [diff] [review] Part 1(v2): Update ril_worker and xpcshell test Review of attachment 8635158 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +5502,5 @@ > + if (segments.length === 2) { > + tosca = this.parseInt(segments[1], TOA_UNKNOWN, 10); > + } > + > + smsc = segments[0].replace(/\"/ig, ""); nits: no need to ignore case: smsc = segments[0].replace(/\"/g, "");

ril_worker.js: - Remove SMSC attribute. - Rewrite REQUEST_GET_SMSC_ADDRESS function. - Remove the usage of rilRequestError since it's removed in Bug 991582. test_ril_worker_smsc_address.js: - Add a test case for getSmscAddress.

Comment on attachment 8635224 [details] [diff] [review] Part 1(v3): Update ril_worker and xpcshell test Hi Bevis, Would you help to review the updated patch?

Comment on attachment 8635224 [details] [diff] [review] Part 1(v3): Update ril_worker and xpcshell test Review of attachment 8635224 [details] [diff] [review]: ----------------------------------------------------------------- r=me after the comments addressed. Thanks! ::: dom/system/gonk/ril_worker.js @@ +5509,5 @@ > + // Convert the NPI value to the corresponding index of CALLED_PARTY_BCD_NPI > + // array. If the value does not present in the array, use > + // CALLED_PARTY_BCD_NPI_ISDN. > + let npi = CALLED_PARTY_BCD_NPI.indexOf(tosca & 0xf); > + if (npi === -1) nit: Please always have {} closure protected even there is only one statement in the if-statement. In addition, please add a brief note at: https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js#1414 that the indexed value shall be consistent to nsISmsService::NUMBER_PLAN_IDENTIFICATION_*.

ril_worker.js: - Remove SMSC attribute. - Rewrite REQUEST_GET_SMSC_ADDRESS function. - Remove the usage of rilRequestError since it's removed in Bug 991582. test_ril_worker_smsc_address.js: - Add a test case for getSmscAddress. ril_const.js: - Add more clear comment for CALLED_PARTY_BCD_NPI array.

try result https://treeherder.mozilla.org/#/jobs?repo=try&revision=497d872454f5

https://hg.mozilla.org/integration/b2g-inbound/rev/281c89d5582f https://hg.mozilla.org/integration/b2g-inbound/rev/8288be78c5f5 https://hg.mozilla.org/integration/b2g-inbound/rev/dd174c137868 https://hg.mozilla.org/integration/b2g-inbound/rev/a254def80fe6 https://hg.mozilla.org/integration/b2g-inbound/rev/d5f6df9fc4a7

https://hg.mozilla.org/mozilla-central/rev/281c89d5582f https://hg.mozilla.org/mozilla-central/rev/8288be78c5f5 https://hg.mozilla.org/mozilla-central/rev/dd174c137868 https://hg.mozilla.org/mozilla-central/rev/a254def80fe6 https://hg.mozilla.org/mozilla-central/rev/d5f6df9fc4a7