Closed Bug 814581 Opened 8 years ago Closed 7 years ago

B2G Multi-SIM: add MSimRadioInterfaceLayer

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
blocking-b2g -

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(8 files, 14 obsolete files)

3.50 KB, patch
hsinyi
: superreview+
Details | Diff | Splinter Review
4.97 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
2.30 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
1.55 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
12.35 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
83.70 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
17.04 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
58.83 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
Add a superior manager object to manager and dispatch events to and from multiple RadioInterfaceLayer instances. We should also provide a unique/read-only way to query the number of sim cards.
Blocks: 814584
Assignee: nobody → allstars.chh
Blocks: 826931
Depends on: 826977
No longer blocks: 826931
Blocks: 832808
Depends on: 834160
blocking-b2g: --- → leo?
Blocking-, no longer targeting Leo release. Product will retarget for future release.
blocking-b2g: leo? → -
Assignee: allstars.chh → vyang
Blocks: 878748
Attached patch patch 1/7 - interface changes (obsolete) — Splinter Review
Attachment #760840 - Flags: review?(htsai)
Attachment #760840 - Flags: review?(allstars.chh)
Attachment #760841 - Flags: review?(htsai)
Attachment #760841 - Flags: review?(allstars.chh)
Attachment #760842 - Flags: review?(htsai)
Attachment #760842 - Flags: review?(allstars.chh)
Kanru, as in person discussion, this patch assumes supl data connection always comes from ril0.  Let's have further discussion of AGPS under MultiSIM scheme n bug 878748.
Attachment #760843 - Flags: review?(kchen)
Attached patch patch 5/7 - fix MobileMessage (obsolete) — Splinter Review
Attachment #760844 - Flags: review?(gene.lian)
Attached patch patch 6/7 - fix test cases (obsolete) — Splinter Review
That's the only test script that involves nsIRadioInterfaceLayer changes.
Attachment #760845 - Flags: review?(allstars.chh)
Then we have:

  RadioInterfaceLayer
  RadioInterface[<clientId>]
  RILNetworkInterface[<clientId>:<type>]
  RIL Worker[<clientId>]

as debug message tags.
Attachment #760847 - Flags: review?(htsai)
Attachment #760847 - Flags: review?(allstars.chh)
I have local all green Marionette test results, but still a full try is ongoing in https://tbpl.mozilla.org/?tree=Try&rev=0f0dcf6f8d32
Comment on attachment 760840 [details] [diff] [review]
patch 1/7 - interface changes

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

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +126,5 @@
> +  readonly attribute unsigned long numRadioInterfaces;
> +
> +  nsIRadioInterface getRadioInterface(in long clientId);
> +
> +  void updateRILNetworkInterface();

Why move updateRILNetworkInterface to here?

I think even without this, other modules still can do the same thing by
nsIRadioInterfaceLayer.getRadioInterface(clientId).updateRILNetworkInterface
Comment on attachment 760841 [details] [diff] [review]
patch 2/7 - create multiple intances of RadioInterface

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +254,5 @@
> +    this.radioInterfaces.push(new RadioInterface(options));
> +  }
> +}
> +RadioInterfaceLayer.prototype = {
> +

extra line?

@@ +287,5 @@
> +      }
> +    }
> +
> +    return this._numRadioInterfaces;
> +  },

Can we use
XPCOMUtils.defineLazyGetter(nsIRadioInterfaceLayer.prototype, "numRadioInterfaces", function () {
    ...
  });
for this?

@@ +2094,5 @@
>    // APN data for making data calls.
> +  dataCallSettings: null,
> +  dataCallSettingsMMS: null,
> +  dataCallSettingsSUPL: null,
> +  _dataCallSettingsToRead: null,

Now where did you init this variable?

