Last Comment Bug 777057 - MobileConnection: expose whether or not the radio is searching
: MobileConnection: expose whether or not the radio is searching
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Philipp von Weitershausen [:philikon]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: webmobileconnection
  Show dependency treegraph
 
Reported: 2012-07-24 13:42 PDT by Philipp von Weitershausen [:philikon]
Modified: 2012-08-08 09:34 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
wip 1 (10.98 KB, patch)
2012-07-25 17:03 PDT, Philipp von Weitershausen [:philikon]
marshall: feedback+
Details | Diff | Splinter Review
v1 (17.79 KB, patch)
2012-08-01 14:38 PDT, Philipp von Weitershausen [:philikon]
marshall: review+
jonas: superreview+
Details | Diff | Splinter Review
v1.1 (24.17 KB, patch)
2012-08-06 14:25 PDT, Philipp von Weitershausen [:philikon]
marshall: review+
Details | Diff | Splinter Review
tests v1 (5.11 KB, patch)
2012-08-06 20:26 PDT, Philipp von Weitershausen [:philikon]
marshall: review+
Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2012-07-24 13:42:31 PDT
Proposal: add a nsIDOMMozMobileConnectionInfo::searching attribute that is true when the voice registration state is NETWORK_CREG_STATE_SEARCHING or NETWORK_CREG_STATE_SEARCHING_EMERGENCY_CALLS.
Comment 1 Philipp von Weitershausen [:philikon] 2012-07-24 14:30:19 PDT
We also don't really expose NETWORK_CREG_STATE_NOT_SEARCHING and NETWORK_CREG_STATE_DENIED. I'm beginning to think that a `state` string property as I had originally envisioned would've been a better idea than many different booleans.
Comment 2 Philipp von Weitershausen [:philikon] 2012-07-25 15:25:05 PDT
If https://github.com/mozilla-b2g/gaia/issues/2763 is found to be blocking basecamp, this bug will also need to block.
Comment 3 Philipp von Weitershausen [:philikon] 2012-07-25 17:03:51 PDT
Created attachment 645953 [details] [diff] [review]
wip 1

In this patch I'm proposing to add another attribute to navigator.mozMobileConnection.{voice|data} that provides more detailed information about the network registration state.

For the voice network, when state == "registered", connected will be true. This won't necessarily be true for the data network. Only when state == "registered" and the data call is active, connected will true.

As part of this work I started refactoring a bit the way we update the registration state. My plan is to pull most of this into ril_worker.js, making RadioInterfaceLayer just a dumb message passer between the thread and content processes (much like in other cases).
Comment 4 Marshall Culpepper [:marshall_law] 2012-07-26 15:03:31 PDT
Comment on attachment 645953 [details] [diff] [review]
wip 1

Review of attachment 645953 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! A few follow up questions:

- Does this also fire the MobileConnection voicechange / datachange events when |state| has changed?
- Any idea what the expected behavior of the UI would be when the state is "denied", and emergency calls aren't allowed?
Comment 5 Philipp von Weitershausen [:philikon] 2012-07-26 17:47:25 PDT
(In reply to Marshall Culpepper [:marshall_law] from comment #4)
> - Does this also fire the MobileConnection voicechange / datachange events
> when |state| has changed?

That's the plan.

> - Any idea what the expected behavior of the UI would be when the state is
> "denied", and emergency calls aren't allowed?

I have no idea. https://github.com/mozilla-b2g/gaia/issues/2763 or even https://github.com/mozilla-b2g/gaia/issues/2776 is probably the better place to discuss this.
Comment 6 Andrew Overholt [:overholt] 2012-07-31 13:48:22 PDT
Platform support for blocking gaia issue.
Comment 7 Philipp von Weitershausen [:philikon] 2012-08-01 14:38:44 PDT
Created attachment 648093 [details] [diff] [review]
v1
Comment 8 Marshall Culpepper [:marshall_law] 2012-08-02 09:32:27 PDT
Comment on attachment 648093 [details] [diff] [review]
v1

Review of attachment 648093 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, this looks great! Thanks for cleaning up that state management while you were in there :)

