Philipp von Weitershausen [:philikon] Assignee
	Description • 12 years ago

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.

	Philipp von Weitershausen [:philikon] Assignee
	Comment 1 • 12 years ago

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.

	Philipp von Weitershausen [:philikon] Assignee
	Comment 2 • 12 years ago

If https://github.com/mozilla-b2g/gaia/issues/2763 is found to be blocking basecamp, this bug will also need to block.

	Philipp von Weitershausen [:philikon] Assignee
	Comment 3 • 12 years ago

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

	Marshall Culpepper [:marshall_law]
	Comment 4 • 12 years ago

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?

	Philipp von Weitershausen [:philikon] Assignee
	Comment 5 • 12 years ago

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

	Andrew Overholt [:overholt]
	Comment 6 • 12 years ago

Platform support for blocking gaia issue.

	Marshall Culpepper [:marshall_law]
	Comment 8 • 12 years ago

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/

	Philipp von Weitershausen [:philikon] Assignee
	Comment 9 • 12 years ago

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

	Marshall Culpepper [:marshall_law]
	Comment 10 • 12 years ago

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

	Marshall Culpepper [:marshall_law]
	Comment 11 • 12 years ago

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.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 12 • 12 years ago

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

	Philipp von Weitershausen [:philikon] Assignee
	Comment 13 • 12 years ago

Rebased with some minor improvements in the network info handling code (mostly debugging and code clarity). Asking for review for those changes.

	Marshall Culpepper [:marshall_law]
	Comment 14 • 12 years ago

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

	Philipp von Weitershausen [:philikon] Assignee
	Comment 15 • 12 years ago

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

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 16 • 12 years ago

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

	Philipp von Weitershausen [:philikon] Assignee
	Comment 17 • 12 years ago

(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;
  }

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 18 • 12 years ago

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

	Philipp von Weitershausen [:philikon] Assignee
	Comment 19 • 12 years ago

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

	Philipp von Weitershausen [:philikon] Assignee
	Comment 20 • 12 years ago

There you go, sir. Sorry for the delay.

	Marshall Culpepper [:marshall_law]
	Comment 21 • 12 years ago

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

	Philipp von Weitershausen [:philikon] Assignee
	Comment 22 • 12 years ago

Thanks for the review! Addressed your review comments and folded into one patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/26e02cf3a1f9

	Ed Morley [:emorley]
	Comment 23 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/26e02cf3a1f9