Closed Bug 873351 Opened 7 years ago Closed 5 years ago

B2G SMS: move SMS code out of RadioInterfaceLayer to SmsService

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.2+, tracking-b2g:backlog)

RESOLVED FIXED
2.2 S2 (19dec)
feature-b2g 2.2+
tracking-b2g backlog

People

(Reporter: vicamo, Assigned: bevis)

References

Details

(Whiteboard: [priority2])

Attachments

(3 files, 14 obsolete files)

18.00 KB, patch
bevis
: review+
Details | Diff | Splinter Review
86.95 KB, patch
bevis
: review+
Details | Diff | Splinter Review
65.47 KB, patch
bevis
: review+
Details | Diff | Splinter Review
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.
Depends on bug 899885 for easier communication between SmsService and RadioInterface.
Depends on: 899885
When SmsService is moved out of RIL.js, we need some way to broadcast "RIL:VoicemailNotification" inside SmsService.
Depends on: 833229
Attached patch WIP (obsolete) — Splinter Review
Assignee: nobody → vyang
Depends on bug 907585 instead to keep things rolling fast.
Depends on: 907585
No longer depends on: 833229
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #792876 - Attachment is obsolete: true
Attachment #793404 - Flags: review?(gene.lian)
Reviewing...
Comment on attachment 793404 [details] [diff] [review]
patch

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

Great job! I cannot find any defects.
Attachment #793404 - Flags: review?(gene.lian) → review+
Attached patch patch : v2 (obsolete) — Splinter Review
rebase after bug 864485
Attachment #793404 - Attachment is obsolete: true
Attachment #799321 - Flags: review+
Needs clobber on Windows, followup to touch CLOBBER file:
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/679f571996e8
Attached patch patch : v3 (obsolete) — Splinter Review
Rebase after bug 903403 (currently in b2g-inbound)
Attachment #799321 - Attachment is obsolete: true
Attachment #799613 - Flags: review+
The marionette-webapi bustage is a result of bug 869778 landed last night.
Attached patch patch : v4 (obsolete) — Splinter Review
address Mnw bustage. Try: https://tbpl.mozilla.org/?tree=Try&rev=54f4ffc1d5fd
Attachment #799613 - Attachment is obsolete: true
Attachment #800071 - Flags: review+
Hi, Vicamo, 
Please also test Bug 911690 and Bug 911026 on your patch.
(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
Attached patch patch : v5 (obsolete) — Splinter Review
Rebase after bug 911690.
Attachment #800071 - Attachment is obsolete: true
Attachment #800660 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/001a849303de
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
(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.
Depends on: 912902
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.
Backed out, https://hg.mozilla.org/integration/b2g-inbound/rev/0fadd373bf99 . Still have to fix bug 912902 before landing this patch.
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!
Just a friendly reminder, since Bug 913777 also lands first, be careful to include the CPU wake lock stuff in this work.
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Blocks: 945089
To change internal ril interface.
No longer blocks: 945089
Put this bug into backlog.
blocking-b2g: --- → backlog
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.
Depends on: 833229
No longer depends on: 907585
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.
Depends on: 864484
Blocks: 1097993
Hi Vicamo,

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

Thanks!
Flags: needinfo?(vicamo)
feature-b2g: --- → 2.2?
Whiteboard: [priority2]
Take this bug to follow up. :)
Assignee: vicamo → btseng
Flags: needinfo?(vicamo)
Target Milestone: --- → 2.2 S3 (9jan)
Patch part 1 is to rewrite the SmsService in JS.

Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #800660 - Attachment is obsolete: true
Attachment #8532126 - Flags: review?(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!
Attachment #8532131 - Flags: review?(echen)
Patch Part 3 is to move the implementation of receiving the decoded SMS PDUs from RadioInterfaceLayer to SmsService.
Attachment #8532132 - Flags: review?(echen)
Blocks: 1105021
Address the suggestion in bug 1105021 comment 12.
Attachment #8532132 - Attachment is obsolete: true
Attachment #8532132 - Flags: review?(echen)
Attachment #8532343 - Flags: review?(echen)
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.
Attachment #8532126 - Flags: review?(echen)
Duplicate of this bug: 1097993
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".
Duplicate of this bug: 1105021
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();
Attachment #8532131 - Flags: review?(echen)
(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! :)
Re-name RilSmsService to GonkSmsService.
Attachment #8532126 - Attachment is obsolete: true
Attachment #8534846 - Flags: review?(echen)
1. Move Segmentation related logic from SmsService to SmsSegmentHelper.
2. Separate saving/sending logic from SmsService.send().
Attachment #8532131 - Attachment is obsolete: true
Attachment #8534852 - Flags: review?(echen)
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;
Attachment #8532343 - Flags: review?(echen)
Address suggestion in comment 48.
Attachment #8532343 - Attachment is obsolete: true
Attachment #8536351 - Flags: review?(echen)
Sorry, update the comments properly.
Attachment #8536351 - Attachment is obsolete: true
Attachment #8536351 - Flags: review?(echen)
Attachment #8536354 - Flags: review?(echen)
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?
Attachment #8534846 - Flags: review?(echen) → review+
(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.
Per triage we are confident to make it before FL.
feature-b2g: 2.2? → 2.2+
Blocks: 1019443
Blocks: 733331
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.
Attachment #8534852 - Flags: review?(echen) → review+
(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 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.
Attachment #8536354 - Flags: review?(echen) → review+
(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. :)
update final patch of part_1 to address comment 51.
Attachment #8534846 - Attachment is obsolete: true
Attachment #8539046 - Flags: review+
update final patch part 2 to address comment 54.
Attachment #8534852 - Attachment is obsolete: true
Attachment #8539047 - Flags: review+
Update final patch of part 3 to address comment 56.
Attachment #8536354 - Attachment is obsolete: true
Attachment #8539048 - Flags: review+
Blocks: 1108900
https://hg.mozilla.org/mozilla-central/rev/0450c909333d
https://hg.mozilla.org/mozilla-central/rev/99f8fe8f76a4
https://hg.mozilla.org/mozilla-central/rev/c8057e079f2d
Status: REOPENED → RESOLVED
Closed: 7 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: 2.2 S3 (9jan) → 2.2 S2 (19dec)
Awesome, thank you Bevis and Edgar :)
Depends on: 1138757
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.