Last Comment Bug 712933 - B2G SMS: split overlong messages and stitch them together when receiving
: B2G SMS: split overlong messages and stitch them together when receiving
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Vicamo Yang [:vicamo][:vyang]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 729876 733300 733331
Blocks: b2g-sms
  Show dependency treegraph
 
Reported: 2011-12-22 06:46 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-03-16 19:51 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP V1 (10.06 KB, patch)
2012-03-07 03:41 PST, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js (15.29 KB, patch)
2012-03-09 04:29 PST, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 2: refactor calculateUserDataLength() (8.25 KB, patch)
2012-03-09 04:30 PST, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 3: fix getNumberOfMessagesForText() (9.66 KB, patch)
2012-03-09 04:31 PST, Vicamo Yang [:vicamo][:vyang]
vicamo: review-
Details | Diff | Splinter Review
Part 2: refactor calculateUserDataLength() - V2 (8.32 KB, patch)
2012-03-09 04:50 PST, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 3: fix getNumberOfMessagesForText() : V2 (9.54 KB, patch)
2012-03-09 04:52 PST, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 4: Send divided multipart SMS : WIP (12.23 KB, patch)
2012-03-09 05:55 PST, Vicamo Yang [:vicamo][:vyang]
vicamo: review-
Details | Diff | Splinter Review
Part 4: Send divided multipart SMS : V2 (12.27 KB, patch)
2012-03-09 06:19 PST, Vicamo Yang [:vicamo][:vyang]
vicamo: review-
Details | Diff | Splinter Review
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V2 (21.65 KB, patch)
2012-03-12 03:57 PDT, Vicamo Yang [:vicamo][:vyang]
vicamo: review-
Details | Diff | Splinter Review
Part 2: refactor calculateUserDataLength() - V3 (12.61 KB, patch)
2012-03-12 03:58 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 3: fix getNumberOfMessagesForText() : V3 (14.40 KB, patch)
2012-03-12 03:58 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V3 (21.26 KB, patch)
2012-03-13 06:41 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review-
Details | Diff | Splinter Review
Part 3: fix getNumberOfMessagesForText() : V4 (14.81 KB, patch)
2012-03-13 06:48 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review-
Details | Diff | Splinter Review
Part 4: Support sending multipart SMS : V3 (4.95 KB, patch)
2012-03-13 06:52 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 5: refactor GsmPDUHelper readUserData() (3.81 KB, patch)
2012-03-13 06:55 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 6: Support receiving multipart SMS (4.70 KB, patch)
2012-03-13 06:55 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V4 (19.45 KB, patch)
2012-03-14 03:13 PDT, Vicamo Yang [:vicamo][:vyang]
vicamo: review-
Details | Diff | Splinter Review
Part 2: refactor calculateUserDataLength() - V4 (12.70 KB, patch)
2012-03-15 05:43 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 3: fix getNumberOfMessagesForText() : V5 (15.49 KB, patch)
2012-03-15 06:10 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 4: Support sending multipart SMS : V4 (13.01 KB, patch)
2012-03-15 06:22 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 5: refactor GsmPDUHelper readUserData() : V2 (4.21 KB, patch)
2012-03-15 06:25 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 6: Support receiving multipart SMS : V2 (6.58 KB, patch)
2012-03-15 06:43 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V5 (19.42 KB, patch)
2012-03-15 07:26 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V6 (19.04 KB, patch)
2012-03-15 22:23 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 2: refactor calculateUserDataLength() - V5 (12.71 KB, patch)
2012-03-15 22:26 PDT, Vicamo Yang [:vicamo][:vyang]
vicamo: review+
Details | Diff | Splinter Review
Part 3: fix getNumberOfMessagesForText() : V6 (15.48 KB, patch)
2012-03-15 22:27 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 4: Support sending multipart SMS : V5 (13.32 KB, patch)
2012-03-15 22:32 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 5: refactor GsmPDUHelper readUserData() : V3 (4.21 KB, patch)
2012-03-15 22:33 PDT, Vicamo Yang [:vicamo][:vyang]
vicamo: review+
Details | Diff | Splinter Review
Part 6: Support receiving multipart SMS : V3 (6.81 KB, patch)
2012-03-15 22:35 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
DNS: debug only, for shorten max per message length (3.50 KB, patch)
2012-03-15 22:43 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
DNS: debug only, show text length, segments to send in gaia sms app (1.32 KB, patch)
2012-03-15 22:50 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2011-12-22 06:46:13 PST
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.
Comment 1 Philipp von Weitershausen [:philikon] 2011-12-22 13:21:51 PST
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().
Comment 2 Vicamo Yang [:vicamo][:vyang] 2012-02-15 02:25:13 PST
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.
Comment 3 Philipp von Weitershausen [:philikon] 2012-02-15 18:22:47 PST
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.
Comment 4 Vicamo Yang [:vicamo][:vyang] 2012-03-07 03:41:06 PST
Created attachment 603660 [details] [diff] [review]
WIP V1
Comment 5 Vicamo Yang [:vicamo][:vyang] 2012-03-07 03:46:24 PST
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()
Comment 6 Vicamo Yang [:vicamo][:vyang] 2012-03-09 04:29:37 PST
Created attachment 604373 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js
Comment 7 Vicamo Yang [:vicamo][:vyang] 2012-03-09 04:30:07 PST
Created attachment 604374 [details] [diff] [review]
Part 2: refactor calculateUserDataLength()
Comment 8 Vicamo Yang [:vicamo][:vyang] 2012-03-09 04:31:38 PST
Created attachment 604375 [details] [diff] [review]
Part 3: fix getNumberOfMessagesForText()
Comment 9 Vicamo Yang [:vicamo][:vyang] 2012-03-09 04:43:34 PST
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
Comment 10 Vicamo Yang [:vicamo][:vyang] 2012-03-09 04:50:45 PST
Created attachment 604378 [details] [diff] [review]
Part 2: refactor calculateUserDataLength() - V2
Comment 11 Vicamo Yang [:vicamo][:vyang] 2012-03-09 04:52:18 PST
Created attachment 604379 [details] [diff] [review]
Part 3: fix getNumberOfMessagesForText() : V2

