Philipp von Weitershausen [:philikon] Reporter
	Description • 13 years ago

+++ 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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 1 • 13 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 2 • 13 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 3 • 13 years ago

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"

	Philipp von Weitershausen [:philikon] Reporter
	Comment 4 • 13 years ago

(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.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 5 • 13 years ago

(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.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 6 • 13 years ago

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();
    ...

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 7 • 13 years ago

(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!

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 8 • 13 years ago

(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.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 9 • 13 years ago

(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?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 10 • 13 years ago

(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 :)

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 11 • 13 years ago

Following patch set comes from separating my previous attachment 601916 [details] [diff] [review], and fixes review suggestions.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 15 • 13 years ago

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.

	Thinker Li [:sinker]
	Comment 16 • 13 years ago

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.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 17 • 13 years ago

(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.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 18 • 13 years ago

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] = ...

	Philipp von Weitershausen [:philikon] Reporter
	Comment 19 • 13 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 20 • 13 years ago

(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).

	Philipp von Weitershausen [:philikon] Reporter
	Comment 21 • 13 years ago

(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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 22 • 13 years ago

Accommodate to review suggestions.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 24 • 13 years ago

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.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 25 • 13 years ago

(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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 29 • 13 years ago

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

	Philipp von Weitershausen [:philikon] Reporter
	Comment 30 • 13 years ago

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.)

	Philipp von Weitershausen [:philikon] Reporter
	Comment 31 • 13 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 32 • 13 years ago

add comments

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 33 • 13 years ago

fix typo

	Philipp von Weitershausen [:philikon] Reporter
	Comment 34 • 13 years ago

Comment on attachment 603202 [details] [diff] [review]
Part 3: support sending sms of other alphabets: V4

Perfect, thanks!

	Philipp von Weitershausen [:philikon] Reporter
	Comment 35 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/7392246c7a3a
https://hg.mozilla.org/mozilla-central/rev/b33e731035a0
https://hg.mozilla.org/mozilla-central/rev/89ccb9f4d5c1
https://hg.mozilla.org/mozilla-central/rev/00b93cf47142