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

Long messages are split up into multiple SMS by the sender. We should stitch them back together on the receiving end before we notify the DOM. Probably best to do this right in the RIL worker.

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

This should also include outgoing SMS. The splitting logic needs to live in nsITelephone (or whatever it will be called in the future, see bug 711671) because it already needs to implement getNumberOfMessagesForText().

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

For outgoing SMS, Android pre-processes each message in Mms app and divides it into several parts when necessary. In fact, Android SmsManager.sendTextMessage() is never referenced but in cts/, tests/, or samples/. All outgoing messages goes SmsManager.sendMultipartTextMessage() instead. Besides, message dividing code might also depends on bug 712804 to determine the boundary properly.

For incoming PDUs, user data header parsing should be implemented in https://mxr.mozilla.org/mozilla-central/source/dom/system/b2g/ril_worker.js#2485 . PDU reassembling takes place in SMSDispatcher.processMessagePart() in Android's case, but there seems no corresponding role in WebSMS.

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

In nearly all cases Android's code organization is going to be a bad or at least misleading example. Gecko's system of worker + main thread component has very different design constraints and means that the code is going to be organized entirely differently.

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

Comment on attachment 603660 [details] [diff] [review]
WIP V1

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

::: dom/system/b2g/ril_worker.js
@@ +2583,5 @@
>          headerLen += 3; // IEI + len + langShiftIndex
>        }
>  
>        // Calculate full user data length, note the extra byte is for header len
> +      let headerSeptets = Math.ceil((headerLen ? headerLen + 1 : 0) * 8 / 7);

fix an error described in https://bugzilla.mozilla.org/show_bug.cgi?id=733300#c4

@@ -2582,5 @@
> -
> -      if (userDataLength <= options.body.length) {
> -        // Found minimum user data length already
> -        return;
> -      }

removed as described in https://bugzilla.mozilla.org/show_bug.cgi?id=733300#c4

@@ +2746,5 @@
>    writeUserDataHeader: function writeUserDataHeader(options) {
>      this.writeHexOctet(options.userDataHeaderLength);
>  
> +    if (typeof options.concatRef != "undefined") {
> +      if (options.concatRef >= 256) {

branch condition differs from that in calculateUserDataLength()

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

Comment on attachment 604375 [details] [diff] [review]
Part 3: fix getNumberOfMessagesForText()

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

These patches invalidate test scripts committed in bug 733300, should update them as well.

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +622,5 @@
>     */
>    _calculateUserDataLength: function _calculateUserDataLength(message) {
>      let options = this._calculateUserDataLength7Bit(message);
> +    if (typeof options == "undefined") {
> +      options = this._calculateUserDataLengthUCS2(message);

these changes should go into part2

@@ +627,4 @@
>      }
>  
> +    debug("_calculateUserDataLength: " + JSON.stringify(options));
> +    return options;

these changes should go into part2

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

fix comment 9: these changes should go into part2

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

Comment on attachment 604388 [details] [diff] [review]
Part 4: Send divided multipart SMS : WIP

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

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +752,5 @@
>      options.processId = processId;
> +    if (options.multipart) {
> +      options.concatRef = this.getValidConcatRef();
> +      options.concatMax = options.parts.length;
> +      for (let i = 0; i < parts.length; i++) {

should be options.parts.length

::: dom/system/b2g/ril_worker.js
@@ +2177,5 @@
>      // object may get sent eventually.)
>      options.SMSC = this.SMSC;
>  
>      //TODO: verify values on 'options'
> +    for (part in options.parts) {

error: assignment to undeclared variable part

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

all parts sent (to myself), but only one received.

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

Seems to be something wrong in receiving, my X10 mini pro received all parts and automatically stitch them together :(

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

update test scripts

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

update test scripts

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

update test scripts

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

Comment on attachment 604877 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V2

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

::: dom/system/b2g/RadioInterfaceLayer.manifest
@@ +1,2 @@
>  component {2d831c8d-6017-435b-a80c-e5d422810cea} RadioInterfaceLayer.js
> +contract @mozilla.org/telephony/radiointerfacelayer;1 {2d831c8d-6017-435b-a80c-e5d422810cea}

accidentally committed, should be removed

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

Rebase to r88812. I got build failure http://pastebin.com/Nymch0JK with changes r88813 and r88814, which were landed in bug 734057.

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

1) rebase
2) s/multipart/concatMax/
3) s/divide/fragment/
4) let _fragmentText() returns options object directly for further operations

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

1) rebase
2) move handling code into RadioInterfaceLayer, keep worker as clean as possible

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

refactor GsmPDUHelper.readUserData() to accept one additional object, populate it instead of returning message body only.

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

Comment on attachment 605373 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V3

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

Looks good in general, just r- on how you're dealing with RadioInterfaceLayer.js in the tests.

::: dom/system/b2g/Makefile.in
@@ +53,5 @@
>    net_worker.js \
>    ril_consts.js \
>    ril_worker.js \
>    systemlibs.js \
> +  RadioInterfaceLayer.js \

Nope, we don't want to do that just because you want to load RadioInterfaceLayer.js in tests. See below for my suggested solution.

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +21,5 @@
>   * Contributor(s):
>   *   Ben Turner <bent.mozilla@gmail.com> (Original Author)
>   *   Philipp von Weitershausen <philipp@weitershausen.de>
>   *   Sinker Li <thinker@codemud.net>
> + *   Vicamo Yang <vicamo@gmail.com>

Instead of modifying the old license header, you can just replace it with the new one: http://www.mozilla.org/MPL/headers/

::: dom/system/b2g/ril_worker.js
@@ +21,5 @@
>   * Contributor(s):
>   *   Kyle Machulis <kyle@nonpolynomial.com>
>   *   Philipp von Weitershausen <philipp@weitershausen.de>
>   *   Fernando Jimenez <ferjmoreno@gmail.com>
> + *   Vicamo Yang <vicamo@gmail.com>

Ditto.

::: dom/system/b2g/tests/header_helpers.js
@@ +126,5 @@
> +    },
> +  };
> +
> +  subscriptLoader.loadSubScript("resource://gre/modules/RadioInterfaceLayer.js", ril_ns);
> +  return new ril_ns.RadioInterfaceLayer();

