729877 - B2G Wifi: hook up to settings API

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[John Hammink]

Latest pull (gonk, gecko, gaia) 2/21

In Settings, if you uncheck "Wifi" and restart the phone, Wifi still works (e.g. it is still possible to browse to a remote page off the device).

Comment 1

[Gene Lian (I already quit Mozilla)]

Philipp,

I'd like to ask a quick question about where to listen to Settings API (i.e., |wifi.enabled|) to control the Wifi. Should we put the "mozsettings-changed" listening logic in the content process (DOMWifiManager.js) or the parent process (WifiWorker.js)? I think if there is only a single |wifi.enabled| value, it sounds more reasonable to put it in the parent process. Right?

Thanks for your reply in advance!

Comment 2

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch

Hi Blake :)

I've uploaded a very first patch to address this issue (not yet cleaned up and tested). Could you please give me some feedbacks on this patch to let me make sure I'm on the right way to do it?

Basically, I stripped out the |setEnabled()| and |enabled| from the DOM IDL and content process (DOMWifiManager.js). Also, I added the the Settings API logic in the parent process (WifiWorker.js), which would listen to the |wifi.enabled| attribute to turn on or off the Wifi.

My biggest concern is the complicated logic in the WifiWorker.setWifiEnabled() for solving multiple requests. I'm kind of confused about how to deal with them after applying the Settings API logic. Could your please give some suggestions on this? Really appreciate.

Thanks,
Gene

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 644270 [details] [diff] [review]
WIP Patch

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

This looks like a decent approach overall, some questions below.

::: dom/wifi/WifiWorker.js
@@ +23,5 @@
>  
>  XPCOMUtils.defineLazyServiceGetter(this, "gNetworkManager",
>                                     "@mozilla.org/network/manager;1",
>                                     "nsINetworkManager");
> +                                   

Super nitpick: please nuke this trailing whitespace.

