Closed
Bug 1207066
Opened 9 years ago
Closed 7 years ago
[NetworkManager] implement nsINetworkInterface.activate()/deactivate() in wifi network interface
Categories
(Firefox OS Graveyard :: Wifi, defect)
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
Reporter | ||
Updated•9 years ago
|
Blocks: 904542
Summary: [NetworkManager] implement nsINetworkInterface.active()/deactive in wifi network interface → [NetworkManager] implement nsINetworkInterface.activate()/deactivate() in wifi network interface
Comment 1•9 years ago
|
||
Henry and Tim,
We need your support on this bug.
Updated•9 years ago
|
Assignee: nobody → tihuang
Comment 2•9 years ago
|
||
Attachment #8699938 -
Flags: feedback?(hchang)
Comment 3•9 years ago
|
||
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+
Comment 4•9 years ago
|
||
(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
Comment 5•9 years ago
|
||
(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() {
});
Comment 6•9 years ago
|
||
(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.
Comment 7•9 years ago
|
||
Updated•9 years ago
|
Attachment #8699938 -
Attachment is obsolete: true
Comment 8•9 years ago
|
||
I've been shifted to other project, leaving this bug to nobody.
Assignee: tihuang → nobody
Comment 9•7 years ago
|
||
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•