The `readSwappedNibbleBCD` assumes all numeric BCDs in its input and returns a calculated number accordingly. However, for a national number, which may be prefixed with zeros, the decimal numeric string won't preserve them and results in a wrong number.

We supports only one(and incomplete) type of SMS addressing for now. The missing zeros problem could be solved by turning parsed numeric result into a string one. This is also a must especially when we'll going to support different TOAs(types of address). See 3GPP TS 24.011[1], 3GPP 29.002[2] and 3GPP TS 23.040[3] as well. [1]: http://www.3gpp.org/ftp/Specs/html-info/24011.htm [2]: http://www.3gpp.org/ftp/Specs/html-info/29002.htm [3]: http://www.3gpp.org/ftp/Specs/html-info/23040.htm

Comment on attachment 623925 [details] [diff] [review] add readSwappedNibbleBcdString for complex GSM address Review of attachment 623925 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +3034,5 @@ > + } > + > + str += this.semiOctetToBcdChar(nibbleL); > + if (nibbleH != 0x0F) { > + str += semiOctetToBcdChar(nibbleH); str += this.semiOctetToBcdChar(nibbleH); ^^^^^

address review comment ＃3

Just don't use magic number

Comment on attachment 624275 [details] [diff] [review] add readSwappedNibbleBcdString for complex GSM address : V3 Review of attachment 624275 [details] [diff] [review]: ----------------------------------------------------------------- I've helped to verify this patch on SIM contacts part, and it works well. Thanks for that patch. Remember to rebase again. I might not have privilege to review that, so I just change it to feedback+. ::: dom/system/gonk/ril_worker.js @@ +3028,5 @@ > + let str = ""; > + for (let i = 0; i < pairs; i++) { > + let nibbleH = this.readHexNibble(); > + let nibbleL = this.readHexNibble(); > + if (nibbleL == 0x0F) { maybe 0x0f? I see all other code uses 0x0f @@ +3048,5 @@ > * @return the decimal as a number. > */ > readStringAsBCD: function readStringAsBCD() { > let length = Buf.readUint32(); > + let bcd = this.readSwappedNibbleBcdString(length / 2); This function readStringAsBCD is removed in Bug 754777, you need to rebase for that.

Rebase to dev tip

Comment on attachment 624343 [details] [diff] [review] add readSwappedNibbleBcdString for complex GSM address : V4 Review of attachment 624343 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, but it should have unit tests.

Comment on attachment 624343 [details] [diff] [review] add readSwappedNibbleBcdString for complex GSM address : V4 = >+ >+ str += this.semiOctetToBcdChar(nibbleL); >+ if (nibbleH != 0x0F) { >+ str += semiOctetToBcdChar(nibbleH); I think there is another |this.| for semiOctetToBcdChar missing.

(In reply to Gregor Wagner [:gwagner] from comment #9) > >+ > >+ str += this.semiOctetToBcdChar(nibbleL); > >+ if (nibbleH != 0x0F) { > >+ str += semiOctetToBcdChar(nibbleH); > > > I think there is another |this.| for semiOctetToBcdChar missing. ... which is why it needs to have tests... :)

Vicamo, please file a separate bug for the tests and promise to fix it soon :) The contacts-import patch that relies on this patch already landed so I will land this now.