Currently in DSDS each client share the same setting, ril.data.roaming_enabled, for roaming preference of data connection. We need to separate it for each client.

A simple solution could be consider making the value of 'ril.data.roaming_enabled' an array, just like 'ril.data.apnSettings'.

Change the setting interfaces for RIL.

Patch works fine with single sim, need to test it with gaia patch or on emulator-jb for multisim.

Comment on attachment 8364254 [details] [diff] [review] proposed patch, v1. Review of attachment 8364254 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RadioInterfaceLayer.js @@ +2872,5 @@ > if (DEBUG) this.debug("'ril.radio.preferredNetworkType' is now " + aResult); > this.setPreferredNetworkTypeBySetting(aResult); > break; > case "ril.data.roaming_enabled": > if (DEBUG) this.debug("'ril.data.roaming_enabled' is now " + aResult); if aResult is an array, we don't see the value. Don't forget to modify the debug message.

Wilfred, Please provide guidance on whether we designed the feature for separate data connection per client.

Wesely, please don't uplift this bug for 1.3 as 1.3 is done. For that matter, please don't mark any RIL interface changes as 1.3 unless it is marked blocking by us or a customer.

(In reply to Anshul from comment #6) > Wesely, please don't uplift this bug for 1.3 as 1.3 is done. For that > matter, please don't mark any RIL interface changes as 1.3 unless it is > marked blocking by us or a customer. Note - The reality is that we can have certification issues with this feature that might require interface changes later. We can't guarantee a full interface freeze. We need a product call here to find out if they are willing to live with the problem seen in the blocking bug or not & determine if our operators are okay with it or not.

(In reply to Jason Smith [:jsmith] from comment #7) > (In reply to Anshul from comment #6) > > Wesely, please don't uplift this bug for 1.3 as 1.3 is done. For that > > matter, please don't mark any RIL interface changes as 1.3 unless it is > > marked blocking by us or a customer. > > Note - The reality is that we can have certification issues with this > feature that might require interface changes later. We can't guarantee a > full interface freeze. We need a product call here to find out if they are > willing to live with the problem seen in the blocking bug or not & determine > if our operators are okay with it or not. Jason since there are no commercial devices on 1.3 for DSDS (or that I know of) I don't think this change is necessary for 1.3. Please correct me I am wrong.

(In reply to Anshul from comment #8) > (In reply to Jason Smith [:jsmith] from comment #7) > > (In reply to Anshul from comment #6) > > > Wesely, please don't uplift this bug for 1.3 as 1.3 is done. For that > > > matter, please don't mark any RIL interface changes as 1.3 unless it is > > > marked blocking by us or a customer. > > > > Note - The reality is that we can have certification issues with this > > feature that might require interface changes later. We can't guarantee a > > full interface freeze. We need a product call here to find out if they are > > willing to live with the problem seen in the blocking bug or not & determine > > if our operators are okay with it or not. > Jason since there are no commercial devices on 1.3 for DSDS (or that I know > of) I don't think this change is necessary for 1.3. Please correct me I am > wrong. Fugu is a commercial device we are shipping on the 1.3 branch. I still would like to see a product call here before we make a blocking decision.

See https://bugzilla.mozilla.org/show_bug.cgi?id=960388#c14.

Jessica, would you like to take this bug?

Sure!

Rebased after Bug 905568.

Comment on attachment 8375981 [details] [diff] [review] Part 1: separate roaming preference for each client, v2. Hsinyi, it seems that we can dump array correctly. May I have your review on this? Thank you.

Comment on attachment 8375981 [details] [diff] [review] Part 1: separate roaming preference for each client, v2. Review of attachment 8375981 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RadioInterfaceLayer.js @@ +881,5 @@ > this.debug("'ril.data.roaming_enabled' is now " + result); > this.debug("Default id for data call: " + this._dataDefaultClientId); > } > + if (Array.isArray(result)) { > + for (let clientId in this._connectionHandlers) { I learned for-in was discouraged for iterating arrays (bug 969231 comment 4). Let's use for (let i = 0, n = this._connectionHandlers.length; i < n; ++i) { // Do what we need to. } @@ +883,5 @@ > } > + if (Array.isArray(result)) { > + for (let clientId in this._connectionHandlers) { > + let connHandler = this._connectionHandlers[clientId]; > + if (connHandler) { In any situation 'connHandler' could be null?

Joe, So, this fix is not needed in 1.4 right? Can we move this to 1.5?

Remove 1.4? as it's not blocking to any 1.4 key focus features.

is it a feature or is it a bug? i feel this is more like a bug. in the UX spec, when you goto Settings -> Cellular & Data, you will be asked to select SIM1 or SIM2 so it is expected that the roaming setting is to be maintained seperately for SIM1 / SIM2. if feel this should be resolved.

It doesn't block any key feature or blocker bugs. Let's fix this in 1.5.

Behavior-wise it's like a bug, but intrinsically a RIL interface change (feature). Jessica supposedly will work on IPv6 until Mar14. So let's defer it to 1.5? ni? Sandip/Wilfred to see if this is a priority.

if this is a RIL change then it would be wiser to do this for v1.5.

- Use a simpler way to handle this.

Comment on attachment 8399762 [details] [diff] [review] Part 2: add data connection tests for DSDS, v2. This test needs less than 10 seconds to run locally, and it is skipped on try server since ICS is a single-sim device.

Comment on attachment 8399759 [details] [diff] [review] Part 1: separate roaming preference for each client, v3. Review of attachment 8399759 [details] [diff] [review]: ----------------------------------------------------------------- Neat :)

Comment on attachment 8399762 [details] [diff] [review] Part 2: add data connection tests for DSDS, v2. Review of attachment 8399762 [details] [diff] [review]: ----------------------------------------------------------------- There are helper functions defined in head.js. I think we could take advantage of them. ::: dom/mobileconnection/tests/marionette/head.js @@ +328,5 @@ > return doSetAndWait("voice", aRoaming) > .then(() => doSetAndWait("data", aRoaming)); > } > > +let _numOfRadioInterfaces; Don't see a reason of introducing this global variable. ::: dom/mobileconnection/tests/marionette/test_dsds_mobile_data_connection.js @@ +22,5 @@ > + currentDataDefaultId = defaultId; > + }); > +} > + > +function setEmulatorRoaming(roaming) { setEmulatorRoamingAndWait is already defined in head.js. Coud we take advantage of that? @@ +107,5 @@ > +function testEnableData() { > + return Promise.resolve() > + .then(() => setApnSettings()) > + .then(() => setSettings1(SETTINGS_KEY_DATA_ENABLED, true)) > + .then(() => waitForDataState(currentDataDefaultId, true)); Looks we could extend "setDataEnabledAndWait" and make use of it. For example: setDataEnabledAndWait(aEnabled, aClientId); // aClientId is Optional. If not defined, it's always 0. waitForManagerEventAt(aEventName, aClientId); @@ +113,5 @@ > + > +function testSwitchDefaultDataToSimTwo() { > + return Promise.resolve() > + .then(() => switchDataDefaultId(1)) > + .then(() => waitForDataState(currentDataDefaultId, true)); Let's try to rely on waitForManagerEventAt().

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #27) > Comment on attachment 8399762 [details] [diff] [review] > Part 2: add data connection tests for DSDS, v2. > > Review of attachment 8399762 [details] [diff] [review]: > ----------------------------------------------------------------- > > There are helper functions defined in head.js. I think we could take > advantage of them. > > ::: dom/mobileconnection/tests/marionette/head.js > @@ +328,5 @@ > > return doSetAndWait("voice", aRoaming) > > .then(() => doSetAndWait("data", aRoaming)); > > } > > > > +let _numOfRadioInterfaces; > > Don't see a reason of introducing this global variable. The variable is used in getNumOfRadioInterfaces(), to avoid reading preference everytime? > > ::: dom/mobileconnection/tests/marionette/test_dsds_mobile_data_connection.js > @@ +22,5 @@ > > + currentDataDefaultId = defaultId; > > + }); > > +} > > + > > +function setEmulatorRoaming(roaming) { > > setEmulatorRoamingAndWait is already defined in head.js. Coud we take > advantage of that? Sure, will extend the function to support variable clientId for mobileConnection. > > @@ +107,5 @@ > > +function testEnableData() { > > + return Promise.resolve() > > + .then(() => setApnSettings()) > > + .then(() => setSettings1(SETTINGS_KEY_DATA_ENABLED, true)) > > + .then(() => waitForDataState(currentDataDefaultId, true)); > > Looks we could extend "setDataEnabledAndWait" and make use of it. > > For example: > setDataEnabledAndWait(aEnabled, aClientId); // aClientId is Optional. If not > defined, it's always 0. > > waitForManagerEventAt(aEventName, aClientId); Sure, will extend these functions to support variable clientId for mobileConnection. > > @@ +113,5 @@ > > + > > +function testSwitchDefaultDataToSimTwo() { > > + return Promise.resolve() > > + .then(() => switchDataDefaultId(1)) > > + .then(() => waitForDataState(currentDataDefaultId, true)); > > Let's try to rely on waitForManagerEventAt(). Sure, will extend the function to support variable clientId for mobileConnection.

(In reply to Jessica Jong [:jjong] [:jessica] from comment #28) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #27) > > Comment on attachment 8399762 [details] [diff] [review] > > Part 2: add data connection tests for DSDS, v2. > > > > Review of attachment 8399762 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > There are helper functions defined in head.js. I think we could take > > advantage of them. > > > > ::: dom/mobileconnection/tests/marionette/head.js > > @@ +328,5 @@ > > > return doSetAndWait("voice", aRoaming) > > > .then(() => doSetAndWait("data", aRoaming)); > > > } > > > > > > +let _numOfRadioInterfaces; > > > > Don't see a reason of introducing this global variable. > > The variable is used in getNumOfRadioInterfaces(), to avoid reading > preference everytime? > Aha, I miss that :( Having the variable is fine.

Thank you Hsinyi, here is an updated patch addressing review comment 27: - Extend functions in head.js to support variable client id for mobile connection. - Make use of the functions in head.js.

Comment on attachment 8402446 [details] [diff] [review] Part 2: add data connection tests for DSDS, v3. Review of attachment 8402446 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobileconnection/tests/marionette/head.js @@ +263,4 @@ > let deferred = Promise.defer(); > > + let mobileConn = mobileConnection; > + if (aServiceId) { What if defualtId isn't 0 and 0 is assigned to aServiceId? Seems we need to separate the cases of aServiceId not defined and a serviceId equal to 0. @@ +300,5 @@ > promises.push(setDataEnabled(aEnabled)); > Promise.all(promises).then(function keepWaiting() { > + let mobileConn = mobileConnection; > + if (aServiceId) { > + mobileConn = navigator.mozMobileConnections[aServiceId]; ditto. @@ +340,5 @@ > return Promise.all(promises) > + .then(() => { > + let mobileConn = mobileConnection; > + if (aServiceId) { > + mobileConn = navigator.mozMobileConnections[aServiceId]; ditto. ::: dom/mobileconnection/tests/marionette/test_dsds_mobile_data_connection.js @@ +79,5 @@ > + .then(() => muxModem(0)); > +} > + > +function testEnableData() { > + log("Turn data on."); Please have checks on the precondition. E.g., is(conneciton.data.connected, false); @@ +87,5 @@ > + .then(() => setDataEnabledAndWait(true, currentDataDefaultId)); > +} > + > +function testSwitchDefaultDataToSimTwo() { > + log("Switch data connection to sim 2."); ditto: is(currentDataDefaultId, 0); @@ +90,5 @@ > +function testSwitchDefaultDataToSimTwo() { > + log("Switch data connection to sim 2."); > + > + return Promise.resolve() > + .then(() => switchDataDefaultId(1)) Let's have a check: .then(() => { is(currentDataDefaultId, 1); }) @@ +96,5 @@ > +} > + > +function testDisableDataRoamingWhileRoaming() { > + log("Disable data roaming setting while roaming."); > + ditto: e.g. is(connection.data.connected, true); @@ +106,5 @@ > +} > + > +function testEnableDataRoamingWhileRoaming() { > + log("Enable data roaming setting while roaming."); > + ditto: e.g. is(connection.data.connected, false); @@ +114,5 @@ > +} > + > +function testDisableData() { > + log("Turn data off."); > + ditto.

Thank you Hsinyi, review comments in comment 31 have been addressed: - handle the case where parameter aServiceId is 0. - add preconditions for each test case.

Sorry, forgot to add period at end of log message.

Comment on attachment 8403185 [details] [diff] [review] Part 2: add data connection tests for DSDS, v5. Review of attachment 8403185 [details] [diff] [review]: ----------------------------------------------------------------- Great! Thank you :)

Add r=hsinyi.

Add r=hsinyi.

Try: https://tbpl.mozilla.org/?tree=Try&rev=cead9ef023a7

Rebased after bug 979134.

Try: https://tbpl.mozilla.org/?tree=Try&rev=879ff636bd2d All green!

https://hg.mozilla.org/integration/b2g-inbound/rev/59f927e60a53 https://hg.mozilla.org/integration/b2g-inbound/rev/6892cdd30dd7

https://hg.mozilla.org/mozilla-central/rev/59f927e60a53 https://hg.mozilla.org/mozilla-central/rev/6892cdd30dd7