873351 - B2G SMS: move SMS code out of RadioInterfaceLayer to SmsService

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

Currently we stuff up RadioInterfaceLayer with a lot SMS specific code.  These state handlings, calculations have little to do with radio and can be handled separately to slim down code size of RadioInterfaceLayer and improve modulation as well.  Moving these code into SmsService also implies rewriting it by javascript.

Comment 1

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

Depends on bug 899885 for easier communication between SmsService and RadioInterface.

Comment 2

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

When SmsService is moved out of RIL.js, we need some way to broadcast "RIL:VoicemailNotification" inside SmsService.

Comment 3

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

Attachment: WIP

Comment 4

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

Depends on bug 907585 instead to keep things rolling fast.

Comment 5

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

Attachment: patch

1) Rebased onto bug 907585 for emitting voicemail messages with RilMessageManager.

2) New Gonk only interface nsIRilSmsService created so that RadioInterface can directly communicate to it.  It has only one method -- notifyMessageReceived.

3) Remove "builtinclass" flag from nsISmsService because its descendant, nsIRilSmsService, is implemented by javascript.

4) Remove "isSilentNumber" method from nsISmsService as well.  It was originally called from RadioInterface to SmsService to determine whether a newly incoming SMS is a silent message for payment providers.  Since we have moved all SMS code into SmsService itself, this interface is no longer necessary.

5) Remove extra LOCAL_INCLUDES from Makefile.  They have become redundant for a long time.

6) Add nsIRadioInterface::sendWorkerMessage so that SmsService may ever send message to ril worker.

Comment 6

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

https://tbpl.mozilla.org/?tree=Try&rev=e8b43c5f16ad

Comment 7

[Gene Lian (I already quit Mozilla)]

Reviewing...

Comment 8

[Gene Lian (I already quit Mozilla)]

Comment on attachment 793404 [details] [diff] [review]
patch

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

Great job! I cannot find any defects.

Comment 9

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

Attachment: patch : v2

rebase after bug 864485

Comment 10

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

https://hg.mozilla.org/integration/b2g-inbound/rev/8b6235d4c2ac

Comment 11

[Ed Morley [:emorley]]

Needs clobber on Windows, followup to touch CLOBBER file:
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/679f571996e8

Comment 12

[Ed Morley [:emorley]]

Backed out for conflicts with the backout of bug 864485:
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/66c66282afef
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/c77424d651ee

Comment 13

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

Attachment: patch : v3

Rebase after bug 903403 (currently in b2g-inbound)

Comment 14

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

full try: https://tbpl.mozilla.org/?tree=Try&rev=81ab41747678

Comment 15

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

The marionette-webapi bustage is a result of bug 869778 landed last night.

Comment 16

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

Attachment: patch : v4

address Mnw bustage. Try: https://tbpl.mozilla.org/?tree=Try&rev=54f4ffc1d5fd

Comment 17

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

Full try for bug 864485, bug 907585, and bug 873351: https://tbpl.mozilla.org/?tree=Try&rev=2b850251174f

Comment 18

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Hi, Vicamo, 
Please also test Bug 911690 and Bug 911026 on your patch.

Comment 19

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

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #18)
> Hi, Vicamo, 
> Please also test Bug 911690 and Bug 911026 on your patch.

It's already included. https://hg.mozilla.org/try/rev/e9400836b69b#l12.734

Comment 20

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

Attachment: patch : v5

Rebase after bug 911690.

Comment 21

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

https://hg.mozilla.org/integration/b2g-inbound/rev/001a849303de

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/001a849303de

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out at gwagner's request because apparently we like it when phones are able to boot. Who knew.
https://hg.mozilla.org/integration/b2g-inbound/rev/dc4758d44b11

Comment 24

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> The marionette-webapi bustage is a result of bug 869778 landed last night.

Turns out solution for bug 869778 here triggers that permission assert.  Depending on its follow-up bug 912902.

Comment 25

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

But that's strange.  SmsService only launches on referenced and that's not happened during Gaia startup process.  Besides, why does Marionette test cases run with success?  After further analysis, it's not related to this bug but bug 907585.  Continue analysis there.

Comment 26

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

https://hg.mozilla.org/integration/b2g-inbound/rev/2a22b79edf4d

Comment 27

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

Backed out, https://hg.mozilla.org/integration/b2g-inbound/rev/0fadd373bf99 . Still have to fix bug 912902 before landing this patch.

Comment 28