@@ +2001,5 @@
> +  // nsIObserver implementation
> +  observe: function observe(subject, topic, data) {
> +    switch (topic) {
> +      case kMozSettingsChangedObserverTopic:
> +        let setting = JSON.parse(data);

Nit: If you're going to declare |setting| here, please put braces around the body of the case statement.

@@ +2004,5 @@
> +      case kMozSettingsChangedObserverTopic:
> +        let setting = JSON.parse(data);
> +        switch (setting.key) {
> +          case "wifi.enabled":
> +            if (WifiManager.enabled == setting.value)

Here and below: === instead of ==, please.

@@ +2020,5 @@
> +
> +  // nsISettingsServiceCallback implementation
> +  handle: function handle(aName, aResult) {
> +    if (aName == "wifi.enabled") {
> +      WifiManager.setWifiEnabled(aResult, function (ok) {

This should probably go through this.setWifiEnabled so that requests to turn on and off wifi in rapid succession are handled properly (and we don't start wpa_supplicant after we unload the driver or something like that).

::: dom/wifi/nsIWifi.idl
@@ -26,5 @@
> -     * onsuccess: Wifi has been successfully activated and can start
> -     *            attempting to connect to networks. request.value will be true.
> -     * onerror: Wifi was not successfully activated. (TODO provide details!)
> -     */
> -    nsIDOMDOMRequest setEnabled(in boolean enabled);

Need to bump the UUID for this interface.

@@ -66,5 @@
>      /**
> -     * TODO Remove in favor of a settings API.
> -     * Returns whether or not wifi is currently enabled.
> -     */
> -    readonly attribute boolean enabled;

If wpa_supplicant dies on its own, it might be possible for the value of the setting to disagree with the actual value of wifi. In cases like that, we send out a notification, but I think we might need some sort of API to handle that case.

Comment 4

[Gene Lian (I already quit Mozilla)]

Sorry for the delay on this issue. I've been spinning around with other basecamp+ issues. Now, this one also becomes a basecamp+ one. :O I should be able to upload a new patch for this issue today. Please stay tuned. :)

Comment 5

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Hi Blake again :)

In addition to fixing the comments at comment #3, changes are summarized as below:

1. In WifiManager, I use a setter/getter to wrap the |WifiManager.enabled|, which can ensure it is always consistent with the |wifi.enabled| setting, so that we don't need to worry about if the wpa_supplicant status could be dynamically changed.

2. I use a |isInternalSet| marker which will be carried in the "mozsettings-changed" event (depending on bug 785298). This is necessary because we don't need to deal with the setting done from WifiManager which could make redundant settings and cause internal chaos.

3. The |_stateRequests| now becomes a simple true/false element array, since we don't need to keep the message being sent back to content processes anymore.

4. In summary, the Gaia App will simply use the |wifi.enabled| to turn on/off the Wifi. Also, Gecko will be in charge of ensuring |wifi.enabled| always consistent with the dynamical system status.

Thanks for your reviews! :)

Comment 6

[Gene Lian (I already quit Mozilla)]

Btw, we used to have a debate that if we really need to remove the |enabled| attribute form Web IDL and completely depend on the |wifi.enabled| setting? Gaia's member said it could be difficult to get the Wifi enabled status from settings because the settings.getLock().get('wifi.enabled') is an async function.

Blake, may I also have your comments on this? Note that in the current patch, I remove the |enabled| attribute from Web IDL.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Gene Lian [:gene] from comment #6)
> Blake, may I also have your comments on this? Note that in the current
> patch, I remove the |enabled| attribute from Web IDL.

I saw that, and I was pretty concerned about it. I think we need to add it back both because its synchronous (and is the companion for wifiUp and wifiDown) and because even if the wifi backend tries as hard as possible to restart the supplicant when it dies, there will still be times when wifi is down and IMO that state should be available to apps.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 654965 [details] [diff] [review]
Patch

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

No + for the .enabled sync API removal and one more comment.

::: dom/wifi/WifiWorker.js
@@ +571,5 @@
> +    _enabled: false,
> +    get enabled() {
> +      return this._enabled;
> +    },
> +    set enabled(aEnabled) {

Nobody should be setting enabled on the manager. Instead, they should be going through setWifiEnabled (at best), so I don't see a need for this (if anything, it could be read-only).

@@ +1356,5 @@
>  
>    messages.forEach((function(msgName) {
>      this._mm.addMessageListener(msgName, this);
>    }).bind(this));
> +  

Uber-nit: nuke the trailing whitespace.

@@ +2205,5 @@
> +    // To avoid WifiWorker setting the wifi again, don't need to deal with
> +    // the "mozsettings-changed" event fired from internal WifiManager.
> +    if (setting.message !== undefined &&
> +        setting.message.isInternalSet !== undefined &&
> +        setting.message.isInternalSet)

No need to test explicitly against undefined, since if it isn't set, undefined evaluates to false. If we had to do something special in the case of undefined === false, then it'd be useful to test explicitly.

That said, multiline conditions mean that we put braces around the body of the if.

::: dom/wifi/nsIWifi.idl
@@ -80,5 @@
>      /**
> -     * TODO Remove in favor of a settings API.
> -     * Returns whether or not wifi is currently enabled.
> -     */
> -    readonly attribute boolean enabled;

Per the comments above, this probably needs to come back.

Comment 9

[Gene Lian (I already quit Mozilla)]

(In reply to Blake Kaplan (:mrbkap) from comment #8)
> Comment on attachment 654965 [details] [diff] [review]
> Patch
> 
> Review of attachment 654965 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> No + for the .enabled sync API removal and one more comment.

Great! :) I agree with you to add the |enabled| back, where |wifi.enabled| setting is more like a toggling control and the |enabled| is more like a dynamic Wifi status. They could be different with a time delay.

> 
> ::: dom/wifi/WifiWorker.js
> @@ +571,5 @@
> > +    _enabled: false,
> > +    get enabled() {
> > +      return this._enabled;
> > +    },
> > +    set enabled(aEnabled) {
> 
> Nobody should be setting enabled on the manager. Instead, they should be
> going through setWifiEnabled (at best), so I don't see a need for this (if
> anything, it could be read-only).

Just one clarification. I was intentionally doing this so that we don't need to write something like:

WifiManager.enabled = true;
gSettingsService.getLock().set(
  "wifi.enabled", true, null, { isInternalSet: true });

with duplicative codes for settings whenever we need to reassign 
WifiManager.enabled. That is, the setter/getter will uniformly handle that. Does that make sense to you?

Comment 10

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

Hi Blake,

This patch includes your suggested changes, except for comment #9. I still feel more comfortable to use a setter/getter to access the WifiManager.enabled, which can ensure to update the settings (wifi.enabled) to the latest Wifi status whenever anyone wants to assign a new value to WifiManager.enabled. To be honest, I don't quite understand why you mentioned the WifiManager.enabled is read-only. ^^a Don't we already have lots of codes done in the WifiWorker as below?

  WifiManager.enabled = true/false;

Please feel free to let me know if I misunderstood. :) I'll be glad to fix it. Also, just one more code clean up. I think we can move the check |if (self._stateRequests.length > 0)| into the _notifyAfterStateChange(...).

Comment 11

[Gene Lian (I already quit Mozilla)]

Note that the setter is:

  set enabled(aEnabled) {
  }

not

  setEnabled(aEnabled) {
  }

This will work when we want to use WifiManager.enabled = XXX. ;)

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Gene Lian [:gene] from comment #10)
> Please feel free to let me know if I misunderstood. :) I'll be glad to fix
> it. Also, just one more code clean up.

Looking through the code, the only sets I see to WifiManager.enabled are in onsupplicantconnection/lost/failed. These aren't trying to turn on wifi, but rather changing the manager's state to reflect reality. That's why I don't see the point of the setter. All of the other uses of WifiManager.enabled are gets.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 655911 [details] [diff] [review]
Patch, V2

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

Clearing request based on comment 12. The rest of this looks great.

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 656810 [details] [diff] [review]
Patch, V3

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

Thanks a lot for this. You might have to rebase on top of the tethering code that just landed before this can land, unfortunately.

Comment 16

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3.1

Hi Vincent,

Since there are huge codes for tethering checked in recently, I'd like to invite you to take a review on this patch to see if I could destroy any existing functionalities after rebasing. Blake has already had review+ for this patch.

Also, I'm wondering there could be an error at https://bugzilla.mozilla.org/show_bug.cgi?id=735547#c109, which has been solved in this patch as well. In addition, I prefer s/enable/enabled as the parameter name for the tethering functions, which follows most of the convention.

Thanks for your reviews in advance!

Comment 17

[Gene Lian (I already quit Mozilla)]

Hi Blake! I just saw your e-mail after uploading the new patch :) Including you to review the new rebased patch again. Btw, this bug still depends on bug 785298, which can support the message to be carried with settings. However, it's still under review by Gregor.

Comment 18

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3.2

Just correcting a minor typo in comments and doing the rebase. Everything works well on the Otoro. ;)

Blake already had review+ on this.

Comment 19

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3.3

Do more code clean-ups (like adding ";" at the end of function-defined statements).

Blake already had review+ on this.

Comment 20

[Daniel Holbert [:dholbert]]

Landed:  https://hg.mozilla.org/integration/mozilla-inbound/rev/9020feae5d3c
(I changed "r=blake" to "r=mrbkap" in the commit message, too, since I think that's the more commonly-used moniker for him in "r=" notes.)

Comment 21

[Gene Lian (I already quit Mozilla)]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> Landed:  https://hg.mozilla.org/integration/mozilla-inbound/rev/9020feae5d3c
> (I changed "r=blake" to "r=mrbkap" in the commit message, too, since I think
> that's the more commonly-used moniker for him in "r=" notes.)

Great! Thanks Daniel!

Comment 22

[Gene Lian (I already quit Mozilla)]

Hi Evelyn,

The logic of hooking up setting is going to be landed, which could break the current function later. Please make sure the Gaia end will have the corresponding changes. I used to modify the apps/system/js/wifi.js myself by stripping out the hooking up logic on the Gaia's end. Everything works well :) (the rapid toggling behavior is even more stable than before).

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9020feae5d3c