This involves several parts: (a) implementing navigator.mozMobileConnection.getNetworks so that the UI can read the available list of networks (b) reading/watching the settings API for a manual/automatic network selection and if manual, which network (c) sending the necessary parcels in the RIL worker

Hi, Marshall For (a) and (c) you can find some sample code in Bug 731786 thanks

I ended up added a new OperatorInfo type to describe each result from the getNetworks() request, let me know if this is the right way (it provides a little more metadata than just simply "name" which might be relevant)

Phil, I just noticed https://wiki.mozilla.org/WebAPI/WebMobileConnection lists the API as "listAvailableNetworks". Should I rename getNetworks to this in the interface / impl?

(In reply to Marshall Culpepper [:marshall_law] from comment #3) > Phil, I just noticed https://wiki.mozilla.org/WebAPI/WebMobileConnection > lists the API as "listAvailableNetworks". Should I rename getNetworks to > this in the interface / impl? Nope, the wiki is outdated. Can you update it, please?

Comment on attachment 625655 [details] [diff] [review] Initial patch for implementation of mozMobileConnection.getNetworks() Review of attachment 625655 [details] [diff] [review]: ----------------------------------------------------------------- Great start! Just a few suggestions/nits beliow. Note that this is also only one half of the feature. We would still need to allow a user to manually choose the operator. Perhaps we should do that in a follow-up though, since we might want to bikeshed^W debate the exact mechanism (settings API and if so, which value? setting `operator`? ...) ::: dom/network/interfaces/nsIDOMMobileConnection.idl @@ +31,5 @@ > > /** > * Search for available networks. > * > + * If successful, the request result will be an array of nsIDOMMozMobileOperatorInfo. I'm generally in favour of this plan. Should we also make nsIDOMMozMobileConnectionInfo::operator an nsIDOMMozMobileOperatorInfo? In any case, asking sicking for superreview on the interface. @@ +119,5 @@ > + > + /** > + * Numeric id of the network operator (in the form of a String) > + */ > + readonly attribute DOMString numeric; Can you find out if we can make this a `long`? @@ +124,5 @@ > + > + /** > + * State of this network operator. > + * > + * Possible values: 'unknown', 'available', 'connected', 'forbidden' In other APIs we've used `null` instead of the string 'unknown' ::: dom/system/gonk/RILContentHelper.js @@ +121,5 @@ > > cardState: RIL.GECKO_CARDSTATE_UNAVAILABLE, > voiceConnectionInfo: null, > dataConnectionInfo: null, > + _domRequests: null, Please rebase your patch on top of bug 731786 where this has already been implemented. ::: dom/system/gonk/ril_worker.js @@ +1098,5 @@ > > /** > + * Get the available networks (GSM only?) > + */ > + getAvailableNetworks: function getAvailableNetworks(message) { For no good reason, our convention is to call the parameter `options` here. (I would not be opposed to a file-global renaming, but we should keep it consistent for now.) @@ +1100,5 @@ > + * Get the available networks (GSM only?) > + */ > + getAvailableNetworks: function getAvailableNetworks(message) { > + if (DEBUG) debug("Getting available networks"); > + Buf.newParcel(REQUEST_QUERY_AVAILABLE_NETWORKS, { requestId: message.requestId }); Just pass the message object. No need to create objects unnecessarily. @@ +2591,5 @@ > }; > RIL[REQUEST_SET_NETWORK_SELECTION_AUTOMATIC] = null; > RIL[REQUEST_SET_NETWORK_SELECTION_MANUAL] = null; > +RIL[REQUEST_QUERY_AVAILABLE_NETWORKS] = function REQUEST_QUERY_AVAILABLE_NETWORKS(length, options) { > + let message = {type: "getAvailableNetworks", requestId: options.requestId}; If you pass the message object in `getAvailableNetworks`, the `options` parameter will contain exactly this information already. @@ +2595,5 @@ > + let message = {type: "getAvailableNetworks", requestId: options.requestId}; > + > + if (options.rilRequestError) { > + message.error = options.rilRequestError; > + this.sendDOMMessage(message); Again, you can just morph `options` into the object you want to throw over the wall to the main thread. @@ +2596,5 @@ > + > + if (options.rilRequestError) { > + message.error = options.rilRequestError; > + this.sendDOMMessage(message); > + return; Also, you want to notify a different message name, e.g. "RIL:GetAvailableNetworks:KO", to indicate the error. See bug 731786 for some good examples. @@ +2607,5 @@ > + networks.push({ > + longName: strings[i], > + shortName: strings[i+1], > + numeric: strings[i+2], > + state: strings[i+3] Indent by two spaces, please. @@ +2612,5 @@ > + }); > + } > + > + message.networks = networks; > + this.sendDOMMessage(message); See above re: reusing `options`

Updated to address Phil's comments. As noted, this patch is only partial now because we will need to address how automatic / manual network selection APIs work in conjunction w/ the Settings API. Regarding the KO/OK pattern I'll be filing a bug shortly to discuss a more human friendly pattern that saves us some duplication as well.. Also, when re-basing I noticed that none of the new cardlock based APIs are correctly cleaning up requests after their responses are fired. I will also be creating a new bug for this soon.

(In reply to Marshall Culpepper [:marshall_law] from comment #6) > Also, when re-basing I noticed that none of the new cardlock based APIs are > correctly cleaning up requests after their responses are fired. I will also > be creating a new bug for this soon. Thanks for catching this, do you need me to file the bug first?

(In reply to Yoshi Huang[:yoshi] from comment #7) > Thanks for catching this, do you need me to file the bug first? Just filed here: Bug 757512

I've done some more research WRT the "numeric" field in the response from QUERY_AVAILABLE_NETWORKS. According to ril.h, what Android calls "numeric" is supposed to be a 5 or 6 digit tuple, containing both the MCC (country code) + MNC (network code): https://github.com/mozilla-b2g/android-hardware-ril/blob/master/include/telephony/ril.h#L2015 After a quick survey of all the MCC / MNC codes available on Wikipedia, it looks like all countries have 3 digit MCC with the MNC being 2 or 3 digits. There is a "test network" that has 001 as it's MCC -- it seems any non-3 digit MCC is padded on the left with zeroes. Here's the page I read for reference: http://en.wikipedia.org/wiki/Mobile_Network_Code The Wikipedia page above does call out that some SDKs (such as Blackberry) expose the MCC+MNC tuple as binary-coded decimal. I'm not sure if this is a carrier requirement, or just an oddity of their platform. In my limited testing with my T-Mobile SIM, I'm currently getting tuples of 31026 (MCC 310, MNC 26) for T-Mobile, and 310410 (MCC 310, MNC 410) for AT&T, which both match up to the table in Wikipedia. After reading these, and comparing with my local testing, I feel fairly confident that we could actually extract the useful MCC / MNC directly and expose them as integers. I'll attach a modified version of the OperatorInfo interface and corresponding RIL support shortly.

Updated to provide MNC / MCC in OperatorInfo, "unknown" status now becomes null per Phil.

Comment on attachment 626202 [details] [diff] [review] mozMobileConnection.getNetworks - v3 Review of attachment 626202 [details] [diff] [review]: ----------------------------------------------------------------- Looking good... some comments below. ::: dom/system/gonk/RILContentHelper.js @@ +92,5 @@ > + * containing MCC (country code) and MNC (network code). > + * AFAICT, MCC should always be 3 digits, making the remaining > + * portion the MNC. > + */ > + setNumericTuple: function setNumericTuple(numeric) { Is there a reason not do this in ril_worker.js? That's how most of the other systems work right now, with minimal logic on the main thread and content processes. @@ +389,5 @@ > + } > + > + if (message.error) { > + debug("Received error from getAvailableNetworks: " + message.error); > + Services.DOMRequest.fireError(request, message.error); You're now exposing a RIL internal error flag to the web. That's not a good way to make a portable API and definitely not compatible with the existing error conventions. We should convert the error flag to a meaningful error string, like with calls for instance: https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js#1361 @@ +394,5 @@ > + return; > + } > + > + let networks = message.networks; > + for (let i in networks) { `for (let i = 0; ...)` preferred @@ +411,5 @@ > + } > + > + let state = network.state; > + if (state === "unknown") { > + info.state = null; Push this down to ril_worker.js?

This version addresses Phil's comments, and adds the appropriate Marionette emulator unit tests. Note: to actually run this test, you will need to re-init repo from my fork of b2g-manifest using the queryAvailableNetworks branch (which also includes my fork of platform_hardware_ril w/ the same branch name) like so: ./repo init -u git://github.com/marshall/b2g-manifest -b queryAvailableNetworks Before this is merged, we'll need to make sure we move my platform_hardware_ril fork to the mozilla-b2g group, and update the manifest to point there accordingly. Phil, I know you'll also want to take a look at the custom reference RIL code that supports this, so here's a link :) https://github.com/marshall/platform_hardware_ril/commit/e933a4712c6594b6ba17ad1d4b47e3b2eee5c11f

(In reply to Marshall Culpepper [:marshall_law] from comment #12) > This version addresses Phil's comments, and adds the appropriate Marionette > emulator unit tests. <3 > Before this is merged, we'll need to make sure we move my > platform_hardware_ril fork to the mozilla-b2g group, and update the manifest > to point there accordingly. Let's decouple the two and disable the test, for now. But please do start the process of using our fork of platform_hardware_ril, at least for emulator builds. I suggest talking to mwu and jhford.

Comment on attachment 627980 [details] [diff] [review] mozMobileConnection.getNetworks - v4 Review of attachment 627980 [details] [diff] [review]: ----------------------------------------------------------------- I like. ::: dom/system/gonk/ril_worker.js @@ +1984,5 @@ > + debug("Error processing operator tuple: " + e); > + } > + > + let state = strings[i+3]; > + if (state === "unknown") { Nitpicking: I forgot to mention in my previous preview that it would be good to const the possible state values (in ril_consts.js), including "unknown". Also, spaces between operators at all times, thx!

Comment on attachment 627980 [details] [diff] [review] mozMobileConnection.getNetworks - v4 Review of attachment 627980 [details] [diff] [review]: ----------------------------------------------------------------- Forgot to comment on the test file. Just a few nits. ::: dom/network/tests/marionette/test_mobile_networks.js @@ +1,4 @@ > + > +// getNetworks() can take some time.. > +MARIONETTE_TIMEOUT = 30000; > +MARIONETTE_CONTEXT = "chrome"; Would be better to tweak the whitelist permission pref and have it operate in content land. See bug 756607 for examples. @@ +3,5 @@ > +MARIONETTE_TIMEOUT = 30000; > +MARIONETTE_CONTEXT = "chrome"; > + > +ok(navigator.mozMobileConnection instanceof MozMobileConnection, > + "mozMobileConnection is instanceof " + navigator.mozMobileConnection.constructor); Align arguments (here and everywhere else) @@ +10,5 @@ > +ok(request instanceof DOMRequest, > + "request is instanceof " + request.constructor); > + > +request.onerror = function() { > + ok(false, request.error); Indent two spaces (here and below)

I've broken out automatic and manual selection mode APIs into Bug 759637

(In reply to Philipp von Weitershausen [:philikon] from comment #13) > Let's decouple the two and disable the test, for now. Actually, my Marionette .ini isn't currently being loaded from the main unit-test.ini. I've been running the test directly by passing the JS script instead.

FYI, my b2g-manifest changes and platform_hardware_ril fork are up in mozilla-b2g, and they can be retrieved with a vanilla ./repo sync. Should I go ahead and add my test to the default unit-tests.ini?

(In reply to Marshall Culpepper [:marshall_law] from comment #18) > FYI, my b2g-manifest changes and platform_hardware_ril fork are up in > mozilla-b2g, and they can be retrieved with a vanilla ./repo sync. Yay! > Should I go ahead and add my test to the default unit-tests.ini? Sure!

Added the new suite to Marionette's top level manifest, formatting and other updates for Phil.

Forgot to rebase -- apologies for spam

Comment on attachment 628379 [details] [diff] [review] mozMobileConnection.getNetworks - v5 (rebased) Review of attachment 628379 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/tests/marionette/test_mobile_networks.js @@ +6,5 @@ > +MARIONETTE_TIMEOUT = 30000; > + > +const WHITELIST_PREF = "dom.mobileconnection.whitelist"; > +let uriPrePath = window.location.protocol + "//" + window.location.host; > +SpecialPowers.setCharPref(WHITELIST_PREF, uriPrePath); Nit: clearUserPref() at the end of the test, just before finish(), to ensure proper clean up. ::: dom/system/gonk/ril_consts.js @@ +295,5 @@ > > +const NETWORK_STATE_UNKNOWN = "unknown"; > +const NETWORK_STATE_AVAILABLE = "available"; > +const NETWORK_STATE_CONNECTED = "connected"; > +const NETWORK_STATE_FORBIDDEN = "forbidden"; <3

Just needs super review for DOM addition of OperatorInfo at this point

Comment on attachment 628500 [details] [diff] [review] mozMobileConnection.getNetworks - v6 Review of attachment 628500 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RILContentHelper.js @@ +131,5 @@ > > getNetworks: function getNetworks(window) { > + if (window == null) { > + throw Components.Exception("Can't get window object", > + Cr.NS_ERROR_UNEXPECTED); Not sure if it's worth null-checking here, only internal code can call this function, right? Up to you if you want to leave it as-is. ::: dom/system/gonk/ril_worker.js @@ +2214,5 @@ > + let tupleLen = networkTuple.length; > + let mcc = 0, mnc = 0; > + > + if (tupleLen == 5 || tupleLen == 6) { > + mcc = parseInt(networkTuple.substr(0, 3)); I think it's generally a good idea to specify the second 'radix' parameter to parseInt. Otherwise strings starting with 0 or 0x aren't parsed as base-10.

(In reply to Jonas Sicking (:sicking) from comment #24) > ::: dom/system/gonk/RILContentHelper.js > @@ +131,5 @@ > > > > getNetworks: function getNetworks(window) { > > + if (window == null) { > > + throw Components.Exception("Can't get window object", > > + Cr.NS_ERROR_UNEXPECTED); > > Not sure if it's worth null-checking here, only internal code can call this > function, right? Sure, but they could still pass a window for `null` here and make us crash in DOMRequestService. For instance, the caller here will be the DOM implementation passing GetOwner(); mounir pointed out that that can return `null` on occasion. > ::: dom/system/gonk/ril_worker.js > @@ +2214,5 @@ > > + let tupleLen = networkTuple.length; > > + let mcc = 0, mnc = 0; > > + > > + if (tupleLen == 5 || tupleLen == 6) { > > + mcc = parseInt(networkTuple.substr(0, 3)); > > I think it's generally a good idea to specify the second 'radix' parameter > to parseInt. Otherwise strings starting with 0 or 0x aren't parsed as > base-10. Good catch! I should have seen that... Marshall, please change to parseInt(x, 10); Thanks!

Updated parseInt calls to explicitly use base 10 radix, looks like this is ready to land now :)