944225 - B2G DSDS: support setting preferred network type for each sim

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect, P1)

Description

[Jessica Jong [:jessica] (inactive)]

Currently, we use 'ril.radio.preferredNetworkType' setting key to set preferred network type. For DSDS, we need to differentiate for which sim we are setting the preferred network type.

Comment 1

[sam.hua]

1. SIM1 can work in different mode,WCDMA / GSM
2. SIM2 only works in "GSM only" mode and can't be set preferred network.

Comment 2

[Jessica Jong [:jessica] (inactive)]

(In reply to sam.hua from comment #1)
> 1. SIM1 can work in different mode,WCDMA / GSM
> 2. SIM2 only works in "GSM only" mode and can't be set preferred network.

But I am not getting any error when setting preferred network type on sim2, it always return success, is this expected?

Comment 3

[Anshul]

Kevin, this bug is blocking a lot of our FC test cases related to Cdma + GSM DSDS combination as we are enable to set the network type for individual SIMs. Could you please expedite this?

Comment 4

[Anshul]

(In reply to Anshul from comment #3)
> Kevin, this bug is blocking a lot of our FC test cases related to Cdma + GSM
> DSDS combination as we are enable to set the network type for individual
> SIMs. Could you please expedite this?
Sorry typo, unable and not enable.

Comment 5

[Ken Chang[:kenkai]]

Jessica, it is a 1.3+. Please take this bug.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 1: idl

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: dom

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

Is there any gaia side bug for this change? If it lands before bug 928297 I could take care of it in that bug otherwise please file the new bug and I'll take care of it as well. Thanks!

Comment 9

[Anshul]

Jessica, please also provide the changes to the RILContentHelper as part of this change and I can give that a try.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl

nsIDOMMobileConnection and nsIMobileConnectionProvider idl.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom

MobileConnection changes.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: ril

ril implementation

Comment 13

[Jessica Jong [:jessica] (inactive)]

(In reply to Anshul from comment #9)
> Jessica, please also provide the changes to the RILContentHelper as part of
> this change and I can give that a try.

Anshul, with the attached three patches, you will be able to use the new APIs to set/get preferred network type on a corresponding sim/service:

var conn = window.navigator.mozMobileConnections[serviceId];
conn.setPreferredNetworkType('wcdma');

Please note the patches are still in the beginning of the review process, they might change in the future.

Comment 14

[Jessica Jong [:jessica] (inactive)]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
> Is there any gaia side bug for this change? If it lands before bug 928297 I
> could take care of it in that bug otherwise please file the new bug and I'll
> take care of it as well. Thanks!

José, I am not sure when are these patches going to be landed. I would not want to block the work in bug 928297, so what do you say we file another gaia bug for this?

Comment 15

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #14)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
> > Is there any gaia side bug for this change? If it lands before bug 928297 I
> > could take care of it in that bug otherwise please file the new bug and I'll
> > take care of it as well. Thanks!
> 
> José, I am not sure when are these patches going to be landed. I would not
> want to block the work in bug 928297, so what do you say we file another
> gaia bug for this?

Much better, please file the new bug at will and add me to CC list please. Thanks!

Comment 16

[Kai-Chih Hu [:khu]]

Since Jessica is handling, clear the ni.

Comment 17

[Anshul]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #15)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #14)
> > (In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
> > > Is there any gaia side bug for this change? If it lands before bug 928297 I
> > > could take care of it in that bug otherwise please file the new bug and I'll
> > > take care of it as well. Thanks!
> > 
> > José, I am not sure when are these patches going to be landed. I would not
> > want to block the work in bug 928297, so what do you say we file another
> > gaia bug for this?
> 
> Much better, please file the new bug at will and add me to CC list please.
> Thanks!
Filed the bug 946548 for gaia support. Antonio, would you be working on this bug?

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8342234 [details] [diff] [review]
Part 3: ril

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

Thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1527,5 @@
> +    message.networkType = networkType;
> +
> +    this.workerMessenger.send("setPreferredNetworkType", message, (function(response) {
> +      if (response.success) {
> +        this._preferredNetworkType = response.networkType;

Please also add:
if (DEBUG) {
    this.debug("_preferredNetworkType is now " +
                      RIL.RIL_PREFERRED_NETWORK_TYPE_TO_GECKO[this._preferredNetworkType]);
}

@@ +1538,5 @@
> +      return false;
> +    }).bind(this));
> +  },
> +
> +  setPreferredNetworkTypeBySetting: function setPreferredNetworkTypeBySetting(value) {

I expect this is gonna removed after bug 946548 is done. Please file a follow-up gecko bug for this, and have a TODO comment. Thanks.

@@ +2541,5 @@
>  
>    // nsISettingsServiceCallback
>    handle: function handle(aName, aResult) {
>      switch(aName) {
>        case "ril.radio.preferredNetworkType":

ditto.

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: [Final] Part 1: idl.

Add r=hsinyi.

Comment 20

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom, v2.

Add f=hsinyi.

Comment 21

[Jessica Jong [:jessica] (inactive)]

Attachment: [Final] Part 3: ril.

Add r=hsinyi and address review comment 18.

Comment 22

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: tests, v1.

Comment 23

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: tests, v2.

move the original test to test_mobile_preferred_network_type_by_setting.js, which has been (and still is) disabled.

Comment 24

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8343550 [details] [diff] [review]
Part 2: dom, v2.

Olli, we are replacing the use of 'ril.radio.preferredNetworkType' with these new apis in mobileconnection. May I have your review? Thank you.

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8343551 [details] [diff] [review]
[Final] Part 3: ril.

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

Looks good!

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8343555 [details] [diff] [review]
Part 4: tests, v2.

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

::: dom/network/tests/marionette/manifest.ini
@@ +7,5 @@
>  disabled = Bug 808783
>  [test_mobile_voice_state.js]
>  [test_mobile_operator_names.js]
>  [test_mobile_preferred_network_type.js]
> +[test_mobile_preferred_network_type_by_setting.js]

Thanks for taking care of this :)

::: dom/network/tests/marionette/test_mobile_preferred_network_type.js
@@ +36,3 @@
>  
> +  request.onsuccess = function onsuccess() {
> +    ok(false, "request should not success");

nit: s/success/succeed/

@@ +101,4 @@
>    finish();
>  }
>  
> +runNextTest();

In general, I'd like to have more flexible testSetPreferredNetworkType() and testGetPreferredNetworkType() functions which take 'type' parameter. In this way, if we want to test more types, we don't need to have duplicate code.

Also, can we test all the types being mentioned in .idl?

Comment 27

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: tests, v3.

Thank you Hsinyi, I have addressed review comment 26.

While testing all supported preferred network types, I found that the emulator seems not able to handle switching from "cdma/evdo" to "cdma" well, the network type is not changed in this case. I haven't looked into it yet, I just changed the network types order a little for now.

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8344473 [details] [diff] [review]
Part 4: tests, v3.

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

::: dom/network/tests/marionette/test_mobile_preferred_network_type.js
@@ +81,5 @@
> +}
> +
> +function testInvalidNetworkTypes() {
> +  for (let i = 0; i < invalidTypes.length; i++) {
> +    failToSetPreferredNetworkType(invalidTypes[i], "InvalidParameter");

This looks okay but I'd like to be more careful that we jump to the next case while the pending request is completed for sure. So how about we follow the same style as 'testPreferredNetworkTypes'?

@@ +95,5 @@
> +  'cdma/evdo',
> +  'evdo',
> +  'cdma',
> +  'wcdma/gsm/cdma/evdo',
> +  'wcdma/gsm' // restore to default

Good! :)

Comment 29

[Jessica Jong [:jessica] (inactive)]

Attachment: [Final] Part 4: tests.

Thank you Hsinyi, I have addressed review comment 28. Could you help check it again? Thanks.

Comment 30

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8344523 [details] [diff] [review]
[Final] Part 4: tests.

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

Looks great, thank you :)

Comment 31

[Jessica Jong [:jessica] (inactive)]

Attachment: [Final] Part 2: dom.

Add r=smaug.

Comment 32

[Jessica Jong [:jessica] (inactive)]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=6793f61722f3

Comment 33

[Jessica Jong [:jessica] (inactive)]

All green! \o/

Comment 34

[Jessica Jong [:jessica] (inactive)]

Hi Fabrice,

This patch adds a new API to support setting preferred network type for each sim. It replaces the use of 'ril.radio.preferredNetworkType', but we are keeping the setting handler until the corresponding gaia bug (bug 946548) is landed.

To answer your questions:
- is the code fairly isolated? ("I want to refactor the RIL worker" would not pass).
  yes, it's in RIL but does not touch other RIL function.
- has this code caused churn or pain before?
  it's a new api, so it hasn't caused churn or pain before.
- is it easy to turn the feature off?
  we can just back it out to turn it off.
- do we have good test coverage?
  we have added test cases for the function.
- do we have good QA coverage, ie in smoketests?
  need to wait gaia bug 946548 for QA to test.
- do we have strong partner expectations for the feature?
  yes yes! QC is expecting this for their test cases.

Could you help examine and decide whether we can land this or not? Thank you.

Comment 35

[[:fabrice] Fabrice Desré]

Jessica, I won't make a decision before bug 946548 is ready. Once this is the case, can you get QA to do a smoketest with patches from both bugs and needinfo me again? thanks!

Comment 36

[[:fabrice] Fabrice Desré]

In case it's not clear, you're free to land on master, not yet on 1.3

Comment 37

[Ken Chang[:kenkai]]

Jessica, please land on master first. Thanks.

Comment 38

[Jessica Jong [:jessica] (inactive)]

(In reply to Fabrice Desré [:fabrice] from comment #36)
> In case it's not clear, you're free to land on master, not yet on 1.3

We will land on master first. Thank you Fabrice.

Comment 39

[Jessica Jong [:jessica] (inactive)]

Enpei, I'd like to confirm with you, do we have smoketests on setting/getting preferred network type? Thank you.

Comment 40

[Enpei]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #39)
> Enpei, I'd like to confirm with you, do we have smoketests on
> setting/getting preferred network type? Thank you.
Once the patch is landed on master build, I will arrange smoke test plus some basic setting test cases to cover it.

Comment 41

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks, Jessica :)

   https://hg.mozilla.org/integration/b2g-inbound/rev/d5251436a70a
   https://hg.mozilla.org/integration/b2g-inbound/rev/47130d04f60a
   https://hg.mozilla.org/integration/b2g-inbound/rev/d9b19d9d3eab
   https://hg.mozilla.org/integration/b2g-inbound/rev/6cfaf4b5a806

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d5251436a70a
https://hg.mozilla.org/mozilla-central/rev/47130d04f60a
https://hg.mozilla.org/mozilla-central/rev/d9b19d9d3eab
https://hg.mozilla.org/mozilla-central/rev/6cfaf4b5a806

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6df12b775e51
https://hg.mozilla.org/releases/mozilla-aurora/rev/6e9fa78cc96a
https://hg.mozilla.org/releases/mozilla-aurora/rev/c322e496efc6
https://hg.mozilla.org/releases/mozilla-aurora/rev/db8b2ff2ef9b