Last Comment Bug 759637 - B2G RIL: Add DOM APIs for automatic and manual network selection mode
: B2G RIL: Add DOM APIs for automatic and manual network selection mode
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: P1 normal (vote)
: mozilla16
Assigned To: Marshall Culpepper [:marshall_law]
:
Mentors:
Depends on: 744344 761482
Blocks: b2g-ril
  Show dependency treegraph
 
Reported: 2012-05-29 21:30 PDT by Marshall Culpepper [:marshall_law]
Modified: 2012-06-20 02:19 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
MobileConnection network selection APIs - v1 (27.75 KB, patch)
2012-06-11 15:48 PDT, Marshall Culpepper [:marshall_law]
philipp: feedback+
Details | Diff | Review
MobileConnection network selection APIs - v2 (29.84 KB, patch)
2012-06-12 14:58 PDT, Marshall Culpepper [:marshall_law]
philipp: review+
Details | Diff | Review
MobileConnection network selection APIs - v3 (37.34 KB, patch)
2012-06-14 21:50 PDT, Marshall Culpepper [:marshall_law]
philipp: review+
Details | Diff | Review
MobileConnection network selection APIs - v4 (39.82 KB, patch)
2012-06-19 09:02 PDT, Marshall Culpepper [:marshall_law]
no flags Details | Diff | Review
MobileConnection network selection APIs - v5 (40.66 KB, patch)
2012-06-19 12:42 PDT, Marshall Culpepper [:marshall_law]
philipp: review+
Details | Diff | Review
MobileConnection network selection APIs - v5 (rebased) (40.66 KB, patch)
2012-06-19 13:58 PDT, Marshall Culpepper [:marshall_law]
no flags Details | Diff | Review

Description Marshall Culpepper [:marshall_law] 2012-05-29 21:30:20 PDT
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?)
Comment 1 Philipp von Weitershausen [:philikon] 2012-05-29 23:09:23 PDT
Can't these just be settings?
Comment 2 Marshall Culpepper [:marshall_law] 2012-05-30 09:30:32 PDT
(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..
Comment 3 Philipp von Weitershausen [:philikon] 2012-05-30 10:03:25 PDT
(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.
Comment 4 Marshall Culpepper [:marshall_law] 2012-06-11 15:48:04 PDT
Created attachment 632046 [details] [diff] [review]
MobileConnection network selection APIs - v1

Initial API for network selection modes, built on top of the patch in Bug 761482.
Comment 5 Philipp von Weitershausen [:philikon] 2012-06-11 16:10:42 PDT
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!
Comment 6 Marshall Culpepper [:marshall_law] 2012-06-12 12:41:13 PDT
(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.
Comment 7 Philipp von Weitershausen [:philikon] 2012-06-12 13:51:23 PDT
(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.
Comment 8 Marshall Culpepper [:marshall_law] 2012-06-12 14:58:10 PDT
Created attachment 632426 [details] [diff] [review]
MobileConnection network selection APIs - v2

Updated Marionette tests to cover when a network is already the currently selected test. Fixed up "immediate" request onsuccess calls to be async.
Comment 9 Philipp von Weitershausen [:philikon] 2012-06-12 15:41:57 PDT
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)`.
Comment 10 Marshall Culpepper [:marshall_law] 2012-06-14 21:50:00 PDT
Created attachment 633398 [details] [diff] [review]
MobileConnection network selection APIs - v3

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 11 Philipp von Weitershausen [:philikon] 2012-06-15 15:41:50 PDT
Comment on attachment 633398 [details] [diff] [review]
MobileConnection network selection APIs - v3

Great work!
Comment 12 Marshall Culpepper [:marshall_law] 2012-06-19 09:02:38 PDT
Created attachment 634459 [details] [diff] [review]
MobileConnection network selection APIs - v4

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.
Comment 13 Marshall Culpepper [:marshall_law] 2012-06-19 12:42:33 PDT
Created attachment 634552 [details] [diff] [review]
MobileConnection network selection APIs - v5

Added more comments to explain the new behavior, and renamed _processNetworkInfoResult -> _receivedNetworkInfo
Comment 14 Philipp von Weitershausen [:philikon] 2012-06-19 13:08:04 PDT
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
Comment 15 Marshall Culpepper [:marshall_law] 2012-06-19 13:58:40 PDT
Created attachment 634581 [details] [diff] [review]
MobileConnection network selection APIs - v5 (rebased)
Comment 16 Philipp von Weitershausen [:philikon] 2012-06-19 15:52:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5969cfec442
Comment 17 Ed Morley [:emorley] 2012-06-20 02:19:37 PDT
https://hg.mozilla.org/mozilla-central/rev/e5969cfec442

Note You need to log in before you can comment on or make changes to this bug.