787420 - B2G RIL: add setting for switching between 2G/3G

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

Most devices/systems have a way of turning 3G data off, typically to save battery. We should do the same thing.

Comment 1

[Philipp von Weitershausen [:philikon]]

I tried looking at ril.h, but I couldn't really find a specific request that would correspond to this. m1, do you have an idea perhaps?

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

There are two parcels that can be used to config/retrieve preferred network settings: REQUEST_SET_PREFERRED_NETWORK_TYPE and REQUEST_GET_PREFERRED_NETWORK_TYPE. I have some initial patches and will take this issue for cell broadcast testing.

Also request a blocking-basecamp flag for it's important for every cell phone, I think.

Comment 3

[Philipp von Weitershausen [:philikon]]

From my initial research, it seemed that REQUEST_SET_PREFERRED_NETWORK_TYPE was for GSM vs. CDMA vs. LTE etc., not for 2G/3G.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: WIP

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Blocking bug 778093. The functionality of cell broadcast doesn't actually blocked by this issue. However, it's only available in 2G network in Brazil.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> From my initial research, it seemed that REQUEST_SET_PREFERRED_NETWORK_TYPE
> was for GSM vs. CDMA vs. LTE etc., not for 2G/3G.

GSM is for 2G and WCDMA for 3G. If we can set prefered network type to PREFERRED_NETWORK_TYPE_GSM_ONLY, then we'll be on 2G network.

Comment 7

[Andrew Overholt [:overholt]]

I spoke about this with philikon and while it's a definite nice-to-have, it's not a v1 blocker.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Andrew Overholt [:overholt] from comment #7)
> I spoke about this with philikon and while it's a definite nice-to-have,
> it's not a v1 blocker.

But Cell Broadcast in bug 778093 requires 2G network to function properly, and it is already a v1 blocker.

Comment 9

[Beatriz Rodríguez [:brg]]

I think we will need to have a way to forze the device to "only 2G" and "only 3G" mode for testing pourposes in lab environment. Do you think you can have some mechanism to do it for testing?

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: RIL imp

1. Use Settings API instead of some DOM API

2. Move radio power, preferred network type initialization to UNSOLICITED_RIL_CONNECTED event handler as Android does. I'm not quite sure whether Akami is still a tier 1 device that should be fully supported, and I still have to do more tests on devices other than Otoro, which is the only one I have for now.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: test scripts

Android emulator doesn't support {SET,GET}_PREFERRED_NETWORK_TYPE at all. Need more work on it.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Beatriz Rodríguez [:brg] from comment #9)
> I think we will need to have a way to forze the device to "only 2G" and
> "only 3G" mode for testing pourposes in lab environment. Do you think you
> can have some mechanism to do it for testing?

Yes, we can, that's how I debug the patch. But I think there might be still time for a formal UI.

Comment 13

