Closed Bug 1207066 Opened 6 years ago Closed 3 years ago

[NetworkManager] implement nsINetworkInterface.activate()/deactivate() in wifi network interface

Categories

(Firefox OS Graveyard :: Wifi, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: jessica, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

In bug 911713, we are introducing new two methods in nsINetworkInterface: activate() and deactivate(). The purpose for this is to let NetworkManager handle all networks connection as well as policy control in one same place, so NetworkManager is the one that decides which network gets activated and which gets deactivated.

Therefore, we need wifi network interface to support this two methods, and it should wait for "activate()" to be called to actually connect to wifi.

Please refer to this diagram for the current design in mind: http://goo.gl/j8TDng
Blocks: 904542
Summary: [NetworkManager] implement nsINetworkInterface.active()/deactive in wifi network interface → [NetworkManager] implement nsINetworkInterface.activate()/deactivate() in wifi network interface
Henry and Tim,
We need your support on this bug.
Assignee: nobody → tihuang
Attached patch bug_1207066_v1.patch (obsolete) — Splinter Review
Attachment #8699938 - Flags: feedback?(hchang)
Comment on attachment 8699938 [details] [diff] [review]
bug_1207066_v1.patch

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

The implementation looks definitely are on the right track! What I am uncertain is the use of observer topic "profile-change-teardown".

::: dom/wifi/WifiWorker.js
@@ +1114,4 @@
>          // command blocking in control thread until socket timeout.
>          notify("stopconnectioninfotimer");
>  
> +        manager.deactivate(function() {

Should we do this on our own? I assume NetworkManager is supposed to do this properly?

@@ +1587,5 @@
>      return sdkVersion;
>    }
>  
> +  manager.activate = function(aCallback) {
> +    // If the interface has activated, return.

has been activated?

@@ +1594,5 @@
> +      return;
> +    }
> +
> +    // If the supplicant does not start when calling activate, we only set the
> +    // flag to indicate the wifi interface has activated. The rest activating

has been activated?

@@ +1603,5 @@
> +      return;
> +    }
> +
> +    wifiCommand.enableNetwork("all", false, function() {});
> +    wifiCommand.saveConfig(function() {

Should we do "saveConfing" in the callback of enableNetwork?

@@ +1933,4 @@
>  
>    Services.obs.addObserver(this, kMozSettingsChangedObserverTopic, false);
>    Services.obs.addObserver(this, "xpcom-shutdown", false);
> +  Services.obs.addObserver(this, "profile-change-teardown", false);

According to 

https://dxr.mozilla.org/mozilla-central/source/toolkit/profile/notifications.txt#9

"profile-change-teardown" is defined as the following:

  All async activity must be stopped in this phase. Typically,
  the application level observer will close all open windows.
  This is the last phase in which the subject's vetoChange()
  method may still be called.
  The next notification will be either
  profile-change-teardown-veto or profile-before-change.

I am not sure what it means to this module actually. May I know how do you find this topic and choose it? I typically use xpcom-shutdown to capture the XPCOM "will shutdown" event.
Attachment #8699938 - Flags: feedback?(hchang) → feedback+
(In reply to Henry Chang [:henry] from comment #3)
> Comment on attachment 8699938 [details] [diff] [review]
> bug_1207066_v1.patch
> 
> Review of attachment 8699938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The implementation looks definitely are on the right track! What I am
> uncertain is the use of observer topic "profile-change-teardown".
> 
> ::: dom/wifi/WifiWorker.js
> @@ +1114,4 @@
> >          // command blocking in control thread until socket timeout.
> >          notify("stopconnectioninfotimer");
> >  
> > +        manager.deactivate(function() {
> 
> Should we do this on our own? I assume NetworkManager is supposed to do this
> properly?

We perform a deactivate here since the wpa_supplicant is going to be terminated before we disable the wifi network interface. In other words, we cannot write the wpa_supplicant.conf at the time that network manager deactivates the wifi network interface because the wpa_supplicant had been terminated at this moment. Hence, we need to deactivate the interface before the termination of the wpa_supplicant when disabling Wifi.

> 
> @@ +1587,5 @@
> >      return sdkVersion;
> >    }
> >  
> > +  manager.activate = function(aCallback) {
> > +    // If the interface has activated, return.
> 
> has been activated?
> 
> @@ +1594,5 @@
> > +      return;
> > +    }
> > +
> > +    // If the supplicant does not start when calling activate, we only set the
> > +    // flag to indicate the wifi interface has activated. The rest activating
> 
> has been activated?
> 
> @@ +1603,5 @@
> > +      return;
> > +    }
> > +
> > +    wifiCommand.enableNetwork("all", false, function() {});
> > +    wifiCommand.saveConfig(function() {
> 
> Should we do "saveConfing" in the callback of enableNetwork?

The reason we save config here is that we want to make sure the state of networks, enable or disable, is synchronized between wpa_supplicant and wpa_supplicant.conf. Or don't we have to make sure they are sync?

> 
> @@ +1933,4 @@
> >  
> >    Services.obs.addObserver(this, kMozSettingsChangedObserverTopic, false);
> >    Services.obs.addObserver(this, "xpcom-shutdown", false);
> > +  Services.obs.addObserver(this, "profile-change-teardown", false);
> 
> According to 
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/profile/notifications.
> txt#9
> 
> "profile-change-teardown" is defined as the following:
> 
>   All async activity must be stopped in this phase. Typically,
>   the application level observer will close all open windows.
>   This is the last phase in which the subject's vetoChange()
>   method may still be called.
>   The next notification will be either
>   profile-change-teardown-veto or profile-before-change.
> 
> I am not sure what it means to this module actually. May I know how do you
> find this topic and choose it? I typically use xpcom-shutdown to capture the
> XPCOM "will shutdown" event.

The first time that I learned this notification is from [1]. And then, I found this page[2] describes "profile-change-teardown" as a part of shutdown. And it seems that the "xpcom-shutdown" would not be called when I shutdown my aries, but "profile-change-teardown" does. So I choose this "profile-change-teardown" to deactivate wifi network.

Perhaps the network manager will take care the deactivating of network interfaces when shutdown the system, but we don't know this yet. So we put a deactivate here for making sure the interface is going to be deactivated when shutdown the system.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmService.jsm#57 
[2] https://developer.mozilla.org/en/docs/Observer_Notifications
(In reply to Tim Huang[:timhuang] from comment #4)
> (In reply to Henry Chang [:henry] from comment #3)
> > Comment on attachment 8699938 [details] [diff] [review]
> > bug_1207066_v1.patch
> > 
> > Review of attachment 8699938 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > 
> > @@ +1587,5 @@
> > >      return sdkVersion;
> > >    }
> > >  
> > > +  manager.activate = function(aCallback) {
> > > +    // If the interface has activated, return.
> > 
> > has been activated?
> > 
> > @@ +1594,5 @@
> > > +      return;
> > > +    }
> > > +
> > > +    // If the supplicant does not start when calling activate, we only set the
> > > +    // flag to indicate the wifi interface has activated. The rest activating
> > 
> > has been activated?
> > 
> > @@ +1603,5 @@
> > > +      return;
> > > +    }
> > > +
> > > +    wifiCommand.enableNetwork("all", false, function() {});
> > > +    wifiCommand.saveConfig(function() {
> > 
> > Should we do "saveConfing" in the callback of enableNetwork?
> 
> The reason we save config here is that we want to make sure the state of
> networks, enable or disable, is synchronized between wpa_supplicant and
> wpa_supplicant.conf. Or don't we have to make sure they are sync?
> 

I meant, shouldn't it be:

wifiCommand.enableNetwork("all", false, function() {
  wifiCommand.saveConfig(function() {
});
(In reply to Henry Chang [:henry] from comment #5)
> (In reply to Tim Huang[:timhuang] from comment #4)
> 
> I meant, shouldn't it be:
> 
> wifiCommand.enableNetwork("all", false, function() {
>   wifiCommand.saveConfig(function() {
> });

Oh, I misunderstood your words. Yes, I agree with your suggestion that we should put saveConfig in the callback of enableNetwork.
Attachment #8699938 - Attachment is obsolete: true
I've been shifted to other project, leaving this bug to nobody.
Assignee: tihuang → nobody
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.