Closed Bug 929864 Opened 7 years ago Closed 7 years ago

B2G DSDS: correctly update RILNetworkInterface when wifi connection changes

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: hsinyi, Assigned: jessica)

References

Details

Attachments

(1 file, 1 obsolete file)

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
(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
Assignee: nobody → jjong
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?
Attachment #820866 - Flags: feedback?(htsai)
Attachment #820866 - Flags: feedback?(echen)
(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 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 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.
Attachment #820866 - Flags: feedback?(htsai) → feedback+
Attachment #820866 - Flags: feedback?(echen) → feedback+
(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!
(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! :)
Review comments in Comment 4 and Comment 5 addressed:
1. Use |this.mRIL.numRadioInterfaces| directly
2. Add f= and r= in patch title
Attachment #820866 - Attachment is obsolete: true
Attachment #821542 - Flags: review?(htsai)
Comment on attachment 821542 [details] [diff] [review]
update-all-ril-network-ifaces-v2.patch

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

Looks good. Thanks.
Attachment #821542 - Flags: review?(htsai) → review+
https://hg.mozilla.org/mozilla-central/rev/f0bd0d0d0cd5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: b2g-dsds-1.4
No longer blocks: b2g-dsds-1.4
You need to log in before you can comment on or make changes to this bug.