[Andrew Overholt [:overholt]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #8)
> (In reply to Andrew Overholt [:overholt] from comment #7)
> > I spoke about this with philikon and while it's a definite nice-to-have,
> > it's not a v1 blocker.
> 
> But Cell Broadcast in bug 778093 requires 2G network to function properly,
> and it is already a v1 blocker.

Oops, sorry, I didn't see that.  I've marked this as a blocker.

Vicamo, please add a rough level of effort estimate to the whiteboard field.  Use [LOE:S] for <= 1 week's worth of full-time effort, [LOE:M] for 1-3 weeks, and [LOE:L] for > 3 weeks.  Thanks!

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 659247 [details] [diff] [review]
Part 1: RIL imp

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

Thanks! Bunch of minor niggles and a few questions.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +463,5 @@
>        case "stksessionend":
>          ppmm.broadcastAsyncMessage("RIL:StkSessionEnd", null);
>          break;
> +      case "setPreferredNetworkType":
> +        this.worker.postMessage({rilMessageType: "getPreferredNetworkType"});

Why this?

@@ +468,5 @@
> +        break;
> +      case "getPreferredNetworkType":
> +        if (!message.error) {
> +          this.preferredNetworkType = message.networkType;
> +        }

I think we should do here what Hsinyi did in bug 784220: if applying the settings value was not successful, we should switch the setting back to its original value. (In bug 784220, the setting is just a boolean; here it's a multi-value field, so we need to cache its previous value... but that shouldn't be too hard.)

@@ +610,5 @@
>    },
>  
> +  preferredNetworkType: null,
> +  setPreferredNetworkType: function setPreferredNetworkType(value) {
> +    let networkType = parseInt(value, 10);

So the possible values for this setting are the following values from this ril.h excerpt, correct?

 * ((int *)data)[0] is == 0 for GSM/WCDMA (WCDMA preferred)
 * ((int *)data)[0] is == 1 for GSM only
 * ((int *)data)[0] is == 2 for WCDMA only
 * ((int *)data)[0] is == 3 for GSM/WCDMA (auto mode, according to PRL)
 * ((int *)data)[0] is == 4 for CDMA and EvDo (auto mode, according to PRL)
 * ((int *)data)[0] is == 5 for CDMA only
 * ((int *)data)[0] is == 6 for EvDo only
 * ((int *)data)[0] is == 7 for GSM/WCDMA, CDMA, and EvDo (auto mode, according to PRL)
 * ((int *)data)[0] is == 8 for LTE, CDMA and EvDo
 * ((int *)data)[0] is == 9 for LTE, GSM/WCDMA
 * ((int *)data)[0] is == 10 for LTE, CDMA, EvDo, GSM/WCDMA
 * ((int *)data)[0] is == 11 for LTE only

Are these values documented in some 3GPP spec? We definitely shouldn't expose RIL-specific constants through a Web API. I'm inclined to do what we often do in this case and map these constants to strings.

Or, maybe, to keep things really simple, we just have a Boolean setting, and let the RIL figure out the rest. Actually, come to think of it, I'd prefer that.

::: dom/system/gonk/ril_worker.js
@@ +3864,5 @@
> +    });
> +    return;
> +  }
> +
> +  let networkType = PREFERRED_NETWORK_TYPE_GSM_WCDMA_CDMA_EVDO_AUTO;

This default value for `networkType` is not assigned to `this.preferredNetworkType` in case of an error. Was that intended?

@@ +3866,5 @@
> +  }
> +
> +  let networkType = PREFERRED_NETWORK_TYPE_GSM_WCDMA_CDMA_EVDO_AUTO;
> +  let length = Buf.readUint32();
> +  if (length) {

`length` is already a parameter to this function. ril.h seems to indicate there's no separate length integer in this parcel... Can you double-check your code here, please?

@@ +3873,5 @@
> +
> +  this.sendDOMMessage({
> +    rilMessageType: "getPreferredNetworkType",
> +    networkType: networkType
> +  });

You can cover this and also the error case above in one slew:

  options.networkType = networkType;
  options.success = options.rilRequestError == ERROR_SUCCESS;
  this.sendDOMMessage(options);

@@ +4099,5 @@
>    }
>  
>    this.initRILState();
> +
> +  this.setRadioPower({on: false});

This seems somewhat unrelated. Is this a fix for the Otoro problem documented in bug 787366?

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #14)
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +463,5 @@
> >        case "stksessionend":
> >          ppmm.broadcastAsyncMessage("RIL:StkSessionEnd", null);
> >          break;
> > +      case "setPreferredNetworkType":
> > +        this.worker.postMessage({rilMessageType: "getPreferredNetworkType"});
> 
> Why this?

In setPreferredNetworkType command callback in Settings app, Android queries current preferred network type setting and reset its menu setting to correct value. If we just reset the value from Settings API, then we don't need this. I'll remove, and should I remove implementation of getPreferredNetworkType() as well, for it will be referenced by nobody.

> @@ +468,5 @@
> > +        break;
> > +      case "getPreferredNetworkType":
> > +        if (!message.error) {
> > +          this.preferredNetworkType = message.networkType;
> > +        }
> 
> I think we should do here what Hsinyi did in bug 784220: if applying the
> settings value was not successful, we should switch the setting back to its
> original value. (In bug 784220, the setting is just a boolean; here it's a
> multi-value field, so we need to cache its previous value... but that
> shouldn't be too hard.)

Ok.

> @@ +610,5 @@
> >    },
> >  
> > +  preferredNetworkType: null,
> > +  setPreferredNetworkType: function setPreferredNetworkType(value) {
> > +    let networkType = parseInt(value, 10);
> 
> So the possible values for this setting are the following values from this
> ril.h excerpt, correct?
> 
>  * ((int *)data)[0] is == 0 for GSM/WCDMA (WCDMA preferred)
>  ...
> 
> Are these values documented in some 3GPP spec? We definitely shouldn't
> expose RIL-specific constants through a Web API. I'm inclined to do what we
> often do in this case and map these constants to strings.

There are several AT commands involved in 'operator' selection like CPOL, CPLS, COPS, but neither of them selects 'network type' directly. Besides, the Android libril-reference doesn't implement set/get preferred network type and it looks like only available in proprietary code. For example, AT+KSRAT(Set Radio Access Technology) is a similar proprietary command only for Hilo 3G chips.

> Or, maybe, to keep things really simple, we just have a Boolean setting, and
> let the RIL figure out the rest. Actually, come to think of it, I'd prefer
> that.

For now, I guess we only need following three values:

"wcdma/gsm": GSM/WCDMA (WCDMA preferred)
"gsm": GSM only
"wcdma": WCDMA only

> ::: dom/system/gonk/ril_worker.js
> @@ +3864,5 @@
> > +    });
> > +    return;
> > +  }
> > +
> > +  let networkType = PREFERRED_NETWORK_TYPE_GSM_WCDMA_CDMA_EVDO_AUTO;
> 
> This default value for `networkType` is not assigned to
> `this.preferredNetworkType` in case of an error. Was that intended?