I would prefer if we didn't actually create a new instance of this but instead just worked with the singleton. We can acquire that singleton via XPCOM:

  Cc["@mozilla.org/telephony/system-worker-manager;1"]
    .getService(Ci.nsIInterfaceRequestor)
    .getInterface(Ci.nsIRadioInterfaceLayer);

In any case, making RadioInterfaceLayer.js a module is out of the question. Worst case scenario would be to load it from resource://gre/components/RadioInterfaceLayer.js, but I'd really like to avoid that.

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

Comment on attachment 604878 [details] [diff] [review]
Part 2: refactor calculateUserDataLength() - V3

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

Just two small nits, r=me with those!

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +502,2 @@
>      //TODO: support multipart SMS, see bug 712933
> +    let options;

I would prefer an explicit `null` for the case when we 7bit does not apply, so please make this `let options = null;`

@@ +586,5 @@
> +   *        Table index used for escaped 7-bit encoded character lookup.
> +   */
> +  _calculateUserDataLength: function _calculateUserDataLength(message) {
> +    let options = this._calculateUserDataLength7Bit(message);
> +    if (typeof options == "undefined") {

if (!options)

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

Comment on attachment 604878 [details] [diff] [review]
Part 2: refactor calculateUserDataLength() - V3

>+   * @return an options object with attributes `dcs`, `userDataHeaderLength`,
>+   *         `encodedBodyLength`, `langIndex`, `langShiftIndex` set.

Oh, and perhaps you should add: "... or null" :)

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

(In reply to Philipp von Weitershausen [:philikon] from comment #25)
> Comment on attachment 605373 [details] [diff] [review]
> Part 1: Move GsmPDUHelper.calculateUserDataLength() to
> RadioInterfaceLayer.js : V3
> 
> Review of attachment 605373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good in general, just r- on how you're dealing with
> RadioInterfaceLayer.js in the tests.
> 
> ::: dom/system/b2g/RadioInterfaceLayer.js
> @@ +21,5 @@
> >   * Contributor(s):
> >   *   Ben Turner <bent.mozilla@gmail.com> (Original Author)
> >   *   Philipp von Weitershausen <philipp@weitershausen.de>
> >   *   Sinker Li <thinker@codemud.net>
> > + *   Vicamo Yang <vicamo@gmail.com>
> 
> Instead of modifying the old license header, you can just replace it with
> the new one: http://www.mozilla.org/MPL/headers/

I think I'd better left it for another bug that updates all old license headers. I'll remove these changes from further patches.

> ::: dom/system/b2g/tests/header_helpers.js
> @@ +126,5 @@
> > +    },
> > +  };
> > +
> > +  subscriptLoader.loadSubScript("resource://gre/modules/RadioInterfaceLayer.js", ril_ns);
> > +  return new ril_ns.RadioInterfaceLayer();
> 
> I would prefer if we didn't actually create a new instance of this but
> instead just worked with the singleton. We can acquire that singleton via
> XPCOM:
> 
>   Cc["@mozilla.org/telephony/system-worker-manager;1"]
>     .getService(Ci.nsIInterfaceRequestor)
>     .getInterface(Ci.nsIRadioInterfaceLayer);

