Closed Bug 732982 Opened 13 years ago Closed 13 years ago

Expose a Wifi API to the DOM

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(19 files, 13 obsolete files)

37.16 KB, patch
Details | Diff | Splinter Review
14.81 KB, patch
cjones
: review+
Details | Diff | Splinter Review
1.70 KB, patch
cjones
: review+
Details | Diff | Splinter Review
2.95 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.56 KB, patch
cjones
: review+
Details | Diff | Splinter Review
1.35 KB, patch
cjones
: review+
Details | Diff | Splinter Review
1021 bytes, patch
Details | Diff | Splinter Review
3.95 KB, patch
cjones
: review+
Details | Diff | Splinter Review
2.92 KB, patch
cjones
: review+
Details | Diff | Splinter Review
855 bytes, patch
cjones
: review+
Details | Diff | Splinter Review
3.80 KB, patch
cjones
: review+
Details | Diff | Splinter Review
11.94 KB, patch
Details | Diff | Splinter Review
1.02 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.08 KB, patch
cjones
: review+
Details | Diff | Splinter Review
3.66 KB, patch
cjones
: review+
Details | Diff | Splinter Review
1.59 KB, patch
cjones
: review+
Details | Diff | Splinter Review
4.32 KB, patch
cjones
: review+
Details | Diff | Splinter Review
5.27 KB, patch
cjones
: review+
Details | Diff | Splinter Review
4.11 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
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.
Attachment #602912 - Flags: review?(gal)
This should help at least a little if we're not in range of any of our networks and is a little cleaner.
Attachment #602913 - Flags: review?(gal)
Attached patch Expose navigator.mozWifiManager (obsolete) — Splinter Review
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.
Attachment #602915 - Flags: review?(fabrice)
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.
Attachment #602924 - Flags: review?(gal)
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.
Attachment #602925 - Flags: review?(gal)
Attached patch Implement WifiManager.select (obsolete) — Splinter Review
This implements WifiManager.select, which can be used to connect to a given network returned from getNetworks.
Attachment #602929 - Flags: review?(gal)
Attached file Page using the API (obsolete) —
This is the page I've been using for testing locally on my device.
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
Attachment #602915 - Flags: review?(fabrice) → review-
This patch addresses fabrice's comments. I'll add the new service to the wiki when I land it.
Attachment #602915 - Attachment is obsolete: true
Attached patch Patch for fabrice (obsolete) — Splinter Review
This patch includes the entirety of the new component plus the changes to the chrome service that make it use the message manager.
Attachment #603292 - Flags: review?(fabrice)
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?
Attachment #603292 - Flags: review?(fabrice) → feedback+
Attached patch flesh out the wifimanager API (obsolete) — Splinter Review
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.
Attachment #603736 - Flags: review?(gal)
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.
Attachment #603739 - Flags: review?(gal)
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.
Attachment #602912 - Flags: review?(gal) → review?(jones.chris.g)
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>
Sure. I'll do that as a followup patch before landing.
(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.
Blocks: 734018
Attachment #602912 - Flags: review?(jones.chris.g) → review+
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>
Attached patch Full patchSplinter Review
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!
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.
Attached patch Dummy patch for "review" (obsolete) — Splinter 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.
Attachment #602912 - Attachment is obsolete: true
Attachment #602913 - Attachment is obsolete: true
Attachment #602924 - Attachment is obsolete: true
Attachment #602925 - Attachment is obsolete: true
Attachment #602929 - Attachment is obsolete: true
Attachment #602930 - Attachment is obsolete: true
Attachment #603289 - Attachment is obsolete: true
Attachment #603292 - Attachment is obsolete: true
Attachment #603733 - Attachment is obsolete: true
Attachment #603736 - Attachment is obsolete: true
Attachment #603739 - Attachment is obsolete: true
Attachment #604490 - Flags: review?(jones.chris.g)
Attachment #602913 - Flags: review?(gal)
Attachment #602924 - Flags: review?(gal)
Attachment #602925 - Flags: review?(gal)
Attachment #602929 - Flags: review?(gal)
Attachment #603733 - Flags: review?(gal)
Attachment #603736 - Flags: review?(gal)
Attachment #603739 - Flags: review?(gal)
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! :)
Attachment #604490 - Flags: review?(gal)
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!
Attachment #604490 - Attachment is obsolete: true
Attachment #604490 - Flags: review?(jones.chris.g)
Attachment #604490 - Flags: review?(gal)
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.
Attachment #605472 - Flags: review?(jones.chris.g)
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.
Attachment #605474 - Flags: review?(jones.chris.g)
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.
Attachment #605476 - Flags: review+
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.
Attachment #605477 - Flags: review?(jones.chris.g)
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).
Attachment #605478 - Flags: review?(jones.chris.g)
Nothing to see here... unimportant hunk missing from the first patch.
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.
Attachment #605484 - Flags: review?(jones.chris.g)
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.
Attachment #605487 - Flags: review?(jones.chris.g)
This is good hygiene IMO so that we can use "in" with impunity.
Attachment #605488 - Flags: review?(jones.chris.g)
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.
Attachment #605491 - Flags: review?(jones.chris.g)
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;"
Attachment #605494 - Flags: review?(jones.chris.g)
This is already r=fabrice
Attachment #605497 - Flags: review+
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.
Attachment #605499 - Flags: review?(jones.chris.g)
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.
Attachment #605500 - Flags: review?(jones.chris.g)
Attached patch As describedSplinter Review
This actually does the priority number dance.
Attachment #605501 - Flags: review?(jones.chris.g)
This makes the manager's connectionInfo resemble reality more of the time as well as makes scan results' .connected and .signal members more useful.
Attachment #605502 - Flags: review?(jones.chris.g)
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 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.
Attachment #605472 - Flags: review?(jones.chris.g) → review+
Attachment #605474 - Flags: review?(jones.chris.g) → review+
Attachment #605477 - Flags: review?(jones.chris.g) → review+
Attachment #605478 - Flags: review?(jones.chris.g) → review+
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.)
Attachment #605484 - Flags: review?(jones.chris.g) → review+
Attachment #605487 - Flags: review?(jones.chris.g) → review+
Attachment #605488 - Flags: review?(jones.chris.g) → review+
Attachment #605491 - Flags: review?(jones.chris.g) → review+
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.
Attachment #605494 - Flags: review?(jones.chris.g)
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.
Attachment #605499 - Flags: review?(jones.chris.g)
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"
Attachment #605500 - Flags: review?(jones.chris.g) → review+
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.
Attachment #605501 - Flags: review?(jones.chris.g)
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.
Attachment #605502 - Flags: review?(jones.chris.g) → review+
(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.
(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 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.
Attachment #605499 - Flags: review?(jones.chris.g)
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.
Attachment #605501 - Flags: review?(jones.chris.g)
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.
Attachment #605763 - Flags: review?(jones.chris.g)
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.
Attachment #605499 - Flags: review?(jones.chris.g) → review+
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 :).
Attachment #605501 - Flags: review?(jones.chris.g) → review+
(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.
(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 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.
Attachment #605763 - Flags: review?(jones.chris.g) → review+
I'd forgotten to add this back in... While I was here, I fixed a couple of typos in Webapps.js
Attachment #605923 - Flags: review?(fabrice)
Attachment #605923 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/b52d3db4efa7 I'll file all of the followups asked for in this bug tomorrow.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: