732982 - Expose a Wifi API to the DOM

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make the waitForScan internal API saner

This is separate from bug 729877 since we need something sooner rather than later. For the moment, we can have a bunch of functions on nsIDOMWifiManager and then cull them as appropriate once the settings manager is implemented.

This bug will be a collection of patches that I've written to make this work. The first patch is only tangentially related, but doesn't really deserve its own bug. Basically, it makes it easily possible to add and remove internal callbacks. It also clears the callback list after every scan, since every use case that I've had for it so far only needs one set of scan results.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Trigger a first scan no matter what and don't get scan results if nobody is looking for them

This should help at least a little if we're not in range of any of our networks and is a little cleaner.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Expose navigator.mozWifiManager

Fabrice, I actually cribbed a bunch of code from Webapps.js, so I think you should be able to review it. Please ignore cloneObjectInto, I realized that it wasn't necessary in a followup patch. I also stripped out the permission checking code for now. We'll want to add it back in before landing, but for my simple testing, this was easier.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: -w patch to properly expose networks

This makes getNetworks somewhat usable. This patch returns as "networks" the full list of "networks in range" and "networks known to wpa_supplicant" in true Android style. Jonas wasn't sure if that's the right API for this or not. One of the goals here is to share the network objects everywhere so that changes in configuration are immediately reflected.

There are some wpa_supplicant-isms that we should definitely not expose to content: for example, wpa_supplicant requires that ssids are contained in double quotes. We probably want to make sure to dequote on exit from nsWifiWorker.js and requote on entry. Does that sound like a sane approach?

I'm also not entirely sure how to expose networks that don't have SSIDs at all.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix removeNetworkCommand and use it

This is an easy patch that adds a missing argument to removeNetworkCommand and removes the currently-being-added network if there's an error in one of the configuration options.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Implement WifiManager.select

This implements WifiManager.select, which can be used to connect to a given network returned from getNetworks.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Page using the API

This is the page I've been using for testing locally on my device.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 602915 [details] [diff] [review]
Expose navigator.mozWifiManager

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

::: dom/wifi/nsDOMWifiManager.js
@@ +55,5 @@
> +    this.innerWindowID = util.currentInnerWindowID;
> +    this._manager = Cc["@mozilla.org/telephony/system-worker-manager;1"]
> +                      .getService(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIWifi)
> +                      .wrappedJSObject; // XXX correct in the face of e10s?

if @mozilla.org/telephony/system-worker-manager runs in the parent process, this will not work. You really need to remote the call to the parent process, the easiest way being using a jsm loaded in the parent. That's what the contacts and webapps API do. using e10s also gives you an async model for free, perfectly inline with DOMRequest

Here you're instantiating a new service in the child, and I don't know what will happen on the low level wifi layer.

::: toolkit/content/Services.jsm
@@ +92,5 @@
>    ["ww", "@mozilla.org/embedcomp/window-watcher;1", "nsIWindowWatcher"],
>    ["startup", "@mozilla.org/toolkit/app-startup;1", "nsIAppStartup"],
>    ["sysinfo", "@mozilla.org/system-info;1", "nsIPropertyBag2"],
> +  ["clipboard", "@mozilla.org/widget/clipboard;1", "nsIClipboard"],
> +  ["DOMRequest", "@mozilla.org/dom/dom-request-service;1", "nsIDOMRequestService"]

Please update https://developer.mozilla.org/en/JavaScript/Code_modules/Services.jsm

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch addressing fabrice's comments

This patch addresses fabrice's comments. I'll add the new service to the wiki when I land it.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch for fabrice

This patch includes the entirety of the new component plus the changes to the chrome service that make it use the message manager.

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 603292 [details] [diff] [review]
Patch for fabrice

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

I landed a helper to deal with DOMRequests and IPC messages while not leaking : https://hg.mozilla.org/integration/mozilla-inbound/rev/d91de454670d
Gregor plans to update its contacts implementation to use it, and it saves quite a bit of housekeeping.