@@ +3098,5 @@
>    },
>  };
>  
> +function RILNetworkInterface(radioInterface, type) {
> +  this.mRadioInterface = radioInterface;

Should we remove the 'm' prefix?
I found other members don't use 'm' prefix either.

::: modules/libpref/src/init/all.js
@@ +4222,5 @@
>  
>  // Debug enabler for MMS.
>  pref("mms.debugging.enabled", false);
>  
> +// Number of RadioInterface instance to create.

nit, 'instances' with s
Attachment #760841 - Flags: review?(allstars.chh) → review+
Comment on attachment 760840 [details] [diff] [review]
patch 1/7 - interface changes

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

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +126,5 @@
> +  readonly attribute unsigned long numRadioInterfaces;
> +
> +  nsIRadioInterface getRadioInterface(in long clientId);
> +
> +  void updateRILNetworkInterface();

Same as Yoshi's question: Cann't we leave updateRILNetworkInterface() in nsIRadioInterface?
Comment on attachment 760844 [details] [diff] [review]
patch 5/7 - fix MobileMessage

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

Since you're hoping to let the callers decide the |.sender| when calling |.saveSendingMessage()| and the |.receiver| when calling |.saveReceivedMessage()|. Where do you assign values to them for *MMS*? I didn't see that part. Did I miss something?

Also, please add sanity checks at the top of |.saveSendingMessage()| and |.saveReceivedMessage()| for checking |.sender| and |.receiver| respectively.

::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
@@ +31,5 @@
>    /**
>     * |aMessage| Object: should contain the following properties for internal use:
>     *   - |type| DOMString: "sms" or "mms"
>     *   - |sender| DOMString: the phone number of sender
>     *   - |timestamp| Number: the timestamp of received message

Please add the comment for |receiver| up to here.

@@ +35,5 @@
>     *   - |timestamp| Number: the timestamp of received message
>     *
>     *   - If |type| == "sms", we also need:
>     *     - |messageClass| DOMString: the message class of received message
> +   *     - |receiver| DOMString: the phone numbers of receiver

s/numbers/number

@@ +40,5 @@
>     *
>     *   - If |type| == "mms", we also need:
>     *     - |delivery| DOMString: the delivery state of received message
>     *     - |deliveryStatus| DOMString Array: the delivery status of received message
>     *     - |receivers| DOMString Array: the phone numbers of receivers

I think you also need to add a |receiver| here for MMS. Right? Please just move |receiver| up to the common part.

::: dom/mobilemessage/src/ril/SmsService.h
@@ +20,5 @@
>    NS_DECL_NSISMSSERVICE
>    SmsService();
>  
>  protected:
> +  // TODO: Bug 814635 - WebSMS API: support multiple sim cards

Please redirect this to:

// TODO: Bug 854326 - B2G Multi-SIM: support multiple SIM cards for SMS/MMS
Attachment #760844 - Flags: review?(gene.lian)
Comment on attachment 760841 [details] [diff] [review]
patch 2/7 - create multiple intances of RadioInterface

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

Looks great, but I would like to see my comment being answered (as well as idl change) first.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +293,5 @@
> +  getRadioInterface: function getRadioInterface(clientId) {
> +    return this.radioInterfaces[clientId];
> +  },
> +
> +  updateRILNetworkInterface: function updateRILNetworkInterface() {

Move this to nsIRadioInterface?
Attachment #760841 - Flags: review?(htsai)
Comment on attachment 760842 [details] [diff] [review]
patch 3/7 - dispatch RIL messages

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +469,5 @@
> +    }
> +  },
> +
> +  sendTargetMessage: function sendTargetMessage(topic, message, options) {
> +

extra line.

@@ +507,5 @@
> +    target.sendAsyncMessage(requestType, options);
> +  }
> +};
> +
> +function RadioInterfaceLayer() {

I think there's some problem in this patch,
I believe these RadioInterface(Layer) have shown in previous patches.
Attachment #760842 - Flags: review?(allstars.chh)
Attachment #760845 - Flags: review?(allstars.chh) → review+
Comment on attachment 760842 [details] [diff] [review]
patch 3/7 - dispatch RIL messages

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

Having a separate MessageManager is a good idea! However, this patch seems cover part2. Could you provide the right patch, then asking for review again?
Attachment #760842 - Flags: review?(htsai)
Comment on attachment 760843 [details] [diff] [review]
patch 4/7 - fix GonkGPSGeolocationProvider

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +575,5 @@
> +  nsCOMPtr<nsIRadioInterfaceLayer> ril = do_GetService("@mozilla.org/ril;1");
> +  if (ril) {
> +    // TODO: Bug 878748 - B2G GPS: acquire correct RadioInterface instance in
> +    // MultiSIM configuration
> +    ril->GetRadioInterface(0, getter_AddRefs(mRadioInterface));

ril->GetRadioInterface(0 /* index */, getter_AddRefs(mRadioInterface));
Attachment #760843 - Flags: review?(kchen) → review+
Comment on attachment 760847 [details] [diff] [review]
patch 7/7: fix debug messages as well

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

