Closed Bug 906631 Opened 6 years ago Closed 6 years ago

[Wifi] Hotspot and Wifi might both be enabled at same time.

Categories

(Firefox OS Graveyard :: Wifi, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.2 FC (16sep)

People

(Reporter: chucklee, Assigned: chucklee)

References

Details

Attachments

(2 files, 4 obsolete files)

STR
1. Enable hotspot
2. Enable Wifi
3. Disable Wifi and then Enable Wifi quickly.

Expect:
Wifi enabled, Hotspot disabled

Actual:
Status bar shows Wifi and Hotspot are enabled.
Attached patch 0001. Refactory command queue. (obsolete) — Splinter Review
1. Make enable/disable request queue into producer-consumer pattern.
2. Make sure every request is executed in order without race condition.
Attachment #794435 - Flags: review?(vchang)
Attachment #794435 - Flags: review?(mrbkap)
Move check rule into queue callback to make sure the state is synchronized.
Attachment #794436 - Flags: review?(vchang)
Attachment #794436 - Flags: review?(mrbkap)
Attached patch 0001. Refactory command queue. (obsolete) — Splinter Review
Fix nit
Attachment #794435 - Attachment is obsolete: true
Attachment #794435 - Flags: review?(vchang)
Attachment #794435 - Flags: review?(mrbkap)
Attachment #794440 - Flags: review?(vchang)
Attachment #794440 - Flags: review?(mrbkap)
Blocks: 908426
Comment on attachment 794440 [details] [diff] [review]
0001. Refactory command queue.

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

::: dom/wifi/WifiWorker.js
@@ +2060,1 @@
>  

Why do we need to call requestDone() here ?

@@ +2074,5 @@
>      WifiManager.state = "UNINITIALIZED";
>      debug("Supplicant died!");
>  
> +    // Wifi disable request is done here.
> +    self.requestDone();

The Same here. Why do we need to call requestDone() ? If we really have to do something here, can we have a better naming for the function ?

@@ +2883,5 @@
> +        let retryTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +        retryTimer.initWithCallback(function(timer) {
> +          self._setWifiEnabledCallback(status).bind(self);
> +        }, 1000, Ci.nsITimer.TYPE_ONE_SHOT);
> +

Do we still need to check driver state ? This function always called after loadDriver or unLoadDriver in WifiManager.setWifiEnabled(). In that case, driver should be loaded or unloaded successfully.

@@ +3249,2 @@
>        }
> +    } while (!("callback" in request));

The request should contain callback, right. If it is true, we don't need the do {}while here, and make the code clearly.
Attachment #794440 - Flags: review?(vchang)
Attachment #794436 - Flags: review?(vchang) → review+
Comment on attachment 794440 [details] [diff] [review]
0001. Refactory command queue.

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

::: dom/wifi/WifiWorker.js
@@ +2874,5 @@
>    },
>  
> +  _setWifiEnabledCallback: function(status) {
> +    if (status) {
> +      let newState = (status === "no change" ? true : false);

Nit: Please write this as |let newState = (status === "no change");|

@@ +2904,5 @@
> +  },
> +
> +  queueRequest: function(enabled, callback) {
> +    if (callback) {
> +      // Push only valid request

Who would call this function without a callback? What'd be the meaning of that? I'd rather throw an exception if it's truly unexpected.

@@ +2985,5 @@
>    setWifiApEnabled: function(enabled, callback) {
>      let configuration = this.getWifiTetheringParameters(enabled);
>  
>      if (!configuration) {
> +      this.requestDone();

Why is it ok to not call callback here? Can you add a comment?

@@ +3203,5 @@
> +  requestProcessing: false,   // Hold while dequeue and execution a request.
> +                              // Released upon the request is fully executed,
> +                              // i.e, mostly after callback is done.
> +  requestTimer: null,         // Timer for handling request queue.
> +  requestTimerRunning: false, // Is requestTimer running.

Is the timer really necessary? My hope here was that we'd be able to do everything based on callbacks without having to guess (which we always do conservatively). Especially when e.g. we're turning the wifi off, we don't want to have to wake the phone up every 500 seconds... I'd really rather run this off of callbacks called from the WifiManager.
Attachment #794440 - Flags: review?(mrbkap)
Attachment #794436 - Flags: review?(mrbkap) → review+
Comment on attachment 794440 [details] [diff] [review]
0001. Refactory command queue.

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

::: dom/wifi/WifiWorker.js
@@ +2074,5 @@
>      WifiManager.state = "UNINITIALIZED";
>      debug("Supplicant died!");
>  
> +    // Wifi disable request is done here.
> +    self.requestDone();

After some review, I think we might be able to get ride of these two calls.
I'll test it.

@@ +2883,5 @@
> +        let retryTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +        retryTimer.initWithCallback(function(timer) {
> +          self._setWifiEnabledCallback(status).bind(self);
> +        }, 1000, Ci.nsITimer.TYPE_ONE_SHOT);
> +

You are right, I will remove the check and test on phones.

@@ +2904,5 @@
> +  },
> +
> +  queueRequest: function(enabled, callback) {
> +    if (callback) {
> +      // Push only valid request

Will do!

@@ +2985,5 @@
>    setWifiApEnabled: function(enabled, callback) {
>      let configuration = this.getWifiTetheringParameters(enabled);
>  
>      if (!configuration) {
> +      this.requestDone();

I think it's because the callback is |notifyTetheringOff()| or |notifyTetheringOn| here.
When we have no |configuration| for |WifiManager.setWifiApEnabled()|, these callbacks can't be called.

@@ +3203,5 @@
> +  requestProcessing: false,   // Hold while dequeue and execution a request.
> +                              // Released upon the request is fully executed,
> +                              // i.e, mostly after callback is done.
> +  requestTimer: null,         // Timer for handling request queue.
> +  requestTimerRunning: false, // Is requestTimer running.

The timer will be canceled after request queue is empty, so phone won't be woken up continuously.

And if every request callbacks call |requestDone()| on complete, we don't need this timer.
It's more like a backup plan here to make sure the request queue can work correctly if some callback doesn't follow the rule.

I'll remove the timer and add some comment to |queueRequest()| to address this rule.
Address reviewers' comments.
Attachment #794440 - Attachment is obsolete: true
Attachment #800034 - Flags: review?(vchang)
Attachment #800034 - Flags: review?(mrbkap)
Comment on attachment 800034 [details] [diff] [review]
0001. Refactory command queue. V2

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

Looks good, thanks you.
Attachment #800034 - Flags: review?(vchang) → review+
Add reviewer in summary.
Attachment #800034 - Attachment is obsolete: true
Attachment #800034 - Flags: review?(mrbkap)
Try is OK, let's see what will happen in the real world....
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8ff506c57197
https://hg.mozilla.org/mozilla-central/rev/b3cb8e312203
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)
No longer blocks: 908426
You need to log in before you can comment on or make changes to this bug.