::: dom/wifi/nsDOMWifiManager.js
@@ +31,5 @@
> +                                         Ci.nsIDOMGlobalPropertyInitializer]),
> +
> +  // nsIDOMGlobalPropertyInitializer implementation
> +  init: function(aWindow) {
> +    dump("nsDOMWifiManager::init() " + aWindow + "\n");

remove

@@ +54,5 @@
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    let wId = aSubject.QueryInterface(Ci.nsISupportsPRUint64).data;
> +    if (wId == this.innerWindowID) {
> +      Services.obs.removeObserver(this, "inner-window-destroyed");

you also need to remove your message listeners, and clear the _requests object.

@@ +113,5 @@
> +      case "WifiManager:select:Return:KO":
> +        request = this._getRequest(msg.rid);
> +        Services.DOMRequest.fireError(request, "Unable to add the network");
> +        break;
> +    }

don't you want to remove the request from this._requests at this point?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: stop connecting automatically to mozilla networks

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: flesh out the wifimanager API

This is very definitely a WIP and needs to go through all sorts of scrutiny, but it gets us up and limping along fast enough to write a real settings page complete with state transitions. This checkpoint can only connect to open networks.

One theme that needs to be cleaned up internally in nsWifiWorker.js is the separation between the network objects we hand out to the DOM and the ones we store internally. Right now, it's done in a very ad-hoc manner, but it's close enough for government work.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: allow connecting to secured networks

This patch allows the settings API to pass a key management protocol via an incoming network.keyManagement and network.psk (or network.identity/password). That part of the patch is mostly untested, so there might be bugs.

This also fixes a race, where the supplicant could connect before we'd grabbed our list of networks... The fix isn't very clean, but again, works for now. One thing that's probably missing is that we should automatically re-enable all disabled networks when we disconnect, so that we have a better chance of re-connecting to a pre-existing network after losing the connection.

There's a bunch more "API" here that needs to be ironed out: notably, we need some sort of spec for the network objects and how they pass information back and forth between the WifiManager and the DOM. For now, we mostly follow wpa_supplicant's lead, with a couple of niceties like passing an array for the available key management schemes.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 602912 [details] [diff] [review]
Make the waitForScan internal API saner

Andreas is out of action for the rest of the week, so cjones is taking the review-bullet.

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 603292 [details] [diff] [review]
Patch for fabrice

<bikeshed>I heard several times now that we shouldn't really use the 'ns' prefix for new XPCOM components anymore. Can't remember where and from whom, but certainly all the ones that I've seen created in the past few months, including in B2G/DOM, don't have it...</bikeshed>

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Sure. I'll do that as a followup patch before landing.

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> ::: toolkit/content/Services.jsm
> @@ +92,5 @@
> >    ["ww", "@mozilla.org/embedcomp/window-watcher;1", "nsIWindowWatcher"],
> >    ["startup", "@mozilla.org/toolkit/app-startup;1", "nsIAppStartup"],
> >    ["sysinfo", "@mozilla.org/system-info;1", "nsIPropertyBag2"],
> > +  ["clipboard", "@mozilla.org/widget/clipboard;1", "nsIClipboard"],
> > +  ["DOMRequest", "@mozilla.org/dom/dom-request-service;1", "nsIDOMRequestService"]
> 
> Please update
> https://developer.mozilla.org/en/JavaScript/Code_modules/Services.jsm

mrbkap++ for choosing a sensible name for the Services shortcut. I filed bug 734018 to fix Webapps.js.

Comment 18

[Philipp von Weitershausen [:philikon]]

Comment on attachment 603292 [details] [diff] [review]
Patch for fabrice

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

::: dom/wifi/nsDOMWifiManager.manifest
@@ +1,4 @@
> +# nsDOMWifiManager.js
> +component {2cf775a7-1837-410c-9e26-323c42e076da} nsDOMWifiManager.js
> +contract @mozilla.org/wifimanager;1 {2cf775a7-1837-410c-9e26-323c42e076da}
> +category JavaScript-navigator-property mozWifiManager @mozilla.org/wifimanager;1

<bikeshed>Instead of "mozWifiManager" I suggested "mozWifi" or "mozWifiConnection" on the dev-webapi list. I'm leaning towards "mozWifi" seems that's analogous to "mozSms", "mozTelephony", etc. See http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/ed980c42261c5f4a#msg_e58c6de7eb063501 for discussion and Jonas's response.</bikeshed>

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Full patch

So, with this patch applied as well as kaze's patch in the gaia repo, we can connect to WPA-EAP, WPA-PSK (untested, but should work) and WEP networks. There's UI for all of those too. The networks are remembered across reboots and things appear to work. There are some known bugs in both the UI code and the backend code as well as some "interface bugs" that need ironing out. However, this should be good enough for tonight. From playing around here are some things that I've noticed:

* The backend component doesn't deal well with someone fiddling with wpa_supplicant's settings while it's running.
* Right now select() chooses a network by disabling all other networks. We should use wpa_supplicant's priority field instead.
* We don't detect when a password is wrong (or the network is misconfigured) and we're looping trying to connect.
* There's still a bit of confusion in the backend between "configured networks" and skeletal networks (with just a ssid/bssid). There is, however, a split between a "scan result" and a network.
* The whole "network object" API needs to be defined, right now we have:

ScanResult:
  * ssid
  * bssid (currently chosen based on position in the scan results list, should be chosen based on signal strength)
  * capabilities (an array of security mechanisms supported)
  * signal (a signal strength from 1-100).
  * connected (boolean indicating if we're connected to this result)
  * password (* if there's a set password)
  * identity (identity for WPA-EAP)

Network (as seen in content code)
 * ssid
 * known (boolean indicating whether this network is registered to the system)

Problems in the interface:
* Right now the decision about whether to enable/disable wifi is a function/getter pair on the interface, it should be a setting.
* There's a getNetworks function that triggers a scan. It would be more useful to register a "scan listener" and then trigger scans "once in a while" on the backend. We probably want a getNetworks function as well.
* Missing a forgetNetwork function.
* Missing a notification where we're about to start connecting to a network. The idea was to use select's DOMRequest for this, but in practice this misses other means of starting a connection to a network (i.e. wpa_supplicant choosing a new network for us).
* Relatedly, we might want a single "network state changed" function instead of individual callbacks.

Problems in the UI:
* It's a little unclear that if you have a single * as the password for any of the security modes, that means "leave it alone." (relatedly: we should probably not pop up that dialog if you've already configured a network, instead we should simply try to connect.
* I think I somehow managed to break the "connecting to network" notification (see also the second to last point in "problems in the interface").
* I'm sure there are others, but I leave that up to the UI people to decide.

The patch is also still missing the component renaming from comment 18.

Chris, I'm going to see if I can break this into smaller pieces, if not, we'll have to figure out a way to review this anyway. Sorry!

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Something I forgot to mention:
* IMO one of the API's goals should be to allow backends that aren't wpa_supplicant. The Android API is literally just a Java wrapper around it. Therefore, if you are wondering why quote and dequote exist: it's because the quotation marks exist specifically to help wpa_supplicant.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Dummy patch for "review"

The URL points to a github branch that contains 12 changesets of them, 3 have already been reviewed. The rest have an optimistic r=cjones. They all have commit messages describing the basic idea. A couple of them are still rather big, I apologize for that.

The first one (adding the DOM component) really only needs a skim through since fabrice already read through an earlier version.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 604490 [details] [diff] [review]
Dummy patch for "review"

I would really like to get this checked in... any reviewer who wants these patches can take them! :)

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 604490 [details] [diff] [review]
Dummy patch for "review"

Going to attach the patches one-by-one to the bug... here we go!

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Create a wifi DOM API and matching component

This fills out the nsIDOMWifiManager API. The basic implementation of the component was already r+'d by fabrice, but there were a few additions since his r+. I also have yet to add back the permission manager checking.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make the waitForScan internal API saner

This provides an internal-to-the-worker API called waitForScan that adds listeners for exactly one scan results (we'll probably be changing it in the near future) and uses it in order to implement another internal getNetworks function.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch from bug 730830

I'm adding this here because it's needed for the rest of the patches to apply. Vivien already reviewed the important bits (the var -> let changes and making sure we call the callback if there are no configured networks). The bottom hunk of this patch goes away in the final patch (which is unfortunately large) where we stop adding Mozilla networks by default.

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Trigger a first scan no matter what and don't get scan results if nobody is looking for them

This fixes a bug where on some devices wpa_supplicant doesn't scan right away and therefore won't connect to any networks. It also makes us ignore scan results that we don't need.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix removeNetworkCommand and use it

removeNetworkCommand was missing an argument to doBooleanCommand and didn't work before this... This also removes networks that we failed in the middle of configuring (which avoids having half-configured networks around).

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for the first patch

Nothing to see here... unimportant hunk missing from the first patch.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make nsWifiWorker talk back to the DOM

This patch makes nsWifiWorker use the message manager to talk back to the DOM component and "implements" the APIs defined in nsIDOMWifiManager. The final patch provides a bunch of support for the APIs and implements a few more.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix bugs in setEnabled and in startup

There were a couple of typos in setEnabled that this fixes. This also makes us always put the WifiManager in the UNINITIALIZED state when it's disabled. This also suppresses connected/disconnected events that we might send while connecting to an already-running wpa_supplicant. We suppress them and then when our internal state is set up, we ask the supplicant for its status at which point we can fire the connected/disconnected events.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Super-small patch to make sure that a map doesn't have a prototype

This is good hygiene IMO so that we can use "in" with impunity.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add DHCP notifications and keep more information about the current network around

In order to know that we're in the connecting state, we need to notify internally when we have DHCP info. This patch also starts maintaining local state about the currently-connected network (which is useful for notifying the DOM about it) and collects way more information on startup.

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: The big one: Connect the DOM to the manager

This patch finishes implementing the DOM API on the chrome side. That means that it sends out all of the necessary notifications, listens to all of the required events and pumps the supplicant state machine in line with what the DOM has requested.

Reading through the patch, I notice that there's a useless this.currentNetwork; line. Please pretend it says "this.currentNetwork = null;"

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add the DOMRequest service to Services.jsm

This is already r=fabrice

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Knock us out of the dormant state if we end up in it

In the dormant state, the wpa_supplicant stops trying to do anything and generally becomes useless. A simple reconnect command should be enough to get it going again.

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Re-do the way we choose networks

I stole this idea from Android, but it's basically the best way to do this. In order to force wpa_supplicant's hand, we disable all of the networks except the one that we want and then re-enable them later. This has the side-effect that most of the time, if we lose the connection, we'll be able to connect to any of the configured networks and not just the most-recently-conneted one. In order to make sure that once everything is reconnected that we remember the user's choice, we have to assign priorities to our networks as well.

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: As described

This actually does the priority number dance.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Manage connectionInfo and scan results properties better

This makes the manager's connectionInfo resemble reality more of the time as well as makes scan results' .connected and .signal members more useful.

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Oh, and another todo: once we connect or otherwise change our state, we need to fire off a scan results notification so that the settings page's idea of our world is correct (an annoying side-effect of not being able to share objects directly between the backend and the frontend).

Comment 41

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

Comment on attachment 605472 [details] [diff] [review]
Create a wifi DOM API and matching component

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

>+    this._messages = ["WifiManager:setEnabled:Return:OK", "WifiManager:setEnabled:Return:KO",
>+                      "WifiManager:getNetworks:Return:OK", "WifiManager:getNetworks:Return:KO",
>+                      "WifiManager:select:Return:OK", "WifiManager:select:Return:KO",
>+                      "WifiManager:onassociate", "WifiManager:onconnect", "WifiManager:ondisconnect"];

Is ":KO" an emerging pattern?  It doesn't scan well to me.  Would
recommend using "NO" or "XX" or something more obviously bad.

>+  observe: function(aSubject, aTopic, aData) {

Not sure what usual practice is, but I would assert
"inner-window-destroyed" is the message here.

>+  _sendMessage: function(name, data, request) {

Not too enamored of the side effect of stashing the request object
away here.  If you can't think of a name that makes that clearer, ok
by me.

>+  _getRequest: function(id) {

Would recommend calling this "_takeRequest()", "_removeRequest()" or
something.  The side effect of this isn't obvious from the name.

>diff --git a/dom/wifi/nsIWifi.idl b/dom/wifi/nsIWifi.idl

>+[scriptable, uuid(1509221f-470e-4445-b476-88e74fd5c617)]
>+interface nsIDOMWifiManager : nsISupports {
>+    /**
>+     * TODO Remove in favor of a settings API.
>+     * Activates or disactivates wifi.
>+     * onsuccess: Wifi has been successfully activated and can start
>+     *            attempting to connect to networks. request.value will be true.

Activated -> true, deactivated -> false?  Looks we always return true
above.  Not too important.

>+    /**
>+     * Takes one of the networks returned from getNetworks and tries to
>+     * connect to it.
>+     * onsuccess: We have started attempting to associate with the network.
>+     *            request.value is true.
>+     * onerror: We were unable to select the network. This most likely means a
>+     *          configuration error.
>+     */
>+    nsIDOMDOMRequest select(in jsval network);
>+

s/select/connect/, or maybe associate(), unless you have a specific
reason not to.

Is this a network object or SSID?  I assume network object.  Please
clarify.

>+    /**
>+     * A network object describing the currently connected network.
>+     */
>+    readonly attribute jsval connected;
>+

s/connected/connectedNetwork/ plz.

>+[scriptable, uuid(4674c6f1-ea64-44db-ac2f-e7bd6514dfd6)]
>+interface nsIDOMWifiStateChangeEvent : nsIDOMEvent {
>+    readonly attribute jsval network;

Network object right?

All nits and naming, r=me with edits.

Comment 42

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

Comment on attachment 605484 [details] [diff] [review]
Make nsWifiWorker talk back to the DOM

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

>+  _sendMessage: function(message, success, data, rid, mid) {
>+    this._mm.sendAsyncMessage(message + (success ? "OK" : "KO"),

Would recommend sticking the magic colon here, before ":OK", and not
putting that burden on callers.  There will be a mistake :).  (Though
not here, after I finished looking.)

Comment 43

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

Comment on attachment 605494 [details] [diff] [review]
The big one: Connect the DOM to the manager

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

>+// These constants shamelessly ripped from WifiManager.java
>+const MIN_RSSI = -100;
>+const MAX_RSSI = -55;
>+

Are these dB by any chance?  I'm kind of lost in unitless values here.

>+function calculateSignal(strength) {
>+  if (strength > 0)
>+    strength -= 256;

Er why?

>+  return Math.floor(((strength - MIN_RSSI) / (MAX_RSSI - MIN_RSSI)) * 100);

Unless there's a strong reason otherwise (like a spec says so),
usually better to normalize to [0.0, 1.0].  Even better to leave in
dB, if that's what we're getting back from the chip.

>+function isHexString(s) {

This is really asking, "does this look like a WEP hex key".  Hex
strings can be of any length :).  Please change the name.

>+  // configuredNetworks is a map from SSID -> our view of a network. It only
>+  // lists networks known to the wpa_supplicant. The SSID field (and other
>+  // fields) are quoted for ease of use with WifiManager commands.

The quote() here doesn't escape inner quotes.  Why is that OK?  Does
something else sanitize the strings?

>+  this.currentNetwork;
> 

Nuke.

>+  // Given a connection status network
>+  netToDOM = function(net) {
>+    // Takes a network from self.configuredNetworks and prepares it for the
>+    // DOM.

Not a fan of the inner/outer comment style, please unify into a single
comment.

>+  netFromDOM = function(net) {

>+    function checkAssign(name, checkStar) {
>+      if (name in net) {
>+        let value = net[name];
>+        if (checkStar && value === '*')
>+          delete net[name];
>+        else
>+          net[name] = quote(value);

Why is it OK to not sanitize these.

>     WifiManager.getConfiguredNetworks(function(networks) {

>+        if (!network.ssid) {
>+          delete networks[net]; // TODO support these?

What's the motivation for ignoring networks without SSIDs?

>+            // Note: we don't hand out passwords here! The * marks that there
>+            // is a password that we're hiding.
>+            if ("psk" in known && known.psk)
>+              network.password = "*";
>+            if ("identity" in known && known.identity)
>+              network.identity = dequote(known.identity);
>+            if ("password" in known && known.password)
>+              network.password = "*";
>+            if ("wep_key0" in known && known.wep_key0)
>+              network.password = "*";

if ("psk" in known ... ||
    "password" in known ... ||
    "wep_key0" in known ...)
  network.password = "*";

Quoting here makes me uneasy.  Clearing request pending argument why
it's OK.

Comment 44

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

Comment on attachment 605499 [details] [diff] [review]
Knock us out of the dormant state if we end up in it

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

>+    if (this.state === "DORMANT") {
>+      // The dormant state is a bad state to be in since we won't
>+      // automatically connect. Try to knock us out of it.
>+      WifiManager.reconnect(function(){});

What does the dormant state mean?  How do we get into it?  A little
nervous we're interfering with a power-saving feature or something
here.

Comment 45

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

Comment on attachment 605500 [details] [diff] [review]
Re-do the way we choose networks

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

>+  // In order to select a specific network, we disable the rest of the
>+  // networks known to us. However, in general, we want the supplicant to
>+  // connect to which ever network it thinks is best, so when we select the
>+  // proper network (or fail to), we need to re-enabled the rest.

"re-enable"

Comment 46

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

Comment on attachment 605501 [details] [diff] [review]
As described

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

>+    // XXX Do we have to worry about overflow/going too high here?
>+    privnet.priority = ++this._highestPriority;

This is going to "overflow" into inexact floating point, right?  That
seems like it would be bad ... or tell me why it wouldn't be.

Comment 47

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

Comment on attachment 605502 [details] [diff] [review]
Manage connectionInfo and scan results properties better

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

>+          let signal = calculateSignal(Number(match[3]));
>+          if (signal > network.signal)
>+            network.signal = signal;

Seems more reasonable to return the most recent signal strength, no?

r=me with change or argument otherwise.

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #41)
> Is ":KO" an emerging pattern?  It doesn't scan well to me.  Would
> recommend using "NO" or "XX" or something more obviously bad.

I stole the pattern from webapps. I can change it to NO.

> >+  _sendMessage: function(name, data, request) {
> 
> Not too enamored of the side effect of stashing the request object
> away here.  If you can't think of a name that makes that clearer, ok
> by me.

Is _sendMessageForRequest any better?

> Would recommend calling this "_takeRequest()", "_removeRequest()" or
> something.  The side effect of this isn't obvious from the name.

Sold on _takeRequest.

> s/select/connect/, or maybe associate(), unless you have a specific
> reason not to.

I'll go with associate.

> Is this a network object or SSID?  I assume network object.  Please
> clarify.

Yeah. It's a network object that specifies how to connect to the network (e.g. it can have a keyManagement field that specifies the type of security).

> >+[scriptable, uuid(4674c6f1-ea64-44db-ac2f-e7bd6514dfd6)]
> >+interface nsIDOMWifiStateChangeEvent : nsIDOMEvent {
> >+    readonly attribute jsval network;
> 
> Network object right?

Yes. Right now it's a pretty stripped down network object, though... It's basically a glorified SSID. This is one of the APIs that needs revisiting.

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #43)
> Are these dB by any chance?  I'm kind of lost in unitless values here.

Some googling around shows that yes, these are dBs.

> >+function calculateSignal(strength) {
> >+  if (strength > 0)
> >+    strength -= 256;
> 
> Er why?

Apparently some drivers store the signal strength as a straight-up 8-bit integer and don't want to lose a bit representing negative values. For those drivers, they add 256 to the actual value, so in order to normalize, we have to convert back to dB.

> Unless there's a strong reason otherwise (like a spec says so),
> usually better to normalize to [0.0, 1.0].  Even better to leave in
> dB, if that's what we're getting back from the chip.

My general feeling is that dB isn't usually what consumers of this API are going to want... This field is almost always (possibly always) used to display the signal strength in the scan results. It seems like it might be worth it to factor out the work of converting from dB to a relative scale.

> The quote() here doesn't escape inner quotes.  Why is that OK?  Does
> something else sanitize the strings?

There isn't a way to escape inner quotes. Instead, the supplicant takes the first and last quotes as the string and ignores inner quotes (it can do that because all values are the last parameter and so it can scan for the NUL byte at the end of the string).

> What's the motivation for ignoring networks without SSIDs?

It isn't clear how to expose them in the API (which is pretty heavily SSID-based at this point). I'd prefer to tackle this in a followup.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #44)
> What does the dormant state mean?  How do we get into it?  A little
> nervous we're interfering with a power-saving feature or something
> here.

DORMANT is a pseudo-state that indicates that we disconnected as the result of an explicit disconnect command. Right now the only way we can get into this state is if DHCP fails. Once we have more power-saving stuff added we can not automatically reconnect.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #46)
> This is going to "overflow" into inexact floating point, right?  That
> seems like it would be bad ... or tell me why it wouldn't be.

That would be bad... but you would have to explicitly click on your wireless networks 2^26 times or so. IMO we can deal with re-prioritizing in a followup.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #47)
> Seems more reasonable to return the most recent signal strength, no?

This isn't a question of recent verses not, it's a question of which base station we take the signal strength from. The scan results include all BSSs broadcasting a given id in random order. We probably need a followup here on making sure not to confuse two networks with different security capabilities but with the same SSID.

Anything I didn't respond to explicitly, I'll fix.

Comment 50

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 605499 [details] [diff] [review]
Knock us out of the dormant state if we end up in it

I extended the comment in onstatechange to be:
      // The dormant state is a bad state to be in since we won't
      // automatically connect. Try to knock us out of it. We only
      // hit this state when we've failed to run DHCP, so trying
      // again isn't the worst thing we can do. Eventually, we'll
      // need to detect if we're looping in this state and bail out.

Comment 51

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 605501 [details] [diff] [review]
As described

Re-requesting review. Dealing with this in a followup should be fine as long as nobody clicks on a network more than 2^26 times.

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Interdiff for comments on attachment 605494

Chris, let me know if you want the full patch again.

I think with this patch and my comments above, all of your comments should be addressed or at least pushed into followups.

Comment 53

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

Comment on attachment 605499 [details] [diff] [review]
Knock us out of the dormant state if we end up in it

I don't see the updated comment here.  Please add.

Also please file a followup for this.  Sounds like a potential battery drain.

Comment 54

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

Comment on attachment 605501 [details] [diff] [review]
As described

File followup please.

It would take years to "overflow" into inexact, so not incredibly high priority, but this is a bad example to set for the kids :).

Comment 55

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

(In reply to Blake Kaplan (:mrbkap) from comment #48)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #41)
> > Is ":KO" an emerging pattern?  It doesn't scan well to me.  Would
> > recommend using "NO" or "XX" or something more obviously bad.
> 
> I stole the pattern from webapps. I can change it to NO.
> 

If it's some emerging style, OK, though not to my taste.  Wouldn't r- over that.

> > >+  _sendMessage: function(name, data, request) {
> > 
> > Not too enamored of the side effect of stashing the request object
> > away here.  If you can't think of a name that makes that clearer, ok
> > by me.
> 
> Is _sendMessageForRequest any better?
> 

Yeah, a little.

Comment 56

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

(In reply to Blake Kaplan (:mrbkap) from comment #49)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #43)
> > >+function calculateSignal(strength) {
> > >+  if (strength > 0)
> > >+    strength -= 256;
> > 
> > Er why?
> 
> Apparently some drivers store the signal strength as a straight-up 8-bit
> integer and don't want to lose a bit representing negative values. For those
> drivers, they add 256 to the actual value, so in order to normalize, we have
> to convert back to dB.
> 

OK, makes some sense.  Please add comment.

> > Unless there's a strong reason otherwise (like a spec says so),
> > usually better to normalize to [0.0, 1.0].  Even better to leave in
> > dB, if that's what we're getting back from the chip.
> 
> My general feeling is that dB isn't usually what consumers of this API are
> going to want... This field is almost always (possibly always) used to
> display the signal strength in the scan results. It seems like it might be
> worth it to factor out the work of converting from dB to a relative scale.
> 

dB is most "correct" to report, and I've seen some UIs report raw dB.  It's pretty easy to convert from dB to normalized pseudo-scale, but can't go backwards.

What about reporting both dB and [0, 1]-normalized values, with the caveat in spec that dB is optional?  (In case some chips give us some other unit.)

> > The quote() here doesn't escape inner quotes.  Why is that OK?  Does
> > something else sanitize the strings?
> 
> There isn't a way to escape inner quotes. Instead, the supplicant takes the
> first and last quotes as the string and ignores inner quotes (it can do that
> because all values are the last parameter and so it can scan for the NUL
> byte at the end of the string).
> 

OK.  Please document this :).  It will come up in secreview.

> > What's the motivation for ignoring networks without SSIDs?
> 
> It isn't clear how to expose them in the API (which is pretty heavily
> SSID-based at this point). I'd prefer to tackle this in a followup.
> 

Fine by me.

> (In reply to Chris Jones [:cjones] [:warhammer] from comment #47)
> > Seems more reasonable to return the most recent signal strength, no?
> 
> This isn't a question of recent verses not, it's a question of which base
> station we take the signal strength from. The scan results include all BSSs
> broadcasting a given id in random order. We probably need a followup here on
> making sure not to confuse two networks with different security capabilities
> but with the same SSID.
> 

Oh, I see.  I misread the code.  Followup for SSID confusion sounds very good.

Comment 57

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

Comment on attachment 605763 [details] [diff] [review]
Interdiff for comments on attachment 605494 [details] [diff] [review]

Oh, you already added what I requested.

We can move dB/normalized scale discussion to followup.

Comment 58

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Re-add security-checking code

I'd forgotten to add this back in... While I was here, I fixed a couple of typos in Webapps.js

Comment 59

[Blake Kaplan (:mrbkap) (inactive)]

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

I'll file all of the followups asked for in this bug tomorrow.