929864 - B2G DSDS: correctly update RILNetworkInterface when wifi connection changes

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Hsin-Yi Tsai [:hsinyi]]

When wifi connection changes, currently we update the network interface only for radioInterface(0). We need to improve that for DSDS scenario.


[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkManager.js#247

Comment 1

[Hsin-Yi Tsai [:hsinyi]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #0)
> When wifi connection changes, currently we update the network interface only
> for radioInterface(0). We need to improve that for DSDS scenario.
> 
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkManager.
> js#247

And here: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkManager.js#275

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: update-all-ril-network-ifaces.patch

In this patch, we update all radio interfaces' ril network interface when wifi is connected/disconnected, then each of the radio interfaces will check if they need to do anything about it.

BTW, do we have a preference for data connection default service id, as Bug 926302 does for other services? I was thinking only updating the ril network interface for the default one.. but the current way will work if we are going to support dual-active phones. Any suggestions?

Comment 3

[Hsin-Yi Tsai [:hsinyi]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #2)
> Created attachment 820866 [details] [diff] [review]
> update-all-ril-network-ifaces.patch
> 
> In this patch, we update all radio interfaces' ril network interface when
> wifi is connected/disconnected, then each of the radio interfaces will check
> if they need to do anything about it.
> 
> BTW, do we have a preference for data connection default service id, as Bug
> 926302 does for other services? I was thinking only updating the ril network
> interface for the default one.. but the current way will work if we are
> going to support dual-active phones. Any suggestions?

No, at this moment, there's no preference for data connection default service id as in this corresponding API, we don't have the concept of 'default' for data connection. But for gaia, there might be a setting key recording the user preference for indication of the primary data connection service. 

The current solution looks simple, feasible and flexible to me. I am a proponent of that. :)

Comment 4

[Edgar Chen [:edgar]]

Comment on attachment 820866 [details] [diff] [review]
update-all-ril-network-ifaces.patch

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

::: dom/system/gonk/NetworkManager.js
@@ +32,5 @@
>                                     "nsIDNSService");
>  
> +XPCOMUtils.defineLazyGetter(this, "gNumRadioInterfaces", function() {
> +  try {
> +    return Services.prefs.getIntPref("ril.numRadioInterfaces");

NetworkManager is in chrome process. Do we need to access preference directly or we should use the |RadioInterfaceLayer.numRadioInterfaces|?

Comment 5

[Hsin-Yi Tsai [:hsinyi]]

Comment on attachment 820866 [details] [diff] [review]
update-all-ril-network-ifaces.patch

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

::: dom/system/gonk/NetworkManager.js
@@ +30,5 @@
>  XPCOMUtils.defineLazyServiceGetter(this, "gDNSService",
>                                     "@mozilla.org/network/dns-service;1",
>                                     "nsIDNSService");
>  
> +XPCOMUtils.defineLazyGetter(this, "gNumRadioInterfaces", function() {

After bug 920551, this needs to be guarded by #ifdef MOZ_B2G_RIL.

@@ +32,5 @@
>                                     "nsIDNSService");
>  
> +XPCOMUtils.defineLazyGetter(this, "gNumRadioInterfaces", function() {
> +  try {
> +    return Services.prefs.getIntPref("ril.numRadioInterfaces");

We've been encountering various problems with Bug 926302. Vicamo provided a nice solution eventually. After that, in chrome, we could still use getService(Ci.nsIRadioInterfaceLayer).numRadioInterfaces, while in content, we have no way excepting reading preference. 

Please help change this to |getService(Ci.nsIRadioInterfaceLayer).numRadioInterfaces|. I am sorry about that.

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #4)
> Comment on attachment 820866 [details] [diff] [review]
> update-all-ril-network-ifaces.patch
> 
> Review of attachment 820866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +32,5 @@
> >                                     "nsIDNSService");
> >  
> > +XPCOMUtils.defineLazyGetter(this, "gNumRadioInterfaces", function() {
> > +  try {
> > +    return Services.prefs.getIntPref("ril.numRadioInterfaces");
> 
> NetworkManager is in chrome process. Do we need to access preference
> directly or we should use the |RadioInterfaceLayer.numRadioInterfaces|?

Yes, you are right. I'd use |this.mRIL.numRadioInterfaces| directly.
Thanks!

Comment 7

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #5)
> Comment on attachment 820866 [details] [diff] [review]
> update-all-ril-network-ifaces.patch
> 
> Review of attachment 820866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +30,5 @@
> >  XPCOMUtils.defineLazyServiceGetter(this, "gDNSService",
> >                                     "@mozilla.org/network/dns-service;1",
> >                                     "nsIDNSService");
> >  
> > +XPCOMUtils.defineLazyGetter(this, "gNumRadioInterfaces", function() {
> 
> After bug 920551, this needs to be guarded by #ifdef MOZ_B2G_RIL.

I think I will get rid of gNumRadioInterfaces and use |this.mRIL.numRadioInterfaces| directly,

> @@ +32,5 @@
> >                                     "nsIDNSService");
> >  
> > +XPCOMUtils.defineLazyGetter(this, "gNumRadioInterfaces", function() {
> > +  try {
> > +    return Services.prefs.getIntPref("ril.numRadioInterfaces");
> 
> We've been encountering various problems with Bug 926302. Vicamo provided a
> nice solution eventually. After that, in chrome, we could still use
> getService(Ci.nsIRadioInterfaceLayer).numRadioInterfaces, while in content,
> we have no way excepting reading preference. 
> 
> Please help change this to
> |getService(Ci.nsIRadioInterfaceLayer).numRadioInterfaces|. I am sorry about
> that.

Thank you! :)

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: update-all-ril-network-ifaces-v2.patch

Review comments in Comment 4 and Comment 5 addressed:
1. Use |this.mRIL.numRadioInterfaces| directly
2. Add f= and r= in patch title

Comment 9

[Hsin-Yi Tsai [:hsinyi]]

Comment on attachment 821542 [details] [diff] [review]
update-all-ril-network-ifaces-v2.patch

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

Looks good. Thanks.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Try results: https://tbpl.mozilla.org/?tree=Try&rev=028c802d1038

Comment 11

[Ed Morley [:emorley]]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/f0bd0d0d0cd5

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0bd0d0d0cd5