superreview requested: [Bug 744344] B2G RIL: Allow network to be selected manually : [Attachment 625655] Initial patch for implementation of mozMobileConnection.getNetworks()

[bugzill...@mozilla.org]

Philipp von Weitershausen [:philikon] <phi...@weitershausen.de> has asked
Jonas Sicking (:sicking) <jo...@sicking.cc> for superreview:
Bug 744344: B2G RIL: Allow network to be selected manually
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 625655: Initial patch for implementation of
mozMobileConnection.getNetworks()
https://bugzilla.mozilla.org/attachment.cgi?id=625655&action=edit


------- Additional Comments from Philipp von Weitershausen [:philikon]
<phi...@weitershausen.de>
Review of attachment 625655:
-----------------------------------------------------------------

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`

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has asked Jonas

Sicking (:sicking) <jo...@sicking.cc> for superreview:
Bug 744344: B2G RIL: Allow network to be selected manually
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 625806: mozMobileConnection.getNetworks - v2
https://bugzilla.mozilla.org/attachment.cgi?id=625806&action=edit


------- Additional Comments from Marshall Culpepper [:marshall_law]
<mars...@mozilla.com>
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.

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has canceled Philipp
von Weitershausen [:philikon] <phi...@weitershausen.de>'s request for

superreview:
Bug 744344: B2G RIL: Allow network to be selected manually
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 625655: Initial patch for implementation of
mozMobileConnection.getNetworks()
https://bugzilla.mozilla.org/attachment.cgi?id=625655&action=edit

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has canceled Marshall
Culpepper [:marshall_law] <mars...@mozilla.com>'s request for superreview:

Bug 744344: B2G RIL: Allow network to be selected manually
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 625806: mozMobileConnection.getNetworks - v2
https://bugzilla.mozilla.org/attachment.cgi?id=625806&action=edit

------- Additional Comments from Marshall Culpepper [:marshall_law]
<mars...@mozilla.com>

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

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has asked Jonas

Sicking (:sicking) <jo...@sicking.cc> for superreview:

Bug 744344: B2G RIL: Allow network to be selected manually
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 626202: mozMobileConnection.getNetworks - v3
https://bugzilla.mozilla.org/attachment.cgi?id=626202&action=edit

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has canceled Marshall

Culpepper [:marshall_law] <mars...@mozilla.com>'s request for superreview:

Bug 744344: B2G RIL: Allow network to be selected manually
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 626202: mozMobileConnection.getNetworks - v3
https://bugzilla.mozilla.org/attachment.cgi?id=626202&action=edit


------- Additional Comments from Marshall Culpepper [:marshall_law]
<mars...@mozilla.com>

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/e933a4712c6594b6ba17ad
1d4b47e3b2eee5c11f

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has asked Jonas

Sicking (:sicking) <jo...@sicking.cc> for superreview:

Bug 744344: B2G RIL: Allow network to be selected manually
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 627980: mozMobileConnection.getNetworks - v4
https://bugzilla.mozilla.org/attachment.cgi?id=627980&action=edit

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has asked Jonas
Sicking (:sicking) <jo...@sicking.cc> for superreview:

Bug 744344: B2G RIL: Add DOM API for getting the list of available networks
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 628356: mozMobileConnection.getNetworks - v5
https://bugzilla.mozilla.org/attachment.cgi?id=628356&action=edit

------- Additional Comments from Marshall Culpepper [:marshall_law]
<mars...@mozilla.com>

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

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has canceled Marshall

Culpepper [:marshall_law] <mars...@mozilla.com>'s request for superreview:

Bug 744344: B2G RIL: Add DOM API for getting the list of available networks
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 627980: mozMobileConnection.getNetworks - v4
https://bugzilla.mozilla.org/attachment.cgi?id=627980&action=edit

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has asked Jonas

Sicking (:sicking) <jo...@sicking.cc> for superreview:

Bug 744344: B2G RIL: Add DOM API for getting the list of available networks
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 628379: mozMobileConnection.getNetworks - v5 (rebased)
https://bugzilla.mozilla.org/attachment.cgi?id=628379&action=edit

------- Additional Comments from Marshall Culpepper [:marshall_law]
<mars...@mozilla.com>

Forgot to rebase -- apologies for spam

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has canceled Marshall

Culpepper [:marshall_law] <mars...@mozilla.com>'s request for superreview:

Bug 744344: B2G RIL: Add DOM API for getting the list of available networks
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 628356: mozMobileConnection.getNetworks - v5
https://bugzilla.mozilla.org/attachment.cgi?id=628356&action=edit

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has canceled Marshall
Culpepper [:marshall_law] <mars...@mozilla.com>'s request for superreview:
Bug 744344: B2G RIL: Add DOM API for getting the list of available networks
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 628379: mozMobileConnection.getNetworks - v5 (rebased)
https://bugzilla.mozilla.org/attachment.cgi?id=628379&action=edit

------- Additional Comments from Marshall Culpepper [:marshall_law]
<mars...@mozilla.com>

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

[bugzill...@mozilla.org]

Marshall Culpepper [:marshall_law] <mars...@mozilla.com> has asked Jonas

Sicking (:sicking) <jo...@sicking.cc> for superreview:

Bug 744344: B2G RIL: Add DOM API for getting the list of available networks
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 628500: mozMobileConnection.getNetworks - v6
https://bugzilla.mozilla.org/attachment.cgi?id=628500&action=edit

[bugzill...@mozilla.org]

Jonas Sicking (:sicking) <jo...@sicking.cc> has granted Marshall Culpepper
[:marshall_law] <mars...@mozilla.com>'s request for superreview:

Bug 744344: B2G RIL: Add DOM API for getting the list of available networks
https://bugzilla.mozilla.org/show_bug.cgi?id=744344

Attachment 628500: mozMobileConnection.getNetworks - v6
https://bugzilla.mozilla.org/attachment.cgi?id=628500&action=edit

------- Additional Comments from Jonas Sicking (:sicking) <jo...@sicking.cc>
Review of attachment 628500:
-----------------------------------------------------------------

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