Closed Bug 944225 Opened 6 years ago Closed 6 years ago

B2G DSDS: support setting preferred network type for each sim

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 Sprint 6 - 12/6
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

Details

(Whiteboard: [CR 582660])

Attachments

(4 files, 9 obsolete files)

5.26 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
10.83 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
6.70 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
1.51 KB, patch
jessica
: review+
Details | Diff | Splinter Review
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.
1. SIM1 can work in different mode,WCDMA / GSM
2. SIM2 only works in "GSM only" mode and can't be set preferred network.
(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?
Blocks: 930299
blocking-b2g: --- → 1.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?
Flags: needinfo?(khu)
(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.
Severity: normal → critical
blocking-b2g: 1.3? → 1.3+
Priority: -- → P1
Whiteboard: [CR 582660]
Target Milestone: --- → 1.3 Sprint 6 - 12/6
Jessica, it is a 1.3+. Please take this bug.
Assignee: nobody → jjong
Attached patch (WIP) Part 1: idl (obsolete) — Splinter Review
Attached patch (WIP) Part 2: dom (obsolete) — Splinter Review
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!
Jessica, please also provide the changes to the RILContentHelper as part of this change and I can give that a try.
Attached patch Part 1: idl (obsolete) — Splinter Review
nsIDOMMobileConnection and nsIMobileConnectionProvider idl.
Attachment #8341513 - Attachment is obsolete: true
Attached patch Part 2: dom (obsolete) — Splinter Review
MobileConnection changes.
Attachment #8341514 - Attachment is obsolete: true
Attached patch Part 3: ril (obsolete) — Splinter Review
ril implementation
(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.
(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?
Attachment #8342232 - Flags: feedback?(htsai)
Attachment #8342233 - Flags: feedback?(htsai)
Attachment #8342234 - Flags: feedback?(htsai)
(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!
Since Jessica is handling, clear the ni.
Flags: needinfo?(khu)
Blocks: 946548
(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?
Attachment #8342232 - Flags: feedback?(htsai) → feedback+
Attachment #8342233 - Flags: feedback?(htsai) → feedback+
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.
Attachment #8342234 - Flags: feedback?(htsai) → feedback+
Blocks: 946589
Add r=hsinyi.
Attachment #8342232 - Attachment is obsolete: true
Attached patch Part 2: dom, v2. (obsolete) — Splinter Review
Add f=hsinyi.
Attachment #8342233 - Attachment is obsolete: true
Blocks: 942267
No longer blocks: 930299
Add r=hsinyi and address review comment 18.
Attachment #8342234 - Attachment is obsolete: true
Attached patch Part 4: tests, v1. (obsolete) — Splinter Review
Attached patch Part 4: tests, v2. (obsolete) — Splinter Review
move the original test to test_mobile_preferred_network_type_by_setting.js, which has been (and still is) disabled.
Attachment #8343553 - Attachment is obsolete: true
Attachment #8343548 - Flags: review?(htsai)
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.
Attachment #8343550 - Flags: review?(bugs)
Attachment #8343551 - Flags: review?(htsai)
Attachment #8343555 - Flags: review?(htsai)
Attachment #8343548 - Flags: review?(htsai) → review+
Comment on attachment 8343551 [details] [diff] [review]
[Final] Part 3: ril.

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

Looks good!
Attachment #8343551 - Flags: review?(htsai) → review+
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?
Attachment #8343555 - Flags: review?(htsai)
Attachment #8343550 - Flags: review?(bugs) → review+
Attached patch Part 4: tests, v3. (obsolete) — Splinter Review
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.
Attachment #8343555 - Attachment is obsolete: true
Attachment #8344473 - Flags: review?(htsai)
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! :)
Attachment #8344473 - Flags: review?(htsai)
Thank you Hsinyi, I have addressed review comment 28. Could you help check it again? Thanks.
Attachment #8344473 - Attachment is obsolete: true
Attachment #8344523 - Flags: review?(htsai)
Comment on attachment 8344523 [details] [diff] [review]
[Final] Part 4: tests.

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

Looks great, thank you :)
Attachment #8344523 - Flags: review?(htsai) → review+
Attachment #8343548 - Attachment description: Part 1: idl, v2. → [Final] Part 1: idl.
Add r=smaug.
Attachment #8343550 - Attachment is obsolete: true
Attachment #8344536 - Flags: review+
Attachment #8343551 - Attachment description: Part 3: ril, v2. → [Final] Part 3: ril.
Attachment #8344523 - Attachment description: Part 4: tests, v4. → [Final] Part 4: tests.
All green! \o/
Keywords: checkin-needed
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.
Flags: needinfo?(fabrice)
Keywords: checkin-needed
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!
Flags: needinfo?(fabrice)
In case it's not clear, you're free to land on master, not yet on 1.3
Jessica, please land on master first. Thanks.
(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.
Keywords: checkin-needed
Enpei, I'd like to confirm with you, do we have smoketests on setting/getting preferred network type? Thank you.
Flags: needinfo?(echu)
(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.
Flags: needinfo?(echu)
Duplicate of this bug: 792660
You need to log in before you can comment on or make changes to this bug.