Vicamo Yang [:vicamo][:vyang] Assignee
	Description • 12 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 1 • 12 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 2 • 12 years ago

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

	Dietrich Ayala (:dietrich)
	Comment 3 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 7 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 9 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 10 • 11 years ago

Then we have:

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

as debug message tags.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 11 • 11 years ago

I have local all green Marionette test results, but still a full try is ongoing in https://tbpl.mozilla.org/?tree=Try&rev=0f0dcf6f8d32

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 12 • 11 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 13 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 14 • 11 years ago

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?

	Gene Lian (I already quit Mozilla)
	Comment 15 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 16 • 11 years ago

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?

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 17 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 18 • 11 years ago

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?

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 19 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 20 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 21 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 22 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 23 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 24 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 25 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 26 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 27 • 11 years ago

Rebase & remove an extra empty line only.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 28 • 11 years ago

Address comment 19.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 29 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 31 • 11 years ago

fix part numbering in commit summary only.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 32 • 11 years ago

rebase only.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 33 • 11 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 34 • 11 years ago

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?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 35 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 36 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 37 • 11 years ago

Rebase only

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 38 • 11 years ago

accommodate to latest changes made in part 3/8

	Gene Lian (I already quit Mozilla)
	Comment 39 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 40 • 11 years ago

rebase

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 41 • 11 years ago

rebase

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 42 • 11 years ago

Rebase

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 43 • 11 years ago

rebase

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 44 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 45 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 46 • 11 years ago

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