We're supposed to pass default network type as constructor parameter of ril_worker, but that's impossible. However, the default value can always be overridden by Settings API at boot time.

> @@ +3866,5 @@
> > +  }
> > +
> > +  let networkType = PREFERRED_NETWORK_TYPE_GSM_WCDMA_CDMA_EVDO_AUTO;
> > +  let length = Buf.readUint32();
> > +  if (length) {
> 
> `length` is already a parameter to this function. ril.h seems to indicate
> there's no separate length integer in this parcel... Can you double-check
> your code here, please?

Sorry for that.

> @@ +3873,5 @@
> > +
> > +  this.sendDOMMessage({
> > +    rilMessageType: "getPreferredNetworkType",
> > +    networkType: networkType
> > +  });
> 
> You can cover this and also the error case above in one slew:
> 
>   options.networkType = networkType;
>   options.success = options.rilRequestError == ERROR_SUCCESS;
>   this.sendDOMMessage(options);

Ok.

> @@ +4099,5 @@
> >    }
> >  
> >    this.initRILState();
> > +
> > +  this.setRadioPower({on: false});
> 
> This seems somewhat unrelated. Is this a fix for the Otoro problem
> documented in bug 787366?

No, just how Android initializes RIL. Since this has no special effect on this issue, I'll rollback this change.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> Created attachment 659248 [details] [diff] [review]
> Part 2: test scripts
> 
> Android emulator doesn't support {SET,GET}_PREFERRED_NETWORK_TYPE at all.
> Need more work on it.

https://github.com/android/platform_hardware_ril/commit/e964504
Android just committed multi-mode support 15 days ago and is only available in master tree (RILv7).

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: V2

Address review comment #14:
1. reset settings value on error, don't invoke getPreferredNetoworkType
2. use "WCDMA/GSM", "GSM" and "WCDMA" as settings values instead
3. assign default value in ril_worker as well
4. rollback setRadioPower related changes
5. some some fix

Also a major difference from previous patch: make preferredNetworkType settings value a prerequisite before setting radio power on/off in order to have a correct setting at very beginning.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [DNS] Gaia debug patch

Select WCDMA/GSM through modified Sound settings.

Comment 19

[Philipp von Weitershausen [:philikon]]

Comment on attachment 660742 [details] [diff] [review]
V2

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

::: dom/system/gonk/ril_consts.js
@@ +336,5 @@
>  ];
>  
> +const GECKO_PREFERRED_NETWORK_TYPE_WCDMA_GSM = "WCDMA/GSM";
> +const GECKO_PREFERRED_NETWORK_TYPE_GSM_ONLY = "GSM";
> +const GECKO_PREFERRED_NETWORK_TYPE_WCDMA_ONLY = "WCDMA";

Nit: let's make those strings lower case, more analogous to the GECKO_RADIO_TECH strings.

::: dom/system/gonk/ril_worker.js
@@ +4141,5 @@
>    }
>  
>    this.initRILState();
> +
> +  this.setPreferredNetworkType();

Why do we do this? AFAICT this will have us return early from `setPreferredNetworkType` because `preferredNetworkType` is `null` initially.

r+ pending an answer to this question.

Comment 20

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> > Android emulator doesn't support {SET,GET}_PREFERRED_NETWORK_TYPE at all.
> > Need more work on it.
> 
> https://github.com/android/platform_hardware_ril/commit/e964504
> Android just committed multi-mode support 15 days ago and is only available
> in master tree (RILv7).

