The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

(Blocks: 1 bug)

unspecified
mozilla14
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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

5 years ago
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.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #610553 - Flags: review?(jones.chris.g)
(Assignee)

Comment 2

5 years ago
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.
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+
Attachment #610555 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 4

5 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

5 years ago
http://hg.mozilla.org/mozilla-central/rev/d2612e79e456
http://hg.mozilla.org/mozilla-central/rev/f4fe6a118139
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.