Closed Bug 787421 Opened 7 years ago Closed 7 years ago

Dealing with the interaction between wifi and wifi tethering settings

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+)

RESOLVED FIXED
blocking-basecamp +

People

(Reporter: vchang, Assigned: vchang)

References

Details

(Whiteboard: [LOE:S])

Attachments

(4 files, 6 obsolete files)

1.88 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.39 KB, patch
Details | Diff | Splinter Review
13.87 KB, patch
Details | Diff | Splinter Review
9.44 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 735172
1. Support setWifiTethering function
2. Support get wifi tethering enable/disable status
The WifiWorker uses these two functions to turn on/off wifi tethering.
Attachment #660731 - Flags: review?(philipp)
Implementation of nsINetworkManager.idl
1. Support setWifiTethering function
2. Support get wifi tethering enable/disable status
Attachment #660733 - Flags: review?(philipp)
Remove unnecessary function definition.
Attachment #660734 - Flags: review?(mrbkap)
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.
Attachment #660735 - Flags: review?(mrbkap)
(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+.
blocking-basecamp: --- → ?
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.
(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.
Attachment #660734 - Flags: review?(mrbkap) → review+
blocking-basecamp: ? → +
Whiteboard: [LOE:S]
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.
Attachment #660735 - Flags: review?(mrbkap) → review+
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
   *        ...
   */
Attachment #660731 - Flags: review?(philipp) → review-
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.
Attachment #660733 - Flags: review?(philipp) → review-
(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;
  }
Hi Philikon, 

Thanks for your help to review my patch. 
I updated the patch to address comment 9.
Attachment #660731 - Attachment is obsolete: true
Attachment #663339 - Flags: review?(philipp)
Hi Philikon, 

Thanks for your help to review my patch. 
I updated the patch to address comment 10 and 11.
Attachment #660733 - Attachment is obsolete: true
Attachment #663340 - Flags: review?(philipp)
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."
Attachment #663339 - Flags: review?(philipp) → review+
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/
Attachment #663340 - Flags: review?(philipp) → review+
Updated the patch to address comment 14.
Attachment #663339 - Attachment is obsolete: true
Updated the patch to address comment 15.
Attachment #663340 - Attachment is obsolete: true
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.
Attachment #660735 - Attachment is obsolete: true
Remove trail blanks in the patch.
Attachment #663932 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.