Closed Bug 729876 Opened 8 years ago Closed 8 years ago

B2G SMS: Support other 7bit alphabets

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: philikon, Assigned: vicamo)

References

Details

Attachments

(4 files, 8 obsolete files)

6.55 KB, patch
philikon
: review+
Details | Diff | Splinter Review
28.51 KB, patch
philikon
: review+
Details | Diff | Splinter Review
20.55 KB, patch
philikon
: review+
Details | Diff | Splinter Review
11.11 KB, patch
philikon
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #712804 +++

Making bug 712804 purely about UCS2, so this is about implementing the other 7bit alphabets.
Assignee: nobody → vyang
Blocks: 712933
No longer blocks: b2g-sms
Locale language tables are specified in user data header, which is also required for bug 712933. Block it and let's have user data header r/w support in this issue.
You may verify this patch by sending yourself a sms with only one character '`', which is defined in Portuguese national language locking shift table(see TS 23.038 A.3.3). For verifying default single shift table, use '\~^|[]{}€'. For other characters, we need additional keyboards to be even able to key in.

Note that iPhone may not be a good receiver for sms encoded with these non-default alphabets. It seems to implement only UCS2 encoding and 7-bit GSM default tables. The '`' character sent with this patch from b2g can be correctly shown in other Android devices, but 'ñ' instead in iPhone. 'ñ' has the same code value in GSM default locking shift table with '`' in Portuguese one.
Attachment #601916 - Flags: review?(philipp)
Comment on attachment 601916 [details] [diff] [review]
support all 7-bit national languages other than GSM default