Acquiring nsIRadioInterfaceLayer brings exception "cannot open base URI" in ChromeWorker. Besides, I will not be able to access non-interface-defined functions inside RadioInterfaceLayer.

> In any case, making RadioInterfaceLayer.js a module is out of the question.
> Worst case scenario would be to load it from
> resource://gre/components/RadioInterfaceLayer.js, but I'd really like to
> avoid that.

Thanks. I'll give it a try later.

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

(In reply to Vicamo Yang from comment #28)
> > Instead of modifying the old license header, you can just replace it with
> > the new one: http://www.mozilla.org/MPL/headers/
> 
> I think I'd better left it for another bug that updates all old license
> headers. I'll remove these changes from further patches.

Fair enough. I think an automatic conversion of all remaining files is planned, so we don't have to worry about it.

> Acquiring nsIRadioInterfaceLayer brings exception "cannot open base URI" in
> ChromeWorker.

Weird.

> Besides, I will not be able to access non-interface-defined
> functions inside RadioInterfaceLayer.

Oh, that's a good point. There's a way around that, but sure, let's go with resource://gre/components/RadioInterfaceLayer.js for now.

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

Comment on attachment 605376 [details] [diff] [review]
Part 3: fix getNumberOfMessagesForText() : V4

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

r- mostly because of the performance implications in getNumberOfMessagesForText and _fragmentText(). Rest looks goods, nice work!

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +542,5 @@
>  
>    /**
> +   * Use 16-bit reference number for concatenated outgoint messages
> +   */
> +  concatUse16bitRef: false,

Under which circumstances will this be set to true? (Also spelling for "outgoing" and missing period at the end.)

@@ +730,5 @@
>     *        Table index used for escaped 7-bit encoded character lookup.
> +   * @param concatMax
> +   *        Max sequence number of a multi-part messages, or 1 for single one.
> +   *        This number might not be accurate for a multi-part message until
> +   *        it's processed by #_fragmentText() again.

Variables and attributes should be nouns and "concat" is a verb. Let's call it "maxNumFragments". (See my review for Part 4 regarding naming of "concatSeq" and "concatRef".)

@@ +855,5 @@
> +    } else if (options.concatMax == 1) {
> +      options.parts = [{
> +        body: text,
> +        encodedBodyLength: options.encodedBodyLength,
> +      }];

Seems like in these two cases we don't have to create the `parts` array at all. Just make it `null` and check for its existence when you compute e.g. concatMax, falling back to options.body, options.encodedBodyLength, etc.

@@ +875,2 @@
>    getNumberOfMessagesForText: function getNumberOfMessagesForText(text) {
> +    return this._fragmentText(text).concatMax;

One thing to remember about this method is that it might be called extremely frequently by the UI, e.g. on every keystroke. In my opinion we should do the least amount of work necessary here. In my opinion, making a good approximation with _calculateUserDataLength7Bit() (what you call "concatMax" right now) is absolutely ok, and it would save us from doing all the work in _fragmentText().

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

Comment on attachment 605376 [details] [diff] [review]
Part 3: fix getNumberOfMessagesForText() : V4

Err, I meant r-

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

Comment on attachment 605377 [details] [diff] [review]
Part 4: Support sending multipart SMS : V3

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

Sweet! Only a few nits regarding code clarity. r=me with those!

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +434,5 @@
> +        this.worker.postMessage(message);
> +        return;
> +      }
> +
> +      message.body = message.fullBody;

Instead of fiddling with body (it changes meaning over time... initially it's the full body, then it's the individual parts' bodies, and then it's the full body again), why don't you change the code below (gSmsDatabaseService.saveSentMessage, gSmsService.createSmsMessage, etc.) to use message.fullBody.

@@ +565,5 @@
>    /**
> +   * Get valid SMS concatenation reference number.
> +   *
> +   * SMS concatenation reference number is valid in 1 ~ 255 for 8 bit width and
> +   * 1 ~ 65535 for 16 bit width.

I'm not familiar with using tilde for this. I typically see 1..255 and 1..65535. But anyway, this comment is pretty superfluous. I think the reader will understand what 8 bit and 16 bit means :)

@@ +574,5 @@
> +
> +    this._concatRef %= (this.concatUse16bitRef ? 65535 : 255);
> +
> +    return ref + 1;
> +  },

I suggest calling this attribute "multipartMessageRef" because it's valid for all parts of a multipart message, right? (Please also adjust the naming of "concatUse16bitRef" accordingly. I suggest "multipartMessageRef16bit" or something like that.)

@@ +919,5 @@
> +
> +    this._fragmentText(message, options);
> +    if (options.concatMax > 1) {
> +      options.concatRef = this.concatRef;
> +      options.concatSeq = 1;

I think something like "nextFragmentIndex" would be a clearer name for this (it's not a sequence, and "concat" is a verb.)

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

Comment on attachment 605378 [details] [diff] [review]
Part 5: refactor GsmPDUHelper readUserData()

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

r=me with nits addressed.

::: dom/system/b2g/ril_worker.js
@@ +2795,5 @@
>    /**
>     * User data can be 7 bit (default alphabet) data, 8 bit data, or 16 bit
>     * (UCS2) data.
>     */
> +  readUserData: function readUserData(msg, length, codingScheme, hasHeader) {

At this point it might be good to document what the parameters are, using a JavaDoc-style comment (@param, etc.) and what readUserData() will do to the `msg` object.

@@ +2872,1 @@
>      }

An explicit `msg.body = null` at the top of the `switch (encoding)` block might be nice for those cases where we don't actually set `msg.body`.

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

Comment on attachment 605379 [details] [diff] [review]
Part 6: Support receiving multipart SMS

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

r=me with nits addressed.

All in all, very nice work!

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +410,5 @@
> +   * Hash map for received multipart sms fragments. Messages are hashed with
> +   * its sender address and concatenation reference number. Three additional
> +   * attributes `concatMax`, `receivedParts`, `parts` are inserted.
> +   */
> +  _receivedSmsFragmentsMap: {},

This will create a single `_receviedSmsFragmentsMap` attribute object that is shared between all instances of RadioInterfaceLayer. Now, this should not be a big issue normally since RadioInterfaceLayer is a singleton. However, (a) I think it's bad form and (b) in your tests you're creating many instances of it.

Please create this as a `null` attribute and initialize it in the constructor.

@@ +428,5 @@
> +    let hash = message.sender + ":" + message.header.concatRef;
> +    let seq = message.header.concatSeq;
> +
> +    let options = this._receivedSmsFragmentsMap[hash];
> +    if (typeof options == "undefined") {

if (!options)

@@ +435,5 @@
> +
> +      options.concatMax = message.header.concatMax;
> +      options.receivedParts = 0;
> +      options.parts = [];
> +    } else if (typeof options.parts[seq] != "undefined") {

Ditto: if (options.parts[seq])

@@ +446,5 @@
> +    options.parts[seq] = message.body;
> +    options.receivedParts++;
> +    if (options.receivedParts < options.concatMax) {
> +      debug("Got " + seq + "th part of multipart SMS: "
> +            + JSON.stringify(options));

Nit: simply adding "th" to the end of a number doesn't work so well with 1, 2, and 3. ;) I suggest rephrasing to something like "Got part no. " + seq + " of multipart SMS".

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

Comment on attachment 605373 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V3

Gah, I also meant r- on this one.

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

Comment on attachment 605377 [details] [diff] [review]
Part 4: Support sending multipart SMS : V3

A thought that just crossed my mind: in your current implementation, we loop back and forth between the main thread and the RIL worker until all parts of the SMS are sent. It would be nicer if we could avoid that. Basically don't notify "sms-sent" in the ril_worker.js until all parts have been sent.

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

remove contributor change

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

Comment on attachment 605704 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V4

review- due to RadioInterfaceLayer loading in test scripts is yet fixed

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

(In reply to Philipp von Weitershausen [:philikon] from comment #30)
> Comment on attachment 605376 [details] [diff] [review]
> Part 3: fix getNumberOfMessagesForText() : V4
> 
> Review of attachment 605376 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- mostly because of the performance implications in
> getNumberOfMessagesForText and _fragmentText(). Rest looks goods, nice work!
> 
> ::: dom/system/b2g/RadioInterfaceLayer.js
> @@ +542,5 @@
> >  
> >    /**
> > +   * Use 16-bit reference number for concatenated outgoint messages
> > +   */
> > +  concatUse16bitRef: false,
> 
> Under which circumstances will this be set to true? (Also spelling for
> "outgoing" and missing period at the end.)

No. It seems to be an implementation decision, won't change at runtime.

> @@ +730,5 @@
> > +   * @param concatMax
> 
> Variables and attributes should be nouns and "concat" is a verb. Let's call
> it "maxNumFragments". (See my review for Part 4 regarding naming of
> "concatSeq" and "concatRef".)

I also want to rename all "parts" to "fragments" for an unified naming.

> @@ +855,5 @@
> > +    } else if (options.concatMax == 1) {
> > +      options.parts = [{
> > +        body: text,
> > +        encodedBodyLength: options.encodedBodyLength,
> > +      }];
> 
> Seems like in these two cases we don't have to create the `parts` array at
> all. Just make it `null` and check for its existence when you compute e.g.
> concatMax, falling back to options.body, options.encodedBodyLength, etc.

This will break the consistency of parts array semantics, but I don't have strong opinion about it.

> @@ +875,2 @@
> >    getNumberOfMessagesForText: function getNumberOfMessagesForText(text) {
> > +    return this._fragmentText(text).concatMax;
> 
> One thing to remember about this method is that it might be called extremely
> frequently by the UI, e.g. on every keystroke. In my opinion we should do
> the least amount of work necessary here. In my opinion, making a good
> approximation with _calculateUserDataLength7Bit() (what you call "concatMax"
> right now) is absolutely ok, and it would save us from doing all the work in
> _fragmentText().

I'm well aware of it, and had verify this patch with a modified sms js app that utilizes this API. Before discussing whether should this function return an approximated value, there might be more information in need than it is now for real SMS UI design. Let's say an ordinary SMS app might have some of following numbers on screen:

1) Total fragments to send: As what this API originally meants.
2) Chars available per fragment: Depending on dcs, this might be 140/70. It might also vary with concatenation involved.
3) Chars already input in current fragment.
4) Chars remains available in current fragment.

iPhone gets "4)/2)", while Android gets "4)/1)". Both need additional information about the length of the last fragment. And, of course, if an UCS2-encoding char is appended to an all ASCII text, Android will updates both numbers with correct ones.

I also noticed that this API is already written on https://wiki.mozilla.org/WebAPI/WebSMS . Is it a proposal or already set in stone? We may return approximated value from `concatMax` in this patch, but I think sms app will need more than that.

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

(In reply to Philipp von Weitershausen [:philikon] from comment #32)
> Comment on attachment 605377 [details] [diff] [review]
> Part 4: Support sending multipart SMS : V3
> 
> Review of attachment 605377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sweet! Only a few nits regarding code clarity. r=me with those!
> 
> ::: dom/system/b2g/RadioInterfaceLayer.js
> @@ +434,5 @@
> > +        this.worker.postMessage(message);
> > +        return;
> > +      }
> > +
> > +      message.body = message.fullBody;
> 
> Instead of fiddling with body (it changes meaning over time... initially
> it's the full body, then it's the individual parts' bodies, and then it's
> the full body again), why don't you change the code below
> (gSmsDatabaseService.saveSentMessage, gSmsService.createSmsMessage, etc.) to
> use message.fullBody.

1) `fullBody` might not be available for single part SMS in this patch.
2) GsmPDUHelper references to `body`.
3) In comment #30, I should fallback to `body` for single part SMS.
hmmmm.....