Cool! Let's file a follow-up bug so we can track this.

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #19)
> ::: dom/system/gonk/ril_worker.js
> @@ +4141,5 @@
> >    }
> >  
> >    this.initRILState();
> > +
> > +  this.setPreferredNetworkType();
> 
> Why do we do this? AFAICT this will have us return early from
> `setPreferredNetworkType` because `preferredNetworkType` is `null` initially.
> 
> r+ pending an answer to this question.

Because we might have rild crashes and have to reset preferred network type again. There are actually two reset tries, one at RIL_CONNECTED, another at SIM_READY.

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

Attachment: V3

Following bug 792660 was opened for test cases. Comment #19 addressed.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [DNS] Gaia debug patch

rebase & lower cased settings value

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

Gaia issue: https://github.com/mozilla-b2g/gaia/issues/4944

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21)
> (In reply to Philipp von Weitershausen [:philikon] from comment #19)
> > ::: dom/system/gonk/ril_worker.js
> > @@ +4141,5 @@
> > >    }
> > >  
> > >    this.initRILState();
> > > +
> > > +  this.setPreferredNetworkType();
> > 
> > Why do we do this? AFAICT this will have us return early from
> > `setPreferredNetworkType` because `preferredNetworkType` is `null` initially.
> > 
> > r+ pending an answer to this question.
> 
> Because we might have rild crashes and have to reset preferred network type
> again. There are actually two reset tries, one at RIL_CONNECTED, another at
> SIM_READY.

Hi Philipp, do you think this free you from further questions? I need your reply before checking in.

Comment 26

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21)
> > >    this.initRILState();
> > > +
> > > +  this.setPreferredNetworkType();
> > 
> > Why do we do this? AFAICT this will have us return early from
> > `setPreferredNetworkType` because `preferredNetworkType` is `null` initially.
> > 
> > r+ pending an answer to this question.
> 
> Because we might have rild crashes and have to reset preferred network type
> again. There are actually two reset tries, one at RIL_CONNECTED, another at
> SIM_READY.

Ah that makes sense. Go ahead then, land it!

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ad096174ea

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a5ad096174ea

Comment 29

[:gerard-majax]

This commit broke somehow:
E/GeckoConsole(   76): [JavaScript Error: "[Exception... "Could not convert JavaScript argument arg 2 [nsISettingsServiceLock.set]"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js :: setPreferredNetworkType :: line 658"  data: no]" {file: "jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js" line: 658}]

Comment 30

[Vicamo Yang [:vicamo][:vyang]]

we need a new settings entry in Gaia for this. I'll merge it tonight.

Comment 31

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #30)
> we need a new settings entry in Gaia for this. I'll merge it tonight.

Gecko should be resilient against missing Settings. Reading the patch again, it seems we're not doing a great job of making sure that this._preferredNetworkType is non-null and thus the radio can start up without hindrance.

I'm going to back this out. This and the issue reported in comment 29/bug 793450 indicates that we've been too sloppy here, both you as an implementer and I as a reviewer. Too many people have wasted valuable hours of debug time because of this. We must be more respectful of their time.

Comment 32

[Philipp von Weitershausen [:philikon]]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec54f1bf1c83

To my previous note in comment 31 I will add that this code should have been tested manually and automatically, at the very least by running the Marionette tests which it appears to have broken. This in and of itself should already have raised red flags.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec54f1bf1c83

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

Attachment: v4

1. include change in bug 793450
2. handle one more possible null value of _preferredNetworkType
3. https://tbpl.mozilla.org/?tree=Try&rev=32e76244badc, though I don't think this might help, because it had already passed inbound check.

Comment 35

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #32)
> Backed out:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ec54f1bf1c83
> 
> To my previous note in comment 31 I will add that this code should have been
> tested manually and automatically, at the very least by running the
> Marionette tests which it appears to have broken. This in and of itself
> should already have raised red flags.

About Marionette tests, we don't have code in emulator so setting 2G/3G will always get an "unsupported" error in ril_worker. We can test only rollback-on-error for now. Besides, I don't know whether or not it's possible to cancel a settings entry initialization in Gaia. If not, then you can't justify the that test case is effective for bug 793450 after Gaia solves its issue-4994.

Comment 36

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: v5

fix yet another possible RIL bread in v4.

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: test scripts

Comment 38

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #36)
> Created attachment 664449 [details] [diff] [review]
> Part 1: v5
> 
> fix yet another possible RIL bread in v4.