fix comment 9: these changes should go into part2
Comment 12 Vicamo Yang [:vicamo][:vyang] 2012-03-09 05:55:17 PST
Created attachment 604388 [details] [diff] [review]
Part 4: Send divided multipart SMS : WIP
Comment 13 Vicamo Yang [:vicamo][:vyang] 2012-03-09 06:18:12 PST
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
Comment 14 Vicamo Yang [:vicamo][:vyang] 2012-03-09 06:19:00 PST
Created attachment 604392 [details] [diff] [review]
Part 4: Send divided multipart SMS : V2

all parts sent (to myself), but only one received.
Comment 15 Vicamo Yang [:vicamo][:vyang] 2012-03-09 06:21:35 PST
Seems to be something wrong in receiving, my X10 mini pro received all parts and automatically stitch them together :(
Comment 16 Vicamo Yang [:vicamo][:vyang] 2012-03-12 03:57:31 PDT
Created attachment 604877 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V2

update test scripts
Comment 17 Vicamo Yang [:vicamo][:vyang] 2012-03-12 03:58:10 PDT
Created attachment 604878 [details] [diff] [review]
Part 2: refactor calculateUserDataLength() - V3

update test scripts
Comment 18 Vicamo Yang [:vicamo][:vyang] 2012-03-12 03:58:56 PDT
Created attachment 604879 [details] [diff] [review]
Part 3: fix getNumberOfMessagesForText() : V3

update test scripts
Comment 19 Vicamo Yang [:vicamo][:vyang] 2012-03-12 04:02:01 PDT
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
Comment 20 Vicamo Yang [:vicamo][:vyang] 2012-03-13 06:41:22 PDT
Created attachment 605373 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V3

Rebase to r88812. I got build failure http://pastebin.com/Nymch0JK with changes r88813 and r88814, which were landed in bug 734057.
Comment 21 Vicamo Yang [:vicamo][:vyang] 2012-03-13 06:48:57 PDT
Created attachment 605376 [details] [diff] [review]
Part 3: fix getNumberOfMessagesForText() : V4

1) rebase
2) s/multipart/concatMax/
3) s/divide/fragment/
4) let _fragmentText() returns options object directly for further operations
Comment 22 Vicamo Yang [:vicamo][:vyang] 2012-03-13 06:52:50 PDT
Created attachment 605377 [details] [diff] [review]
Part 4: Support sending multipart SMS : V3

