If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in 1.4 S5 (11apr)

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: edgar, Assigned: jessica)

Tracking

(Blocks: 3 bugs)

unspecified
1.4 S5 (11apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

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.

Updated

4 years ago
Blocks: 960388
blocking-b2g: --- → 1.3?
Whiteboard: [FT:RIL]
(Assignee)

Comment 1

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

Comment 2

4 years ago
Change the setting interfaces for RIL.
Blocks: 959978
(Assignee)

Comment 3

4 years ago
Created attachment 8364254 [details] [diff] [review]
proposed patch, v1.

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)

Comment 6

4 years ago
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.

Comment 8

4 years ago
(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?
See https://bugzilla.mozilla.org/show_bug.cgi?id=960388#c14.
blocking-b2g: 1.3? → 1.4?

Updated

4 years ago
Flags: needinfo?(wmathanaraj)
Blocks: 799023
Flags: needinfo?(whuang)
Blocks: 942446

Comment 11

4 years ago
Jessica, would you like to take this bug?
Flags: needinfo?(jjong)
(Assignee)

Comment 12

4 years ago
Sure!
Assignee: nobody → jjong
Flags: needinfo?(jjong)
(Assignee)

Comment 13

4 years ago
Created attachment 8375981 [details] [diff] [review]
Part 1: separate roaming preference for each client, v2.

Rebased after Bug 905568.
Attachment #8364254 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Created attachment 8375982 [details] [diff] [review]
Part 2: add data connection tests for DSDS, v1.
(Assignee)

Comment 15

4 years ago
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)
(Assignee)

Updated

4 years ago
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)
No longer blocks: 942446
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)

Comment 20

4 years ago
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)

Updated

4 years ago
Flags: needinfo?(skamat)
(Assignee)

Comment 23

4 years ago
Created attachment 8399759 [details] [diff] [review]
Part 1: separate roaming preference for each client, v3.

- Use a simpler way to handle this.
Attachment #8375981 - Attachment is obsolete: true
(Assignee)

Comment 24

4 years ago
Created attachment 8399762 [details] [diff] [review]
Part 2: add data connection tests for DSDS, v2.
Attachment #8375982 - Attachment is obsolete: true
Attachment #8375982 - Flags: review?(htsai)
(Assignee)

Updated

4 years ago
Attachment #8399759 - Flags: review?(htsai)
(Assignee)

Comment 25

4 years ago
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)
(Assignee)

Comment 28

4 years ago
(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.
(Assignee)

Comment 30

4 years ago
Created attachment 8402446 [details] [diff] [review]
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.
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)
(Assignee)

Comment 32

4 years ago
Created attachment 8403183 [details] [diff] [review]
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.
Attachment #8402446 - Attachment is obsolete: true
Attachment #8403183 - Flags: review?(htsai)
(Assignee)

Comment 33

4 years ago
Created attachment 8403185 [details] [diff] [review]
Part 2: add data connection tests for DSDS, v5.

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+
(Assignee)

Comment 35

4 years ago
Created attachment 8403749 [details] [diff] [review]
Part 1: separate roaming preference for each client (final)

Add r=hsinyi.
Attachment #8399759 - Attachment is obsolete: true
Attachment #8403749 - Flags: review+
(Assignee)

Comment 36

4 years ago
Created attachment 8403750 [details] [diff] [review]
Part 2: add data connection tests for DSDS (final)

Add r=hsinyi.
Attachment #8403185 - Attachment is obsolete: true
Attachment #8403750 - Flags: review+
(Assignee)

Comment 37

4 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=cead9ef023a7
(Assignee)

Comment 38

4 years ago
Created attachment 8404387 [details] [diff] [review]
Part 2: add data connection tests for DSDS (final)

Rebased after bug 979134.
Attachment #8403750 - Attachment is obsolete: true
Attachment #8404387 - Flags: review+
(Assignee)

Comment 39

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

All green!
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Target Milestone: --- → 1.4 S5 (11apr)
https://hg.mozilla.org/integration/b2g-inbound/rev/59f927e60a53
https://hg.mozilla.org/integration/b2g-inbound/rev/6892cdd30dd7
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/59f927e60a53
https://hg.mozilla.org/mozilla-central/rev/6892cdd30dd7
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Duplicate of this bug: 970192

Updated

4 years ago
blocking-b2g: 2.0? → ---
Whiteboard: [FT:RIL]
You need to log in before you can comment on or make changes to this bug.