We need new DOM API(s) to support automatic and manual selection of cellular networks. At the outset, this could simply include: * Getter for the current network selection mode (automatic/manual) * Setter for automatic mode (the modem chooses a network) * Setter for manual mode w/ operator (or mcc/mnc?)

Can't these just be settings?

(In reply to Philipp von Weitershausen [:philikon] from comment #1) > Can't these just be settings? Can the settings changes just manipulate the DOM APIs? :) It seems odd that we would expose a list of networks via getNetworks() but not have the ability to also manually change networks via the same API..

(In reply to Marshall Culpepper [:marshall_law] from comment #2) > (In reply to Philipp von Weitershausen [:philikon] from comment #1) > > Can't these just be settings? > > Can the settings changes just manipulate the DOM APIs? :) It seems odd that > we would expose a list of networks via getNetworks() but not have the > ability to also manually change networks via the same API.. I ... tend to agree, but it seems the general approach we're taking with a lot of our APIs (e.g. Wifi) is to use settings. We already configure 3G data and the cell radio power status that way.

Initial API for network selection modes, built on top of the patch in Bug 761482.

Comment on attachment 632046 [details] [diff] [review] MobileConnection network selection APIs - v1 Review of attachment 632046 [details] [diff] [review]: ----------------------------------------------------------------- Very, very nice. Some points below. ::: dom/system/gonk/RILContentHelper.js @@ +160,5 @@ > cardState: RIL.GECKO_CARDSTATE_UNAVAILABLE, > voiceConnectionInfo: null, > dataConnectionInfo: null, > + networkSelectionMode: RIL.GECKO_NETWORK_SELECTION_UNKNOWN, > + selectingNetwork: null, Maybe separate `selectingNetwork` from the other members with an empty line since it's internal state. In fact, adding a comment above it would be nice, too. Could also add an underscore to it, but whatever. @@ +182,5 @@ > + Cr.NS_ERROR_UNEXPECTED); > + } > + > + if (this.selectingNetwork) { > + throw new Error("Already selecting a network: " + this.selectingNetwork); Why not throw a Components.Exception here? @@ +205,5 @@ > + && this.voiceConnectionInfo.network === network) { > + > + // Already manually selected this network, so skip > + Services.DOMRequest.fireSuccess(request, null); > + return request; AFAIK `fireSuccess` fires the event synchronously, so this code won't really work. You need to return the request and fire the success event on the next event loop tick: Services.tm.currentThread.dispatch(this.fireRequestSuccess.bind(this, requestId, null), Ci.nsIThread.DISPATCH_NORMAL); return request; (This means you'll have to get a `requestId` for it in this case, too, which is a Good Thing(tm), because it prevents the request from being GC'ed.) Also, ahem, tests :) @@ +212,5 @@ > + this.selectingNetwork = network; > + let requestId = this.getRequestId(request); > + > + cpmm.sendAsyncMessage("RIL:SelectNetwork", { > + requestId: requestId, mnc: mnc, mcc: mcc Nit: each field on its own line, please. ::: dom/system/gonk/ril_consts.js @@ +7,5 @@ > + > +// Set individually to debug specific layers > +const DEBUG_WORKER = false || DEBUG_ALL; > +const DEBUG_CONTENT_HELPER = false || DEBUG_ALL; > +const DEBUG_RIL = false || DEBUG_ALL; Ooooh I like that!

(In reply to Philipp von Weitershausen [:philikon] from comment #5) > @@ +182,5 @@ > > + Cr.NS_ERROR_UNEXPECTED); > > + } > > + > > + if (this.selectingNetwork) { > > + throw new Error("Already selecting a network: " + this.selectingNetwork); > > Why not throw a Components.Exception here? > I was under the impression that Components.Exception is used in the case of unexpected system errors (i.e, a non-null DOMWindow should've been passed in here directly from the system, if not we're in big trouble!) I'm using Error in the cases where the function params (or call state) are invalid, due to user input. This seems slightly less harsh.. wdyt? > @@ +205,5 @@ > > + && this.voiceConnectionInfo.network === network) { > > + > > + // Already manually selected this network, so skip > > + Services.DOMRequest.fireSuccess(request, null); > > + return request; > > AFAIK `fireSuccess` fires the event synchronously, so this code won't really > work. You need to return the request and fire the success event on the next > event loop tick: > > Services.tm.currentThread.dispatch(this.fireRequestSuccess.bind(this, > requestId, null), Ci.nsIThread.DISPATCH_NORMAL); > return request; > > (This means you'll have to get a `requestId` for it in this case, too, which > is a Good Thing(tm), because it prevents the request from being GC'ed.) > > Also, ahem, tests :) Ahh good catch :) I'll fix this up and make sure it's tested. Other than this, did you see all the fancy new selectNetwork tests? :) > ::: dom/system/gonk/ril_consts.js > @@ +7,5 @@ > > + > > +// Set individually to debug specific layers > > +const DEBUG_WORKER = false || DEBUG_ALL; > > +const DEBUG_CONTENT_HELPER = false || DEBUG_ALL; > > +const DEBUG_RIL = false || DEBUG_ALL; > > Ooooh I like that! I almost went off on a tangent here.. One of the things that I really like about the Android logging system is the builtin log categories and log levels. Right now we are only using the "Gecko" category and the "INFO" log level. At some point I might expose the actual android log API to our various JS layers, that way we can use real categories and log levels and get some of the other niceties like knowing when a specific log level is enabled or disabled in the system.

(In reply to Marshall Culpepper [:marshall_law] from comment #6) > I was under the impression that Components.Exception is used in the case of > unexpected system errors (i.e, a non-null DOMWindow should've been passed in > here directly from the system, if not we're in big trouble!) > > I'm using Error in the cases where the function params (or call state) are > invalid, due to user input. This seems slightly less harsh.. wdyt? I don't quite understand where you get that from, but ok. Either way both kinds of exceptions will bubble to content and be thrown there, so it doesn't really matter. I'll defer to Jonas or any other DOM peer to make a decision here. > Ahh good catch :) I'll fix this up and make sure it's tested. Other than > this, did you see all the fancy new selectNetwork tests? :) Yes, yes. Very nice! > At some point I might expose the actual android log API to our various JS > layers, that way we can use real categories and log levels and get some of > the other niceties like knowing when a specific log level is enabled or > disabled in the system. You and me, brother. Bug 723354.

Updated Marionette tests to cover when a network is already the currently selected test. Fixed up "immediate" request onsuccess calls to be async.

Comment on attachment 632426 [details] [diff] [review] MobileConnection network selection APIs - v2 Review of attachment 632426 [details] [diff] [review]: ----------------------------------------------------------------- I like. ::: dom/network/tests/marionette/test_mobile_networks.js @@ +110,5 @@ > + > +function testSelectNetworkAutomatically() { > + let request = connection.selectNetworkAutomatically(); > + ok(request instanceof DOMRequest, > + "request instanceof " + request.constructor); Nit: align args @@ +157,5 @@ > + // attempt to selectNetwork while one request has already been sent > + throwsException(function() { > + let request1 = connection.selectNetwork(telkilaNetwork); > + request1.onsuccess = function() { > + setTimeout(testSelectExistingNetworkManual, 1); Might as well use 0 instead of 1, since there's a minimum of ~10ms anyway. @@ +192,5 @@ > + }; > + } > + > + request.onsuccess = function() { > + is(voiceChanged, false); Can we rely on "voicechange" etc. firing before the request's success event? Might be safer to do check and nextTest() on a `window.setTimeout(..., 0)`.

Requesting another review. After putting the registration callback checks on the next tick, I started getting failures. This comes back the problem that RIL:VoiceInfoChanged / "voicechange" events were being sent in duplicate, which I originally mistaked for intended behavior. This new patch tries to mitigate this in a few ways: * in ril_worker.js, requestNetworkInfo() now batches all the change messages into one super "networkinfochanged" message back to RadioInterfaceLayer * in RadioInterfaceLayer.js, there is a new handler that only fires a RIL:VoiceInfoChanged / "voicechange" once, even if both an "operatorchange" and "voiceregistrationchange" event are received. There is also some additional logic to check whether or not the radioState.voice object changed in updateVoiceRegistrationState (see comments)

Comment on attachment 633398 [details] [diff] [review] MobileConnection network selection APIs - v3 Great work!

After further testing on device, there were some race conditions with the way the bulk requestNetworkInfo() was being handled. This final patch addresses those race conditions, and has been confirmed working on both emulator w/ Marionette and Nexus S.

Added more comments to explain the new behavior, and renamed _processNetworkInfoResult -> _receivedNetworkInfo

Comment on attachment 634552 [details] [diff] [review] MobileConnection network selection APIs - v5 Review of attachment 634552 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adding those additional comments! r=me