r- just because it would be good to have some associated tests (if that's not possible just let me know)

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +226,5 @@
>  interface nsIDOMMozMobileConnectionInfo : nsISupports
>  {
>    /**
> +   * State of the connection.
> +   * 

nit: trailing space

@@ +230,5 @@
> +   * 
> +   * Possible values: 'notSearching', 'searching', 'denied', 'registered'.
> +   * null if the state is unknown.
> +   */
> +  readonly attribute DOMString state;

Somehow missed this on the first go-around.. there's an existing |state| attribute on nsIDOMMozMobileNetworkInfo, which might add some confusion here.

I'd suggest adding some clarity to this comment (and the one above MobileNetworkInfo.state) about what the set of states mean and how/if they are different.

Off the top of my head, the  states for MobileNetworkInfo:
- available: Network is available to be selected by the modem, but is not currently selected
- connected: The currently connected / selected network
- forbidden: Isn't well explained in RIL, but I assume this means the network is seen, but for some reason the modem has been forbidden to connect (maybe when the device is carrier locked, or there is some kind of private network?)

Does it sound like there might be a potential conflict here?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +488,5 @@
> +    voiceInfo.type = "gsm";
> +
> +    // Make sure we also reset the operator and signal strength information
> +    // if we drop off the network.
> +    if (newInfo.regState == RIL.NETWORK_CREG_STATE_UNKNOWN) {

Funny that the original code checked !state after accessing state.regState. Is it possible for newInfo to ever be null/undefined here?

@@ +563,5 @@
> +      voiceInfo.signalStrength = message.gsmDBM;
> +      voiceInfo.relSignalStrength = message.gsmRelative;
> +      ppmm.sendAsyncMessage("RIL:VoiceInfoChanged", voiceInfo);
> +    }
> + 

nit: trailing space

::: dom/system/gonk/ril_consts.js
@@ +1464,5 @@
> +NETWORK_CREG_TO_GECKO_MOBILE_CONNECTION_STATE[NETWORK_CREG_STATE_REGISTERED_ROAMING] = GECKO_MOBILE_CONNECTION_STATE_REGISTERED;
> +NETWORK_CREG_TO_GECKO_MOBILE_CONNECTION_STATE[NETWORK_CREG_STATE_NOT_SEARCHING_EMERGENCY_CALLS] = GECKO_MOBILE_CONNECTION_STATE_NOTSEARCHING;
> +NETWORK_CREG_TO_GECKO_MOBILE_CONNECTION_STATE[NETWORK_CREG_STATE_SEARCHING_EMERGENCY_CALLS] = GECKO_MOBILE_CONNECTION_STATE_SEARCHING;
> +NETWORK_CREG_TO_GECKO_MOBILE_CONNECTION_STATE[NETWORK_CREG_STATE_DENIED_EMERGENCY_CALLS] = GECKO_MOBILE_CONNECTION_STATE_DENIED;
> +NETWORK_CREG_TO_GECKO_MOBILE_CONNECTION_STATE[NETWORK_CREG_STATE_UNKNOWN_EMERGENCY_CALLS] = GECKO_MOBILE_CONNECTION_STATE_UNKNOWN;

Begging for it ;)

http://www.quickmeme.com/meme/3qbqd1/
Comment 9 Philipp von Weitershausen [:philikon] 2012-08-02 12:43:54 PDT
(In reply to Marshall Culpepper [:marshall_law] from comment #8)
> r- just because it would be good to have some associated tests (if that's
> not possible just let me know)

I'll look into tests.

> > +   * 
> > +   * Possible values: 'notSearching', 'searching', 'denied', 'registered'.
> > +   * null if the state is unknown.
> > +   */
> > +  readonly attribute DOMString state;
> 
> Somehow missed this on the first go-around.. there's an existing |state|
> attribute on nsIDOMMozMobileNetworkInfo, which might add some confusion here.

Hmm, yes. Part of me hopes that it's clear that one is about the connection's state and the other about the operator/network's state.

> I'd suggest adding some clarity to this comment (and the one above
> MobileNetworkInfo.state) about what the set of states mean and how/if they
> are different.
> 
> Off the top of my head, the  states for MobileNetworkInfo:
> - available: Network is available to be selected by the modem, but is not
> currently selected
> - connected: The currently connected / selected network
> - forbidden: Isn't well explained in RIL, but I assume this means the
> network is seen, but for some reason the modem has been forbidden to connect
> (maybe when the device is carrier locked, or there is some kind of private
> network?)

Adding comments along those lines is a good idea. In fact, I shall explicitly compare and contrast the two `state` attributes.

Another thought: we could rename nsIDOMMozMobileNetworkInfo::state to e.g. nsIDOMMozMobileNetworkInfo::availability. What do you think?

> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +488,5 @@
> > +    voiceInfo.type = "gsm";
> > +
> > +    // Make sure we also reset the operator and signal strength information
> > +    // if we drop off the network.
> > +    if (newInfo.regState == RIL.NETWORK_CREG_STATE_UNKNOWN) {
> 
> Funny that the original code checked !state after accessing state.regState.
> Is it possible for newInfo to ever be null/undefined here?

Nope. That's why I removed the check. :)

> ::: dom/system/gonk/ril_consts.js
> @@ +1464,5 @@
> > +NETWORK_CREG_TO_GECKO_MOBILE_CONNECTION_STATE[NETWORK_CREG_STATE_REGISTERED_ROAMING] = GECKO_MOBILE_CONNECTION_STATE_REGISTERED;
> > +NETWORK_CREG_TO_GECKO_MOBILE_CONNECTION_STATE[NETWORK_CREG_STATE_NOT_SEARCHING_EMERGENCY_CALLS] = GECKO_MOBILE_CONNECTION_STATE_NOTSEARCHING;
> > +NETWORK_CREG_TO_GECKO_MOBILE_CONNECTION_STATE[NETWORK_CREG_STATE_SEARCHING_EMERGENCY_CALLS] = GECKO_MOBILE_CONNECTION_STATE_SEARCHING;
> > +NETWORK_CREG_TO_GECKO_MOBILE_CONNECTION_STATE[NETWORK_CREG_STATE_DENIED_EMERGENCY_CALLS] = GECKO_MOBILE_CONNECTION_STATE_DENIED;
> > +NETWORK_CREG_TO_GECKO_MOBILE_CONNECTION_STATE[NETWORK_CREG_STATE_UNKNOWN_EMERGENCY_CALLS] = GECKO_MOBILE_CONNECTION_STATE_UNKNOWN;
> 
> Begging for it ;)
> 
> http://www.quickmeme.com/meme/3qbqd1/

http://www.quickmeme.com/meme/3qbtc5/
Comment 10 Marshall Culpepper [:marshall_law] 2012-08-02 13:35:34 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #9)

> Another thought: we could rename nsIDOMMozMobileNetworkInfo::state to e.g.
> nsIDOMMozMobileNetworkInfo::availability. What do you think?

I've opened a follow up in Bug 779972

> > http://www.quickmeme.com/meme/3qbqd1/
> 
> http://www.quickmeme.com/meme/3qbtc5/

*raises the white flag in surrender*
Comment 11 Marshall Culpepper [:marshall_law] 2012-08-03 10:44:54 PDT
Comment on attachment 648093 [details] [diff] [review]
v1

r+, tests will be addressed in another patch, and API naming now has a follow up filed.
Comment 12 Tim Guan-tin Chien [:timdream] (please needinfo) 2012-08-06 04:06:02 PDT
https://github.com/mozilla-b2g/gaia/pull/3181/files?diff-0=0-0#L0R205
Gaia code updated according to patch here.

(In reply to Marshall Culpepper [:marshall_law] from comment #4)
> - Any idea what the expected behavior of the UI would be when the state is
> "denied", and emergency calls aren't allowed?

What does "denied" stands for? What's the label of a usual phone if the state is "denied"?
Comment 13 Philipp von Weitershausen [:philikon] 2012-08-06 14:25:38 PDT
Created attachment 649417 [details] [diff] [review]
v1.1

Rebased with some minor improvements in the network info handling code (mostly debugging and code clarity). Asking for review for those changes.
Comment 14 Marshall Culpepper [:marshall_law] 2012-08-06 15:46:45 PDT
Comment on attachment 649417 [details] [diff] [review]
v1.1

Review of attachment 649417 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thanks for adding all of those debugging messages :)

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +233,5 @@
> +   */
> +  readonly attribute DOMString state;
> +
> +  /**
> +   * Indicates whether the connection is ready. This may be different 

nit: trailing space
Comment 15 Philipp von Weitershausen [:philikon] 2012-08-06 17:36:07 PDT
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #12)
> https://github.com/mozilla-b2g/gaia/pull/3181/files?diff-0=0-0#L0R205
> Gaia code updated according to patch here.

Nice!

> (In reply to Marshall Culpepper [:marshall_law] from comment #4)
> What does "denied" stands for?

When the radio tries to connect to a network and gets denied. For instance, when I put my Taiwanese SIM card into my phone and turn it on here in the U.S., I get a bunch of "registered" and "denied" states as it loops through every network it can find trying to register. ("registered" here doesn't necessarily mean `connected == true`; in this case it just registered enough so that emergency calls are possible, as indicated by `emergencyCallsOnly == true`).

> What's the label of a usual phone if the state is "denied"?

I don't think it's generally shown. Neither is "searching" to be honest...
Comment 16 Tim Guan-tin Chien [:timdream] (please needinfo) 2012-08-06 19:24:29 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #15)
> When the radio tries to connect to a network and gets denied. For instance,
> when I put my Taiwanese SIM card into my phone and turn it on here in the
> U.S., I get a bunch of "registered" and "denied" states as it loops through
> every network it can find trying to register. ("registered" here doesn't
> necessarily mean `connected == true`; in this case it just registered enough
> so that emergency calls are possible, as indicated by `emergencyCallsOnly ==
> true`).
> 
> > What's the label of a usual phone if the state is "denied"?
> 
> I don't think it's generally shown. Neither is "searching" to be honest...

Got it, so except state = 'notSearching', the searching indication should be shown as the phone is in the middle of the process of searching for a network to register. Gaia pull req updated accordingly.
Comment 17 Philipp von Weitershausen [:philikon] 2012-08-06 19:31:37 PDT
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #16)
> Got it, so except state = 'notSearching', the searching indication should be
> shown as the phone is in the middle of the process of searching for a
> network to register.

No. That's not all what I said. The state can be "registered" for instance. Also, I don't think "denied" counts as searching. It can be an intermediary state, but it doesn't have to be. Though of course, for the purposes of the UI, you could treat "denied" and "searching" the same way. To the user it doesn't make a difference. He/she doesn't have service.

Your logic should be something like this:

  if (voice.connected) {
    showOperatorName();
    if (voice.roaming) {
      showRoamingLabel();
    }
    return;
  }
  if (voice.emergencyCallsOnly) {
    showEmergencyCallsOnly();
    return;
  }
  if (voice.state == "searching" || voice.state == "denied") {
    showSearchingLabel();
    return;
  }
Comment 18 Tim Guan-tin Chien [:timdream] (please needinfo) 2012-08-06 20:00:26 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #17)
> 
> Your logic should be something like this:
> 
>   if (voice.connected) {
>     showOperatorName();
>     if (voice.roaming) {
>       showRoamingLabel();
>     }
>     return;
>   }
>   if (voice.emergencyCallsOnly) {
>     showEmergencyCallsOnly();
>     return;
>   }
>   if (voice.state == "searching" || voice.state == "denied") {
>     showSearchingLabel();
>     return;
>   }

What I did is almost the same, except I wrote it upside down in the actual statusbar.js:

  if (voice.connected) {
    showOperatorName();
    if (voice.roaming) {
      showRoamingLabel();
    }
    return;
  }
  if (voice.emergencyCallsOnly) {
    showEmergencyCallsOnly();
    return;
  }
  if (voice.state == "searching" || voice.state == "denied" || voice.state == "registered") {
    showSearchingLabel();
    return;
  }
  showNoNetwork();

Since as you stated at comment 15, it's possible to be "registered" without emergency call capability nor connection.

And the last `if` can be reverted into (voice.state !== "notSearching").
Comment 19 Philipp von Weitershausen [:philikon] 2012-08-06 20:12:03 PDT
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #18)
> What I did is almost the same, except I wrote it upside down in the actual
> statusbar.js:
> 
>   if (voice.connected) {
>     showOperatorName();
>     if (voice.roaming) {
>       showRoamingLabel();
>     }
>     return;
>   }
>   if (voice.emergencyCallsOnly) {
>     showEmergencyCallsOnly();
>     return;
>   }
>   if (voice.state == "searching" || voice.state == "denied" || voice.state
> == "registered") {

You failed to mention the implied !voice.connected and !voice.emergencyCallsOnly that's in place at this point. ;)

> Since as you stated at comment 15, it's possible to be "registered" without
> emergency call capability nor connection.

Fair.

> And the last `if` can be reverted into (voice.state !== "notSearching").

Yes, I think so.
Comment 20 Philipp von Weitershausen [:philikon] 2012-08-06 20:26:37 PDT
Created attachment 649530 [details] [diff] [review]
tests v1

There you go, sir. Sorry for the delay.
Comment 21 Marshall Culpepper [:marshall_law] 2012-08-07 08:00:38 PDT
Comment on attachment 649530 [details] [diff] [review]
tests v1

Review of attachment 649530 [details] [diff] [review]:
-----------------------------------------------------------------

Woot for tests! We should probably address emulator command errors (see below), but that is relatively easy, so r+ with that :)

::: dom/network/tests/marionette/test_mobile_voice_state.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Many of our marionette tests have a Public Domain header.. any idea which is actually right?

@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function todo_ok() {}
> +function todo_is() {}

nit: looks like these placeholder functions aren't actually being used

@@ +5,5 @@
> +function todo_ok() {}
> +function todo_is() {}
> +
> +MARIONETTE_TIMEOUT = 30000;
> + 

nit: trailing space

@@ +36,5 @@
> +
> +    testSearching();
> +  });
> +
> +  runEmulatorCmd("gsm voice unregistered");

We should probably be checking the emulator command's result in case there is an error.. test timeouts can be hard to debug :( Check out my runEmulatorCmd wrapper in test_voicemail_statuschanged.js for an example:
https://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/test_voicemail_statuschanged.js#12

@@ +41,5 @@
> +}
> +
> +function testSearching() {
> +  // For some reason, requesting the "searching" state puts the fake modem
> +  // into "registered"... Skipping this test for now.

Should we also fix this in the emulator modem? (Maybe a follow up?)
Comment 22 Philipp von Weitershausen [:philikon] 2012-08-07 12:12:31 PDT
Thanks for the review! Addressed your review comments and folded into one patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/26e02cf3a1f9
Comment 23 Ed Morley [:emorley] 2012-08-08 09:34:35 PDT
https://hg.mozilla.org/mozilla-central/rev/26e02cf3a1f9

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