961921 - B2G RIL: [DSDS] Separate roaming preference for each client.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

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.

Comment 1

[Jessica Jong [:jessica] (inactive)]

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

Comment 2

[Ken Chang[:kenkai]]

Change the setting interfaces for RIL.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: proposed patch, v1.

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

Comment 4

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

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.

Comment 5

[Preeti Raghunath(:Preeti)]

Wilfred,

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

Comment 6

[Anshul]

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.

Comment 7

[Jason Smith [:jsmith]]

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

Comment 8

[Anshul]

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

Comment 9

[Jason Smith [:jsmith]]

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

Comment 10

[Jason Smith [:jsmith]]

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

Comment 11

[Ken Chang[:kenkai]]

Jessica, would you like to take this bug?

Comment 12

[Jessica Jong [:jessica] (inactive)]

Sure!

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: separate roaming preference for each client, v2.

Rebased after Bug 905568.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: add data connection tests for DSDS, v1.

Comment 15

[Jessica Jong [:jessica] (inactive)]

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 16

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

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?

Comment 17

[Preeti Raghunath(:Preeti)]

Joe,

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

Comment 18

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

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

Comment 19

[Joe Cheng [:jcheng] (please needinfo)]

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.

Comment 20

[Ken Chang[:kenkai]]

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

Comment 21

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

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.

Comment 22

[Wilfred Mathanaraj [:WDM]]

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

Comment 23

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: separate roaming preference for each client, v3.

- Use a simpler way to handle this.

Comment 24

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: add data connection tests for DSDS, v2.

Comment 25

[Jessica Jong [:jessica] (inactive)]

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 26

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

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 27

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

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

Comment 28

[Jessica Jong [:jessica] (inactive)]

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

Comment 29

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

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

Comment 30

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: add data connection tests for DSDS, v3.

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 31

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

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.

Comment 32

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: add data connection tests for DSDS, v4.

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.

Comment 33

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: add data connection tests for DSDS, v5.

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

Comment 34

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

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

Comment 35

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: separate roaming preference for each client (final)

Add r=hsinyi.

Comment 36

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: add data connection tests for DSDS (final)

Add r=hsinyi.

Comment 37

[Jessica Jong [:jessica] (inactive)]

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

Comment 38

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: add data connection tests for DSDS (final)

Rebased after bug 979134.

Comment 39

[Jessica Jong [:jessica] (inactive)]

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

All green!

Comment 40

[Carsten Book [:Tomcat]]

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

Comment 41

[Ed Morley [:emorley]]

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