According to 3GPP TS 23.038, the Spanish national language locking shift table is undefined and should fallback to GSM 7 bit default alphabet. However the current definition in ril_consts.js is an invalid table: > /** > * National Language Identifier: 0x02 > * A.3.2 Void > */ > // 0123456789A.BCD.EF > " \n \r " > // 0123456789AB.....CDEF > + " \uffff " > // 0123456789ABCDEF > + " " > // 0123456789ABCDEF > + " " > // 0123456789ABCDEF > + " " > // 0123456789ABCDEF > + " " > // 0123456789ABCDEF > + " " > // 0123456789ABCDEF > + " ", The table should be replaced with GSM 7 bit default alphabet and the corresponding test case for SmsSegmentHelper should be updated as well.

(In reply to Samael Wang [:samael][:sawang] from comment #0) > According to 3GPP TS 23.038 [1], the Spanish national language locking shift > table is undefined and should fallback to GSM 7 bit default alphabet. > However the current definition in ril_consts.js [2] is an invalid table: > The table should be replaced with GSM 7 bit default alphabet and the > corresponding test case for SmsSegmentHelper should be updated as well. Nice catch! After thinking this twice, there is one more thing to be taken into consideration: If there is no |locking shift table| for Spanish, that means when providing the value to |enabledGsmTableTuples| for Spanish, |PDU_NL_IDENTIFIER_DEFAULT| shall always be set for |locking shift table|. This save 1 IEI(3 octets) in SMS PDU which allows more characters in text body. BTW, update references in comment 0 as followed. :) [1] Table 6.2.1.2.4.1 in http://www.etsi.org/deliver/etsi_ts/123000_123099/123038/11.00.00_60/ts_123038v110000p.pdf [2] https://hg.mozilla.org/mozilla-central/file/0b3c520002ad/dom/system/gonk/ril_consts.js#l1776

Comment on attachment 8572473 [details] [diff] [review] [B2G][SMS] Incorrect Spanish national language locking shift table definition Review of attachment 8572473 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comments inline. In addition, please have "Bug XXXXX -" with patch description that explains what has been done in this patch instead of the title of the bug. You can also separate patches into 2 parts for review, for example: 1. The Fix of the PDU_NL_LOCKING_SHIFT_TABLES in ril_consts.js. 2. The Corresponding test cases. Then, you can focus on the patches not review-granted yet. :) Thanks! ::: dom/mobilemessage/tests/xpcshell/test_sms_segment_helper.js @@ +38,5 @@ > // Test UCS fallback > // - No any default enabled nl tables > test_calc("A", [PDU_DCS_MSG_CODING_16BITS_ALPHABET, 2, 0,], []); > + // - Character not defined in enabled nl tables (Bengali character). > + test_calc("\u09ce", [PDU_DCS_MSG_CODING_16BITS_ALPHABET, 2, 0,], [[2, 2]]); We don't understand Bengali. For better readability: // - Character not defined in enabled nl tables ("\u4e00" -> '一' is one in Chinese.) test_calc("\u4e00", [PDU_DCS_MSG_CODING_16BITS_ALPHABET, 2, 0,], [[2, 2]]); @@ +58,5 @@ > // - Header contains only single shift table IEI > test_calc("^", [PDU_DCS_MSG_CODING_7BITS_ALPHABET, 2, 3, 0, 1], [[0, 1]]); > > // Test minimum cost nl tables selection > + // - '\u00e7' is defined in Turkish locking shift table Please also explain '\u00e7' is 'ç' in comment. // - '\u00e7' -> 'ç' is defined in locking shift table (Turkish) @@ +61,5 @@ > // Test minimum cost nl tables selection > + // - '\u00e7' is defined in Turkish locking shift table > + test_calc("\u00e7", [PDU_DCS_MSG_CODING_7BITS_ALPHABET, 1, 3, 1, 0], [[1, 0], [2, 0]]); > + test_calc("\u00e7", [PDU_DCS_MSG_CODING_7BITS_ALPHABET, 1, 3, 1, 0], [[2, 0], [1, 0]]); > + // - '\u09fa' is defined in Bengali single shift table ditto: // - '\u09fa' -> '৺' is defined in single shift table (Bengali) @@ +65,5 @@ > + // - '\u09fa' is defined in Bengali single shift table > + test_calc("\u09fa", [PDU_DCS_MSG_CODING_7BITS_ALPHABET, 2, 6, 2, 4], [[2, 0], [2, 4]]); > + test_calc("\u09fa", [PDU_DCS_MSG_CODING_7BITS_ALPHABET, 2, 6, 2, 4], [[2, 4], [2, 0]]); > + // - '\u00e7' is defined in both Turkish locking shift table and Spanish > + // single shift table. //- '\u00e7' -> 'ç' in locking shift table(Turkish) of one tuple and in single shift // table of another(Spanish). nits: remove trailing SP after Spanish. @@ +72,1 @@ > We don't have a test case dedicated to this bug to see if GSM 7bit characters, for example 'A', is actually encoded in 7BITS_ALPHABET when Spanish Tuple is applied. (That means both length and value are expected.) ::: dom/system/gonk/ril_consts.js @@ +1729,5 @@ > > +/* > + * 3GPP TS 23.038 - 6.2.1 GSM 7 bit Default Alphabet > + */ > +this.PDU_NL_GSM_DEFAULT_ALPHABET = remove trailing space. @@ +1753,5 @@ > + /** > + * National Language Identifier: 0x00 > + * 6.2.1 GSM 7 bit Default Alphabet > + */ > + this.PDU_NL_GSM_DEFAULT_ALPHABET, Use SP instead of Tab for Indentation: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Indentation @@ +1784,2 @@ > */ > + this.PDU_NL_GSM_DEFAULT_ALPHABET, no need to refer from 'this': PDU_NL_GSM_DEFAULT_ALPHABET,

