The GsmPDUHelper.enabledGsmTableTuples contains only default alphabet table for now. Different countries/carriers might request additional default enabled tables.

(In reply to Vicamo Yang from comment #0) > The GsmPDUHelper.enabledGsmTableTuples contains only default alphabet table > for now. Different countries/carriers might request additional default > enabled tables. As bug 712933 moves `enabledGsmTableTuples` into RadioInterfaceLayer.js, the change to be made in this issue will also be in the same place.

The summary of this issue is changed to cover multiple static/runtime settings for the RadioInterfaceLayer/RIL worker, etc. We have now two items to be implemented: 1) default enabled GSM national languages 2) concatenation reference number width, 8 or 16 bits

Hi, I'm a new contributor. Want to work on this. Would you help me?

(In reply to Rahid Hasan [:recursive] from comment #4) > Hi, I'm a new contributor. Want to work on this. Would you help me? Sure, why not? Do you have already B2G build environment setup? Please see https://github.com/mozilla-b2g/B2G/blob/master/README.md :)

Hi Yang! Yes I already set up the environment. Can you pls guide me for rest of the process?

(In reply to Rahid Hasan [:recursive] from comment #6) > Hi Yang! > Yes I already set up the environment. Can you pls guide me for rest of the process? One thing we want to do in this bug -- to add preference settings for enabledGsmTableTuples (extra GSM languages enabled) and segmentRef16Bit (concatenation reference number width). You may find detailed technical information in 3GPP TS 23.038[1] Annex A for full list of all standard languages and 3GPP TS 23.040[2] clause 9.2.3.24.1 "concatenated short messages" for effects of segment reference. Two requirements to pass the review: 1.) Preserve original behaviour. That is, the default values of the two newly added preferences must be what they are now. 2.) Test cases attached to prove it's actually working. A few reference bugs/tips: a.) Bug 863130 shows how to add a new preference item with runtime configurable Settings API entry. You don't need the Settings part because the two items to be added here are unlikely to be changed in runtime. b.) Bug 862268 shows how to add a new Marionette test script. You should test all possible combinations of enabledGsmTableTuples. Simply send a text message to yourself (loopback phone number is "5554") for each possible case. You may also want to take a look of test_emulator_loopback.js[3]. c.) Note that "default" alphabet should be always included. Mind value constraints. Have fun! [1]: http://www.3gpp.org/ftp/Specs/html-info/23038.htm [2]: http://www.3gpp.org/ftp/Specs/html-info/23040.htm [3]: https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_emulator_loopback.js

Another tip about how do you test whether an extra language is actually working? We have already a boolean preference "dom.sms.strict7BitEncoding". If set to `true`, and one of the sending characters cannot be transformed under current enabled GSM alphabets, that character will become a '*' character. So, it's your turn. :)

@Vicamo thanks for the help. But I don't know where to start for this bug. And don't understood any of your lines. As a newbie I thought this 'good first bug' will be easy. But seems like not for me. So I quit this bug.

Hello there, seems like Rahid has quit. Can I take this one? Cheers!

(In reply to Genti Tola [:d0kt0r1] from comment #10) > Hello there, > seems like Rahid has quit. Can I take this one? > Cheers! I was to remove the "good first bug" flag since I had a long long description in comment 7. It showed this is really not a good first bug and I should have either removed the flag in whiteboard or torn it down to more small parts. Anyway, taking this bug is still welcome and appreciated :)

Hello Vicamo, Firstly, sorry for such long delay. It's just that now I finally got some time to tackle this one. Secondly, let me briefly explain how I plan to proceed with this one: 1) Regarding the segmentRef16bit, a) I would register the preference to false b) Then update the following line: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#3480 to a function that reads the preference and returns true/false 2) Regarding extra languages a)I am unsure about the preference should specify the extra languaguages to include, or specify all the languages to include? b) Afterwards I am going to insert to replace this http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#3471 with a function that returns an array with the extra languages listed in there http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js#1606 3) Testing a) I think testing extra languages is clear by your hint to look for * b) Regarding testing segmentRef16bit: i) Test for length of max length of sms (now should be -=1 ) ii) Try to send 256 concatenated messages composed of just 2 pieces, if segmentRef16bit would be off. Then the last concatenated message won't work, maybe those parts will be attached to some other message. Otherwise if segmentRef16bit would be on, we would expect to get back 256 concatenated messages. Best Regards, Genti

(In reply to Genti Tola [:d0kt0r1] from comment #12) > Hello Vicamo, > Firstly, sorry for such long delay. It's just that now I finally got some > time to tackle this one. Hi Genti, Sorry for the late reply first. Any help to our code base is welcome and appreciated. I even think to study the lines is a also way to participate open development. > Secondly, let me briefly explain how I plan to proceed with this one: > 1) Regarding the segmentRef16bit, > a) I would register the preference to false > b) Then update the following line: > http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ > RadioInterfaceLayer.js#3480 > to a function that reads the preference and returns true/false And for efficiency, you should register an observer in RadioInterface::observe (http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#469) and keeps a local copy for further retrieval. RadioInterface::nextSegmentRef also needs additional care during the transition from 16bit ref to 8bit ref. In that case we may still get a 16bit ref when it's actually set to 8bit now. > 2) Regarding extra languages > a)I am unsure about the preference should specify the extra languages to > include, or specify all the languages to include? We can have a string of comma separated list of names "default,turkish,portuguese,...", and "default" should be always included no matter it appears in the list or not. > b) Afterwards > I am going to insert to replace this > http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ > RadioInterfaceLayer.js#3471 > with a function that returns an array with the extra languages listed in > there > http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts. > js#1606 > > 3) Testing > a) I think testing extra languages is clear by your hint to look for * > b) Regarding testing segmentRef16bit: > i) Test for length of max length of sms (now should be -=1 ) > ii) Try to send 256 concatenated messages composed of just 2 pieces, if > segmentRef16bit would be off. Then the last concatenated message won't work, > maybe those parts will be attached to some other message. > Otherwise if segmentRef16bit would be on, we would expect to get back 256 > concatenated messages. No matter 16bit ref is on or not, sending numerous, concatenated or not, messages at once should be working, or it's a bug. The task here doesn't affect the ref bits of received messages because that's always depending on IEI of the message user data header. RIL worker doesn't get confused at processing numerous of sending messages[1], but the network side does, and we haven't addressed this yet. To test this bug, I suggest 1) the use of |getSegmentInfoForText| calls to make sure the messages are corrected fragmented: - Available per segment body capacities differ a little bit from 8bit/16bit ref. Strings of a certain length become two segments with 16bit ref while there is only one with 8bit ref. 2) call to |sendSMS|, send to loopback address to make sure a concatenated message with more than 256 segments does succeed with 16bit ref. 3) call to |getSegmentInfoForText| and make sure messages containing special characters in different NL tables get fragmented correctly. Actually, if you want to separate this bug into two ones, one for 16 bit ref and the other for extra NL support, to slim down the complexity here, I'll also give you my vote. Thank you :) > Best Regards, > Genti [1]: one thing in exception, the status report mapping.

Hi Genti, I'd like to take this bug to follow up if you have no concern. Regards, Bevis

Hello Bevis, I suggest we split the bug into two ones as Vicamo suggests. You can take either one, no problem, just let me know, as I also plan to do something. Rock on

1. Update bug summary. 2. Take this bug. 3. split the support of the 8-bit/16-bit configuration of ref_num to Bug 1019443.

It looks we should not enable any national languages by default except GSM default alphabet according to 3GPP TS 23.038 [1] 6.2.1.2.5 NOTE 2: > Encoding of a message using the national locking shift mechanism is not intended to be implemented until > a formal request is issued by the relevant national regulatory body. This is because a receiving entity > not supporting the relevant locking-shift decoding will present different characters from the ones > intended by the sending entity. [1] http://www.etsi.org/deliver/etsi_ts/123000_123099/123038/12.00.00_60/ts_123038v120000p.pdf

(In reply to Samael Wang [:freesamael][:sawang] from comment #17) > It looks we should not enable any national languages by default except GSM > default alphabet according to 3GPP TS 23.038 [1] 6.2.1.2.5 NOTE 2: > > Encoding of a message using the national locking shift mechanism is not intended to be implemented until > > a formal request is issued by the relevant national regulatory body. This is because a receiving entity > > not supporting the relevant locking-shift decoding will present different characters from the ones > > intended by the sending entity. > > [1] > http://www.etsi.org/deliver/etsi_ts/123000_123099/123038/12.00.00_60/ > ts_123038v120000p.pdf IMO, this implies one thing: 1. These tables shall be enabled in country-based, instead of language-based. 2. MCC from inserted UICC provides us the information of the country the UICC belongs to. 3. We could either determinate what to be enabled according to the mcc detected from UICC improve the mechanism of operator-variant customization provided by gaia. [1] (Which also use the detected mcc from UICC for operator related customization.) 4. In addition, that means we have to know what tables are enabled by the authority of each country. - Not pretty sure if we can get any input from TAM about this. - We can also refer to AOSP to know what tables have been enabled in their implementation for different coutries. [1] https://github.com/mozilla-b2g/gaia/blob/f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f/shared/resources/apn/operator_variant.xml#L12

AOSP includes only a customization for Turkey [1]. Although the link http://www.btk.gov.tr/eng/pdf/2009/BY-LAW_SMS.pdf in the comment is dead, we can easily find the document by its number "By-Law Number 27230". Quote By-Law Number 2723 [2] ARTICLE 4 > (1) Devices having short message service which are put on the market must be in accordance with ETSI TS > 123.038 V8.0.0 and ETSI TS 4123.040 8.1.0 technical features or the subsequent versions of these > technical features. > (2) There must be an expression as “This device is in accordance with ETSI TS 1 23.038 V8.0.0 (or the code > of the subsequent version) and ETSI TS 1 23.040 V8.1.0 (or the code of the subsequent version) technical > features that include all the Turkish characters.” on the user manual and on the package and if > necessary information describing applications to be accomplished by user about these technical features > on the user manual of the devices which are in accordance with the technical specifications mentioned in > clause 1. > (3) Putting devices which do not comply with the conditions specified in this By - Law on the market shall > not be permitted... Agree using MCC sounds more reasonable, and maybe we should add the Turkish case since it's already known. [1] https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/core/res/res/values-mcc286/config.xml [2] http://eng.btk.gov.tr/mevzuat/yonetmelikler/dosyalar/By-Law%20On%20Use%20Of%20Turkish%20Characters.pdf

I'm also thinking of that we should split SmsSegmentHelper.enabledGsmTableTuples [1] to enabledGsmLockingShiftTable and enabledGsmSingleShiftTable and update the logic of calculateUserDataLength7Bit [2]. The current implementation of calculateUserDataLength7Bit searches for enabled locking / single shift table pairs and applies both if necessary. However, if a Turkish string, for instance, can be encoded with GSM default alphabet + Turkish single shift table, it's better than applying both Turkish locking shift table and single shift table, as the later needs an extra IEI entity. Quote TS 23.038 [3] 6.2.1.2.5: > It is an implementation option for the sending entity whether to use the single shift mechanism, > the locking shift mechanism or both. [1] https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/gonk/SmsSegmentHelper.jsm?from=smssegmenthelper.jsm#130 [2] https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/gonk/SmsSegmentHelper.jsm?from=smssegmenthelper.jsm#134 [3] http://www.etsi.org/deliver/etsi_ts/123000_123099/123038/12.00.00_60/ts_123038v120000p.pdf

Comment on attachment 8577904 [details] [diff] [review] Part 1: Add the mapping of MCC and corresponding extra national language table tuples to enable in ril_consts.js Review of attachment 8577904 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comments inline. Thanks. ::: dom/system/gonk/ril_consts.js @@ +1699,5 @@ > > +// The mapping of mcc and their extra GSM national language locking / single > +// shift table tuples to enable. The default GSM alphabet and extension table > +// are always enabled and need not to be list here. The content should be > +// updated when a relevant national regulatory body requests. See bug 733331. Remove "See bug 733331" and add the following explanation, then we don't have to revisit bug 733331 again when reading this. // See 'NOTE 2' of 6.2.1.2.5 in 3GPP TS 23.038 : // " // Encoding of a message using the national locking shift mechanism is not // intended to be implemented until a formal request is issued by the // relevant national regulatory body. This is because a receiving entity // not supporting the relevant locking-shift decoding will present different // characters from the ones intended by the sending entity. // "

Comment on attachment 8577905 [details] [diff] [review] Part 2: Update enabledGsmTableTuples when MCC changes in SmsSegmentHelper.jsm Review of attachment 8577905 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comments inlined. Thanks! ::: dom/mobilemessage/gonk/SmsSegmentHelper.jsm @@ +424,5 @@ > + > + /** > + * nsIObserver interface. > + */ > + observe: function(aSubject, aTopic, aData) { It's meaningless to have an implementation of nsIObserver without adding it self into the observer list by "Services.prefs.addObserver(kPrefLastKnownSimMcc, this, false);" Does this patch tested? To make SmsSegmentHelper.jsm as simple as possible without extra access to outer modules like Service, Please move the logic of initializing/updating of "SmsSegmentHelper.enabledGsmTableTuples" in the constructor of SmsService.js & SmsService.observe().

Comment on attachment 8577906 [details] [diff] [review] Part 3: Corresponding marionette test case Review of attachment 8577906 [details] [diff] [review]: ----------------------------------------------------------------- Please request review again after the problem in patch part 2 is addressed. Thanks!

Rather than the constructor of SmsService, I put the initialization of enabledGsmTableTuples in the xpcom lazy getter. Hopefully that keeps the benefit of lazy initialization. Let me know if any concern.

Comment on attachment 8578564 [details] [diff] [review] Part 1 (v2): Add the mapping of MCC and corresponding extra national language table tuples to enable in ril_consts.js Review of attachment 8578564 [details] [diff] [review]: ----------------------------------------------------------------- As bug 1138841 has been merged to mozilla-central, the patch needs to be rebase again.

Comment on attachment 8578565 [details] [diff] [review] Part 2 (v2): Update enabledGsmTableTuples when MCC changes in SmsService.js and update comments in SmsSegmentHelper.jsm Review of attachment 8578565 [details] [diff] [review]: ----------------------------------------------------------------- As bug 1138841 has been merged to mozilla-central, the patch needs to be rebase again.

Comment on attachment 8579098 [details] [diff] [review] Part 1(v3): Add the mapping of MCC and corresponding extra national language table tuples to enable in ril_consts.js Review of attachment 8579098 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Comment on attachment 8579099 [details] [diff] [review] Part 2(v3): Update enabledGsmTableTuples when MCC changes in SmsService.js and update comments in SmsSegmentHelper.jsm Review of attachment 8579099 [details] [diff] [review]: ----------------------------------------------------------------- r=me after the problem inline is addressed. Thanks! ::: dom/mobilemessage/gonk/SmsService.js @@ +1091,5 @@ > + * @return a list of pairs of national language identifiers for locking shift > + * table and single shfit table, respectively. > + */ > +function getEnabledGsmTableTuplesFromMcc() { > + let mcc = Services.prefs.getCharPref(kPrefLastKnownSimMcc); Please have try-catch clause protected for getting preference, thanks! let mcc; try { mcc = Services.prefs.getCharPref(kPrefLastKnownSimMcc); } catch (e) {}

Comment on attachment 8578566 [details] [diff] [review] Part 3 (v2): Add corresponding marionette test case Review of attachment 8578566 [details] [diff] [review]: ----------------------------------------------------------------- Per offline discussion, please also add 2 more test cases for boundary check: 1. all 140 octets are used for Turkish characters in single PDU. 2. all 140 octets are used for Turkish characters in multiple PDUs (2) for concatenation. Thanks!

Comment on attachment 8578566 [details] [diff] [review] Part 3 (v2): Add corresponding marionette test case Review of attachment 8578566 [details] [diff] [review]: ----------------------------------------------------------------- Per offline discussion, please also add 2 more test cases for boundary check: 1. all 140 octets are used for Turkish characters in single PDU. 2. all 140 octets are used for Turkish characters in multiple PDUs (2) for concatenation. Thanks!

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

Comment on attachment 8581388 [details] [diff] [review] Part 2(v4): Update enabledGsmTableTuples when MCC changes in SmsService.js and fix segmentChars in SmsSegmentHelper.jsm. r=btseng Review of attachment 8581388 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks!

Comment on attachment 8581399 [details] [diff] [review] Part 3(v3): Add corresponding marionette test case Review of attachment 8581399 [details] [diff] [review]: ----------------------------------------------------------------- Please address the following suggestion and request review again. Thanks! ::: dom/mobilemessage/tests/marionette/manifest.ini @@ +50,5 @@ > [test_error_of_mms_send.js] > [test_error_of_sms_send.js] > [test_ondeleted_event.js] > [test_decode_spanish_fallback.js] > +[test_update_gsm_nl_on_mmc_chanages.js] \ No newline at end of file I think you what you want to test is on_"mcc"_changes. ::: dom/mobilemessage/tests/marionette/test_update_gsm_nl_on_mmc_chanages.js @@ +66,5 @@ > + > + // Turkey / GSM 7 bits / short Turkish message. > + .then(() => getSegmentInfoForText(MSG_TURKISH)) > + .then((segmentInfo) => is(segmentInfo.charsPerSegment, 152, > + "charsPerSegment for Turkey / GSM 7 bits / short Turkish message.")) Please have all attributes |segments|, |charsPerSegment| and |charsAvailableInLastSegment| verified as well. @@ +70,5 @@ > + "charsPerSegment for Turkey / GSM 7 bits / short Turkish message.")) > + > + // Turkey; GSM 7 bits; longest single segment Turkish message. > + .then(() => getSegmentInfoForText(MSG_TURKISH_LONG)) > + .then((segmentInfo) => is(segmentInfo.segments, 1, Ditto @@ +75,5 @@ > + "# segments for Turkey; GSM 7 bits; longest single segment Turkish message.")) > + > + // Turkey; GSM 7 bits; shortest dual segments Turkish message. > + .then(() => getSegmentInfoForText(MSG_TURKISH_MULTI_SEGS)) > + .then((segmentInfo) => is(segmentInfo.segments, 2, Ditto

Comment on attachment 8582347 [details] [diff] [review] Part 1(v4): Add the mapping of MCC and corresponding extra national language table tuples to enable in ril_consts.js. r=btseng Review of attachment 8582347 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks!

Comment on attachment 8582348 [details] [diff] [review] Part 3(v4): Add corresponding marionette test case. r=btseng Review of attachment 8582348 [details] [diff] [review]: ----------------------------------------------------------------- Nice! :)

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

https://hg.mozilla.org/integration/b2g-inbound/rev/8ea9a523221b https://hg.mozilla.org/integration/b2g-inbound/rev/946b4ab308ec https://hg.mozilla.org/integration/b2g-inbound/rev/b8e7e98b3843

https://hg.mozilla.org/mozilla-central/rev/8ea9a523221b https://hg.mozilla.org/mozilla-central/rev/946b4ab308ec https://hg.mozilla.org/mozilla-central/rev/b8e7e98b3843