1) rebase
2) move handling code into RadioInterfaceLayer, keep worker as clean as possible
Comment 23 Vicamo Yang [:vicamo][:vyang] 2012-03-13 06:55:11 PDT
Created attachment 605378 [details] [diff] [review]
Part 5: refactor GsmPDUHelper readUserData()

refactor GsmPDUHelper.readUserData() to accept one additional object, populate it instead of returning message body only.
Comment 24 Vicamo Yang [:vicamo][:vyang] 2012-03-13 06:55:58 PDT
Created attachment 605379 [details] [diff] [review]
Part 6: Support receiving multipart SMS
Comment 25 Philipp von Weitershausen [:philikon] 2012-03-13 18:41:35 PDT
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.
Comment 26 Philipp von Weitershausen [:philikon] 2012-03-13 18:42:11 PDT
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)
Comment 27 Philipp von Weitershausen [:philikon] 2012-03-13 18:44:07 PDT
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" :)
Comment 28 Vicamo Yang [:vicamo][:vyang] 2012-03-13 19:15:46 PDT
(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.
Comment 29 Philipp von Weitershausen [:philikon] 2012-03-13 20:28:18 PDT
(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.
Comment 30 Philipp von Weitershausen [:philikon] 2012-03-13 21:03:57 PDT
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().
Comment 31 Philipp von Weitershausen [:philikon] 2012-03-13 21:04:15 PDT
Comment on attachment 605376 [details] [diff] [review]
Part 3: fix getNumberOfMessagesForText() : V4

Err, I meant r-
Comment 32 Philipp von Weitershausen [:philikon] 2012-03-13 21:09:11 PDT
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.)
Comment 33 Philipp von Weitershausen [:philikon] 2012-03-13 21:13:48 PDT
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`.
Comment 34 Philipp von Weitershausen [:philikon] 2012-03-13 21:18:54 PDT
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".
Comment 35 Philipp von Weitershausen [:philikon] 2012-03-13 21:19:31 PDT
Comment on attachment 605373 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V3

Gah, I also meant r- on this one.
Comment 36 Philipp von Weitershausen [:philikon] 2012-03-14 02:02:23 PDT
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.
Comment 37 Vicamo Yang [:vicamo][:vyang] 2012-03-14 03:13:01 PDT
Created attachment 605704 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V4

remove contributor change
Comment 38 Vicamo Yang [:vicamo][:vyang] 2012-03-14 03:25:26 PDT
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
Comment 39 Vicamo Yang [:vicamo][:vyang] 2012-03-14 08:37:15 PDT
(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.
Comment 40 Vicamo Yang [:vicamo][:vyang] 2012-03-14 09:21:26 PDT
(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.
Comment 41 Vicamo Yang [:vicamo][:vyang] 2012-03-14 09:26:30 PDT
(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.
Comment 42 Philipp von Weitershausen [:philikon] 2012-03-14 14:18:35 PDT
(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!
Comment 43 Vicamo Yang [:vicamo][:vyang] 2012-03-15 05:43:51 PDT
Created attachment 606175 [details] [diff] [review]
Part 2: refactor calculateUserDataLength() - V4

1) review comment #26, comment #27
2) assign property `body` when calculateUserDataLength() is done
Comment 44 Vicamo Yang [:vicamo][:vyang] 2012-03-15 06:10:03 PDT
Created attachment 606180 [details] [diff] [review]
Part 3: fix getNumberOfMessagesForText() : V5

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
Comment 45 Vicamo Yang [:vicamo][:vyang] 2012-03-15 06:22:38 PDT
Created attachment 606183 [details] [diff] [review]
Part 4: Support sending multipart SMS : V4

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.
Comment 46 Vicamo Yang [:vicamo][:vyang] 2012-03-15 06:25:56 PDT
Created attachment 606184 [details] [diff] [review]
Part 5: refactor GsmPDUHelper readUserData() : V2

review comment #33
Comment 47 Vicamo Yang [:vicamo][:vyang] 2012-03-15 06:43:54 PDT
Created attachment 606186 [details] [diff] [review]
Part 6: Support receiving multipart SMS : V2

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.
Comment 48 Vicamo Yang [:vicamo][:vyang] 2012-03-15 07:26:36 PDT
Created attachment 606200 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V5

review comment #25, load RadioInterfaceLayer as a js component
Comment 49 Vicamo Yang [:vicamo][:vyang] 2012-03-15 07:28:17 PDT
TODO(?): comment #39 raise another discussion about getNumberOfMessagesForText() API, so its implementation remains the same as attachment 605376 [details] [diff] [review].
Comment 50 Philipp von Weitershausen [:philikon] 2012-03-15 17:00:47 PDT
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.)
Comment 51 Philipp von Weitershausen [:philikon] 2012-03-15 17:07:30 PDT
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.
Comment 52 Philipp von Weitershausen [:philikon] 2012-03-15 17:11:36 PDT
(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
Comment 53 Philipp von Weitershausen [:philikon] 2012-03-15 17:25:19 PDT
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)`
Comment 54 Vicamo Yang [:vicamo][:vyang] 2012-03-15 22:23:58 PDT
Created attachment 606466 [details] [diff] [review]
Part 1: Move GsmPDUHelper.calculateUserDataLength() to RadioInterfaceLayer.js : V6

1) rebase to rev 9d19b56f39c5 - Bug 725002 - Part 7
2) review comment #50
Comment 55 Vicamo Yang [:vicamo][:vyang] 2012-03-15 22:26:10 PDT
Created attachment 606467 [details] [diff] [review]
Part 2: refactor calculateUserDataLength() - V5