s/bread/break/

Comment 39

[Philipp von Weitershausen [:philikon]]

Comment on attachment 664449 [details] [diff] [review]
Part 1: v5

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +747,5 @@
> +    if (this._preferredNetworkType == null) {
> +      // We haven't read the initial value from the settings DB yet.
> +      // Wait for that.
> +      return;
> +    }

I don't think we should be blocking on this._preferredNetworkType here at all. There's no reason to wait with bringing up the radio if we haven't read this information yet or if there's no default setting.

I'm probably missing something, but I still don't see how this makes Gecko be resilient against a missing setting. Because this._preferredNetworkType is set in `handleSetPreferredNetworkType()` which only gets called *after* we talk to the radio. But we can't talk to the radio if the radio isn't on, right? Or are you counting on the fact that we're going to get an error, but still notify the main thread?

::: dom/system/gonk/ril_worker.js
@@ +4281,5 @@
>    }
>  
>    this.initRILState();
> +
> +  this.setPreferredNetworkType();

One thought I had: can we make this request at this time? What if the radio state is OFF?

Comment 40

[Philipp von Weitershausen [:philikon]]

Comment on attachment 664450 [details] [diff] [review]
Part 2: test scripts

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

::: dom/network/tests/marionette/test_mobile_preferred_network_type.js
@@ +11,5 @@
> +}
> +SpecialPowers.addPermission("mobileconnection", true, document);
> +SpecialPowers.addPermission("settings", true, document);
> +
> +let mozSettings = window.navigator.mozSettings;

Nit: Just call it `settings`. The `moz` prefix will ideally disappear, and then we only have to change it one place, not everywhere.

@@ +14,5 @@
> +
> +let mozSettings = window.navigator.mozSettings;
> +
> +function test_rollback_on_error() {
> +  log("Testing rolling back on error");

A better name / explanation would be to say that we roll back the preferred network type setting if it's set to an invalid value. "on error" is pretty generic. A test that doesn't explain its intentions super clearly is not very helpful.

@@ +37,5 @@
> +        return;
> +      }
> +
> +      mozSettings.removeObserver(KEY, observer);
> +      ok(setDone, "Rolled back with setting to an invalid value first.");

Would be good to explain when we're getting down here. AFAICT that's when the setting gets reverted. But we don't actually test for the new (old) value, either. So I'm not sure how useful of a test this really is. r- mostly for this.