[Gene Lian (I already quit Mozilla)]

Since this one was backed out, I landed Bug 913436 first which is urgently requested by Gaia. Please rebase for this bug before relanding. Thanks!

Comment 29

[Gene Lian (I already quit Mozilla)]

Just a friendly reminder, since Bug 913777 also lands first, be careful to include the CPU wake lock stuff in this work.

Comment 30

[Ken Chang[:kenkai]]

To change internal ril interface.

Comment 31

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 32

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

Remove bug 907585 from dependency list and add bug 833229. With bug 833229, we may send notification to voicemail service without routing messages through MessageManager in RadioInterfaceLayer.js.

Comment 33

[Bevis Tseng[:bevis][:btseng](Exited)]

Add dependency to bug 864484 because, for the CDMA SMS message with message.messageType == RIL.PDU_CDMA_MSG_TYPE_BROADCAST, it shall be handled by the new CellBroadcastService.js 
in patch part 2 of bug 864484.

Comment 34

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Vicamo,

I'd like to take this bug to follow up if you have no concern.

Thanks!

Comment 35

[Bevis Tseng[:bevis][:btseng](Exited)]

Take this bug to follow up. :)

Comment 36

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v1: Re-write SmsService in JavaScript. r=echen

Patch part 1 is to rewrite the SmsService in JS.

Hi Edgar,

May I have your review for this change?

Thanks!

Comment 37

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v1: Refactor SMS Requests from RadioInterfaceLayer to SmsService. r=echen

Patch Part2 is to:
1. Move the implementation of SMS request from RadioInterfaceLayer to SmsService.
2. Define SmsSegmentHelper as the utility for SMS segmentation that could be used by both SmsService and the xpcshell test cases with ril_worker.

Hi Edgar,

May I have your review for this change?

Thanks!

Comment 38

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3 v1: Refactor SMS Notifications from RadioInterfaceLayer to SmsService. r=echen

Patch Part 3 is to move the implementation of receiving the decoded SMS PDUs from RadioInterfaceLayer to SmsService.

Comment 39

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3 v1: Refactor SMS Notifications from RadioInterfaceLayer to SmsService. r=echen

Address the suggestion in bug 1105021 comment 12.

Comment 40

[Edgar Chen [:edgar]]

Comment on attachment 8532126 [details] [diff] [review]
Part 1 v1: Re-write SmsService in JavaScript. r=echen

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

Let's revise the name a bit to follow the same naming rule in other component. Thank you.

::: dom/mobilemessage/gonk/SmsService.js
@@ +1,1 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

nit: don't need this.

::: dom/mobilemessage/interfaces/nsIRilSmsService.idl
@@ +5,5 @@
> +#include "nsISmsService.idl"
> +
> +%{C++
> +#define RIL_SMSSERVICE_CONTRACTID \
> +        "@mozilla.org/sms/rilsmsservice;1"

Rename the contract id to "@mozilla.org/sms/gonksmsservice;1" and also GONK_SMSSERVICE_CONTRACTID.

@@ +9,5 @@
> +        "@mozilla.org/sms/rilsmsservice;1"
> +%}
> +
> +[scriptable, uuid(63fab75e-73b4-11e4-a10d-dbfa9d05a4f4)]
> +interface nsIRilSmsService : nsISmsService

Rename it to nsIGonkSmsService.

Comment 42

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8532126 [details] [diff] [review]
Part 1 v1: Re-write SmsService in JavaScript. r=echen

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

::: dom/mobilemessage/gonk/SmsService.js
@@ +1,1 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Will do.

::: dom/mobilemessage/interfaces/nsIRilSmsService.idl
@@ +5,5 @@
> +#include "nsISmsService.idl"
> +
> +%{C++
> +#define RIL_SMSSERVICE_CONTRACTID \
> +        "@mozilla.org/sms/rilsmsservice;1"

Will do, and bug 1108900 has been filed to rename the rest gonk-backend services with prefix of "ril" to the ones with prefix of "gonk".

@@ +9,5 @@
> +        "@mozilla.org/sms/rilsmsservice;1"
> +%}
> +
> +[scriptable, uuid(63fab75e-73b4-11e4-a10d-dbfa9d05a4f4)]
> +interface nsIRilSmsService : nsISmsService

Will do, and bug 1108900 has been filed to rename the rest gonk-backend services with prefix of "ril" to the ones with prefix of "gonk".

Comment 44

[Edgar Chen [:edgar]]

