Last Comment Bug 736096 - Wifi: Re-prioritize the configured networks to avoid overly-high network priorities
: Wifi: Re-prioritize the configured networks to avoid overly-high network prio...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Blake Kaplan (:mrbkap)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: b2g-wifi
  Show dependency treegraph
 
Reported: 2012-03-15 08:20 PDT by Blake Kaplan (:mrbkap)
Modified: 2012-04-02 10:15 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (3.79 KB, patch)
2012-03-29 08:24 PDT, Blake Kaplan (:mrbkap)
cjones.bugs: review+
Details | Diff | Splinter Review
additional patch (809 bytes, patch)
2012-03-29 08:25 PDT, Blake Kaplan (:mrbkap)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Blake Kaplan (:mrbkap) 2012-03-15 08:20:22 PDT
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) 2012-03-29 08:24:08 PDT
Created attachment 610553 [details] [diff] [review]
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) 2012-03-29 08:25:44 PDT
Created attachment 610555 [details] [diff] [review]
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 2012-03-29 19:46:16 PDT
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) 2012-04-02 10:08:49 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.