Last Comment Bug 712804 - B2G SMS: Support UCS2 encoding
: B2G SMS: Support UCS2 encoding
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla13
Assigned To: Vicamo Yang [:vicamo][:vyang]
:
Mentors:
Depends on:
Blocks: b2g-sms
  Show dependency treegraph
 
Reported: 2011-12-21 15:50 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-02-23 07:32 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
support UCS2 encoding (3.30 KB, patch)
2012-02-23 03:20 PST, Vicamo Yang [:vicamo][:vyang]
philipp: review-
Details | Diff | Review
support UCS2 encoding, V2 (3.38 KB, patch)
2012-02-23 05:40 PST, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2011-12-21 15:50:38 PST

    
Comment 1 Mounir Lamouri (:mounir) 2011-12-22 03:00:08 PST
Just for information, what is "UCS2 formatted SMS"?
Comment 2 Philipp von Weitershausen [:philikon] 2011-12-22 06:37:23 PST
(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] 2011-12-22 06:42:29 PST
This should be pretty straight forward. Contact me for pointers.
Comment 4 Philipp von Weitershausen [:philikon] 2011-12-22 13:20:25 PST
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] 2012-02-15 17:53:45 PST
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] 2012-02-15 18:07:27 PST
(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] 2012-02-23 03:20:53 PST
Created attachment 599936 [details] [diff] [review]
support UCS2 encoding
Comment 8 Philipp von Weitershausen [:philikon] 2012-02-23 03:24:39 PST
Making this bug solely about the UCS2 encoding.
Comment 9 Philipp von Weitershausen [:philikon] 2012-02-23 03:41:13 PST
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] 2012-02-23 05:40:11 PST
Created attachment 599963 [details] [diff] [review]
support UCS2 encoding, V2

Accommodate to review comments, also verified locally.
Comment 11 Philipp von Weitershausen [:philikon] 2012-02-23 06:42:32 PST
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] 2012-02-23 07:03:10 PST
https://hg.mozilla.org/mozilla-central/rev/722e1ed13f9a

Note You need to log in before you can comment on or make changes to this bug.