Comment on attachment 8532131 [details] [diff] [review]
Part 2 v1: Refactor SMS Requests from RadioInterfaceLayer to SmsService. r=echen

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

Please see my comments below, thank you.

::: dom/mobilemessage/gonk/SmsSegmentHelper.jsm
@@ +21,5 @@
> +   * @param aLangTable
> +   *        locking shift table string.
> +   * @param aLangShiftTable
> +   *        single shift table string.
> +   * @param aStrict7BitEncoding

nit: @param aStrict7BitEncoding [optional]

@@ +28,5 @@
> +   *
> +   * @return encoded length in septets.
> +   *
> +   * @note that the algorithm used in this function must match exactly with
> +   * GsmPDUHelper#writeStringAsSeptets.

nit: indention

@@ +96,5 @@
> +   * alphabets.
> +   *
> +   * @param aMessage
> +   *        a message string to be encoded.
> +   * @param aStrict7BitEncoding

Ditto, @param aFoo [optional]

@@ +113,5 @@
> +   * |segmentRef16Bit|:
> +   *   Use 16-bit reference number for concatenated outgoint messages.
> +   *   TODO: Support static/runtime settings, see bug 733331.
> +   */
> +  enabledGsmTableTuples: [

Add `_` prefix for private member

@@ +211,5 @@
> +   * Calculate user data length and its encoding.
> +   *
> +   * @param aMessage
> +   *        a message string to be encoded.
> +   * @param aStrict7BitEncoding

Ditto, @param aFoo [optional]

@@ +255,5 @@
> +   * @param aLangShiftTable
> +   *        single shift table string.
> +   * @param aSegmentSeptets
> +   *        Number of available spetets per segment.
> +   * @param aStrict7BitEncoding

Ditto, @param aFoo [optional]

@@ +371,5 @@
> +   * length of the encoded segment body in septets.
> +   *
> +   * @param aText
> +   *        Text string to be fragmented.
> +   * @param aOptions

Ditto, @param aFoo [optional]

@@ +374,5 @@
> +   *        Text string to be fragmented.
> +   * @param aOptions
> +   *        Optional pre-calculated option object. The output array will be
> +   *        stored at aOptions.segments if there are multiple segments.
> +   * @param aStrict7BitEncoding

Ditto

::: dom/mobilemessage/gonk/SmsService.js
@@ +51,5 @@
> +  Cu.import("resource://gre/modules/SmsSegmentHelper.jsm", ns);
> +  return ns.SmsSegmentHelper;
> +});
> +
> +XPCOMUtils.defineLazyGetter(this, "PhoneNumberUtils", function() {

We have 'g' prefix for other global variables, let's follow the same rule.
s/gPhoneNumberUtils/PhoneNumberUtils/

@@ +140,5 @@
> +    }
> +
> +    // Workaround an xpconnect issue with undefined string objects.
> +    // See bug 808220
> +    if (number === undefined || number === "undefined") {

We should not have the `undefined` issue in |iccInfo| any more, so I think it is safe to remove this check.

@@ +158,5 @@
> +    let iccId = iccInfo.iccid;
> +
> +    // Workaround an xpconnect issue with undefined string objects.
> +    // See bug 808220
> +    if (iccId === undefined || iccId === "undefined") {

ditto

@@ +245,5 @@
> +  /**
> +   * Get valid SMS concatenation reference number.
> +   */
> +  _segmentRef: 0,
> +  get nextSegmentRef() {

Could we put segment reference number in |gSmsSegmentHelper|?

@@ +281,5 @@
> +      charsInLastSegment = 0;
> +    }
> +
> +    aRequest.notifySegmentInfoForTextGot(options.segmentMaxSeq,
> +                                        options.segmentChars,

nit: indention

@@ +308,5 @@
> +      requestStatusReport = true;
> +    }
> +    options.requestStatusReport = requestStatusReport && !aSilent;
> +    if (options.segmentMaxSeq > 1) {
> +      options.segmentRef16Bit = gSmsSegmentHelper.segmentRef16Bit;

Could we assign |options.segmentRef16Bit| and |options.segmentRef| in |gSmsSegmentHelper|?

@@ +312,5 @@
> +      options.segmentRef16Bit = gSmsSegmentHelper.segmentRef16Bit;
> +      options.segmentRef = this.nextSegmentRef;
> +    }
> +
> +    let notifyResult = (rv, domMessage) => {

s/rv/aRv/
s/domMessage/aDomMessage/

@@ +334,5 @@
> +      let errorCode;
> +      let radioState = connection && connection.radioState;
> +      if (!PhoneNumberUtils.isPlainPhoneNumber(options.number)) {
> +        if (DEBUG) debug("Error! Address is invalid when sending SMS: " +
> +                              options.number);

nit: indention

@@ +357,5 @@
> +                                         null,
> +                                         DOM_MOBILE_MESSAGE_DELIVERY_ERROR,
> +                                         RIL.GECKO_SMS_DELIVERY_STATUS_ERROR,
> +                                         null,
> +                                         (rv, domMessage) => {

s/rv/aRv/
s/domMessage/aDomMessage/

@@ +376,5 @@
> +
> +      // This is the entry point starting to send SMS.
> +      gRadioInterfaces[aServiceId].sendWorkerMessage("sendSMS",
> +                                                     options,
> +                                                     (response) => {

This nested function is really looong and has another nested function inside, do you think moving it out makes code more readable?

@@ +418,5 @@
> +                                           null,
> +                                           DOM_MOBILE_MESSAGE_DELIVERY_ERROR,
> +                                           RIL.GECKO_SMS_DELIVERY_STATUS_ERROR,
> +                                           null,
> +                                           (rv, domMessage) => {

Ditto, `a` prefix.

@@ +434,5 @@
> +                                           null,
> +                                           context.sms.delivery,
> +                                           response.deliveryStatus,
> +                                           null,
> +                                           (rv, domMessage) => {

Ditto, `a` prefix.

@@ +486,5 @@
> +                                         null,
> +                                         DOM_MOBILE_MESSAGE_DELIVERY_SENT,
> +                                         context.sms.deliveryStatus,
> +                                         null,
> +                                         (rv, domMessage) => {

Ditto, `a` prefix.

::: dom/mobilemessage/tests/xpcshell/test_sms_segment_helper.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

nit:
/* ...
 * ... */

::: dom/system/gonk/tests/test_ril_worker_sms.js
@@ +162,1 @@
>    let worker = test_receiving_7bit_alphabets__worker;

It seems having |test_receiving_7bit_alphabets__worker| is not necessary, could we just new worker here?

let worker = newWriteHexOctetAsUint8Worker();

Comment 45

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Edgar Chen [:edgar][:echen] from comment #44)
> Comment on attachment 8532131 [details] [diff] [review]
> Part 2 v1: Refactor SMS Requests from RadioInterfaceLayer to SmsService.
> r=echen
> 
> Review of attachment 8532131 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comments below, thank you.
> 

Thanks for all these suggestions.
I'll address these in next update! :)

Comment 46

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v2: Re-write SmsService in JavaScript. r=echen

Re-name RilSmsService to GonkSmsService.

Comment 47

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v2: Refactor SMS Requests from RadioInterfaceLayer to SmsService. r=echen

1. Move Segmentation related logic from SmsService to SmsSegmentHelper.
2. Separate saving/sending logic from SmsService.send().

Comment 48

[Edgar Chen [:edgar]]

Comment on attachment 8532343 [details] [diff] [review]
Part 3 v1: Refactor SMS Notifications from RadioInterfaceLayer to SmsService. r=echen

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

Overall looks good, but I would like to see it again after you address the `gonk` naming. Thank you.

::: dom/mobilemessage/gonk/SmsService.js
@@ +59,5 @@
>    Cu.import("resource://gre/modules/PhoneNumberUtils.jsm", ns);
>    return ns.PhoneNumberUtils;
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, "WAP", function() {

s/WAP/gWAP/

@@ +577,5 @@
> +        return true;
> +      }
> +    }
> +
> +    let notifyReceived = (rv, domMessage) => {

s/rv/aRv/
s/domMessage/aDomMessage/

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2071,5 @@
>     */
> +  handleSmsReceived: function(aMessage) {
> +    // Concatenation Info:
> +    let header = aMessage.header;
> +    let segmentRef = (header && header.segmentRef !== undefined)

Wondering could we do this as below way, here and below.

let segmentRef = header && header.segmentRef || 1;

@@ +2091,5 @@
> +    let mwiMsgCount = (mwiPresent)? aMessage.mwi.msgCount: 0;
> +    let mwiActive = (mwiPresent)? aMessage.mwi.active: false;
> +    // CDMA related attributes:
> +    let cdmaMessageType = (aMessage.messageType !== undefined)
> +      ? aMessage.messageType : 0;

Ditto, wondering could we do this as below way, here and below.

let cdmaMessageType = aMessage.messageType || 0;

Comment 49

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3 v2: Refactor SMS Notifications from RadioInterfaceLayer to SmsService. r=echen

Address suggestion in comment 48.

Comment 50

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3 v2: Refactor SMS Notifications from RadioInterfaceLayer to SmsService. r=echen

Sorry, update the comments properly.

Comment 51

[Edgar Chen [:edgar]]

Comment on attachment 8534846 [details] [diff] [review]
Part 1 v2: Re-write SmsService in JavaScript. r=echen

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

r=me with below comment addressed or answered. Thank you.

::: dom/mobilemessage/gonk/SmsService.js
@@ +85,5 @@
> +  getSegmentInfoForText: function(aText, aRequest) {
> +    gRadioInterfaces[0].getSegmentInfoForText(aText, aRequest);
> +  },
> +
> +  send: function (aServiceId, aNumber, aMessage, aSilent, aRequest) {

nit: remove the space between `n` and `(`.

@@ +142,5 @@
> +          this.smsDefaultServiceId = this._getDefaultServiceId();
> +        }
> +        break;
> +      case NS_XPCOM_SHUTDOWN_OBSERVER_ID:
> +        Services.prefs.removeObserver(kPrefRilDebuggingEnabled, this);

Do we need to remove observer for `kPrefDefaultServiceId` and `NS_XPCOM_SHUTDOWN_OBSERVER_I` as well here?

Comment 52

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Edgar Chen [:edgar][:echen] from comment #51)
> Comment on attachment 8534846 [details] [diff] [review]
> Part 1 v2: Re-write SmsService in JavaScript. r=echen
> 
> Review of attachment 8534846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with below comment addressed or answered. Thank you.
> 
> 
> nit: remove the space between `n` and `(`.
> 
> @@ +142,5 @@
> > +          this.smsDefaultServiceId = this._getDefaultServiceId();
> > +        }
> > +        break;
> > +      case NS_XPCOM_SHUTDOWN_OBSERVER_ID:
> > +        Services.prefs.removeObserver(kPrefRilDebuggingEnabled, this);
> 
> Do we need to remove observer for `kPrefDefaultServiceId` and
> `NS_XPCOM_SHUTDOWN_OBSERVER_I` as well here?

Thanks for catching these.
I'll remove them in next update.

Comment 53

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Per triage we are confident to make it before FL.

Comment 54

[Edgar Chen [:edgar]]

Comment on attachment 8534852 [details] [diff] [review]
Part 2 v2: Refactor SMS Requests from RadioInterfaceLayer to SmsService. r=echen

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

r=me with below comments addressed or answered. Thank you.

::: dom/mobilemessage/gonk/SmsSegmentHelper.jsm
@@ +43,5 @@
> +   *
> +   * @note The algorithm used in this function must match exactly with
> +   *       GsmPDUHelper#writeStringAsSeptets.
> +   */
> +  countGsm7BitSeptets: function(aMessage, alangTable, aLangShiftTable, aStrict7BitEncoding) {

nit: s/alangTable/aLangTable/

::: dom/mobilemessage/gonk/SmsService.js
@@ +137,5 @@
> +      }
> +      return null;
> +    }
> +
> +    if (!number) {

Wondering why we need this check?

@@ +153,5 @@
> +    }
> +
> +    let iccId = iccInfo.iccid;
> +
> +    if (!iccId) {

Ditto.

@@ +335,5 @@
> +  /**
> +   * A helper to broadcast the system message to launch registered apps
> +   * like Costcontrol, Notification and Message app... etc.
> +   *
> +   * @param aName

Comments here aren't matched with the code. Please help to correct it.

::: dom/system/gonk/tests/test_ril_worker_sms_segment_info.js
@@ +14,3 @@
>  const ESCAPE = "\uffff";
>  const RESCTL = "\ufffe";
>  const SP = " ";

Seems |SP| is no longer needed.

Comment 55

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Edgar Chen [:edgar][:echen] from comment #54)
> Comment on attachment 8534852 [details] [diff] [review]
> Part 2 v2: Refactor SMS Requests from RadioInterfaceLayer to SmsService.
> r=echen
> 
> Review of attachment 8534852 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with below comments addressed or answered. Thank you.
> 

Thanks for reminding!
These seems not necessary to me as well.
I'll clear these up in next update. :)

Comment 56

[Edgar Chen [:edgar]]

Comment on attachment 8536354 [details] [diff] [review]
Part 3 v2: Refactor SMS Notifications from RadioInterfaceLayer to SmsService. r=echen

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

r=me with below comment addressed. Thank you.

::: dom/mobilemessage/gonk/SmsService.js
@@ +214,5 @@
> +      this._smsHandledWakeLock = null;
> +    }
> +  },
> +
> +

one blank line is enough.

@@ +443,5 @@
> +   *   attributes `segmentMaxSeq`, `receivedSegments`, `segments` are inserted.
> +   */
> +  _receivedSmsSegmentsMap: null,
> +  _processReceivedSmsSegment: function(aSegment) {
> +

Remove this blank line.

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

nit: indention, align two `"`.

::: dom/system/gonk/ril_worker.js
@@ +13702,5 @@
>        RIL.iccInfoPrivate.mwis = mwis; //Keep raw MWIS for updateMWIS()
>  
> +      let mwi = {
> +        returnNumber: null,
> +        returnMessage: null

Since we move the most of sms code to SmsService.js, now the |nsIGonkVoicemailService#notifyStatusChanged| in RadioInterfaceLayer.js is only triggered by 'iccmwis' message. So I think we don't have to initialize |returnNumber| and |returnMessage| here, but passing `null` into nsIGonkVoicemailService#notifyStatusChanged directly in RadioInterfaceLayer.js.

Comment 57

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Edgar Chen [:edgar][:echen] from comment #56)
> Comment on attachment 8536354 [details] [diff] [review]
> Part 3 v2: Refactor SMS Notifications from RadioInterfaceLayer to
> SmsService. r=echen
> 
> Review of attachment 8536354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with below comment addressed. Thank you.
> 
> ::: dom/mobilemessage/gonk/SmsService.js
> @@ +214,5 @@
> > +      this._smsHandledWakeLock = null;
> > +    }
> > +  },
> > +
> > +
> 
> one blank line is enough.
> 
> @@ +443,5 @@
> > +   *   attributes `segmentMaxSeq`, `receivedSegments`, `segments` are inserted.
> > +   */
> > +  _receivedSmsSegmentsMap: null,
> > +  _processReceivedSmsSegment: function(aSegment) {
> > +
> 
> Remove this blank line.
> 
> @@ +471,5 @@
> > +    } else if (options.segments[seq]) {
> > +      // Duplicated segment?
> > +      if (DEBUG) {
> > +        debug("Got duplicated segment no." + seq +
> > +                           " of a multipart SMS: " + JSON.stringify(aSegment));
> 
> nit: indention, align two `"`.
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +13702,5 @@
> >        RIL.iccInfoPrivate.mwis = mwis; //Keep raw MWIS for updateMWIS()
> >  
> > +      let mwi = {
> > +        returnNumber: null,
> > +        returnMessage: null
> 
> Since we move the most of sms code to SmsService.js, now the
> |nsIGonkVoicemailService#notifyStatusChanged| in RadioInterfaceLayer.js is
> only triggered by 'iccmwis' message. So I think we don't have to initialize
> |returnNumber| and |returnMessage| here, but passing `null` into
> nsIGonkVoicemailService#notifyStatusChanged directly in
> RadioInterfaceLayer.js.

Thanks!
I'll address these in next update. :)

Comment 58

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Re-write SmsService in JavaScript. r=echen

update final patch of part_1 to address comment 51.

Comment 59

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2: Refactor SMS Requests from RadioInterfaceLayer to SmsService. r=echen

update final patch part 2 to address comment 54.

Comment 60

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3: Refactor SMS Notifications from RadioInterfaceLayer to SmsService. r=echen

Update final patch of part 3 to address comment 56.

Comment 61

[Bevis Tseng[:bevis][:btseng](Exited)]

update try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea1de9f98a39

Comment 62

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0450c909333d
https://hg.mozilla.org/integration/b2g-inbound/rev/99f8fe8f76a4
https://hg.mozilla.org/integration/b2g-inbound/rev/c8057e079f2d

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0450c909333d
https://hg.mozilla.org/mozilla-central/rev/99f8fe8f76a4
https://hg.mozilla.org/mozilla-central/rev/c8057e079f2d

Comment 64

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Awesome, thank you Bevis and Edgar :)

Comment 65

[Eric Chang [:ericcc] [:echang]]

Messages test suites for regression test
https://moztrap.mozilla.org/manage/suites/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-status=active&filter-name=comms&filter-name=messages