736096 - Wifi: Re-prioritize the configured networks to avoid overly-high network priorities

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Right now, each time we specifically choose a network, we bump its priority by one. After 2^26 bumps, we start wandering into floating point approximation territory. While in practice this probably isn't a big deal, it's good hygiene to re-prioritize all of the networks to keep the priority numbers sane.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This renumbers all of the already prioritized networks when the priority gets up to 9999. This does mean that we'll do a bunch of extra work if you've actually added 9999 networks, but I honestly don't think that that will ever happen. Chris, if you think we should worry about that, I can start doing something "clever" like ignoring the lowest 100 networks.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: additional patch

This is needed in order to avoid errors in updateNetwork: if we have fields that are null updateNetwork will "throw" trying to set invalid values. I could have fixed this in setNetworkConfiguration by checking for null values, but I prefer attempting to never have invalid values to begin with.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 610553 [details] [diff] [review]
Proposed fix

>diff --git a/dom/wifi/WifiWorker.js b/dom/wifi/WifiWorker.js

>+    // Skip unsorted networks.
>+    let priority = 0, i;

Please call this |newPriority| or |nextPriority| or something, and
declare in the loop header where it's used.

>+    for (; i >= 0; --i) {
>+      this.configuredNetworks[ordered[i]].priority = priority++;
>+

Save |this.configuredNetworks[ordered[i]]| into a local and reuse
below.

>+      // Note: networkUpdated declared below since it happens logically after
>+      // this loop.
>+      WifiManager.updateNetwork(this.configuredNetworks[ordered[i]], networkUpdated);
>+    }
>+
>+    function networkUpdated(ok) {

This is an error in strict mode, IIRC.  Is WifiWorker.js not using
strict mode?

r=me with nits picked

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> This is an error in strict mode, IIRC.  Is WifiWorker.js not using
> strict mode?

It isn't and yes, respectively. Function hoisting isn't going anywhere anytime soon. I hesitated before writing it this way, but a straw poll of the JS devs around me says that it's acceptable as long as the gap between the use and the declaration is small, so I'm keeping it unless you really don't like it.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/d2612e79e456
http://hg.mozilla.org/mozilla-central/rev/f4fe6a118139