Review of attachment 601916 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/b2g/ril_worker.js
@@ +2350,5 @@
> +          escapeFound = false;
> +          if (septet == PDU_NL_EXTENDED_ESCAPE) {
> +            ret += ' ';
> +          } else {
> +            ret += langShiftTable[septet];

single shift table for Spanish is empty, this might cause out of bound error.

@@ +2486,2 @@
>     */
>    calculateUserDataLength: function calculateUserDataLength(options) {

Shall I move this function into RadioInterfaceLayer.js? Bug 712933 suggested implementation of getNumberOfMessagesForText(), which might also need similar method. If we move it into RadioInterfaceLayer, we will have to invoke it before posting sendSMS messages.

@@ +2571,5 @@
> +            if (DEBUG) bytes = new Uint8Array(length);
> +
> +            for (let i = 0; i < length; ++i) {
> +              let octet = this.readHexOctet();
> +              --dataAvailable;

move out of the loop

@@ +2572,5 @@
> +
> +            for (let i = 0; i < length; ++i) {
> +              let octet = this.readHexOctet();
> +              --dataAvailable;
> +              if (DEBUG) bytes[i] = code;

should be "octet"
(In reply to Vicamo Yang from comment #3)
> ::: dom/system/b2g/ril_worker.js
> @@ +2350,5 @@
> > +          escapeFound = false;
> > +          if (septet == PDU_NL_EXTENDED_ESCAPE) {
> > +            ret += ' ';
> > +          } else {
> > +            ret += langShiftTable[septet];
> 
> single shift table for Spanish is empty, this might cause out of bound error.

There are no out-of-bound errors in JS, you'll just get `undefined` :). But in principle you're right, we should fix that.

> @@ +2486,2 @@
> >     */
> >    calculateUserDataLength: function calculateUserDataLength(options) {
> 
> Shall I move this function into RadioInterfaceLayer.js? Bug 712933 suggested
> implementation of getNumberOfMessagesForText(), which might also need
> similar method. If we move it into RadioInterfaceLayer, we will have to
> invoke it before posting sendSMS messages.

Yes, eventually it will have to move to RadioInterfaceLayer.js for the reason you mention. We can do this now or in a follow-up. I personally prefer small bugs + small patches.
(In reply to Vicamo Yang from comment #2)
> Created attachment 601916 [details] [diff] [review]
> support all 7-bit national languages other than GSM default
> 
> You may verify this patch by sending yourself a sms with only one character
> '`', which is defined in Portuguese national language locking shift
> table(see TS 23.038 A.3.3). For verifying default single shift table, use
> '\~^|[]{}€'. For other characters, we need additional keyboards to be even
> able to key in.
> 
> Note that iPhone may not be a good receiver for sms encoded with these
> non-default alphabets. It seems to implement only UCS2 encoding and 7-bit
> GSM default tables. The '`' character sent with this patch from b2g can be
> correctly shown in other Android devices, but 'ñ' instead in iPhone. 'ñ' has
> the same code value in GSM default locking shift table with '`' in
> Portuguese one.

Woah, that's good to know. I wonder if we should send texts in language specific alphabets at all then. I wonder how Android sends a text that just contains the backtick (`). I just texted a backtick to an iPhone from my Android phone and it showed up correctly.
Comment on attachment 601916 [details] [diff] [review]
support all 7-bit national languages other than GSM default

Review of attachment 601916 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent patch! Some general comments:

* If our suspicions are confirmed that a large portion of devices such as the iPhone can't read non-standard-GSM-alphabet messages, we should probably only send in UCS2 and standard GSM alphabet. This would simplify your logic a lot.

* You seem to prefer ++foo where as all existing code uses foo++. In JS, they're identical and we should keep them same everywhere for consistency's sake.

* It would be good to have tests :)

For some more detailed comments, please see below.

::: dom/system/b2g/ril_worker.js
@@ +797,2 @@
>     *        Byte length of the message body when encoded with the given DCS.
> +   *        For UCS2, in bytes; for 7-bit, in septets.

I don't understand this last addition. Byte length is byte length, no matter what the encoding is. I think you're just adding confusion here.

@@ +2449,5 @@
> +      }
> +
> +      if (septet >= 0) {
> +        ++length;
> +      } else {

Bail out early here, too.

  if (septet >=) {
    ++length;
    continue;
  }

@@ +2496,5 @@
> +    let minUserDataLength = Number.MAX_VALUE;
> +    for (let langIndex = 0; langIndex < PDU_NL_LOCKING_SHIFT_TABLES.length; ++langIndex) {
> +      const langTable = PDU_NL_LOCKING_SHIFT_TABLES[langIndex];
> +
> +      for (let langShiftIndex = 0; langShiftIndex < PDU_NL_SINGLE_SHIFT_TABLES.length; ++langShiftIndex) {

Nit: please wrap overlong line (80 chars is the limit).

@@ +2499,5 @@
> +
> +      for (let langShiftIndex = 0; langShiftIndex < PDU_NL_SINGLE_SHIFT_TABLES.length; ++langShiftIndex) {
> +        const langShiftTable = PDU_NL_SINGLE_SHIFT_TABLES[langShiftIndex];
> +
> +        let length = this._calculateLangEncodedLength(options.body, langTable, langShiftTable);

Ditto

@@ +2508,5 @@
> +        let headerLen = 0;
> +        if (langIndex != PDU_NL_IDENTIFIER_DEFAULT)
> +          headerLen += 3; // IEI + len + langIndex
> +        if (langShiftIndex != PDU_NL_IDENTIFIER_DEFAULT)
> +          headerLen += 3; // IEI + len + langShiftIndex

Nit: always need braces.

@@ +2525,5 @@
> +        options.langIndex = langIndex;
> +        options.langShiftIndex = langShiftIndex;
> +
> +        if (userDataLength <= options.body.length) {
> +          // found minimum user data length already, there can't be even shorter!

The "there can't be even shorter!" is not grammatically correct. It's also unnecessary, so you can just delete it. Also, please capitalize the first word in a sentence ("Found").

@@ +2654,5 @@
>  
> +    let header = {};
> +    let paddingBits = 0;
> +    if (hasHeader) {
> +      this.readUserDataHeader(header);

I would prefer if `readUserDataHeader()` would create the `header` object, e.g.:

  let header;
  if (hasHeader) {
    header = this.readUserDataHeader();
    ...
Attachment #601916 - Flags: review?(philipp) → superreview+
(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> Comment on attachment 601916 [details] [diff] [review]
> support all 7-bit national languages other than GSM default
> 
> Review of attachment 601916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent patch! Some general comments:
> 
> * If our suspicions are confirmed that a large portion of devices such as
> the iPhone can't read non-standard-GSM-alphabet messages, we should probably
> only send in UCS2 and standard GSM alphabet. This would simplify your logic
> a lot.
Some alphabet tables are required for countries like Turkey and India. In Taiwan, these tables are not enabled in Android and it sends '`' in UCS2 as iPhone does, but Android still reserves ability to correctly parse messages encoded with non standard alphabet. So, is there any way to read country/carrier specific configurations?

> * You seem to prefer ++foo where as all existing code uses foo++. In JS,
> they're identical and we should keep them same everywhere for consistency's
> sake.
That's a habit from C++ :)
Just explicitly show that I don't need a temporary value here. I'll modify all of them to accommodate to Mozilla's coding style.
 
> * It would be good to have tests :)
About tests, I had a initial work for testing SMS functions. Somehow I found myself rewriting the whole SMS parsing & constructing work. I could just hard code all raw bytes for testing receiving function, but it won't be convenient for hundreds of test cases coming. Just still unsure what should I do for now.

> For some more detailed comments, please see below.
> 
> @@ +2654,5 @@
> >  
> > +    let header = {};
> > +    let paddingBits = 0;
> > +    if (hasHeader) {
> > +      this.readUserDataHeader(header);
> 
> I would prefer if `readUserDataHeader()` would create the `header` object,
> e.g.:
> 
>   let header;
>   if (hasHeader) {
>     header = this.readUserDataHeader();
>     ...
Cool!
(In reply to Philipp von Weitershausen [:philikon] from comment #5)
> (In reply to Vicamo Yang from comment #2)
> > Note that iPhone may not be a good receiver for sms encoded with these
> > non-default alphabets. It seems to implement only UCS2 encoding and 7-bit
> > GSM default tables. The '`' character sent with this patch from b2g can be
> > correctly shown in other Android devices, but 'ñ' instead in iPhone. 'ñ' has
> > the same code value in GSM default locking shift table with '`' in
> > Portuguese one.
> 
> Woah, that's good to know. I wonder if we should send texts in language
> specific alphabets at all then. I wonder how Android sends a text that just
> contains the backtick (`). I just texted a backtick to an iPhone from my
> Android phone and it showed up correctly.

iPhone implements only UCS2 and default GSM alphabet for both sending & receiving SMS, while Android implements all for receiving but UCS2, default alphabet (and other alphabets required by local law, usually none) for sending.
(In reply to Vicamo Yang from comment #7)
> > * If our suspicions are confirmed that a large portion of devices such as
> > the iPhone can't read non-standard-GSM-alphabet messages, we should probably
> > only send in UCS2 and standard GSM alphabet. This would simplify your logic
> > a lot.
> Some alphabet tables are required for countries like Turkey and India. In
> Taiwan, these tables are not enabled in Android and it sends '`' in UCS2 as
> iPhone does, but Android still reserves ability to correctly parse messages
> encoded with non standard alphabet. So, is there any way to read
> country/carrier specific configurations?

We could make this dependent on the locale. But what do you mean by "required"? Will the receiving end not understand UCS2? It seems like simply falling back to UCS2 is always a good option (apart from the fact that you get less than half the number of characters.)

> > * It would be good to have tests :)
> About tests, I had a initial work for testing SMS functions. Somehow I found
> myself rewriting the whole SMS parsing & constructing work. I could just
> hard code all raw bytes for testing receiving function, but it won't be
> convenient for hundreds of test cases coming. Just still unsure what should
> I do for now.

We can do it as a follow-up. Also, start with something, and then refactor later if we find it's too complicated. Getting the ball rolling is important :)

(In reply to Vicamo Yang from comment #8)
> iPhone implements only UCS2 and default GSM alphabet for both sending &
> receiving SMS, while Android implements all for receiving but UCS2, default
> alphabet (and other alphabets required by local law, usually none) for
> sending.

Right. So how about we do what Android does?
(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> (In reply to Vicamo Yang from comment #7)
> > > * If our suspicions are confirmed that a large portion of devices such as
> > > the iPhone can't read non-standard-GSM-alphabet messages, we should probably
> > > only send in UCS2 and standard GSM alphabet. This would simplify your logic
> > > a lot.
> > Some alphabet tables are required for countries like Turkey and India. In
> > Taiwan, these tables are not enabled in Android and it sends '`' in UCS2 as
> > iPhone does, but Android still reserves ability to correctly parse messages
> > encoded with non standard alphabet. So, is there any way to read
> > country/carrier specific configurations?
> 
> We could make this dependent on the locale. But what do you mean by
> "required"? Will the receiving end not understand UCS2? It seems like simply
> falling back to UCS2 is always a good option (apart from the fact that you
> get less than half the number of characters.)

"required" means you can't sell a mobile without it, but I don't really understand how does Turkish and Indian verify it. Android code base mentioned official document "By-Law Number 27230 (http://www.btk.gov.tr/eng/pdf/2009/BY-LAW_SMS.pdf)" for Turkey, which is already a dead link.

> > > * It would be good to have tests :)
> > About tests, I had a initial work for testing SMS functions. Somehow I found
> > myself rewriting the whole SMS parsing & constructing work. I could just
> > hard code all raw bytes for testing receiving function, but it won't be
> > convenient for hundreds of test cases coming. Just still unsure what should
> > I do for now.
> 
> We can do it as a follow-up. Also, start with something, and then refactor
> later if we find it's too complicated. Getting the ball rolling is important
> :)
> 
> (In reply to Vicamo Yang from comment #8)
> > iPhone implements only UCS2 and default GSM alphabet for both sending &
> > receiving SMS, while Android implements all for receiving but UCS2, default
> > alphabet (and other alphabets required by local law, usually none) for
> > sending.
> 
> Right. So how about we do what Android does?
That would the best solution :)
Following patch set comes from separating my previous attachment 601916 [details] [diff] [review], and fixes review suggestions.
Attachment #601916 - Attachment is obsolete: true
Attachment #602344 - Flags: review?(philipp)
Attachment #602345 - Flags: review?(philipp)
Attachment #602346 - Flags: review?(philipp)
Attachment #602347 - Flags: review?(philipp)
Comment on attachment 602346 [details] [diff] [review]
Part 3: support sending sms of other alphabets

Review of attachment 602346 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still working on test scripts addition.

::: dom/system/b2g/ril_worker.js
@@ +2179,5 @@
>    /**
> +   * List of tuples of national language identifier pairs.
> +   */
> +  enabledGsmTableTuples: [
> +    [PDU_NL_IDENTIFIER_DEFAULT, PDU_NL_IDENTIFIER_DEFAULT],

Now the default action works as exactly Android does, only default alphabet is enabled. To verify different alphabet tables, one *MUST* manually insert corresponding entries into this array. I was planed to add preference config for default enabled tables, but it seems that I can't reference Components.xxx in this js. The config reading have to reside in RadioInterfaceLayer, and notify GsmPDUHelper by posting some private messages. Don't know if there's a better way to do it yet.
xpconnect does not support worker any more. Beside posting messages, you can also pass the configuration by injecting the value into global namespace of ril worker from SystemWorkerManager.  Just like how they inject postRILMessage().  But,I think posting messages is more clear and easy to understand.
(In reply to Thinker Li [:sinker] from comment #16)
> xpconnect does not support worker any more. Beside posting messages, you can
> also pass the configuration by injecting the value into global namespace of
> ril worker from SystemWorkerManager.  Just like how they inject
> postRILMessage().  But,I think posting messages is more clear and easy to
> understand.

Yeah let's not blow up workers any more, we can do this with control messages. Let's file a follow-up bug for that.
Attachment #602344 - Flags: review?(philipp) → review+
Attachment #602345 - Flags: review?(philipp) → review+
Comment on attachment 602346 [details] [diff] [review]
Part 3: support sending sms of other alphabets

Review of attachment 602346 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! Just two questions below that I'd like to see clarified. r=me with those.

::: dom/system/b2g/ril_worker.js
@@ +2414,5 @@
> +    for (let msgIndex = 0; msgIndex < message.length; msgIndex++) {
> +      let septet = langTable.indexOf(message.charAt(msgIndex));
> +      if (septet == PDU_NL_EXTENDED_ESCAPE) {
> +        continue;
> +      }

Why aren't we adding 2 here? Please explain in a comment.

@@ +2426,5 @@
> +      if (septet == -1) {
> +        return -1;
> +      }
> +
> +      length += 2;

When do we reach this case and why is it length += 2? Is this somehow related to the PDU_NL_EXTENDED_ESCAPE case? Please explain in a comment here.

@@ +2467,5 @@
> +    let needUCS2 = true;
> +    let minUserDataLength = Number.MAX_VALUE;
> +    for (let i = 0; i < this.enabledGsmTableTuples.length; i++) {
> +      let langIndex, langShiftIndex;
> +      [langIndex, langShiftIndex] = this.enabledGsmTableTuples[i];

You can do

  let [langIndex, langShiftIndex] = ...
Attachment #602346 - Flags: review?(philipp) → review+
Comment on attachment 602347 [details] [diff] [review]
Part 4: support receiving sms of other alphabets

Review of attachment 602347 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent. Thanks for splitting these out in separate patches! Makes it a lot easier to review.

Once again just two questions and a few nits. r=me with those addressed.

::: dom/system/b2g/ril_worker.js
@@ +2339,5 @@
> +    let escapeFound = false;
> +    const langTable = PDU_NL_LOCKING_SHIFT_TABLES[langIndex];
> +    const langShiftTable = PDU_NL_SINGLE_SHIFT_TABLES[langShiftIndex];
> +    do {
> +      // read as much as fits in 32bit word

Nit: comment style. Please start them with a capital letter and end them with a period (here and everywhere else.)

@@ +2355,5 @@
> +
> +        if (escapeFound) {
> +          escapeFound = false;
> +          if (septet == PDU_NL_EXTENDED_ESCAPE) {
> +            ret += ' ';

So two escapes after each other make a space?

Nit: double quotes

@@ +2365,5 @@
> +        } else {
> +          ret += langTable[septet];
> +        }
> +      }
> +    } while (byteLength);

Why the do/while? Are we guaranteed to always have at least a 32 bit word? That's at least not obvious to me. And if we are, I think I would still prefer a regular while {} loop for readability. The extra check in the first iteration is negligible.
Attachment #602347 - Flags: review?(philipp) → review+
(In reply to Philipp von Weitershausen [:philikon] from comment #19)
> ::: dom/system/b2g/ril_worker.js
> Nit: comment style. Please start them with a capital letter and end them
> with a period (here and everywhere else.)
> 

Sorry. I've double checked again, but there are still missed lines. 

> @@ +2355,5 @@
> > +
> > +        if (escapeFound) {
> > +          escapeFound = false;
> > +          if (septet == PDU_NL_EXTENDED_ESCAPE) {
> > +            ret += ' ';
> 
> So two escapes after each other make a space?

According to 3GPP TS 23.038, section 6.2.1.1, NOTE 1, "On receipt of this code, a receiving entity shall display a space until another extensiion table is defined." I'll put this into a comment as well.
 
> Nit: double quotes
> 
> @@ +2365,5 @@
> > +        } else {
> > +          ret += langTable[septet];
> > +        }
> > +      }
> > +    } while (byteLength);
> 
> Why the do/while? Are we guaranteed to always have at least a 32 bit word?
> That's at least not obvious to me. And if we are, I think I would still
> prefer a regular while {} loop for readability. The extra check in the first
> iteration is negligible.

1) According to MDN documentation https://developer.mozilla.org/en/JavaScript/Reference/Operators/Bitwise_Operators , "Bitwise operators treat their operands as a sequence of 32 bits (zeros and ones)".

2) A while loop can lead to additional tailing handling code. For example, with paddingBits = 1 and length = 1, which happens at a 6 octets header and 1 septet(char) message received, we have [byteLength, data, dataBits] = [0, 0x??, 1], a while-loop in this case will do nothing and additional handling is therefore required. Besides, we cannot rely on [byteLength, dataBits] == [0, 0], either. A no-header-one-septet message will never meet it. I don't actually understand what do you mean by "The extra check in the first iteration is negligible." Because it does break the loop in previous case(a no-header-one-septet message).
(In reply to Vicamo Yang from comment #20)
> > @@ +2355,5 @@
> > > +
> > > +        if (escapeFound) {
> > > +          escapeFound = false;
> > > +          if (septet == PDU_NL_EXTENDED_ESCAPE) {
> > > +            ret += ' ';
> > 
> > So two escapes after each other make a space?
> 
> According to 3GPP TS 23.038, section 6.2.1.1, NOTE 1, "On receipt of this
> code, a receiving entity shall display a space until another extensiion
> table is defined." I'll put this into a comment as well.

Perfect, thanks!

> > Why the do/while? Are we guaranteed to always have at least a 32 bit word?
> > That's at least not obvious to me. And if we are, I think I would still
> > prefer a regular while {} loop for readability. The extra check in the first
> > iteration is negligible.
> 
> 1) According to MDN documentation
> https://developer.mozilla.org/en/JavaScript/Reference/Operators/
> Bitwise_Operators , "Bitwise operators treat their operands as a sequence of
> 32 bits (zeros and ones)".

Right. That's not what I was concerned about. I was concerned about `readHexOctet()` being called unconditionally in the first iteration, but I realize now that

  let bytesToRead = Math.min(byteLength, dataBits ? 3 : 4)

will ensure we don't actually read anything if `byteLength` is 0.

> 2) A while loop can lead to additional tailing handling code. For example,
> with paddingBits = 1 and length = 1, which happens at a 6 octets header and
> 1 septet(char) message received, we have [byteLength, data, dataBits] = [0,
> 0x??, 1], a while-loop in this case will do nothing and additional handling
> is therefore required.

I see. I guess I don't full understand why byteLength would be 0 in this case.
Accommodate to review suggestions.
Attachment #602346 - Attachment is obsolete: true
Attachment #603155 - Flags: review?(philipp)
Attachment #602347 - Attachment is obsolete: true
Attachment #603157 - Flags: review?(philipp)
Attachment #603155 - Flags: review?(philipp) → review+
Attachment #603157 - Flags: review?(philipp) → review+
Comment on attachment 602345 [details] [diff] [review]
Part 2: add all GSM 7-bit national languages

Review of attachment 602345 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/b2g/ril_consts.js
@@ +399,5 @@
> +   * National Language Identifier: 0x02
> +   * A.3.2 Void
> +   */
> +  // 0123456789ABCDEF
> +    "                "

At writing test/verification scripts for these tables, this void table is a special case that doesn't contains escape/CR/LF. The section A.3.2 in 3GPP TS 23.038 is completely empty and there's no information about whether should this table contain those characters or not. Android leaves an empty string instead. I don't think there will be a valid sms sent with this table, but should I update this patch to include those special characters? This will also help removing special case from test scripts, but nothing is really affected.

@@ +656,5 @@
> +   */
> +  // 0123456789A.....BCDEF
> +    "          \u000c     "
> +  // 0123456789ABCDEF
> +  + "    ^           "

TS 23.038 also provides a template for single shift tables. It contains <escape> at 0x1B, <pagebreak> at 0x0A, and one additional <ctrlchar> at 0x0D. The <escape> is for escape to yet another extension table if defined, while <ctrlchar> is for some undocumented control. The table strings defined in this patch doesn't contains <escape> and <ctrlchar> and should be fixed.
(In reply to Vicamo Yang from comment #24)
> At writing test/verification scripts for these tables, this void table is a
> special case that doesn't contains escape/CR/LF. The section A.3.2 in 3GPP
> TS 23.038 is completely empty and there's no information about whether
> should this table contain those characters or not. Android leaves an empty
> string instead. I don't think there will be a valid sms sent with this
> table, but should I update this patch to include those special characters?

Thanks for looking into this! Please file a follow-up bug. I'd like to land these patches a.s.a.p.
Depends on: 733300
Blocks: 733300
No longer depends on: 733300
No longer blocks: 733300
Depends on: 733300
Attachment #603155 - Attachment is obsolete: true
Attachment #603178 - Flags: review?(philipp)
Attachment #603157 - Attachment is obsolete: true
Attachment #603181 - Flags: review?(philipp)
Attachment #603176 - Flags: review?(philipp)
Major differences are:
1) add PDU_NL_SPACE, PDU_NL_LINE_FEED, PDU_NL_CARRIAGE_RETURN, PDU_NL_PAGE_BREAK, PDU_NL_RESERVED_CONTROL for special characters in ril_consts.js
2) change special char code values in "A.3.2 Void" locking shift table in ril_consts.js
3) change <escape>, <resctrl> code values in all single shift tables in ril_consts.js
4) handle PDU_NL_RESERVED_CONTROL in _calculateLangEncodedLength(), writeStringAsSeptets(), readSeptetsToString() as well
Comment on attachment 603181 [details] [diff] [review]
Part 4: support receiving sms of other alphabets: V3

Review of attachment 603181 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with typo addressed.

::: dom/system/b2g/ril_worker.js
@@ +2621,5 @@
> +      dataAvailable -= 2;
> +
> +      switch (id) {
> +        case PDU_IEI_NATIONAL_LANGUAGE_SINGLE_SHIFT:
> +          let langShfitIndex = this.readHexOctet();

Typo: langShfitIndex

(We need tests for this... but in a different bug.)
Attachment #603181 - Flags: review?(philipp) → review+
Comment on attachment 603178 [details] [diff] [review]
Part 3: support sending sms of other alphabets: V3

Review of attachment 603178 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment clarified.

::: dom/system/b2g/ril_consts.js
@@ +368,5 @@
> +const PDU_IEI_ENHANCED_VOICE_MAIL_INFORMATION          = 0x23;
> +const PDU_IEI_NATIONAL_LANGUAGE_SINGLE_SHIFT           = 0x24;
> +const PDU_IEI_NATIONAL_LANGUAGE_LOCKING_SHIFT          = 0x25;
> +
> +// 7bit alphabet escape character, assigned \uffff

What do you mean by "assigned" here? I think you mean a different word, but I'm not sure which one...

@@ +377,5 @@
> +const PDU_NL_LINE_FEED = 0x0A;
> +const PDU_NL_CARRIAGE_RETURN = 0x0D;
> +
> +// <PageBreak>, <ReservedControl> are only defined in single shift tables.
> +// 7bit alphabet page break character, assigned \u000c.

Ditto.

@@ +379,5 @@
> +
> +// <PageBreak>, <ReservedControl> are only defined in single shift tables.
> +// 7bit alphabet page break character, assigned \u000c.
> +const PDU_NL_PAGE_BREAK = 0x0A;
> +// 7bit alphabet reserved control character, assigned \ufffe.

Ditto.
Attachment #603178 - Flags: review?(philipp) → review+
Attachment #603176 - Flags: review?(philipp) → review+
add comments
Attachment #603178 - Attachment is obsolete: true
Attachment #603202 - Flags: review?(philipp)
fix typo
Attachment #603181 - Attachment is obsolete: true
Attachment #603203 - Flags: review?(philipp)
Depends on: 733331
Comment on attachment 603202 [details] [diff] [review]
Part 3: support sending sms of other alphabets: V4

Perfect, thanks!
Attachment #603202 - Flags: review?(philipp) → review+
Attachment #603203 - Flags: review?(philipp) → review+
You need to log in before you can comment on or make changes to this bug.