1) rebase to rev 9d19b56f39c5 - Bug 725002 - Part 7
Comment 56 Vicamo Yang [:vicamo][:vyang] 2012-03-15 22:27:48 PDT
Created attachment 606469 [details] [diff] [review]
Part 3: fix getNumberOfMessagesForText() : V6

1) rebase to rev 9d19b56f39c5 - Bug 725002 - Part 7
2) review comment #51
Comment 57 Vicamo Yang [:vicamo][:vyang] 2012-03-15 22:32:40 PDT
Created attachment 606470 [details] [diff] [review]
Part 4: Support sending multipart SMS : V5

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.
Comment 58 Vicamo Yang [:vicamo][:vyang] 2012-03-15 22:33:37 PDT
Created attachment 606472 [details] [diff] [review]
Part 5: refactor GsmPDUHelper readUserData() : V3

1) rebase to rev 9d19b56f39c5 - Bug 725002 - Part 7
Comment 59 Vicamo Yang [:vicamo][:vyang] 2012-03-15 22:35:26 PDT
Created attachment 606474 [details] [diff] [review]
Part 6: Support receiving multipart SMS : V3

1) rebase to rev 9d19b56f39c5 - Bug 725002 - Part 7
2) review comments #53
Comment 60 Vicamo Yang [:vicamo][:vyang] 2012-03-15 22:43:55 PDT
Created attachment 606475 [details] [diff] [review]
DNS: debug only, for shorten max per message length

This file is attached for verification process if anyone ever need :)
Comment 61 Vicamo Yang [:vicamo][:vyang] 2012-03-15 22:50:43 PDT
Created attachment 606478 [details] [diff] [review]
DNS: debug only, show text length, segments to send in gaia sms app

This file is attached for verification process if anyone ever need :)
Comment 62 Vicamo Yang [:vicamo][:vyang] 2012-03-15 22:58:30 PDT
(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 :)
Comment 64 Vicamo Yang [:vicamo][:vyang] 2012-03-16 19:51:18 PDT
(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 :)

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