787421 - Dealing with the interaction between wifi and wifi tethering settings

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Vincent Chang[:vchang][changyihsin]]

The problem is that when one turns on, the other has to turn off and vice versa. This seems like it should be really simple, but it turns out to be difficult because we're trying to synchronize two prefs across an asynchronous callback and, in addition, turning wifi on and off is itself an asynchronous operation.

Here is proposed steps by Blake, 
1. Make Gaia enforce the invariant that when wifi is turned on,
tethering is turned off and vice versa.

2. Land bug 729877 (B2G Wifi: hook up to settings API).

3. based on top of step.2 to enforce the tethering/wifi interaction on the platform side. 

This bug is tried to fix the step 3.

Comment 1

[Vincent Chang[:vchang][changyihsin]]

Attachment: Dealing with the interaction between wifi and wifi tethering settings(nsINetworkManager.idl)

1. Support setWifiTethering function
2. Support get wifi tethering enable/disable status
The WifiWorker uses these two functions to turn on/off wifi tethering.

Comment 2

[Vincent Chang[:vchang][changyihsin]]

Attachment: Dealing with the interaction between wifi and wifi tethering settings(NetworkManager implementation)

Implementation of nsINetworkManager.idl
1. Support setWifiTethering function
2. Support get wifi tethering enable/disable status

Comment 3

[Vincent Chang[:vchang][changyihsin]]

Attachment: Dealing with the interaction between wifi and wifi tethering settings(nsIWifi.idl)

Remove unnecessary function definition.

Comment 4

[Vincent Chang[:vchang][changyihsin]]

Attachment: Dealing with the interaction between wifi and wifi tethering settings(nsIWifi implementation)

Dealing with the interaction between wifi and wifi tethering settings. 
When wifi is turned on, wifi tethering should be turned off first. 
When wifi tethering is turned off, wifi should be turned off first.

Comment 5

[Vincent Chang[:vchang][changyihsin]]