> @@ +565,5 @@
> >    /**
> > +   * Get valid SMS concatenation reference number.
> > +   *
> > +   * SMS concatenation reference number is valid in 1 ~ 255 for 8 bit width and
> > +   * 1 ~ 65535 for 16 bit width.
> 
> I'm not familiar with using tilde for this.

Maybe `export LC_CTYPE=zh_TW` will solve the difference :)

> I typically see 1..255 and
> 1..65535. But anyway, this comment is pretty superfluous. I think the reader
> will understand what 8 bit and 16 bit means :)

ok.

> @@ +574,5 @@
> > +
> > +    this._concatRef %= (this.concatUse16bitRef ? 65535 : 255);
> > +
> > +    return ref + 1;
> > +  },
> 
> I suggest calling this attribute "multipartMessageRef" because it's valid
> for all parts of a multipart message, right? (Please also adjust the naming
> of "concatUse16bitRef" accordingly. I suggest "multipartMessageRef16bit" or
> something like that.)

The terminologies used in 3GPP 23.040 section 9.2.3.24.1 is `concatenation`, `segment`, `concatenated short message reference number`, `sequence number of the current short message`, `maximum number of short messages in the concatenated short message`. I would like to rename them as `concatenatedMessageRef16Bit`, `concatenatedMessageRef`, `concatenatedMessageMaxSeq`, `concatenatedMessageSeq`.

> @@ +919,5 @@
> > +
> > +    this._fragmentText(message, options);
> > +    if (options.concatMax > 1) {
> > +      options.concatRef = this.concatRef;
> > +      options.concatSeq = 1;
> 
> I think something like "nextFragmentIndex" would be a clearer name for this
> (it's not a sequence, and "concat" is a verb.)

In fact, the term `fragment` is never used throughout the spec and is chosen for verb instead of `segmentalize`. I would like to rename all method names with `fragment` and variable names with `segment`, which is used in the spec.

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

(In reply to Vicamo Yang from comment #39)
> (In reply to Philipp von Weitershausen [:philikon] from comment #30)
> > Comment on attachment 605376 [details] [diff] [review]
> > Part 3: fix getNumberOfMessagesForText() : V4
> > ::: dom/system/b2g/RadioInterfaceLayer.js
> > Variables and attributes should be nouns and "concat" is a verb. Let's call
> > it "maxNumFragments". (See my review for Part 4 regarding naming of
> > "concatSeq" and "concatRef".)
> 
> I also want to rename all "parts" to "fragments" for an unified naming.

I'll use `segments` as said in comment #40.

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

(In reply to Vicamo Yang from comment #39)
> > >    /**
> > > +   * Use 16-bit reference number for concatenated outgoint messages
> > > +   */
> > > +  concatUse16bitRef: false,
> > 
> > Under which circumstances will this be set to true? (Also spelling for
> > "outgoing" and missing period at the end.)
> 
> No. It seems to be an implementation decision, won't change at runtime.

