Closed Bug 961921 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S5 (11apr)

People

(Reporter: edgar, Assigned: jessica)

References

Details

Attachments

(2 files, 9 obsolete files)

1.50 KB, patch
jessica
: review+
Details | Diff | Splinter Review
12.90 KB, patch
jessica
: review+
Details | Diff | Splinter Review
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.
Blocks: 960388
blocking-b2g: --- → 1.3?
Whiteboard: [FT:RIL]
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.
Attached patch proposed patch, v1. (obsolete) — Splinter Review
Patch works fine with single sim, need to test it with gaia patch or on emulator-jb for multisim.
Attachment #8364254 - Flags: feedback?(htsai)
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.
Attachment #8364254 - Flags: feedback?(htsai) → feedback+
Wilfred,

Please provide guidance on whether we designed the feature for separate data connection per client.
Flags: needinfo?(wmathanaraj)
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.
Flags: needinfo?(whuang)
(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.
blocking-b2g: 1.3? → ---
(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.
blocking-b2g: --- → 1.3?
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(whuang)
Jessica, would you like to take this bug?
Flags: needinfo?(jjong)
Sure!
Assignee: nobody → jjong
Flags: needinfo?(jjong)
Rebased after Bug 905568.
Attachment #8364254 - Attachment is obsolete: true
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.
Attachment #8375981 - Flags: review?(htsai)
Attachment #8375982 - Flags: review?(htsai)
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?
Attachment #8375981 - Flags: review?(htsai)
Joe,

So, this fix is not needed in 1.4 right? Can we move this to 1.5?
Flags: needinfo?(jcheng)
Remove 1.4? as it's not blocking to any 1.4 key focus features.
blocking-b2g: 1.4? → ---
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.
blocking-b2g: --- → 1.4?
Flags: needinfo?(jcheng) → needinfo?(whuang)
It doesn't block any key feature or blocker bugs. Let's fix this in 1.5.
blocking-b2g: 1.4? → ---
Flags: needinfo?(whuang)
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.
blocking-b2g: --- → 1.5?
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(skamat)
if this is a  RIL change then it would be wiser to do this for v1.5.
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(skamat)
- Use a simpler way to handle this.
Attachment #8375981 - Attachment is obsolete: true
Attachment #8375982 - Attachment is obsolete: true
Attachment #8375982 - Flags: review?(htsai)
Attachment #8399759 - Flags: review?(htsai)
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.
Attachment #8399762 - Flags: review?(htsai)
Comment on attachment 8399759 [details] [diff] [review]
Part 1: separate roaming preference for each client, v3.

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

Neat :)
Attachment #8399759 - Flags: review?(htsai) → review+
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().
Attachment #8399762 - Flags: review?(htsai)
(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.
Attachment #8399762 - Attachment is obsolete: true
Attachment #8402446 - Flags: review?(htsai)
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.
Attachment #8402446 - Flags: review?(htsai)
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.
Attachment #8402446 - Attachment is obsolete: true
Attachment #8403183 - Flags: review?(htsai)
Sorry, forgot to add period at end of log message.
Attachment #8403183 - Attachment is obsolete: true
Attachment #8403183 - Flags: review?(htsai)
Attachment #8403185 - Flags: review?(htsai)
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 :)
Attachment #8403185 - Flags: review?(htsai) → review+
Add r=hsinyi.
Attachment #8399759 - Attachment is obsolete: true
Attachment #8403749 - Flags: review+
Add r=hsinyi.
Attachment #8403185 - Attachment is obsolete: true
Attachment #8403750 - Flags: review+
Rebased after bug 979134.
Attachment #8403750 - Attachment is obsolete: true
Attachment #8404387 - Flags: review+
Target Milestone: --- → 1.4 S5 (11apr)
Duplicate of this bug: 970192
blocking-b2g: 2.0? → ---
Whiteboard: [FT:RIL]
You need to log in before you can comment on or make changes to this bug.