(In reply to Vincent Chang from comment #2)
> Created attachment 660733 [details] [diff] [review]
> Dealing with the interaction between wifi and wifi tethering
> settings(NetworkManager implementation)
> 
> Implementation of nsINetworkManager.idl
> 1. Support setWifiTethering function
> 2. Support get wifi tethering enable/disable status

The isError() function is used in this patch, it is defined in Bug 788081 - B2G tethering: correctly handle 1xx responses from netd which is review+.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do we need to flip the wifi *setting* to off when wifitethering setting is turned on? Couldn't we simply make the wifi code only turn on wifi if the wifi setting is on and the wifitethering setting is off?

From a user's point of view it even makes sense that the wifi setting is rendered as "on" when wifitethering is on.

In fact, I would go even further and say that wifitethering should only be turned on if wifi setting is turned on and wifitethering setting is turned on. And that "normal" wifi is only turned on if wifi is turned on and wifitethering is turned off.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Jonas Sicking (:sicking) from comment #6)
> Do we need to flip the wifi *setting* to off when wifitethering setting is
> turned on? Couldn't we simply make the wifi code only turn on wifi if the
> wifi setting is on and the wifitethering setting is off?
> 
> From a user's point of view it even makes sense that the wifi setting is
> rendered as "on" when wifitethering is on.

I disagree. When wifi tethering is on, it is impossible to even see the available wireless networks. I think the the wifi "on" setting should correspond to the ability to connect to wifi networks as a data source. If that isn't possible, we shouldn't pretend that wifi is on.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 660735 [details] [diff] [review]
Dealing with the interaction between wifi and wifi tethering settings(nsIWifi implementation)

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

::: dom/wifi/WifiWorker.js
@@ +2280,5 @@
>    },
>  
> +  nextRequest: function nextRequest(state) {
> +    if (this._stateRequests.length > 0 &&
> +      ("callback" in this._stateRequests[0])) {

Nit: the ( should line up with the 't' on the line above. Also, you can invert the sense of the if statement and return early, letting you de-indent the rest of the function, which would be nice.

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 660731 [details] [diff] [review]
Dealing with the interaction between wifi and wifi tethering settings(nsINetworkManager.idl)

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

::: dom/system/gonk/nsINetworkManager.idl
@@ +92,5 @@
> +  /**
> +   * Callback function used to report status.
> +   * result: 0: success, others: fail.
> +   */
> +  void wifiTetheringEnabledChange(in jsval result);

With these success/failure semantics, I suggest naming the parameter `error`. Make it `null` when the operation was successful and an `Exception` object or whatever when it wasn't. Please document in a JavaDoc-style comment (see below.)

@@ +169,5 @@
>  
> +  /**
> +   * Returns whether or not wifi tethering is currently enabled.
> +   */
> +  readonly attribute boolean enabled;

`enabled` is a horribly generic attribute name on `nsINetworkManager`. At the very least this should be `wifiTetheringEnabled`.

r- for that.

@@ +175,5 @@
> +  /**
> +   * Request to enable/disable Wifi Tethering.
> +   * enabled: true or false.
> +   * networkInterface: registered network interface.
> +   * callback: report status to NetworkManager.

Please use the JavaDoc formatting that's used throughout our IDLs:

  /**
   * Enable or disable Wifi Tethering
   *
   * @param enabled
   *        Boolean that indicates whether tethering should be enabled (true) or disabled (false)
   * @param network
   *        The network interface that XYZ... It's not clear to me here *which* network interface this is... the Wifi network interface? the tethered network interface, e.g. RIL? Please explain!
   * @param callback
   *        ...
   */

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 660733 [details] [diff] [review]
Dealing with the interaction between wifi and wifi tethering settings(NetworkManager implementation)

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

::: dom/system/gonk/NetworkManager.js
@@ -440,5 @@
>          this.handleUSBTetheringToggle(aResult);
>          break;
> -      case SETTINGS_WIFI_ENABLED:
> -        this.handleWifiTetheringToggle(aResult);
> -        break;

Do we still want to keep the 

  case SETTINGS_WIFI_ENABLED:

line so that we update this.tetheringSettings[SETTINGS_WIFI_ENABLED]? I guess we're doing this in `setWifiTethering` now. So I suggest adding a comment here explaining that, otherwise people will wonder why this isn't the case.

@@ +643,5 @@
> +      let code = data.resultCode;
> +      let reason = data.resultReason;
> +      let enable = data.enable;
> +      let enableString = enable ? "Enable" : "Disable";
> +      let settingsLock = gSettingsService.createLock();

You only seem to need the `settingsLock` if there was an error. Please move this line into the error handling block so we don't create a settings lock needlessly in the success case.

Same goes for the `settingsLock` one scope up: please create it lazily.

r- for this.

@@ +652,5 @@
> +
> +      if (isError(code)) {
> +        this.tetheringSettings[SETTINGS_WIFI_ENABLED] = false;
> +        settingsLock.set("tethering.wifi.enabled", false, null);
> +        callback.wifiTetheringEnabledChange("netd command error");

Since `callback` is a [function] interface, you should just be able to call it as

  callback("netd command error");

Please fix this here and everywhere else. Also, you have some code duplication for the

@@ +654,5 @@
> +        this.tetheringSettings[SETTINGS_WIFI_ENABLED] = false;
> +        settingsLock.set("tethering.wifi.enabled", false, null);
> +        callback.wifiTetheringEnabledChange("netd command error");
> +      } else {
> +        callback.wifiTetheringEnabledChange(0);

cf. my review for part 1: pass `null` here. Passing strings and numbers is really confusing.

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> Since `callback` is a [function] interface, you should just be able to call
> it as
> 
>   callback("netd command error");
> 
> Please fix this here and everywhere else.

Actually, I think I'm wrong about this. Never mind.

> Also, you have some code duplication for the

Forgot to finish that sentence :). I meant to point out that you have some code duplication for the error handling that you could factor out:

  function notifyError(msg) {
    this.tetheringSettings[SETTINGS_WIFI_ENABLED] = false;
    if (!settingsLock) {
      settingsLock = gSettingsService.createLock();
    }
    settingsLock.set("tethering.wifi.enabled", false, null);
    debug("Tethering error: " + msg);
    callback.wifiTetheringEnabledChange(msg);
  }

and then you can use it further down in the method, e.g.:

  if (!mobile) {
    notifyError("mobile interface is not registered");
    debug("Can't enable wifi tethering, MOBILE interface is not registered.");
    return;
  }

Comment 12

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch part1, nsINetworkManager.idl

Hi Philikon, 

Thanks for your help to review my patch. 
I updated the patch to address comment 9.

Comment 13

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch part2, NetworkManager implementation

Hi Philikon, 

Thanks for your help to review my patch. 
I updated the patch to address comment 10 and 11.

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 663339 [details] [diff] [review]
Patch part1, nsINetworkManager.idl

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

::: dom/system/gonk/nsINetworkManager.idl
@@ +93,5 @@
> +   * Callback function used to report status to WifiManager.
> +   *
> +   * @param error
> +   *        Make error `null` when the operation was successful, reason of error
> +   *        when it wasn't.

Better: "An error message if the operation wasn't successful, or `null` if it was."

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 663340 [details] [diff] [review]
Patch part2, NetworkManager implementation

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

::: dom/system/gonk/NetworkManager.js
@@ +434,5 @@
>          break;
> +      // SETTINGS_WIFI_ENABLED has been moved to WifiManager.js to deal with
> +      // the interaction between wifi and wifi tethering settings. Also, we
> +      // update tetheringSettings[SETTINGS_WIFI_ENABLED] in setWifiTethering
> +      // function.

I don't like historical notes in comments because they don't make sense unless you know the full history of the code. So I suggest s/has been moved to/is handled in/

Comment 16

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch part1, nsINetworkManager.idl v1.2

Updated the patch to address comment 14.

Comment 17

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch part2, NetworkManager implementation, v1.2

Updated the patch to address comment 15.

Comment 18

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch part3, nsIWifi mplementation, v1.1

1. Updated the patch to address comment 8. 
2. Addressed the interface definition change in nsINetworkManager.idl 
   * enabled -> wifiTetheringEnabled
   * wifiTetheringEnabledChange(in jsval error) An error message if the operation  wasn't successful, or `null` if it was.

Comment 19

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch part 3, nsIWifi implementation, v1.2

Remove trail blanks in the patch.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf9b829548c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c614f656bc13
https://hg.mozilla.org/integration/mozilla-inbound/rev/baf1182b6924
https://hg.mozilla.org/integration/mozilla-inbound/rev/598c1564bf41

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1bf9b829548c
https://hg.mozilla.org/mozilla-central/rev/c614f656bc13
https://hg.mozilla.org/mozilla-central/rev/baf1182b6924
https://hg.mozilla.org/mozilla-central/rev/598c1564bf41