Ok. My question still remains, I guess: under which circumstances would we set this to true ever? In any case, this flag seems like something that should be Gecko pref. We can deal with that in a follow-up bug, though.

> > @@ +855,5 @@
> > > +    } else if (options.concatMax == 1) {
> > > +      options.parts = [{
> > > +        body: text,
> > > +        encodedBodyLength: options.encodedBodyLength,
> > > +      }];
> > 
> > Seems like in these two cases we don't have to create the `parts` array at
> > all. Just make it `null` and check for its existence when you compute e.g.
> > concatMax, falling back to options.body, options.encodedBodyLength, etc.
> 
> This will break the consistency of parts array semantics, but I don't have
> strong opinion about it.

The way I see it, a 'null' value is always an acceptable value when something may or may not exist. I would like to avoid creating tons of objects in the common case (I bet most SMS fit in just one segment.)

> I also noticed that this API is already written on
> https://wiki.mozilla.org/WebAPI/WebSMS . Is it a proposal or already set in
> stone? We may return approximated value from `concatMax` in this patch, but
> I think sms app will need more than that.

I agree with you. These are very good points. I don't think the WebSMS API is set in stone yet. Having the information (1) through (4) above, or at least a large subset of it, would be very useful. It should be up to the SMS app to decide which information to present. Could you please bring these points up on the dev-webapi mailinglist? Jonas and Mounir should be able to give us some input there.

(In reply to Vicamo Yang from comment #41)
> > > Variables and attributes should be nouns and "concat" is a verb. Let's call
> > > it "maxNumFragments". (See my review for Part 4 regarding naming of
> > > "concatSeq" and "concatRef".)
> > 
> > I also want to rename all "parts" to "fragments" for an unified naming.
> 
> I'll use `segments` as said in comment #40.

Perfect!

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

1) review comment #26, comment #27
2) assign property `body` when calculateUserDataLength() is done

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

