712804 - B2G SMS: Support UCS2 encoding

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Mounir Lamouri (:mounir)]

Just for information, what is "UCS2 formatted SMS"?

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #1)
> Just for information, what is "UCS2 formatted SMS"?

The SMS payload can be either encoded in a 7 bit alphabet that's represented as 160 septets in 140 octets, or it can be unicode represented as UCS2.

Comment 3

[Philipp von Weitershausen [:philikon]]

This should be pretty straight forward. Contact me for pointers.

Comment 4

[Philipp von Weitershausen [:philikon]]

Actually, thinking about this some more, this isn't as trivial. We need to (a) detect which encoding is the best one for a given string and then (b) implement those various encodings. Right now we only support the default 7bit alphabet.

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

I found Android pre-processes unsolicited response with responseInts(), responseString() in RIL.java. The whole sms message itself can be first read in as an utf-16 string in SmsMessage.newFromCMT(), then IccUtils.hexStringToBytes(), and finally goes SmsMessage.parsePdu(). The current behavior of B2G GsmPDUHelper mixes all these up with readHexOctet(), readHexNibble() and seems not very clear and easy understand of underlying data format. Is there a plan to rewrite it?

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang from comment #5)
> I found Android pre-processes unsolicited response with responseInts(),
> responseString() in RIL.java. The whole sms message itself can be first read
> in as an utf-16 string in SmsMessage.newFromCMT(), then
> IccUtils.hexStringToBytes(), and finally goes SmsMessage.parsePdu(). The
> current behavior of B2G GsmPDUHelper mixes all these up with readHexOctet(),
> readHexNibble() and seems not very clear and easy understand of underlying
> data format. Is there a plan to rewrite it?

No. The ril_worker code has been geared to avoid as many object creations as possible for performance (GC) reasons. I don't see a need to rewrite it. If something is unclear, we should add documentation. I tried to document every function in GsmPDUHelper.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: support UCS2 encoding

Comment 8

[Philipp von Weitershausen [:philikon]]

Making this bug solely about the UCS2 encoding.

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 599936 [details] [diff] [review]
support UCS2 encoding

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

::: dom/system/b2g/ril_worker.js
@@ +2344,5 @@
>     *
>     * @return a string.
>     */
>    readUCS2String: function readUCS2String(length) {
> +    let array = new Uint16Array(length / 2);

Please do this without allocating a new array object.

@@ +2348,5 @@
> +    let array = new Uint16Array(length / 2);
> +    let ii = 0;
> +    while (ii < array.length) {
> +      array[ii++] = (this.readHexOctet() << 8) | this.readHexOctet();
> +    }

`ii`? That's a pretty uncommon loop variable name. Please use `i`.

Also, for consistency's sake, please make this a for loop.

@@ +2351,5 @@
> +      array[ii++] = (this.readHexOctet() << 8) | this.readHexOctet();
> +    }
> +
> +    if (DEBUG) debug("Read UCS2 array: " + Array.slice(array));
> +    let str = String.fromCharCode.apply(null, Array.slice(array));

Array.slice() allocates yet another object!

Please just build up the string as you read the hex octets in the `for` loop.

@@ +2391,5 @@
> +    let ii = 0;
> +    for (; ii < options.body.length; ++ii) {
> +      //TODO: support language tables
> +      if (options.body.charCodeAt(ii) >= 128) {
> +        break;

I would prefer to set a boolean flag here. instead of relying on the fact that we got through the whole string without bailing out. Then we also don't have to leak the loop variable out of the `for` loop (and again, please just call it `i`):

  let needUCS2 = false;
  for (let i = 0; i < options.body.length; i++) {
    if (options.body.charCodeAt(ii) >= 128) {
      needUCS2 = true;
    }
  }

  if (needUCS2) {
    ...
  }

@@ +2401,5 @@
> +      options.bodyLengthInOctets = Math.ceil(options.body.length * 7 / 8);
> +    } else {
> +      options.dcs = PDU_DCS_MSG_CODING_16BITS_ALPHABET;
> +      options.bodyLengthInOctets = options.body.length * 2;
> +      //TODO: support multipart SMS

Please move this TODO comment at the top of this function, and refer to bug 712933 in the comment.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: support UCS2 encoding, V2

Accommodate to review comments, also verified locally.

Comment 11

[Philipp von Weitershausen [:philikon]]

Comment on attachment 599963 [details] [diff] [review]
support UCS2 encoding, V2

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

Nice!

Comment 12

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/722e1ed13f9a