B2G SMS: Support UCS2 encoding

RESOLVED FIXED in mozilla13

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: philikon, Assigned: vicamo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla13
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Just for information, what is "UCS2 formatted SMS"?
(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.
This should be pretty straight forward. Contact me for pointers.
Whiteboard: [good first bug][lang=js][mentor=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.
Summary: B2G SMS: Support UCS2 formatted SMS (GSM) → B2G SMS: Support other 7bit alphabets and UCS2 encoding
Whiteboard: [good first bug][lang=js][mentor=philikon]
(Assignee)

Updated

5 years ago
Assignee: nobody → vyang
(Assignee)

Comment 5

5 years ago
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?
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 599936 [details] [diff] [review]
support UCS2 encoding
Attachment #599936 - Flags: review?(philipp)
Making this bug solely about the UCS2 encoding.
Summary: B2G SMS: Support other 7bit alphabets and UCS2 encoding → B2G SMS: Support UCS2 encoding
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.
Attachment #599936 - Flags: review?(philipp) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 599963 [details] [diff] [review]
support UCS2 encoding, V2

Accommodate to review comments, also verified locally.
Attachment #599936 - Attachment is obsolete: true
Attachment #599963 - Flags: review?(philipp)
Comment on attachment 599963 [details] [diff] [review]
support UCS2 encoding, V2

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

Nice!
Attachment #599963 - Flags: review?(philipp) → review+
https://hg.mozilla.org/mozilla-central/rev/722e1ed13f9a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.