1) add bug 733331 link for `enabledGsmTableTuples` and `segmentRef16Bit`, see comment #30
2) rename s/concatMax/segmentMaxSeq/, s/fragments/segments/, s/concatUse16bitRef/segmentRef16Bit/, s/fragmentSeptets/segmentSeptets/, s/fragmentChars/segmentChars/, s/parts/segments/, s/multipart/segmentMaxSeq/
3) review comment #30, remove array instanciation when no concatenation is needed.
4) refer to options.body at comment #43 item 2 did
5) update test scripts

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

1) review comment #32, multiple fixes
2) review comment #36, move sending queue logic into ril_worker.js
3) rename s/_concatRef/_segmentRef/, s/concatRef/nextSegmentRef/, etc.
4) now _calculateUserDataLength() inserts properties `fullBody` and `encodedFullBodyLength`.
5) RadioInterfaceLayer.sendSMS() also assigns `segmentRef16Bit` as yet another property of `options`, so GsmPDUHelper shares the same config with RIL and generates exactly expected header indicator elements.

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

review comment #33

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

1) move receiving queue logic into ril_worker.js
2) rename s/_receivedSmsFragmentsMap/_receivedSmsSegmentsMap/, s/_processMultipartSmsFragment/_handleReceivedSmsSegment/, s/receivedParts/receivedSegments/, s/parts/segments/
3) review comment #34. Note that the first part concerning shared instance of _handleReceivedSmsSegment is no longer needed in worker.
4) use fullBody if appropriate.

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