@@ +52,5 @@
> +    });
> +  });
> +  getReq.addEventListener("error", function onGetError() {
> +    ok(false, "cannot get default value of '" + KEY + "'");
> +    window.setTimeout(cleanUp, 0);

You can use

  SimpleTest.executeSoon(func);

instead of

  window.setTimeout(func, 0);

The latter uses timers and fuzz, the later executes as soon as possible.

@@ +60,5 @@
> +function cleanUp() {
> +  SpecialPowers.removePermission("mobileconnection", document);
> +  SpecialPowers.removePermission("settings", document);
> +  if (!gSettingsEnabled) {
> +    SpecialPowers.setBoolPref("dom.mozSettings.enabled", false);

You can just do

  SpecialPowers.clearUserPref("dom.mozSettings.enabled");

That way you need to worry about whether the settings API is enabled or not. You can just set the pref to `true` above and then this line will clear your change if there was any.

Comment 41

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #39)
> Comment on attachment 664449 [details] [diff] [review]
> Part 1: v5
> 
> Review of attachment 664449 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +747,5 @@
> > +    if (this._preferredNetworkType == null) {
> > +      // We haven't read the initial value from the settings DB yet.
> > +      // Wait for that.
> > +      return;
> > +    }
> 
> I don't think we should be blocking on this._preferredNetworkType here at
> all. There's no reason to wait with bringing up the radio if we haven't read
> this information yet or if there's no default setting.

I guess the same statement can also be applied to 3G data connections. Anyway, that's nothing but a policy for me. If you insist, I can remove it, and, in fact, I don't like the xxxToRead logic there. BTW, Android makes preferred network type a constructor parameter of CommandInterface -- the gate to rild.

> I'm probably missing something, but I still don't see how this makes Gecko
> be resilient against a missing setting. Because this._preferredNetworkType
> is set in `handleSetPreferredNetworkType()` which only gets called *after*
> we talk to the radio. But we can't talk to the radio if the radio isn't on,
> right? Or are you counting on the fact that we're going to get an error, but
> still notify the main thread?

That's why I have:

> > if ((this._preferredNetworkType != null) && !message.success) {
> >   // Reset the settings value
> > }
> > // Now etch the new value it into `this._preferredNetworkType`.

This will always allow the first call to `setPreferredNetworkType` to assign `this._preferredNetworkType`.

> ::: dom/system/gonk/ril_worker.js
> @@ +4281,5 @@
> >    }
> >  
> >    this.initRILState();
> > +
> > +  this.setPreferredNetworkType();
> 
> One thought I had: can we make this request at this time? What if the radio
> state is OFF?

I think this was already answered in comment #21 and you agreed in comment #26. Actually, we always fails in the first try after cold boot with exactly the reason you mentioned above: radio not ready. So we try the second time automatically without intention from RadioInterfaceLayer in SIM_READY. This time it succeeds. If Gecko crashes and reconnects to rild, then the first try in RIL_CONNECTED takes effect to ensure we have correct preferred network type.

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #40)
> Comment on attachment 664450 [details] [diff] [review]
> Part 2: test scripts
> 
> Review of attachment 664450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/tests/marionette/test_mobile_preferred_network_type.js
> @@ +37,5 @@
> > +        return;
> > +      }
> > +
> > +      mozSettings.removeObserver(KEY, observer);
> > +      ok(setDone, "Rolled back with setting to an invalid value first.");
> 
> Would be good to explain when we're getting down here. AFAICT that's when
> the setting gets reverted. But we don't actually test for the new (old)
> value, either. So I'm not sure how useful of a test this really is. r-
> mostly for this.

Please see my comment #35. There is no setPreferredNetworkType support in emulator, at least for now. The set cannot be successful in any case at all.

Comment 43

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: v6

Don't block RIL init as review comment #39.

Comment 44

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: test scripts : v2

1. s/mozSettings/settings/
2. rename the test function, some clean up and comments
3. use SpecialPowers.clearUserPref

SimpleTest.executeSoon doesn't work for me. The callback function never gets called and marionette gets timeout. Stick with window.setTimeout.

Comment 45

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #44)
> SimpleTest.executeSoon doesn't work for me. The callback function never gets
> called and marionette gets timeout. Stick with window.setTimeout.

Oh, you're in Marionette. I got confused there, I thought you were in Mochitest. Hmm, I should file a bug against Marionette to support the SimpleTest API perhaps.

Comment 46

[Philipp von Weitershausen [:philikon]]

Comment on attachment 664833 [details] [diff] [review]
Part 1: v6

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

This looks good to me. Let's land it!

However, I would like us to do a follow-up in which we debounce the setting. We've had problems with the airplane mode (bug 793073) and mobile data settings (bug 785982). In essence, I would like us to ignore any changes to the "ril.radio.preferredNetworkType" setting while the REQUEST_SET_PREFERRED_NETWORK_TYPE request is being processed. Then, when it completes, we should compare with the setting again to see if it has changed in the mean time. Can you file a follow-up bug for this please? Thanks!

::: dom/system/gonk/ril_worker.js
@@ +4035,5 @@
> +RIL[REQUEST_GET_PREFERRED_NETWORK_TYPE] = function REQUEST_GET_PREFERRED_NETWORK_TYPE(length, options) {
> +  let networkType;
> +  if (!options.rilRequestError) {
> +    networkType = RIL_PREFERRED_NETWORK_TYPE_TO_GECKO.indexOf(GECKO_PREFERRED_NETWORK_TYPE_DEFAULT);
> +    if (Buf.readUint32()) {

What's this uint32? Would be good to assign this to a variable with a descriptive name, even if we only need it in the `if` test.

r=me with that.

Comment 47

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: v7

1. Address review comment #46. Philipp, that's the number of INT32 followed in the response. I also add a inline comment after.
2. Filed bug 795198 for debounce problem.

Since this patch has to be rebased after bug 744700, I'll land it later after some tests again.

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

These patches were previously backed-out, fixed and now pushed again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88b7b8f8ca78
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1faaa874b3

Comment 49

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/88b7b8f8ca78
https://hg.mozilla.org/mozilla-central/rev/5a1faaa874b3

Comment 50

[Eric Shepherd [:sheppy]]

The pref needs to be documented.

Comment 51

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Eric Shepherd [:sheppy] from comment #50)
> The pref needs to be documented.

Thank you. I'll document that newly added preference once this preference documentation page[1] gets fixed.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/Preference_reference