Closed
Bug 736096
Opened 13 years ago
Closed 13 years ago
Wifi: Re-prioritize the configured networks to avoid overly-high network priorities
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: mrbkap, Assigned: mrbkap)
References
Details
Attachments
(2 files)
(deleted),
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
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.
Attachment #610555 -
Flags: review?(jones.chris.g)
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
Attachment #610553 -
Flags: review?(jones.chris.g) → review+
Updated•13 years ago
|
Attachment #610555 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 4•13 years ago
|
||
(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.
Assignee | ||
Comment 5•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d2612e79e456 http://hg.mozilla.org/mozilla-central/rev/f4fe6a118139
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in
before you can comment on or make changes to this bug.
Description
•