review comment #25, load RadioInterfaceLayer as a js component

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

TODO(?): comment #39 raise another discussion about getNumberOfMessagesForText() API, so its implementation remains the same as attachment 605376 [details] [diff] [review].

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

Comment on attachment 606200 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V5

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

Thanks for updating the patch! I'm going to clear the review flag for the question below.

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +928,5 @@
>  
>  const NSGetFactory = XPCOMUtils.generateNSGetFactory([RadioInterfaceLayer]);
>  
> +let EXPORTED_SYMBOLS = [ "RadioInterfaceLayer" ];
> +

I would really like to avoid this. Why can't you use the subscript loader in header_helpers.js?

Please either (a) use the subscript loader to load this file in the tests (this is preferred!) or (b) add a comment here why we're making this XPCOM module look like a JSM as well (so that we can load it in tests.)

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

Comment on attachment 606180 [details] [diff] [review]
Part 3: fix getNumberOfMessagesForText() : V5

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

r=me with two small nits addressed. Thanks for starting the discussion on improving teh getNumberOfMessagesForText API!

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +855,5 @@
> +   *
> +   * @return Populated options object.
> +   */
> +  _fragmentText: function _fragmentText(text, options) {
> +    if (typeof options == "undefined") {

if (!options)

@@ +860,5 @@
> +      options = this._calculateUserDataLength(text);
> +    }
> +
> +    if (options.segmentMaxSeq <= 1) {
> +      return options;

To be explicit I think it would be good to add

  options.segments = null;

before we return.

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

(In reply to Vicamo Yang [:vicamo] from comment #40)
> (In reply to Philipp von Weitershausen [:philikon] from comment #32)
> > Instead of fiddling with body (it changes meaning over time... initially
> > it's the full body, then it's the individual parts' bodies, and then it's
> > the full body again), why don't you change the code below
> > (gSmsDatabaseService.saveSentMessage, gSmsService.createSmsMessage, etc.) to
> > use message.fullBody.
> 
> 1) `fullBody` might not be available for single part SMS in this patch.

It can easily be a reference to 'body'.

> 2) GsmPDUHelper references to `body`.

We can change it :)

> 3) In comment #30, I should fallback to `body` for single part SMS.
> hmmmm.....

