823400 - Wifi toggle should not write settings db twice.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Vincent Chang[:vchang][changyihsin]]

Currently, wifi toggle writes the settings db twice. One is in gaia, the other one is in WifiWorker.js. It confuses gaia and makes it difficult to sync the wifi status between settings(settings app) and quick settings from status bar(system app). Moreover, it should be good to write settings db frequently. Writing settings db results in flash access. It should not be a good idea to write flash frequently in the phone.

Comment 1

[Vincent Chang[:vchang][changyihsin]]

If we fail to enable wifi, ondisabled event is fired to notify settings app instead of setting wifi.enabled to false.

Comment 2

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0

Comment 3

[Andreas Gal :gal]

Comment on attachment 694253 [details] [diff] [review]
Patch v1.0

Please test this well.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 694253 [details] [diff] [review]
Patch v1.0

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

::: dom/wifi/WifiWorker.js
@@ +1811,5 @@
>  
>      // Check if we need to dequeue requests first.
>      self._notifyAfterStateChange(false, false);
> +    // Notify everybody, even if they didn't ask us to come up.
> +    self._fireEvent("wifiDown", {});

With this change, onsupplicantlost and onsupplicantfailed only differ in the first parameter passed to _notifyAfterStateChange. Can you refactor them?

Comment 5

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

Since no motivation was given for why this should block, minusing.

But I'll approve the patch since it seems safe enough.

Comment 6

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

Comment on attachment 694253 [details] [diff] [review]
Patch v1.0

[Triage Comment]

Comment 7

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1

refactor the code based on comment 4.

Comment 8

[Vincent Chang[:vchang][changyihsin]]

Renominating bb+ so that we will not confuse gaia's implementation when wifi.enabled   settings change event is fired.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 695443 [details] [diff] [review]
Patch v1.1

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

Looks good. Thanks.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Actually, Vincent, with this patch it looks like if the supplicant dies (or wifi turns off unexpectedly) we'll now notify gaia via wifiManager.ondisabled, but nothing updates the pref. So, we should either add code here to automatically reconnect to wifi or add code to gaia to update the pref when wifi goes down. Otherwise, we'll end up in a confusing state of "it looks like wifi is up but nothing works."

Comment 11

[Vincent Chang[:vchang][changyihsin]]

(In reply to Blake Kaplan (:mrbkap) from comment #10)
> Actually, Vincent, with this patch it looks like if the supplicant dies (or
> wifi turns off unexpectedly) we'll now notify gaia via
> wifiManager.ondisabled, but nothing updates the pref. So, we should either
> add code here to automatically reconnect to wifi or add code to gaia to
> update the pref when wifi goes down. Otherwise, we'll end up in a confusing
> state of "it looks like wifi is up but nothing works."

You are right. But I think it should be gaia's responsibility to handle the ondisabled event and turn the settings to off. Not sure the idea of gaia people ?

Comment 12

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

Vincent: Does this really need to block? It's proven non-trivial to fix this bug so we should only spend more time on it if it's absolutely necessary.

Can you describe what will not work if we don't fix this bug in addition to unnecessary wear of the flash card.

Comment 13

[Vincent Chang[:vchang][changyihsin]]

Hi Jonas, 

As I describe in comment 8, both gecko and gaia may toggle setttings change of "wifi.enabled", and it may confuse gaia when receiving settings change event. So I got a request to post a patch. 

I am fine if we don't mark it as BB+. Just gaia people may need additional code when receiving settings change which they have done in current repo. 

Regard
Vincent

Comment 14

[Doug Turner (:dougt)]

given that there is a workaround, let's basecamp- this.

Comment 15

[Vincent Chang[:vchang][changyihsin]]

Bug 823783 is landed, so we can land this patch without regressing anything.
Marked it as checkin-needed based on the comment 5 in Bug 823783.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de0d5d8572d0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de0d5d8572d0

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4c476f3a8570