744344 - B2G RIL: Add DOM API for getting the list of available networks

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

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

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

thanks

Comment 2

[Marshall Culpepper [:marshall_law]]

Attachment: Initial patch for implementation of mozMobileConnection.getNetworks()

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)

Comment 3

[Marshall Culpepper [:marshall_law]]

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?

Comment 4

[Philipp von Weitershausen [:philikon]]

(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 5

[Philipp von Weitershausen [:philikon]]

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`

Comment 6

[Marshall Culpepper [:marshall_law]]

Attachment: mozMobileConnection.getNetworks - v2

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.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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?

Comment 8

[Marshall Culpepper [:marshall_law]]

(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

Comment 9

[Marshall Culpepper [:marshall_law]]

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.

Comment 10

[Marshall Culpepper [:marshall_law]]

Attachment: mozMobileConnection.getNetworks - v3

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

Comment 11

[Philipp von Weitershausen [:philikon]]

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?

Comment 12

[Marshall Culpepper [:marshall_law]]

Attachment: mozMobileConnection.getNetworks - v4

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

Comment 13

[Philipp von Weitershausen [:philikon]]

(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 14

[Philipp von Weitershausen [:philikon]]

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 15

[Philipp von Weitershausen [:philikon]]

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)

Comment 16

[Marshall Culpepper [:marshall_law]]

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

Comment 17

[Marshall Culpepper [:marshall_law]]

(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.

Comment 18

[Marshall Culpepper [:marshall_law]]

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?

Comment 19

[Philipp von Weitershausen [:philikon]]

(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!

Comment 20

[Marshall Culpepper [:marshall_law]]

Attachment: mozMobileConnection.getNetworks - v5

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

Comment 21

[Marshall Culpepper [:marshall_law]]

Attachment: mozMobileConnection.getNetworks - v5 (rebased)

Forgot to rebase -- apologies for spam

Comment 22

[Philipp von Weitershausen [:philikon]]

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

Comment 23

[Marshall Culpepper [:marshall_law]]

Attachment: mozMobileConnection.getNetworks - v6

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

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 25

[Philipp von Weitershausen [:philikon]]

(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!

Comment 26

[Marshall Culpepper [:marshall_law]]

Attachment: mozMobileConnection.getNetworks - v7

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

Comment 27

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54200869aeff

Comment 28

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/54200869aeff