Comment on attachment 8573853 [details] [diff] [review] Part 1: Fix PDU_NL_LOCKING_SHIFT_TABLES in ril_consts.js. r=btseng Review of attachment 8573853 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Thanks!

Comment on attachment 8573852 [details] [diff] [review] Part 2: Update the test cases in test_sms_segment_helper.js in corresponding to the changes of PDU_NL_LOCKING_SHIFT_TABLES. r=btseng Review of attachment 8573852 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks!

Comment on attachment 8573851 [details] [diff] [review] Part 3: Add a test case in test_ril_worker_sms_gsmpduhelper.js to cover this bug Review of attachment 8573851 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comments inlined. Per discussion offline, please also have a new marionette test case to test the decoding by receiving an incoming SMS with Spanish Lock Tables enabled. Thanks! ::: dom/system/gonk/tests/test_ril_worker_sms_gsmpduhelper.js @@ +189,5 @@ > > /** > + * Verify that encoding with Spanish locking shift table generates the same > + * septets as with GSM default alphabet table. > + * nits: trailing space. @@ +214,5 @@ > + // Simple message string which is covered by GSM default alphabet. > + let msg = "The quick brown fox jumps over the lazy dog"; > + > + // Encoded with GSM default alphabet. > + helper.writeStringAsSeptets(msg, 0 /* paddingBits */, ditto @@ +220,5 @@ > + let octetsWithDefaultTable = buf; > + buf = []; > + > + // Encoded with Spanish locking shift table. > + helper.writeStringAsSeptets(msg, 0, ditto @@ +225,5 @@ > + PDU_NL_IDENTIFIER_SPANISH, PDU_NL_IDENTIFIER_SPANISH); > + > + // "The quick brown fox jumps over the lazy dog" in septets equals 37 octets > + // + 5 bits, which should be encoded as 38 octets. > + equal(38, buf.length); Replace this check as the one in next comment inlined. @@ +227,5 @@ > + // "The quick brown fox jumps over the lazy dog" in septets equals 37 octets > + // + 5 bits, which should be encoded as 38 octets. > + equal(38, buf.length); > + > + // The content should be equal to what encoded with GSM default alphabet. Add checking for the length here to ensure both length & content are the same to octetsWithDefaultTable. equal(octetsWithDefaultTable.length, buf.length);

A very minor question about the length test, @@ +225,5 @@ > + PDU_NL_IDENTIFIER_SPANISH, PDU_NL_IDENTIFIER_SPANISH); > + > + // "The quick brown fox jumps over the lazy dog" in septets equals 37 octets > + // + 5 bits, which should be encoded as 38 octets. > + equal(38, buf.length); I was preferring to use a known constant as the expected value since it eliminated the possibility that both octetsWithDefaultTable.length and buf.length are wrong. Is it more preferable to compare with octetsWithDefaultTable in this case?

