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?

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?

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

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

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.

nsIDOMMobileConnection and nsIMobileConnectionProvider idl.

MobileConnection changes.

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?

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

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

Add r=hsinyi.

Add f=hsinyi.

Add r=hsinyi and address review comment 18.

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

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 on attachment 8343551 [details] [diff] [review] [Final] Part 3: ril. Review of attachment 8343551 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

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?

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 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! :)

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

Comment on attachment 8344523 [details] [diff] [review] [Final] Part 4: tests. Review of attachment 8344523 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thank you :)

Add r=smaug.

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

All green! \o/

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.

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!

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.

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

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

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

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

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