Yep. I see your latest patch addresses all those points already, so thanks! :)

> The terminologies used in 3GPP 23.040 section 9.2.3.24.1 is `concatenation`,
> `segment`, `concatenated short message reference number`, `sequence number
> of the current short message`, `maximum number of short messages in the
> concatenated short message`. I would like to rename them as
> `concatenatedMessageRef16Bit`, `concatenatedMessageRef`,
> `concatenatedMessageMaxSeq`, `concatenatedMessageSeq`.

Sounds good to me.

> > I think something like "nextFragmentIndex" would be a clearer name for this
> > (it's not a sequence, and "concat" is a verb.)
> 
> In fact, the term `fragment` is never used throughout the spec and is chosen
> for verb instead of `segmentalize`. I would like to rename all method names
> with `fragment` and variable names with `segment`, which is used in the spec.

Sounds good, too. <3 spec

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

Comment on attachment 606186 [details] [diff] [review]
Part 6: Support receiving multipart SMS : V2

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

Fantastic work, as usual. I apologize in advance for breaking your patches a little bit in bug 725002, though it should be relatively easy to resolve the conflicts.

Please rebase on the latest gecko, address the remaining nits, including the ones below, and then we can land this awesome thing!

::: dom/system/b2g/ril_worker.js
@@ +1514,5 @@
> +   *
> +   * @return null for handled segments, and an object containing full message
> +   *         body once all segments are received.
> +   */
> +  _handleReceivedSmsSegment: function _handleReceivedSmsSegment(original) {

For consistency with the work in bug 725002, can you name this method _processReceivedSmsSegment, please? Also, with bug 725002 in place, this method will obviously be on the RIL object, next to the other _process* methods.

@@ +1529,5 @@
> +      options.segments = [];
> +    } else if (options.segments[seq]) {
> +      // Duplicated segment?
> +      debug("Got duplicated segment no." + seq + " of a multipart SMS: "
> +            + JSON.stringify(original))

Please gate all all these debug() calls in ril_worker.js with an `if (DEBUG)`

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

1) rebase to rev 9d19b56f39c5 - Bug 725002 - Part 7
2) review comment #50

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

1) rebase to rev 9d19b56f39c5 - Bug 725002 - Part 7

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

1) rebase to rev 9d19b56f39c5 - Bug 725002 - Part 7
2) review comment #51

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

1) rebase to rev 9d19b56f39c5 - Bug 725002 - Part 7
2) In RIL.sendSMS due to landing of bug 725002, only initialize `segmentSeq` when sending first segment.

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

1) rebase to rev 9d19b56f39c5 - Bug 725002 - Part 7

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

1) rebase to rev 9d19b56f39c5 - Bug 725002 - Part 7
2) review comments #53

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

This file is attached for verification process if anyone ever need :)

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

This file is attached for verification process if anyone ever need :)

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

(In reply to Vicamo Yang [:vicamo] from comment #49)
> TODO(?): comment #39 raise another discussion about
> getNumberOfMessagesForText() API, so its implementation remains the same as
> attachment 605376 [details] [diff] [review].

All patches are now rebased and again verified with newer rev e5f6caa40409 :)

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

Thanks for rebasing all those patches! (For future reference, we don't normally carry the r+ flag over when we update a previously r+'ed patch.)

https://hg.mozilla.org/mozilla-central/rev/c8eff2808a2f
https://hg.mozilla.org/mozilla-central/rev/7a535f81b19b
https://hg.mozilla.org/mozilla-central/rev/4da9c8dd18f0
https://hg.mozilla.org/mozilla-central/rev/66512e033f3e
https://hg.mozilla.org/mozilla-central/rev/bab4c979a5c3
https://hg.mozilla.org/mozilla-central/rev/8ecce10fe9b3

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

(In reply to Philipp von Weitershausen [:philikon] from comment #63)
> Thanks for rebasing all those patches!

:)

> (For future reference, we don't normally carry the r+ flag over when we
> update a previously r+'ed patch.)

Oh! I think you might want to skip the two rebase-only patches. They differ only in file paths due to renaming of b2g -> gonk. Anyway, I got it. Thanks for helping me through this lengthy process. Let's make b2g better and better :)