Looks good. Thank you.
Attachment #760847 - Flags: review?(htsai) → review+
Attachment #760847 - Flags: review?(allstars.chh) → review+
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #12)
> patch 1/7 - interface changes
> Review of attachment 760840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> 
> Why move updateRILNetworkInterface to here?
> I think even without this, other modules still can do the same thing by
> nsIRadioInterfaceLayer.getRadioInterface(clientId).updateRILNetworkInterface

If you get the clientId, yes.  The method is for NetworkManager.  NetworkManager has to call back to RIL whenever a WiFi network interface changes its state.  It's a WiFi interface changed its state, not a RIL one, so you can't really have a clientId in this situation.  In MultiSIM and without a connection manager, you have to notify all radio interfaces this state transition.

That's definitely not a final solution, and for this reason, I agree we may stick with current way instead of inventing another temporary interface method.
(In reply to Gene Lian [:gene] from comment #15)
> patch 5/7 - fix MobileMessage
> Review of attachment 760844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since you're hoping to let the callers decide the |.sender| when calling
> |.saveSendingMessage()| and the |.receiver| when calling
> |.saveReceivedMessage()|. Where do you assign values to them for *MMS*? I
> didn't see that part. Did I miss something?

No, it's me.  I missed them.

> Also, please add sanity checks at the top of |.saveSendingMessage()| and
> |.saveReceivedMessage()| for checking |.sender| and |.receiver| respectively.

What kind of sanity check do you want here?  Considering you may not have MSISDN in some cases.

> ::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
> @@ +31,5 @@
> >    /**
> >     * |aMessage| Object: should contain the following properties for internal use:
> >     *   - |type| DOMString: "sms" or "mms"
> >     *   - |sender| DOMString: the phone number of sender
> >     *   - |timestamp| Number: the timestamp of received message
> 
> Please add the comment for |receiver| up to here.

We have |receiver| for SMS and |receivers| for MMS separately.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #18)
> patch 3/7 - dispatch RIL messages
> Review of attachment 760842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Having a separate MessageManager is a good idea! However, this patch seems
> cover part2. Could you provide the right patch, then asking for review again?

That's a problem in hg generated patch.  Download it and apply to your git tree and you'll see I had added only one line and removed several inside RadioInterfaceLayer().
Comment on attachment 760842 [details] [diff] [review]
patch 3/7 - dispatch RIL messages

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

Great! Thank you~~

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +361,5 @@
> +    messageQueue.push(msg);
> +  },
> +
> +  _resendQueuedTargetMessage: function _resendQueuedTargetMessage() {
> +    this.ready = true;

Not that big deal, simply curious why setting this.ready to true within this function, instead of doing so right after line #426. :) I am thinking what if _resendQueuedTargetMessage() is called at the unexpected timing, e.g. kSysMsgListenerReadyObserverTopic isn't notified? Should the value become true?

But, yes, the patch works.
Attachment #760842 - Flags: review+
1) Keep RadioInterface::updateRILNetworkInterface,
2) move nsIRilMobileMessageDatabaseService.idl changes to this patch as well.
Attachment #760840 - Attachment is obsolete: true
Attachment #760840 - Flags: review?(htsai)
Attachment #760840 - Flags: review?(allstars.chh)
Attachment #763970 - Flags: superreview?(htsai)
1) Keep RadioInterface::updateRILNetworkInterface,
2) Address review comment 13.
Attachment #760841 - Attachment is obsolete: true
Attachment #763971 - Flags: review?(htsai)
Rebase & remove an extra empty line only.
Attachment #760842 - Attachment is obsolete: true
Attachment #763972 - Flags: review?(allstars.chh)
Attachment #763970 - Attachment description: patch 1/7 - interface changes : v2 → patch 1/8 - interface changes : v2
Attachment #763971 - Attachment description: patch 2/7 - create multiple intances of RadioInterface : v2 → patch 2/8 - create multiple intances of RadioInterface : v2
Attachment #763972 - Attachment description: patch 3/7 - dispatch RIL messages : v2 → patch 3/8 - dispatch RIL messages : v2
Address comment 19.
Attachment #760843 - Attachment is obsolete: true
Attachment #763973 - Flags: review+
1) Address comment 15,
2) Move nsIRilMobileMessageDatabaseService.idl changes to part 1.
Attachment #760844 - Attachment is obsolete: true
Attachment #763974 - Flags: review?(gene.lian)
Attachment #763976 - Flags: review?(allstars.chh)
fix part numbering in commit summary only.
Attachment #760845 - Attachment is obsolete: true
Attachment #763977 - Flags: review+
Attachment #763977 - Attachment description: patch 6/8 - fix test cases : v2 → patch 7/8 - fix test cases : v2
rebase only.
Attachment #760847 - Attachment is obsolete: true
Attachment #763978 - Flags: review+
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #24)
> Comment on attachment 760842 [details] [diff] [review]
> patch 3/7 - dispatch RIL messages
> 
> Review of attachment 760842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great! Thank you~~
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +361,5 @@
> > +    messageQueue.push(msg);
> > +  },
> > +
> > +  _resendQueuedTargetMessage: function _resendQueuedTargetMessage() {
> > +    this.ready = true;
> 
> Not that big deal, simply curious why setting this.ready to true within this
> function, instead of doing so right after line #426. :) I am thinking what
> if _resendQueuedTargetMessage() is called at the unexpected timing, e.g.
> kSysMsgListenerReadyObserverTopic isn't notified? Should the value become
> true?

I think the reason we have to queue up these messages is Gaia takes time to warm up.  The messages should never be delivered unless we have received a "system-message-listener-ready" signal, AND we have no more reason to queue message after that signal.  The |ready| flag should control the two behaviours.  Keeping the flexibility to deliver messages without setting |ready| to true doesn't really mean anything.
Attachment #763976 - Flags: review?(allstars.chh) → review+
Attachment #763970 - Flags: superreview?(htsai) → superreview+
Attachment #763971 - Flags: review?(htsai) → review+
Comment on attachment 763972 [details] [diff] [review]
patch 3/8 - dispatch RIL messages : v2

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

Mostly looks good.
But why MessageManager implement nsIObserve?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +239,1 @@
>                                           Ci.nsIObserver]),

You call this object as MessageManager,
but it also manages the lifecycle of this module by implementing nsIObserve, which sounds strange to me.

@@ +256,5 @@
> +    Services.obs.addObserver(this, kSysMsgListenerReadyObserverTopic, false);
> +    this._registerMessageListeners();
> +  },
> +
> +  _shutdown: function _shutdown() {

The same argmument above,

init is called by RadioInterface, however shutdown is called by MessageManager itself.

@@ +544,5 @@
> +
> +  observe: function observe(subject, topic, data) {
> +    // Nothing to do now. Just for profile-after-change.
> +  },
> +

Can't we let RadioInterfaceLayer do observe, instead of MessageMaanger?
Attachment #763972 - Flags: review?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #34)
> Comment on attachment 763972 [details] [diff] [review]
> patch 3/8 - dispatch RIL messages : v2
> 
> Review of attachment 763972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly looks good.
> But why MessageManager implement nsIObserve?

It receives observer event "xpcom-shutdown" and "system-message-listener-ready".

> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +239,1 @@
> >                                           Ci.nsIObserver]),
> 
> You call this object as MessageManager,
> but it also manages the lifecycle of this module by implementing nsIObserve,
> which sounds strange to me.

Don't know what you mean.  Do you want me turn it into a lazy getter?

> @@ +256,5 @@
> > +    Services.obs.addObserver(this, kSysMsgListenerReadyObserverTopic, false);
> > +    this._registerMessageListeners();
> > +  },
> > +
> > +  _shutdown: function _shutdown() {
> 
> The same argmument above,
> 
> init is called by RadioInterface, however shutdown is called by
> MessageManager itself.

s/RadioInterface/RadioInterfaceLayer/
And, what's wrong with this?

> @@ +544,5 @@
> > +
> > +  observe: function observe(subject, topic, data) {
> > +    // Nothing to do now. Just for profile-after-change.
> > +  },
> > +
> 
> Can't we let RadioInterfaceLayer do observe, instead of MessageMaanger?

I don't want to entangle everything in RadioInterfaceLayer again.  That's why I have moved MessageMaanger out of it.  Maybe I should have also moved _send{Icc,Voicemail,Telephony,CellBroadcast,MobileConnection}Message functions to MessageManager as well.
1) move MessageManager into a lazy getter
2) move _send{Icc,Voicemail,Telephony,CellBroadcast,MobileConnection}Message functions to MessageManager as well.
Attachment #763972 - Attachment is obsolete: true
Attachment #764574 - Flags: review+
Rebase only
Attachment #763974 - Attachment is obsolete: true
Attachment #763974 - Flags: review?(gene.lian)
Attachment #764575 - Flags: review?(gene.lian)
accommodate to latest changes made in part 3/8
Attachment #763978 - Attachment is obsolete: true
Attachment #764576 - Flags: review+
Comment on attachment 764575 [details] [diff] [review]
patch 5/8 - fix MobileMessage : v3

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

Nice patches but two minor things I hope you can also add to them:

1. dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl

  /**
   * |aMessage| Object: should contain the following properties for internal use:
   *   ...
   *
   *   - If |type| == "sms", we also need:
   *     ...
   *     - |receiver| DOMString: ???
   *
   *   - If |type| == "mms", we also need:
   *     ...
   *     - |receivers| DOMString Array: the phone numbers of receivers
   *     - |msisdn| DOMString: ???
   *     ...
   *
  long saveReceivedMessage(...);

  /**
   * |aMessage| Object: should contain the following properties for internal use:
   *   ...
   *   - |sender| DOMString: ???
   *
   *   - If |type| == "sms", we also need:
   *     - |receiver| DOMString: the phone number of receiver
   *
   *   - If |type| == "mms", we also need:
   *     - |receivers| DOMString Array: the phone numbers of receivers
   */
  long saveSendingMessage(...);

2. Please see dom/mobilemessage/src/ril/MobileMessageDatabaseService.js. Let's add sanity checks on top of the .saveReceivedMessage(...) and saveSendingMessage(...) respectively. As you mentioned, we might need to expose the DB interface to Android in the (far) future. That would be good if we can add more checks and comments in advance to avoid wrong usages.

::: dom/mobilemessage/src/ril/MmsService.js
@@ +99,5 @@
>                                     "nsIUUIDGenerator");
>  
> +XPCOMUtils.defineLazyGetter(this, "gRadioInterface", function () {
> +  let ril = Cc["@mozilla.org/ril;1"].getService(Ci["nsIRadioInterfaceLayer"]);
> +  // TODO: Bug 814635 - WebSMS API: support multiple sim cards

Please redirect this to:

// TODO: Bug 854326 - B2G Multi-SIM: support multiple SIM cards for SMS/MMS
Attachment #764575 - Flags: review?(gene.lian) → review+
rebase
Attachment #763971 - Attachment is obsolete: true
Attachment #770058 - Flags: review+
rebase
Attachment #764574 - Attachment is obsolete: true
Attachment #770059 - Flags: review+
Rebase
Attachment #764575 - Attachment is obsolete: true
Attachment #770061 - Flags: review+
rebase
Attachment #764576 - Attachment is obsolete: true
Attachment #770062 - Flags: review+
Blocks: 907585
No longer blocks: 854326
You need to log in before you can comment on or make changes to this bug.