814581 - B2G Multi-SIM: add MSimRadioInterfaceLayer

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

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.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://github.com/allstarschh/releases-mozilla-central/tree/bugzilla/814581/master
need to wait for Bug 814579 to be merged first.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://github.com/multi-sim/gonk-misc/pull/1

Comment 3

[Dietrich Ayala (:dietrich)]

Blocking-, no longer targeting Leo release. Product will retarget for future release.

Comment 4

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

Attachment: patch 1/7 - interface changes

Comment 5

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

Attachment: patch 2/7 - create multiple intances of RadioInterface

Comment 6

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

Attachment: patch 3/7 - dispatch RIL messages

Comment 7

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

Attachment: patch 4/7 - fix GonkGPSGeolocationProvider

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.

Comment 8

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

Attachment: patch 5/7 - fix MobileMessage

Comment 9

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

Attachment: patch 6/7 - fix test cases

That's the only test script that involves nsIRadioInterfaceLayer changes.

Comment 10

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

Attachment: patch 7/7: fix debug messages as well

Then we have:

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

as debug message tags.

Comment 11

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

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 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 14

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

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 15

[Gene Lian (I already quit Mozilla)]

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

Comment 16

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

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?

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 18

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

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?

Comment 19

[Kan-Ru Chen [:kanru] (UTC+9)]

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));

Comment 20

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

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.

Comment 21

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

(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.

Comment 22

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

(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.

Comment 23

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

(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 24

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

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.

Comment 25

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

Attachment: patch 1/8 - interface changes : v2

1) Keep RadioInterface::updateRILNetworkInterface,
2) move nsIRilMobileMessageDatabaseService.idl changes to this patch as well.

Comment 26

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

Attachment: patch 2/8 - create multiple intances of RadioInterface : v2

1) Keep RadioInterface::updateRILNetworkInterface,
2) Address review comment 13.

Comment 27

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

Attachment: patch 3/8 - dispatch RIL messages : v2

Rebase & remove an extra empty line only.

Comment 28

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

Attachment: patch 4/8 - fix GonkGPSGeolocationProvider : v2

Address comment 19.

Comment 29

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

Attachment: patch 5/8 - fix MobileMessage : v2

1) Address comment 15,
2) Move nsIRilMobileMessageDatabaseService.idl changes to part 1.

Comment 30

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

Attachment: patch 6/8 - fix NetworkManager

Comment 31

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

Attachment: patch 7/8 - fix test cases : v2

fix part numbering in commit summary only.

Comment 32

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

Attachment: patch 8/8 - fix debug messages as well : v2

rebase only.

Comment 33

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

(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.

Comment 34

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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?

Comment 35

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

(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.

Comment 36

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

Attachment: patch 3/8 - dispatch RIL messages : v3

1) move MessageManager into a lazy getter
2) move _send{Icc,Voicemail,Telephony,CellBroadcast,MobileConnection}Message functions to MessageManager as well.

Comment 37

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

Attachment: patch 5/8 - fix MobileMessage : v3

Rebase only

Comment 38

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

Attachment: patch 8/8 - fix debug messages as well : v3

accommodate to latest changes made in part 3/8

Comment 39

[Gene Lian (I already quit Mozilla)]

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

Comment 40

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

Attachment: patch 2/8 - create multiple intances of RadioInterface : v3

rebase

Comment 41

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

Attachment: patch 3/8 - dispatch RIL messages : v4

rebase

Comment 42

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

Attachment: patch 5/8 - fix MobileMessage : v4

Rebase

Comment 43

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

Attachment: patch 8/8 - fix debug messages as well : v4

rebase

Comment 44

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

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

Comment 45

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

https://hg.mozilla.org/projects/birch/rev/0d16db27afe5
https://hg.mozilla.org/projects/birch/rev/642d12dde1eb
https://hg.mozilla.org/projects/birch/rev/b75655cb5c3a
https://hg.mozilla.org/projects/birch/rev/65c616b5741b
https://hg.mozilla.org/projects/birch/rev/8f778ff4bd1c
https://hg.mozilla.org/projects/birch/rev/7aba75075eee
https://hg.mozilla.org/projects/birch/rev/697be373be37
https://hg.mozilla.org/projects/birch/rev/d18d02172bb5

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d16db27afe5
https://hg.mozilla.org/mozilla-central/rev/642d12dde1eb
https://hg.mozilla.org/mozilla-central/rev/b75655cb5c3a
https://hg.mozilla.org/mozilla-central/rev/65c616b5741b
https://hg.mozilla.org/mozilla-central/rev/8f778ff4bd1c
https://hg.mozilla.org/mozilla-central/rev/7aba75075eee
https://hg.mozilla.org/mozilla-central/rev/697be373be37
https://hg.mozilla.org/mozilla-central/rev/d18d02172bb5