(In reply to Samael Wang [:freesamael][:sawang] from comment #10) > A very minor question about the length test, > > @@ +225,5 @@ > > + PDU_NL_IDENTIFIER_SPANISH, PDU_NL_IDENTIFIER_SPANISH); > > + > > + // "The quick brown fox jumps over the lazy dog" in septets equals 37 octets > > + // + 5 bits, which should be encoded as 38 octets. > > + equal(38, buf.length); > > I was preferring to use a known constant as the expected value since it > eliminated the possibility that both octetsWithDefaultTable.length and > buf.length are wrong. > > Is it more preferable to compare with octetsWithDefaultTable in this case? IMO, the length is expected to be verified in another test case in [1] already. [1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/tests/test_ril_worker_sms_segment_info.js#55

Sounds convinceible. Thanks.

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

Comment on attachment 8576608 [details] [diff] [review] Part 3(v3): Add an XPCShell and a Marionette test case for encoding and decoding examination, respectively Review of attachment 8576608 [details] [diff] [review]: ----------------------------------------------------------------- The test case is almost done. Please see my comment inline. Thanks! ::: dom/mobilemessage/tests/marionette/head.js @@ +522,5 @@ > + * A hex string representing the whole SMS T-PDU. > + * > + * @return A deferred promise. > + */ > +function sendRawSmsToEmulatorAndWait(aPdu) { Please reuse sendMultipleRawSmsToEmulatorAndWait() with 1 PDU in the given array instead. :) ::: dom/mobilemessage/tests/marionette/test_decode_spanish_fallback.js @@ +15,5 @@ > +const PDU_OA = "0AA89021436587"; // 0912345678 > + > +const PDU_PID_NORMAL = "00"; > +const PDU_DCS_GSM_7BIT = "00"; > +const PDU_TIMESTAMP = "51302151470080"; // 2015/3/12 15:47:00 UTC+8 The values of 47 minutes is reversed and the value of TZ for UTC+8 is incorrect. TS 23.040 9.2.3.11 TP-Service-Centre-Time-Stamp (TP-SCTS) The Time Zone indicates the difference, expressed in 'quarters of an hour', between the local time and GMT. In the first of the two semi-octets, the first bit (bit 3 of the seventh octet of the TP-Service-Centre-Time-Stamp field) represents the algebraic sign of this difference (0: positive, 1: negative). Hence, the correct value of PDU_TIMESTAMP shall be 51302151'74'00'20' instead. @@ +29,5 @@ > +const IE_USE_SPANISH_LOCKING_SHIFT_TABLE = "250102"; > +const IE_USE_SPANISH_SINGLE_SHIFT_TABLE = "240102"; > +const PDU_UDHL = "06"; > + > +const PDU_UDL = "0B"; // UDH occupies 7 octests = 8 septes, plus 3 septets data. nits: typo of 'octets' and 'septets'. @@ +46,5 @@ > + * correctly decoded with Spanish locking shift table. See bug 1138841. > + */ > +startTestCommon(function testCaseMain() { > + return Promise.resolve() > + .then(() => sendRawSmsToEmulatorAndWait(PDU)) You don't need this new API. You can reuse sendMultipleRawSmsToEmulatorAndWait() with [PDU] instead. Please see example in https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_replace_short_message_type.js#41

Comment on attachment 8577919 [details] [diff] [review] Part 3(v4): Add an XPCShell and a Marionette test case for encoding and decoding examination, respectively. r=btseng. Review of attachment 8577919 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thank you!

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

https://hg.mozilla.org/integration/b2g-inbound/rev/dbbe6d0c58eb https://hg.mozilla.org/integration/b2g-inbound/rev/e8b2fdd283f9 https://hg.mozilla.org/integration/b2g-inbound/rev/43528afb1ca5

https://hg.mozilla.org/mozilla-central/rev/dbbe6d0c58eb https://hg.mozilla.org/mozilla-central/rev/e8b2fdd283f9 https://hg.mozilla.org/mozilla-central/rev/43528afb1ca5