939046 - B2G RIL: Data call and RILNetworkInterface enhancement

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

There are some flaws in the existing design, which can be improved when we are refactoring this part. I will note down what I have in mind, and any more is welcome!

1. [bug] Properties of apn setting might be lost:
   RILNetworkInterface is shared among apn settings that have the same apn, username and password. However, if other properties differ, e.g. proxy, this info may not be passed to the RILNetworkInterface, due to the fact that if RIL finds that the apn can reuse a created RILNetworkInterface, only its type is concatenated to the apn setting types list.
  
2. [bug] Incorrect RILNetworkInterface.type:
   2.a. If a RILNetworkInterface is shared, for example by default and mms, and both are connected, the RILNetworkInterface.type returns default only cause it is the first found.
   2.b. When a "network-interface-state-changed" notification with DISCONNECTED/UNKNOWN state is fired, the RILNetworkInterface.type contained in the notification is wrong cause RILNetworkInterface.type looks for the type in RILNetworkInterface.connectedTypes, and the disconnected type is no longer in RILNetworkInterface.connectedTypes.

3. [enhancement] State transitions when deactivating data call:
   When disconnecting a data call, state transitions from DISCONNECTING to UNKNOWN. Should we pass through the DISCONNECTED state?

4. [enhancement] Handler for apn settings change:
   We should compare each of the apn setting and only reconnect data call if necessary.

Comment 1

[Jessica Jong [:jessica] (inactive)]

RILNetworkInterface implements nsIRilNetworkInterface which is extended from nsINetworkInterface.
nsINetworkInterface's has properties like type and state, that should be one of the NETWORK_TYPE_*/NETWORK_STATE_* constants.
Due to the nature of nsINetworkInterface, making RILNetworkInterface shareable is liable to error.

For this reason, the idea here is to have one RILNetworkInterface per apnType and introduce a new function 'DataCall' which can be shared among RILNetworkInterfaces.
DataCall represents a real data connection; it has profile information (apn, username, password, authtype, etc.) and link information (ip, gateway, netmask, etc.) if the data connection is connected.
RILNetworkInterface's properties: ip, gateway, name, etc., will refer to the corresponding DataCall's ip, gateway, name.

This is a rough idea of what I am working on, any comments or suggestions are welcome!

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: WIP

Working branch.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8362431 [details]
WIP

As stated in Comment 1 and as discussed before, I have changed for every apn type have a corresponding RILNetworkInterface, and introduce a DataCall object that can be shared among RILNetworkInterfaces. This way the state and type of RILNetworkInterface would be more precise. With these patches the bug mentioned in item 1 and item 2 are solved.

Just wanted to have your feedback to know whether this is going on the right direction. Thank you.

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 1: replace apnSettings with apnContexts map.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: let each apn type has its own RILNetworkInterface.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 3: introduce DataCall.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 4: share DataCall among RILNetworkInterfaces.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attaching git format patches for readability.
Note that the patches need to be rebased after Bug 905568, just wanted to have an early feedback on the idea here. Thank you.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 1: replace apnSettings with apnContexts map, v2.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: add apnType property to RILNetworkInterface, v2.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 3: introduce DataCall, v2.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 4: share DataCall among RILNetworkInterfaces, v2.

Comment 13

[Jessica Jong [:jessica] (inactive)]

Rebased patches (git) after Bug 905568.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Vicamo, Hsinyi and Edgar, still need your opinion on this...
Using ni? to avoid feedback flooding. Thank you!

Comment 15

[Ken Chang[:kenkai]]

Jessica is working on this.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 1: replace apnSettings with apnContexts map, v3.

Since our plan is to have one RILNetworkInterface per type, we won't need the byApn/byType in 'apnSettings'.
Instead we introduce 'apnContexts' map which I think it's clearer and easier to use. The key of the map is the apn type and the value is its ApnContext object, ApnContext is composed of 'apnSetting', 'iface' (RILNetworkInterface), and 'enabled'.

So this patch simply replaces apnSettings with apnContexts and each apn type will have its own RILNetworkInterface. We will handle data call sharing in the upcoming patches.

I am aware that there some performance issues regarding to 'for-of', but I haven't found a better way to iterate through a map.

Comment 17

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: add apnType property to RILNetworkInterface, v3.

This patch simply adds a 'apnType' property to RILNetworkInterface, now that each apn type has its own RILNetworkInterface.

Comment 18

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 3: introduce DataCall, v3.

Introduce DataCall object which represents a real underlying PDP.
DataCall has 'apnProfile' and 'linkInfo' porperties. 'apnProfile' contains all necessary information to setup a data call, and 'linkInfo'contains the link information after the data call has been established succesfully.

Each RILNetworkInterface will have reference to a DataCall when it's enabled. Sharing of DataCall is handled in the next patch.

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 4: share DataCall among RILNetworkInterfaces, v3.

When an apn type is enabled, search for existent DataCalls in all RILNetworkInterfaces, if an existent DataCall can handle the current apn type, reuse that DataCall, otherwise create a new DataCall.

When an apn type is disabled, if the DataCall is still in use, the apn type is removed from the DataCall's 'connectedTypes'; if the DataCall is not used by any other apn type, deactivate the data call and remove the apn type from 'connectedTypes' after it is actually disconnected, this is to know for what apn type to send the DISCONNECTED notifications.

Comment 20

[Edgar Chen [:edgar]]

Comment on attachment 8404474 [details] [diff] [review]
(WIP) Part 1: replace apnSettings with apnContexts map, v3.

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

ApnContext and RILNetworkInterface seems 1-1 mapping and both are one instance per type.
Could we combine them together?
1). Put all attribute of ApnContext into RILNetworkInterface.
2). Have a Map for RILNetworkInterface (use APN type as key) and replace |ApnContexts|.

Thank you.

Comment 21

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #20)
> Comment on attachment 8404474 [details] [diff] [review]
> (WIP) Part 1: replace apnSettings with apnContexts map, v3.
> 
> Review of attachment 8404474 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ApnContext and RILNetworkInterface seems 1-1 mapping and both are one
> instance per type.
> Could we combine them together?
> 1). Put all attribute of ApnContext into RILNetworkInterface.
> 2). Have a Map for RILNetworkInterface (use APN type as key) and replace
> |ApnContexts|.
> 
> Thank you.

You mean using RILNetworkInterface directly as the value of the map? Hmm.. I thought about that too, but then I though it would be more convenient to keep the attributes out of RILNetworkInterface.
I will reconsider about this, then we can get rid of ApnContext. Please do keep reviewing the other patches, thank you. :)

Comment 22

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

Comment on attachment 8404474 [details] [diff] [review]
(WIP) Part 1: replace apnSettings with apnContexts map, v3.

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

1) Why do we still need "RILNetworkInterface.inConnectedTypes" as this patch is making RILNetworkInterface per apnType?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1077,5 @@
>    clientId: 0,
>    radioInterface: null,
>    // Data calls setting.
>    dataCallSettings: null,
> +  apnContexts: null,

I think what DataConnectionHandler manages is "RILNetworkInterface." Sounds reasonable to me to hold ifaces here, instead of apnContexts, though iface and apnContext is 1-1 mapping. And we use 'apnType' as a key to access each iface.

Comment 23

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #22)
> Comment on attachment 8404474 [details] [diff] [review]
> (WIP) Part 1: replace apnSettings with apnContexts map, v3.
> 
> Review of attachment 8404474 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 1) Why do we still need "RILNetworkInterface.inConnectedTypes" as this patch
> is making RILNetworkInterface per apnType?

Oh, I saw you handled that in part3.

Comment 24

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

Comment on attachment 8404479 [details] [diff] [review]
(WIP) Part 3: introduce DataCall, v3.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1348,5 @@
>      this.setupDataCallByType("default");
>    },
>  
> +  notifyRILNetworkInterfaces: function (apnTypes) {
> +    for each (let apnType in apnTypes) {

for each is deprecated. Please don't use it.

Comment 25

[Edgar Chen [:edgar]]

Comment on attachment 8404487 [details] [diff] [review]
(WIP) Part 4: share DataCall among RILNetworkInterfaces, v3.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +4698,5 @@
>    NETWORK_TYPE_MOBILE:      Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE,
>    NETWORK_TYPE_MOBILE_MMS:  Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS,
>    NETWORK_TYPE_MOBILE_SUPL: Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL,
>    NETWORK_TYPE_MOBILE_IMS:  Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_IMS,
>    NETWORK_TYPE_MOBILE_DUN:  Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_DUN,

Please remove NETWORK_TYPE_* as well.

Comment 26

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

Comment on attachment 8404479 [details] [diff] [review]
(WIP) Part 3: introduce DataCall, v3.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1393,5 @@
> +
> +    let dataCall = networkIface.dataCall;
> +    if (!dataCall) {
> +      // TODO: find shareable data call here?
> +      apnContext.iface.dataCall = new DataCall(this, apnContext.apnSetting);

Per offline discussion, new DataCall when ConnectionHandler receives apnsettings.

@@ +4351,5 @@
>    }
>  };
>  
> +function DataCall(dataConnectionHandler, apnSetting) {
> +  this.dataConnectionHandler = dataConnectionHandler;

Per offline discussion, DataCall doesn't hold dataConnectionHandler. It holds related RILNetworkInterface instances.

Comment 27

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

Comment on attachment 8404487 [details] [diff] [review]
(WIP) Part 4: share DataCall among RILNetworkInterfaces, v3.

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

Please refactor the whole structure as we discussed before. Thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1392,5 @@
>      }
>  
>      let dataCall = networkIface.dataCall;
>      if (!dataCall) {
> +      for (let [apnType, apnCtx] of this.apnContexts) {

[1] shows for-of performance issue is still valid. Let's try our best to avoid that. Thank you.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=969231#c9

Comment 28

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

Comment on attachment 8404477 [details] [diff] [review]
(WIP) Part 2: add apnType property to RILNetworkInterface, v3.

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

I'd like to see after this is revised based on my comment for part1.

Comment 29

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 1: replace apnSettings with dataNetworkInterfaces map, v4.

Changes since v3:
  - Get rid 'ApnContext' so now the map holds 'RILNetworkInterface' directly.

Comment 30

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: add apnType property to RILNetworkInterface, v4.

Rebased, no changes since v2.

This patch simply adds a 'apnType' property to RILNetworkInterface, now that each apn type has its own RILNetworkInterface.

Comment 31

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: add apnType property to RILNetworkInterface, v5.

Changes since v4:
  - Missed apn types 'ims' and 'dun' in v4. :(

Comment 32

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 3: introduce DataCall, v4.

Introduce DataCall object which represents a real underlying PDP.
When RILNetworkInterface is created, a DataCall is assigned to it. DataCall can be shared between RILNetworkInterfaces if they consist of the same apn, username and password.

When a RILNetworkInterface is enabled, it requests its DataCall to be connected; the DataCall, which can be shared, will establish the data call if it's not already connected. Similarly, DataCall will disconnect the data call only when no RILNetworkInterface is requesting it.

Changes since v3:
  - RILNetworkInterface is assigned a DataCall at the moment it is created.
  - DataCall hold references of RILNetworkIntefaces that requested it.
  - DataCall hold no reference of DataConnectionHandler; when setting up/deactivating a data call, it calls RadioInterface directly.
  - Remove the use of register/unregister DataCallCallback, DataConnectionHandler will notify corresponding DataCalls directly.

Comment 33

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 4: avoid the use of for-of, v1.

Avoid the use of 'for-of' wherever possible.

Comment 34

[Edgar Chen [:edgar]]

Comment on attachment 8408005 [details] [diff] [review]
(WIP) Part 1: replace apnSettings with dataNetworkInterfaces map, v4.

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

Please see my below comments, thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1334,3 @@
>         return RIL.GECKO_NETWORK_STATE_UNKNOWN;
>      }
> +    if (!networkInterface.inConnectedTypes(apnType)) {

In this patch, we already create RILNetworkInterface per APN Type, I think we don't need to maintain |connectedTypes| any more (To remove it, I guess you probably need to merge this part with part2).

@@ +1380,2 @@
>        }
> +      gNetworkManager.registerNetworkInterface(networkInterface);

Now each RILNetworkInterface represents only one APN Type, do we still need to re-register?

@@ +1405,5 @@
>      // are sure that other data call types still need this connection of this
>      // interface. In this circumstance, we have to directly update the
>      // necessary data call and interface information to RILContentHelper
>      // and network manager for current data call type.
> +    if (networkInterface.connectedTypes.length && networkInterface.connected) {

Ditto.

@@ +1421,2 @@
>        }
> +      gNetworkManager.registerNetworkInterface(networkInterface);

Now each RILNetworkInterface represents only one APN Type, do we still need to re-register?

@@ +1474,3 @@
>      if (apnSetting) {
>        if (message.apn == apnSetting.apn &&
> +          networkInterface.inConnectedTypes("default")) {

Ditto, don't need to maintain |connectedTypes|.

@@ +1492,5 @@
>      let dataCallConnected =
>          (datacall.state == RIL.GECKO_NETWORK_STATE_CONNECTED);
>      if (defaultApnSetting && datacall.ifname) {
>        if (dataCallConnected && datacall.apn == defaultApnSetting.apn &&
> +          defaultNetworkIface.inConnectedTypes("default")) {

Ditto. (In fact, these dataInfo related code will be removed in bug 995109 :))

@@ +2432,2 @@
>      dataInfo.connected = false;
> +    if (defaultNetworkIface) {

After bug 995109 landed, you don't need to handle this. :)

Comment 35

[Edgar Chen [:edgar]]

Comment on attachment 8408014 [details] [diff] [review]
(WIP) Part 2: add apnType property to RILNetworkInterface, v5.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +4318,2 @@
>    this.apnSetting = apnSetting;
>    this.connectedTypes = [];

don't need to maintain |connectedTypes|.

@@ +4371,5 @@
>  
>    get type() {
> +    switch(this.apnType) {
> +      case "default":
> +        return this.NETWORK_TYPE_MOBILE;

Use Ci.nsINetworkInterface.NETWORK_TYPE* and remove those duplicated const, NETWORK_TYPE_*.

@@ +4381,5 @@
> +        return this.NETWORK_TYPE_MOBILE_IMS;
> +      case "dun":
> +        return this.NETWORK_TYPE_MOBILE_DUN;
> +      default:
> +        return this.NETWORK_TYPE_MOBILE_OTHERS;

I don't like handling an unknown type in this way. IMO, we shouldn't even create a RILNetworkInterface for an unknown type.

I am thinking about another thing: get rid of these predefined number const and use string for type directly. It seems more flexible to add a new type in this way. We could discuss about this later, just share my thoughts first. :)

@@ +4631,5 @@
>      return this.connectedTypes.indexOf(type) != -1;
>    },
>  
>    get connected() {
>      return this.state == RIL.GECKO_NETWORK_STATE_CONNECTED;

Use Ci.nsINetworkInterface.NETWORK_STATE_* for state in NetworkInterface.

@@ +4636,5 @@
>    },
>  
> +  connect: function() {
> +    if (this.apnType && !this.inConnectedTypes(this.apnType)) {
> +      this.connectedTypes.push(this.apnType);

don't need to maintain |connectedTypes|.

@@ +4728,5 @@
>                                  Ci.nsITimer.TYPE_ONE_SHOT);
>    },
>  
> +  disconnect: function() {
> +    let index = this.connectedTypes.indexOf(this.apntype);

ditto.

Comment 36

[Edgar Chen [:edgar]]

Comment on attachment 8408034 [details] [diff] [review]
(WIP) Part 3: introduce DataCall, v4.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1062,5 @@
>    radioInterface: null,
>    // Data calls setting.
>    dataCallSettings: null,
>    dataNetworkInterfaces: null,
> +  dataCalls: null,

s/dataCalls/_dataCalls

@@ +1446,5 @@
>      let networkInterface = this.dataNetworkInterfaces.get("default");
>      let apnSetting = networkInterface && networkInterface.apnSetting;
>      if (apnSetting) {
>        if (message.apn == apnSetting.apn &&
> +          networkInterface.enabled) {

if (apnSetting &&
    apnSetting.apn == message.apn &&
    networkInterface.enabled) {
...
}

@@ +4334,3 @@
>  
>    dataCallError: function(message) {
> +    if (message.apn != this.apnProfile.apn) {

Don't need this apn checking, We already do it in |_deliverDataCallMessage|.

@@ +4371,5 @@
>      }
>      // In current design, we don't update status of secondary APN if it shares
>      // same APN name with the default APN.  In this condition, this.cid will
>      // not be set and we don't want to update its status.
> +    if (this.linkInfo.cid == null) {

According to above comments, this check seems used for APN sharing.
Do we still need this in DataCall?

@@ +4479,5 @@
> +    if (DEBUG) {
> +      if (networkInterface !== undefined) {
> +        this.debug("connect: " + networkInterface.apnType);
> +      } else {
> +        this.debug("retrying data call for: " + this.apnProfile.apn);

Hmm, is it possible just executing |setupDataCall| without calling connect() when retrying?

@@ +4497,5 @@
>      }
>  
>      // When the retry mechanism is running in background and someone calls
> +    // disconnect(), this.requestedNetworkIfaces.length has chances to become 0.
> +    if (!this.requestedNetworkIfaces.length) {

Don't need this check. When someone calls disconnect(), we should stop retry mechanism and also cancel the timer.

@@ +4556,5 @@
>      // based on the function: time = A * numer_of_retries^2 + B
>      if (this.apnRetryCounter >= this.NETWORK_APNRETRY_MAXRETRIES) {
>        this.apnRetryCounter = 0;
>        this.timer = null;
> +      this.requestedNetworkIfaces = [];

Why clear |requestedNetworkIfaces| here?

@@ +4636,5 @@
> +  }
> +
> +  this.dataConnectionHandler = dataConnectionHandler;
> +  this.apnType = type;
> +  this.apnSetting = apnSetting;

Does RILNetworkInterface still need apnSetting?

@@ +4654,5 @@
> +
> +  // Hold reference to DataCall object which is determined at initilization.
> +  dataCall: null,
> +
> +  // TODO: should this be ref-counted?

The ref-counter for connection request should be handled in NetworkManager.

@@ +4662,5 @@
> +   * nsINetworkInterface Implementation
> +   */
> +
> +  get state() {
> +    if (!this.dataCall.inConnectedTypes(this.apnType)) {

I guess you what to check this type's connection is requested or not. How about?

if (!this.enabled) {
 ...
}

And we don't need |inConnectedTypes|.

@@ +4697,5 @@
> +  get httpProxyPort() {
> +    return this.apnSetting.port || "";
> +  },
> +
> +  getAddresses: function (ips, prefixLengths) {

nit: no space between function and (

@@ +4706,5 @@
> +
> +    return linkInfo.ips.length;
> +  },
> +
> +  getGateways: function (count) {

Ditto.

@@ +4715,5 @@
> +    }
> +    return linkInfo.gateways.slice();
> +  },
> +
> +  getDnses: function (count) {

Ditto.

Comment 37

[Jessica Jong [:jessica] (inactive)]

Thanks for the feedback!

(In reply to Edgar Chen [:edgar][:echen] from comment #34)
> Comment on attachment 8408005 [details] [diff] [review]
> (WIP) Part 1: replace apnSettings with dataNetworkInterfaces map, v4.
> 
> Review of attachment 8408005 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my below comments, thank you.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1334,3 @@
> >         return RIL.GECKO_NETWORK_STATE_UNKNOWN;
> >      }
> > +    if (!networkInterface.inConnectedTypes(apnType)) {
> 
> In this patch, we already create RILNetworkInterface per APN Type, I think
> we don't need to maintain |connectedTypes| any more (To remove it, I guess
> you probably need to merge this part with part2).

Actually, connectedTypes in RILNetworkInterface is removed in Part 3 (attachment 8408034 [details] [diff] [review]).

> 
> @@ +1380,2 @@
> >        }
> > +      gNetworkManager.registerNetworkInterface(networkInterface);
> 
> Now each RILNetworkInterface represents only one APN Type, do we still need
> to re-register?

This is also removed in Part 3 (attachment 8408034 [details] [diff] [review]).

> 
> @@ +1405,5 @@
> >      // are sure that other data call types still need this connection of this
> >      // interface. In this circumstance, we have to directly update the
> >      // necessary data call and interface information to RILContentHelper
> >      // and network manager for current data call type.
> > +    if (networkInterface.connectedTypes.length && networkInterface.connected) {
> 
> Ditto.

We will use DataCall connectedTypes instead in Part 3 (attachment 8408034 [details] [diff] [review]).

> 
> @@ +1421,2 @@
> >        }
> > +      gNetworkManager.registerNetworkInterface(networkInterface);
> 
> Now each RILNetworkInterface represents only one APN Type, do we still need
> to re-register?

This is also removed in Part 3 (attachment 8408034 [details] [diff] [review]).

> 
> @@ +1474,3 @@
> >      if (apnSetting) {
> >        if (message.apn == apnSetting.apn &&
> > +          networkInterface.inConnectedTypes("default")) {
> 
> Ditto, don't need to maintain |connectedTypes|.

Will use networkInterface.enabled in Part 3 (attachment 8408034 [details] [diff] [review]).

> 
> @@ +1492,5 @@
> >      let dataCallConnected =
> >          (datacall.state == RIL.GECKO_NETWORK_STATE_CONNECTED);
> >      if (defaultApnSetting && datacall.ifname) {
> >        if (dataCallConnected && datacall.apn == defaultApnSetting.apn &&
> > +          defaultNetworkIface.inConnectedTypes("default")) {
> 
> Ditto. (In fact, these dataInfo related code will be removed in bug 995109
> :))

I see, will mark with a TODO comment. Thank you.

> 
> @@ +2432,2 @@
> >      dataInfo.connected = false;
> > +    if (defaultNetworkIface) {
> 
> After bug 995109 landed, you don't need to handle this. :)

I see, will mark with a TODO comment. Thank you.


Mmm.. most of the fixes are done in Part 3, where DataCall is introduced. I think the way I split the patches is not what reviewer expects. I'll see how to split the patches better. Thank you.

Comment 38

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #35)
> Comment on attachment 8408014 [details] [diff] [review]
> (WIP) Part 2: add apnType property to RILNetworkInterface, v5.
> 
> Review of attachment 8408014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +4318,2 @@
> >    this.apnSetting = apnSetting;
> >    this.connectedTypes = [];
> 
> don't need to maintain |connectedTypes|.

This is removed in Part 3 (attachment 8408034 [details] [diff] [review]).

> 
> @@ +4371,5 @@
> >  
> >    get type() {
> > +    switch(this.apnType) {
> > +      case "default":
> > +        return this.NETWORK_TYPE_MOBILE;
> 
> Use Ci.nsINetworkInterface.NETWORK_TYPE* and remove those duplicated const,
> NETWORK_TYPE_*.
> 
> @@ +4381,5 @@
> > +        return this.NETWORK_TYPE_MOBILE_IMS;
> > +      case "dun":
> > +        return this.NETWORK_TYPE_MOBILE_DUN;
> > +      default:
> > +        return this.NETWORK_TYPE_MOBILE_OTHERS;
> 
> I don't like handling an unknown type in this way. IMO, we shouldn't even
> create a RILNetworkInterface for an unknown type.
> 
> I am thinking about another thing: get rid of these predefined number const
> and use string for type directly. It seems more flexible to add a new type
> in this way. We could discuss about this later, just share my thoughts
> first. :)

These predefine consts are removed in Part 3 (attachment 8408034 [details] [diff] [review]), but I will handle the unknown network type in the next version, as discussed offline.

> 
> @@ +4631,5 @@
> >      return this.connectedTypes.indexOf(type) != -1;
> >    },
> >  
> >    get connected() {
> >      return this.state == RIL.GECKO_NETWORK_STATE_CONNECTED;
> 
> Use Ci.nsINetworkInterface.NETWORK_STATE_* for state in NetworkInterface.

Will do.

> 
> @@ +4636,5 @@
> >    },
> >  
> > +  connect: function() {
> > +    if (this.apnType && !this.inConnectedTypes(this.apnType)) {
> > +      this.connectedTypes.push(this.apnType);
> 
> don't need to maintain |connectedTypes|.


This is removed in Part 3 (attachment 8408034 [details] [diff] [review]).

> 
> @@ +4728,5 @@
> >                                  Ci.nsITimer.TYPE_ONE_SHOT);
> >    },
> >  
> > +  disconnect: function() {
> > +    let index = this.connectedTypes.indexOf(this.apntype);
> 
> ditto.

This is removed in Part 3 (attachment 8408034 [details] [diff] [review]).


As discussed offline, I will merge part 1 and part 2, then removed connectedTypes and handling for shared apn in RILNetworkInterface in the same patch. Thank you.

Comment 39

[Edgar Chen [:edgar]]

Comment on attachment 8408034 [details] [diff] [review]
(WIP) Part 3: introduce DataCall, v4.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +4329,5 @@
> +
> +  // Array to hold RILNetworkInterfaces that requested this DataCall.
> +  requestedNetworkIfaces: null,
> +
> +  connecting: false,

Seem we don't need this flag.

@@ +4355,5 @@
>      if (DEBUG) {
>        this.debug("Data call ID: " + datacall.cid + ", interface name: " +
>                   datacall.ifname + ", APN name: " + datacall.apn);
>      }
>      if (this.connecting &&

Don't need to check |connecting|.

@@ +4490,2 @@
>  
>      if (this.connecting || this.connected) {

Use |requestedNetworkIfaces.length| as ref-counter to decide should do "setupDataCall" or just notfiyRILNetworkInterface().

@@ +4802,5 @@
> +  apnType: null,
> +  apnSetting: null,
> +
> +  get connecting() {
> +    return this.dataCall.connecting;

Seems no one use this getter any more, we could remove it.

Comment 40

[Jessica Jong [:jessica] (inactive)]

Thanks for the feedback!

(In reply to Edgar Chen [:edgar][:echen] from comment #36)
> Comment on attachment 8408034 [details] [diff] [review]
> (WIP) Part 3: introduce DataCall, v4.
> 
> Review of attachment 8408034 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1062,5 @@
> >    radioInterface: null,
> >    // Data calls setting.
> >    dataCallSettings: null,
> >    dataNetworkInterfaces: null,
> > +  dataCalls: null,
> 
> s/dataCalls/_dataCalls

Will do, thank you.

> 
> @@ +1446,5 @@
> >      let networkInterface = this.dataNetworkInterfaces.get("default");
> >      let apnSetting = networkInterface && networkInterface.apnSetting;
> >      if (apnSetting) {
> >        if (message.apn == apnSetting.apn &&
> > +          networkInterface.enabled) {
> 
> if (apnSetting &&
>     apnSetting.apn == message.apn &&
>     networkInterface.enabled) {
> ...
> }

Will do, thank you.

> 
> @@ +4334,3 @@
> >  
> >    dataCallError: function(message) {
> > +    if (message.apn != this.apnProfile.apn) {
> 
> Don't need this apn checking, We already do it in |_deliverDataCallMessage|.

Will remove this additional check, thank you.

> 
> @@ +4371,5 @@
> >      }
> >      // In current design, we don't update status of secondary APN if it shares
> >      // same APN name with the default APN.  In this condition, this.cid will
> >      // not be set and we don't want to update its status.
> > +    if (this.linkInfo.cid == null) {
> 
> According to above comments, this check seems used for APN sharing.
> Do we still need this in DataCall?

I can not really tell what this was for but I think we don't need it anymore, will remove it in the next version, thank you.

> 
> @@ +4479,5 @@
> > +    if (DEBUG) {
> > +      if (networkInterface !== undefined) {
> > +        this.debug("connect: " + networkInterface.apnType);
> > +      } else {
> > +        this.debug("retrying data call for: " + this.apnProfile.apn);
> 
> Hmm, is it possible just executing |setupDataCall| without calling connect()
> when retrying?

Mmm, we could add a function for setupDataCall and use this function in connect and retry, since both do similar things. Is that okay?

> 
> @@ +4497,5 @@
> >      }
> >  
> >      // When the retry mechanism is running in background and someone calls
> > +    // disconnect(), this.requestedNetworkIfaces.length has chances to become 0.
> > +    if (!this.requestedNetworkIfaces.length) {
> 
> Don't need this check. When someone calls disconnect(), we should stop retry
> mechanism and also cancel the timer.

Yes, we can remove this additional check, thank you.

> 
> @@ +4556,5 @@
> >      // based on the function: time = A * numer_of_retries^2 + B
> >      if (this.apnRetryCounter >= this.NETWORK_APNRETRY_MAXRETRIES) {
> >        this.apnRetryCounter = 0;
> >        this.timer = null;
> > +      this.requestedNetworkIfaces = [];
> 
> Why clear |requestedNetworkIfaces| here?

When we stop retrying, shouldn't we clear everything? or just leave as it is?

> 
> @@ +4636,5 @@
> > +  }
> > +
> > +  this.dataConnectionHandler = dataConnectionHandler;
> > +  this.apnType = type;
> > +  this.apnSetting = apnSetting;
> 
> Does RILNetworkInterface still need apnSetting?

I think RILNetworkInterface still needs apnSetting for mmsc, mms proxy/port, http proxy/port.

> 
> @@ +4654,5 @@
> > +
> > +  // Hold reference to DataCall object which is determined at initilization.
> > +  dataCall: null,
> > +
> > +  // TODO: should this be ref-counted?
> 
> The ref-counter for connection request should be handled in NetworkManager.

Okay, let's leave it to NetworkManager.

> 
> @@ +4662,5 @@
> > +   * nsINetworkInterface Implementation
> > +   */
> > +
> > +  get state() {
> > +    if (!this.dataCall.inConnectedTypes(this.apnType)) {
> 
> I guess you what to check this type's connection is requested or not. How
> about?
> 
> if (!this.enabled) {
>  ...
> }
> 
> And we don't need |inConnectedTypes|.


Yes, this will do as well. Since we are getting state from DataCall, I just though it would make more sense to check if the current RILNetworkInterface is in DataCall's requestedNetworkTypes or not. But if this is the only place that needs inConnectedTypes(), maybe we should use your way. Thank you.

> 
> @@ +4697,5 @@
> > +  get httpProxyPort() {
> > +    return this.apnSetting.port || "";
> > +  },
> > +
> > +  getAddresses: function (ips, prefixLengths) {
> 
> nit: no space between function and (

Will do, thank you.

> 
> @@ +4706,5 @@
> > +
> > +    return linkInfo.ips.length;
> > +  },
> > +
> > +  getGateways: function (count) {
> 
> Ditto.

Will do, thank you.

> 
> @@ +4715,5 @@
> > +    }
> > +    return linkInfo.gateways.slice();
> > +  },
> > +
> > +  getDnses: function (count) {
> 
> Ditto.

Will do, thank you.

Comment 41

[Jessica Jong [:jessica] (inactive)]

Thanks for the feedback!

(In reply to Edgar Chen [:edgar][:echen] from comment #39)
> Comment on attachment 8408034 [details] [diff] [review]
> (WIP) Part 3: introduce DataCall, v4.
> 
> Review of attachment 8408034 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +4329,5 @@
> > +
> > +  // Array to hold RILNetworkInterfaces that requested this DataCall.
> > +  requestedNetworkIfaces: null,
> > +
> > +  connecting: false,
> 
> Seem we don't need this flag.
> 
> @@ +4355,5 @@
> >      if (DEBUG) {
> >        this.debug("Data call ID: " + datacall.cid + ", interface name: " +
> >                   datacall.ifname + ", APN name: " + datacall.apn);
> >      }
> >      if (this.connecting &&
> 
> Don't need to check |connecting|.

I think we do need it, we would want it to enter this block only the first time state becomes connected. For the following connected events it will need to check what configurations have changed before notifying.

> 
> @@ +4490,2 @@
> >  
> >      if (this.connecting || this.connected) {
> 
> Use |requestedNetworkIfaces.length| as ref-counter to decide should do
> "setupDataCall" or just notfiyRILNetworkInterface().

The problem is that we are not notified of the CONNECTING state, so if we notfiyRILNetworkInterface() when it's actually connecting, the notified state would be UNKNOWN, which is not right. This leads to another discussion that we had before: should we set the state to CONNECTING directly when connect() is called?

> 
> @@ +4802,5 @@
> > +  apnType: null,
> > +  apnSetting: null,
> > +
> > +  get connecting() {
> > +    return this.dataCall.connecting;
> 
> Seems no one use this getter any more, we could remove it.

This is not needed, will remove it in the next version, thank you.

Comment 42

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #40)
> Thanks for the feedback!
> 
> (In reply to Edgar Chen [:edgar][:echen] from comment #36)
> > Comment on attachment 8408034 [details] [diff] [review]
> > (WIP) Part 3: introduce DataCall, v4.
> > 
> > Review of attachment 8408034 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +4479,5 @@
> > > +    if (DEBUG) {
> > > +      if (networkInterface !== undefined) {
> > > +        this.debug("connect: " + networkInterface.apnType);
> > > +      } else {
> > > +        this.debug("retrying data call for: " + this.apnProfile.apn);
> > 
> > Hmm, is it possible just executing |setupDataCall| without calling connect()
> > when retrying?
> 
> Mmm, we could add a function for setupDataCall and use this function in
> connect and retry, since both do similar things. Is that okay?

Even better, thank you. :)

Comment 43

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #40)
> Thanks for the feedback!
> 
> (In reply to Edgar Chen [:edgar][:echen] from comment #36)
> > Comment on attachment 8408034 [details] [diff] [review]
> > (WIP) Part 3: introduce DataCall, v4.
> > 
> > Review of attachment 8408034 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +1062,5 @@
> > >    radioInterface: null,
> > >    // Data calls setting.
> > >    dataCallSettings: null,
> > >    dataNetworkInterfaces: null,
> > > +  dataCalls: null,
> > 
> > s/dataCalls/_dataCalls
> 
> Will do, thank you.
> 
> > 
> > @@ +1446,5 @@
> > >      let networkInterface = this.dataNetworkInterfaces.get("default");
> > >      let apnSetting = networkInterface && networkInterface.apnSetting;
> > >      if (apnSetting) {
> > >        if (message.apn == apnSetting.apn &&
> > > +          networkInterface.enabled) {
> > 
> > if (apnSetting &&
> >     apnSetting.apn == message.apn &&
> >     networkInterface.enabled) {
> > ...
> > }
> 
> Will do, thank you.
> 
> > 
> > @@ +4334,3 @@
> > >  
> > >    dataCallError: function(message) {
> > > +    if (message.apn != this.apnProfile.apn) {
> > 
> > Don't need this apn checking, We already do it in |_deliverDataCallMessage|.
> 
> Will remove this additional check, thank you.
> 
> > 
> > @@ +4371,5 @@
> > >      }
> > >      // In current design, we don't update status of secondary APN if it shares
> > >      // same APN name with the default APN.  In this condition, this.cid will
> > >      // not be set and we don't want to update its status.
> > > +    if (this.linkInfo.cid == null) {
> > 
> > According to above comments, this check seems used for APN sharing.
> > Do we still need this in DataCall?
> 
> I can not really tell what this was for but I think we don't need it
> anymore, will remove it in the next version, thank you.
> 
> > 
> > @@ +4479,5 @@
> > > +    if (DEBUG) {
> > > +      if (networkInterface !== undefined) {
> > > +        this.debug("connect: " + networkInterface.apnType);
> > > +      } else {
> > > +        this.debug("retrying data call for: " + this.apnProfile.apn);
> > 
> > Hmm, is it possible just executing |setupDataCall| without calling connect()
> > when retrying?
> 
> Mmm, we could add a function for setupDataCall and use this function in
> connect and retry, since both do similar things. Is that okay?
> 
> > 
> > @@ +4497,5 @@
> > >      }
> > >  
> > >      // When the retry mechanism is running in background and someone calls
> > > +    // disconnect(), this.requestedNetworkIfaces.length has chances to become 0.
> > > +    if (!this.requestedNetworkIfaces.length) {
> > 
> > Don't need this check. When someone calls disconnect(), we should stop retry
> > mechanism and also cancel the timer.
> 
> Yes, we can remove this additional check, thank you.
> 
> > 
> > @@ +4556,5 @@
> > >      // based on the function: time = A * numer_of_retries^2 + B
> > >      if (this.apnRetryCounter >= this.NETWORK_APNRETRY_MAXRETRIES) {
> > >        this.apnRetryCounter = 0;
> > >        this.timer = null;
> > > +      this.requestedNetworkIfaces = [];
> > 
> > Why clear |requestedNetworkIfaces| here?
> 
> When we stop retrying, shouldn't we clear everything? or just leave as it is?
> 
> > 
> > @@ +4636,5 @@
> > > +  }
> > > +
> > > +  this.dataConnectionHandler = dataConnectionHandler;
> > > +  this.apnType = type;
> > > +  this.apnSetting = apnSetting;
> > 
> > Does RILNetworkInterface still need apnSetting?
> 
> I think RILNetworkInterface still needs apnSetting for mmsc, mms proxy/port,
> http proxy/port.
> 
> > 
> > @@ +4654,5 @@
> > > +
> > > +  // Hold reference to DataCall object which is determined at initilization.
> > > +  dataCall: null,
> > > +
> > > +  // TODO: should this be ref-counted?
> > 
> > The ref-counter for connection request should be handled in NetworkManager.
> 
> Okay, let's leave it to NetworkManager.
> 
> > 
> > @@ +4662,5 @@
> > > +   * nsINetworkInterface Implementation
> > > +   */
> > > +
> > > +  get state() {
> > > +    if (!this.dataCall.inConnectedTypes(this.apnType)) {
> > 
> > I guess you what to check this type's connection is requested or not. How
> > about?
> > 
> > if (!this.enabled) {
> >  ...
> > }
> > 
> > And we don't need |inConnectedTypes|.
> 
> 
> Yes, this will do as well. Since we are getting state from DataCall, I just
> though it would make more sense to check if the current RILNetworkInterface
> is in DataCall's requestedNetworkTypes or not. But if this is the only place
> that needs inConnectedTypes(), maybe we should use your way. Thank you.

I just found out that we can't use 'if (!this.enabled) { ... }', cause 'this.enabled' becomes false as soon as someone calls 'deactivateDataCallByType()', so it would not be able to reflect DataCall's DISCONNECTING -> DISCONNECTED state. I think I will still need to use 'inConnectedTypes()'...

> 
> > 
> > @@ +4697,5 @@
> > > +  get httpProxyPort() {
> > > +    return this.apnSetting.port || "";
> > > +  },
> > > +
> > > +  getAddresses: function (ips, prefixLengths) {
> > 
> > nit: no space between function and (
> 
> Will do, thank you.
> 
> > 
> > @@ +4706,5 @@
> > > +
> > > +    return linkInfo.ips.length;
> > > +  },
> > > +
> > > +  getGateways: function (count) {
> > 
> > Ditto.
> 
> Will do, thank you.
> 
> > 
> > @@ +4715,5 @@
> > > +    }
> > > +    return linkInfo.gateways.slice();
> > > +  },
> > > +
> > > +  getDnses: function (count) {
> > 
> > Ditto.
> 
> Will do, thank you.

Comment 44

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: map each apn type with one RILNetworkInterface, v5.

Changes since v4:
  - Based on bug 995109
  - Merge part 1 (v4) and part 2 (v5)
  - Remove connectedTypes in RILNetworkInterface in this patch
  - Check for apn type validity when creating RILNetworkInterface
  - Get rid if 'apnType' and use 'type' in RILNetworkInterface

Comment 45

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Introduce DataCall, v5.

Changes since v4:
  - s/dataCalls/_dataCalls
  - remove unnecessary checks mentioned in comment 36
  - add setupDataCall() function for connect and retry

Comment 46

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: avoid the use of for-of, v2.

Rebased only.

Comment 47

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8408928 [details] [diff] [review]
Part 1: map each apn type with one RILNetworkInterface, v5.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ -4659,5 @@
> -    // disconnect(), this.connectedTypes.length has chances to become 0.
> -    if (!this.connectedTypes.length) {
> -      return;
> -    }
> -

Oh, this is supposed to be removed in part 2, where we cancel the timer when some calls disconnect(). Will reorganize in the next version.

Comment 48

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

Comment on attachment 8408928 [details] [diff] [review]
Part 1: map each apn type with one RILNetworkInterface, v5.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1237,5 @@
>    },
>  
>    updateRILNetworkInterface: function() {
> +    let networkInterface = this.dataNetworkInterfaces.get("default");
> +    if (!(networkInterface && networkInterface.apnSetting &&

nit: !networkInterface || !networkInterface.apnSetting || !this._validateApnSetting()

@@ +1358,1 @@
>         return RIL.GECKO_NETWORK_STATE_UNKNOWN;

nit: indention

@@ +1371,4 @@
>        }
>        return;
>      }
> +    networkInterface.enabled = true;

What's the difference b/w networkInterface.enabled and networkInterface.connecting? They look quite similar to me, to indicating if a connect request is asked.

Also, I'd like to reply to your comment 41: should we set the state to CONNECTING directly when connect() is called? 

I think we could and we should, as this.connecting seems indicate a transient state which is what CONNECTING represents. And, we don't need to maintain these variables which point to the same thing.

@@ +1393,4 @@
>        }
>        return;
>      }
> +    networkInterface.enabled = false;

Should we check |if (networkInterface.enabled)| before disconnect()? That said, when does the situation happen that we are able to disconnect a iface which is never enabled?

@@ +1444,5 @@
> +    let apnSetting = networkInterface && networkInterface.apnSetting;
> +    if (apnSetting && message.apn == apnSetting.apn &&
> +        networkInterface.enabled) {
> +      gMessageManager.sendMobileConnectionMessage("RIL:DataError",
> +                                                  this.clientId, message);

I'd like to rewrite this a little bit as below and rest networkInterface.enabled to false.

if (networkInterface.enabled) {
  networkInterface.enabled = false;
  let apnSetting = networkInterface && networkInterface.apnSetting;
  if (apnSetting && message.apn == apnSetting.apn) {
    gMessageManager.sendMobileConnection....
  }
}

@@ +4641,5 @@
>                                  Ci.nsITimer.TYPE_ONE_SHOT);
>    },
>  
> +  disconnect: function() {
> +    if (this.state == Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTING ||

Similar to connect(). We could think about moving this.state to DISCONNECTING in disconnect().

Comment 49

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

Comment on attachment 8408931 [details] [diff] [review]
Part 2: Introduce DataCall, v5.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1116,5 @@
> +    for (let i = 0; i < this._dataCalls.length; i++) {
> +      let datacall = this._dataCalls[i];
> +      // Send message only to the corresponding DataCall.
> +      // Currently, args always contain only one datacall info.
> +      if (args[0].apn && args[0].apn == datacall.apnProfile.apn) {

bail-out earlier:
if (!args[0].apn || args[0].apn != datacall.apnProfile.apn) {
  continue;
}

@@ +4435,5 @@
> +            (this.apnProfile.user || '') == (apnSetting.user || '') &&
> +            (this.apnProfile.password || '') == (apnSetting.password || ''));
> +  },
> +
> +  resetDataCall: function() {

We now have DataCall.resetDataCall and DataCall.reset ... Is it better to rename DataCall.reset DataCall.retry and s/resetDataCall/reset/ ?

@@ +4447,5 @@
> +    this.state = Ci.nsINetworkInterface.NETWORK_STATE_UNKNOWN;
> +    this.requestedNetworkIfaces = [];
> +  },
> +
> +  setupDataCall: function() {

nit: s/setupDataCall/set/ since this is a property of DataCall?

@@ +4546,5 @@
> +  disconnect: function(networkInterface) {
> +    if (DEBUG) this.debug("disconnect: " + networkInterface.type);
> +    let index = this.requestedNetworkIfaces.indexOf(networkInterface);
> +    if (index == -1) {
> +      return

; is missing

Comment 50

[Edgar Chen [:edgar]]

Comment on attachment 8408928 [details] [diff] [review]
Part 1: map each apn type with one RILNetworkInterface, v5.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1238,5 @@
>  
>    updateRILNetworkInterface: function() {
> +    let networkInterface = this.dataNetworkInterfaces.get("default");
> +    if (!(networkInterface && networkInterface.apnSetting &&
> +          this._validateApnSetting(networkInterface.apnSetting))) {

We had already checked the apnSetting is validate or not in _setupApnSettings(). If the apn setting is not valid, there is no networkInterface will be created. Hence, checking |networkInterface| seems enough?

@@ +4534,5 @@
>      }
>  
>      // In case the data setting changed while the datacall was being started or
>      // ended, let's re-check the setting and potentially adjust the datacall
>      // state again.

I guess this is for the case that using separated setting key for different apn type.
Now we use one setting key for all apn, do we still need to handle this scenario?

Comment 51

[Edgar Chen [:edgar]]

Comment on attachment 8408931 [details] [diff] [review]
Part 2: Introduce DataCall, v5.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1176,5 @@
>  
> +        if (DEBUG) this.debug("Preparing RILNetworkInterface for type: " + apnType);
> +        // Create DataCall for RILNetworkInterface or reuse one that is shareable.
> +        let dataCall;
> +        for (let [type, networkInterface] of this.dataNetworkInterfaces) {

How about iterating |_dataCalls|?

@@ +4318,5 @@
>      this.reset();
>    },
>  
>    dataCallStateChanged: function(datacall) {
> +    if (this.linkInfo.cid && this.linkInfo.cid != datacall.cid) {

I would say we should make sure the event is dispatched to correct DataCall instance in |_deliverDataCallMessage()|. So please move the check to |_deliverDataCallMessage()| if needed.

@@ +4326,5 @@
>      }
>      // If data call for this connection does not exist, it could be state
>      // change for new data call.  We only update data call state change
>      // if APN name matched.
> +    if (!this.linkInfo.cid && datacall.apn != this.apnProfile.apn) {

Ditto.

@@ +4334,5 @@
>        this.debug("Data call ID: " + datacall.cid + ", interface name: " +
>                   datacall.ifname + ", APN name: " + datacall.apn);
>      }
>      if (this.connecting &&
>          (datacall.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTING ||

There is no CONNECTING state reported from "datacallstatechang", so please remove this.

@@ -4602,5 @@
>          }
>          pdpType = RIL.GECKO_DATACALL_PDP_TYPE_DEFAULT;
>        }
>      }
> -    radioInterface.setupDataCall(radioTechnology,

Seems no one use this any more, please help to remove this method from radioInterface as well. Thank you.

@@ +4507,5 @@
> +
> +    // If retry mechanism is running on background, stop it since we are going
> +    // to setup data call now.
> +    if (this.timer) {
> +      this.timer.cancel();

this.timer = null;

@@ +4563,5 @@
>      if (this.state == Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTING ||
>          this.state == Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED ||
>          this.state == Ci.nsINetworkInterface.NETWORK_STATE_UNKNOWN) {
> +      if (this.timer) {
> +        this.timer.cancel();

this.timer = null;

@@ +4774,5 @@
> +                                 kNetworkInterfaceStateChangedTopic,
> +                                 null);
> +
> +    if ((this.dataCall.state == Ci.nsINetworkInterface.NETWORK_STATE_UNKNOWN ||
> +         this.dataCall.state == Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED) &&

Should we use |this.state| instead?

Comment 52

[Jessica Jong [:jessica] (inactive)]

Thanks for the feedback, Hsinyi!

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #48)
> Comment on attachment 8408928 [details] [diff] [review]
> Part 1: map each apn type with one RILNetworkInterface, v5.
> 
> Review of attachment 8408928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1237,5 @@
> >    },
> >  
> >    updateRILNetworkInterface: function() {
> > +    let networkInterface = this.dataNetworkInterfaces.get("default");
> > +    if (!(networkInterface && networkInterface.apnSetting &&
> 
> nit: !networkInterface || !networkInterface.apnSetting ||
> !this._validateApnSetting()

Will do. But may I ask why? Is it more readable? Thanks.

> 
> @@ +1358,1 @@
> >         return RIL.GECKO_NETWORK_STATE_UNKNOWN;
> 
> nit: indention

Will do.

> 
> @@ +1371,4 @@
> >        }
> >        return;
> >      }
> > +    networkInterface.enabled = true;
> 
> What's the difference b/w networkInterface.enabled and
> networkInterface.connecting? They look quite similar to me, to indicating if
> a connect request is asked.

Sorry that the attribute '.enabled' is not clear enough. 'networkInterface.enable' is true whenever someone acquires this type of connection, for example, MMS app; networkInterface.connecting is the current state of the network interface, networkInterface.connecting becomes false as soon as the data call is connected (or disconnected), but 'networkInterface.enable' becomes false only when no one is using it (all users have released it), that's why I wondered if this needs to be ref-counted (see comment 36).

> 
> Also, I'd like to reply to your comment 41: should we set the state to
> CONNECTING directly when connect() is called? 
> 
> I think we could and we should, as this.connecting seems indicate a
> transient state which is what CONNECTING represents. And, we don't need to
> maintain these variables which point to the same thing.

Yes, I think so too, I can change it if everybody is okay with it. :)

> 
> @@ +1393,4 @@
> >        }
> >        return;
> >      }
> > +    networkInterface.enabled = false;
> 
> Should we check |if (networkInterface.enabled)| before disconnect()? That
> said, when does the situation happen that we are able to disconnect a iface
> which is never enabled?

We can add the check, but it should not happen, it's a bug if it happens.

> 
> @@ +1444,5 @@
> > +    let apnSetting = networkInterface && networkInterface.apnSetting;
> > +    if (apnSetting && message.apn == apnSetting.apn &&
> > +        networkInterface.enabled) {
> > +      gMessageManager.sendMobileConnectionMessage("RIL:DataError",
> > +                                                  this.clientId, message);
> 
> I'd like to rewrite this a little bit as below and rest
> networkInterface.enabled to false.
> 
> if (networkInterface.enabled) {
>   networkInterface.enabled = false;
>   let apnSetting = networkInterface && networkInterface.apnSetting;
>   if (apnSetting && message.apn == apnSetting.apn) {
>     gMessageManager.sendMobileConnection....
>   }
> }

Mmm, as explained above networkInterface.enabled should become false only when all users have released this connection. Another thing is, we should send "RIL:DataError" for non-default data, right?

> 
> @@ +4641,5 @@
> >                                  Ci.nsITimer.TYPE_ONE_SHOT);
> >    },
> >  
> > +  disconnect: function() {
> > +    if (this.state == Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTING ||
> 
> Similar to connect(). We could think about moving this.state to
> DISCONNECTING in disconnect().

Will do it together with CONNECTING, thanks.

Comment 53

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #49)
> Comment on attachment 8408931 [details] [diff] [review]
> Part 2: Introduce DataCall, v5.
> 
> Review of attachment 8408931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1116,5 @@
> > +    for (let i = 0; i < this._dataCalls.length; i++) {
> > +      let datacall = this._dataCalls[i];
> > +      // Send message only to the corresponding DataCall.
> > +      // Currently, args always contain only one datacall info.
> > +      if (args[0].apn && args[0].apn == datacall.apnProfile.apn) {
> 
> bail-out earlier:
> if (!args[0].apn || args[0].apn != datacall.apnProfile.apn) {
>   continue;
> }

Will do, thanks.

> 
> @@ +4435,5 @@
> > +            (this.apnProfile.user || '') == (apnSetting.user || '') &&
> > +            (this.apnProfile.password || '') == (apnSetting.password || ''));
> > +  },
> > +
> > +  resetDataCall: function() {
> 
> We now have DataCall.resetDataCall and DataCall.reset ... Is it better to
> rename DataCall.reset DataCall.retry and s/resetDataCall/reset/ ?

Sounds good to me! :)

> 
> @@ +4447,5 @@
> > +    this.state = Ci.nsINetworkInterface.NETWORK_STATE_UNKNOWN;
> > +    this.requestedNetworkIfaces = [];
> > +  },
> > +
> > +  setupDataCall: function() {
> 
> nit: s/setupDataCall/set/ since this is a property of DataCall?

Mmm, how about 'setup()'?

> 
> @@ +4546,5 @@
> > +  disconnect: function(networkInterface) {
> > +    if (DEBUG) this.debug("disconnect: " + networkInterface.type);
> > +    let index = this.requestedNetworkIfaces.indexOf(networkInterface);
> > +    if (index == -1) {
> > +      return
> 
> ; is missing

Thank you for catching this.

Comment 54

[Jessica Jong [:jessica] (inactive)]

Thanks for your feedback, Edgar!

(In reply to Edgar Chen [:edgar][:echen] from comment #50)
> Comment on attachment 8408928 [details] [diff] [review]
> Part 1: map each apn type with one RILNetworkInterface, v5.
> 
> Review of attachment 8408928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1238,5 @@
> >  
> >    updateRILNetworkInterface: function() {
> > +    let networkInterface = this.dataNetworkInterfaces.get("default");
> > +    if (!(networkInterface && networkInterface.apnSetting &&
> > +          this._validateApnSetting(networkInterface.apnSetting))) {
> 
> We had already checked the apnSetting is validate or not in
> _setupApnSettings(). If the apn setting is not valid, there is no
> networkInterface will be created. Hence, checking |networkInterface| seems
> enough?

Yes, I think checking |networkInterface| will be enough. I will double-check it tomorrow, thanks.

> 
> @@ +4534,5 @@
> >      }
> >  
> >      // In case the data setting changed while the datacall was being started or
> >      // ended, let's re-check the setting and potentially adjust the datacall
> >      // state again.
> 
> I guess this is for the case that using separated setting key for different
> apn type.
> Now we use one setting key for all apn, do we still need to handle this
> scenario?

I think we still need this for the following cases: 1) when user toggles data setting quickly in a short time, we will ignore any changes if data is connecting/disconnecting, so we need to check again when activation/deactivation is done. 2) If data is disconnected by network, we need this to setup data call again if needed.


[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1275

Comment 55

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #51)
> Comment on attachment 8408931 [details] [diff] [review]
> Part 2: Introduce DataCall, v5.
> 
> Review of attachment 8408931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1176,5 @@
> >  
> > +        if (DEBUG) this.debug("Preparing RILNetworkInterface for type: " + apnType);
> > +        // Create DataCall for RILNetworkInterface or reuse one that is shareable.
> > +        let dataCall;
> > +        for (let [type, networkInterface] of this.dataNetworkInterfaces) {
> 
> How about iterating |_dataCalls|?

Oh yes! We have _dataCalls nows, will revise this, thanks.
> 
> @@ +4318,5 @@
> >      this.reset();
> >    },
> >  
> >    dataCallStateChanged: function(datacall) {
> > +    if (this.linkInfo.cid && this.linkInfo.cid != datacall.cid) {
> 
> I would say we should make sure the event is dispatched to correct DataCall
> instance in |_deliverDataCallMessage()|. So please move the check to
> |_deliverDataCallMessage()| if needed.

Okay, will do.

> 
> @@ +4326,5 @@
> >      }
> >      // If data call for this connection does not exist, it could be state
> >      // change for new data call.  We only update data call state change
> >      // if APN name matched.
> > +    if (!this.linkInfo.cid && datacall.apn != this.apnProfile.apn) {
> 
> Ditto.

Okay, will do.

> 
> @@ +4334,5 @@
> >        this.debug("Data call ID: " + datacall.cid + ", interface name: " +
> >                   datacall.ifname + ", APN name: " + datacall.apn);
> >      }
> >      if (this.connecting &&
> >          (datacall.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTING ||
> 
> There is no CONNECTING state reported from "datacallstatechang", so please
> remove this.

Will do, and it make more sense to enter this block only on CONNECTED state.

> 
> @@ -4602,5 @@
> >          }
> >          pdpType = RIL.GECKO_DATACALL_PDP_TYPE_DEFAULT;
> >        }
> >      }
> > -    radioInterface.setupDataCall(radioTechnology,
> 
> Seems no one use this any more, please help to remove this method from
> radioInterface as well. Thank you.

Will do.

> 
> @@ +4507,5 @@
> > +
> > +    // If retry mechanism is running on background, stop it since we are going
> > +    // to setup data call now.
> > +    if (this.timer) {
> > +      this.timer.cancel();
> 
> this.timer = null;

Do we really need to set it to null? Can we reuse it?

> 
> @@ +4563,5 @@
> >      if (this.state == Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTING ||
> >          this.state == Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED ||
> >          this.state == Ci.nsINetworkInterface.NETWORK_STATE_UNKNOWN) {
> > +      if (this.timer) {
> > +        this.timer.cancel();
> 
> this.timer = null;

Do we really need to set it to null? Can we reuse it?

> 
> @@ +4774,5 @@
> > +                                 kNetworkInterfaceStateChangedTopic,
> > +                                 null);
> > +
> > +    if ((this.dataCall.state == Ci.nsINetworkInterface.NETWORK_STATE_UNKNOWN ||
> > +         this.dataCall.state == Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED) &&
> 
> Should we use |this.state| instead?

this.state will return DISCONNECTED if this networkInterface is not in DataCall's connectedTypes, however DataCall might still be CONNECTED if it's still used by another networkInterface, so we should not unregisterNetworkInterface() in this case. It sounds kind of weird right now cause RILNetworkInterface is 'by type' and NetworkManager is by 'network name', but I think it will make more sense when we change NetworkManager to be 'by type'.

Comment 56

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #52)
> Thanks for the feedback, Hsinyi!
> 
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #48)
> > Comment on attachment 8408928 [details] [diff] [review]
> > Part 1: map each apn type with one RILNetworkInterface, v5.
> > 
> > Review of attachment 8408928 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +1237,5 @@
> > >    },
> > >  
> > >    updateRILNetworkInterface: function() {
> > > +    let networkInterface = this.dataNetworkInterfaces.get("default");
> > > +    if (!(networkInterface && networkInterface.apnSetting &&
> > 
> > nit: !networkInterface || !networkInterface.apnSetting ||
> > !this._validateApnSetting()
> 
> Will do. But may I ask why? Is it more readable? Thanks.

Yes, for readability.

> 
> > 
> > @@ +1371,4 @@
> > >        }
> > >        return;
> > >      }
> > > +    networkInterface.enabled = true;
> > 
> > What's the difference b/w networkInterface.enabled and
> > networkInterface.connecting? They look quite similar to me, to indicating if
> > a connect request is asked.
> 
> Sorry that the attribute '.enabled' is not clear enough.
> 'networkInterface.enable' is true whenever someone acquires this type of
> connection, for example, MMS app; networkInterface.connecting is the current
> state of the network interface, networkInterface.connecting becomes false as
> soon as the data call is connected (or disconnected), but
> 'networkInterface.enable' becomes false only when no one is using it (all
> users have released it), that's why I wondered if this needs to be
> ref-counted (see comment 36).
> 
> > 
> > 
> > @@ +1393,4 @@
> > >        }
> > >        return;
> > >      }
> > > +    networkInterface.enabled = false;
> > 
> > Should we check |if (networkInterface.enabled)| before disconnect()? That
> > said, when does the situation happen that we are able to disconnect a iface
> > which is never enabled?
> 
> We can add the check, but it should not happen, it's a bug if it happens.
> 

Safer to have this because we will never know who is going to call this API.

> > 
> > @@ +1444,5 @@
> > > +    let apnSetting = networkInterface && networkInterface.apnSetting;
> > > +    if (apnSetting && message.apn == apnSetting.apn &&
> > > +        networkInterface.enabled) {
> > > +      gMessageManager.sendMobileConnectionMessage("RIL:DataError",
> > > +                                                  this.clientId, message);
> > 
> > I'd like to rewrite this a little bit as below and rest
> > networkInterface.enabled to false.
> > 
> > if (networkInterface.enabled) {
> >   networkInterface.enabled = false;
> >   let apnSetting = networkInterface && networkInterface.apnSetting;
> >   if (apnSetting && message.apn == apnSetting.apn) {
> >     gMessageManager.sendMobileConnection....
> >   }
> > }
> 
> Mmm, as explained above networkInterface.enabled should become false only
> when all users have released this connection. 

I see, thanks for explanation.

> Another thing is, we should
> send "RIL:DataError" for non-default data, right?
> 

According to the current behaviour, no, RIL:DataError is sent for default data.
Your patch is also sticking to what it is now. Are you suggesting changing the behaviour?

> > 
> > @@ +4334,5 @@
> > >        this.debug("Data call ID: " + datacall.cid + ", interface name: " +
> > >                   datacall.ifname + ", APN name: " + datacall.apn);
> > >      }
> > >      if (this.connecting &&
> > >          (datacall.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTING ||
> > 
> > There is no CONNECTING state reported from "datacallstatechang", so please
> > remove this.
> 

Just want to confirm that: we are going to deprecate support for v5Legacy, right? Or we've already done? If the answer is yes, then it's fine; otherwise there's CONNECTING state along with "datacallstatechange."

Comment 57

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #54)
> Thanks for your feedback, Edgar!
> 
> (In reply to Edgar Chen [:edgar][:echen] from comment #50)
> > Comment on attachment 8408928 [details] [diff] [review]
> > Part 1: map each apn type with one RILNetworkInterface, v5.
> > 
> > Review of attachment 8408928 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +4534,5 @@
> > >      }
> > >  
> > >      // In case the data setting changed while the datacall was being started or
> > >      // ended, let's re-check the setting and potentially adjust the datacall
> > >      // state again.
> > 
> > I guess this is for the case that using separated setting key for different
> > apn type.
> > Now we use one setting key for all apn, do we still need to handle this
> > scenario?
> 
> I think we still need this for the following cases: 1) when user toggles
> data setting quickly in a short time, we will ignore any changes if data is
> connecting/disconnecting, so we need to check again when
> activation/deactivation is done. 2) If data is disconnected by network, we
> need this to setup data call again if needed.

I see, thank for clarifying this. :)

> 
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RadioInterfaceLayer.js#1275

Comment 58

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #56)

> > > @@ +4334,5 @@
> > > >        this.debug("Data call ID: " + datacall.cid + ", interface name: " +
> > > >                   datacall.ifname + ", APN name: " + datacall.apn);
> > > >      }
> > > >      if (this.connecting &&
> > > >          (datacall.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTING ||
> > > 
> > > There is no CONNECTING state reported from "datacallstatechang", so please
> > > remove this.
> > 
> 
> Just want to confirm that: we are going to deprecate support for v5Legacy,
> right? Or we've already done? If the answer is yes, then it's fine;
> otherwise there's CONNECTING state along with "datacallstatechange."

Yes, you are right, there is CONNECTING state, I missed the v5Legacy case. Thank you.

Comment 59

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #58)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #56)
> 
> > > > @@ +4334,5 @@
> > > > >        this.debug("Data call ID: " + datacall.cid + ", interface name: " +
> > > > >                   datacall.ifname + ", APN name: " + datacall.apn);
> > > > >      }
> > > > >      if (this.connecting &&
> > > > >          (datacall.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTING ||
> > > > 
> > > > There is no CONNECTING state reported from "datacallstatechang", so please
> > > > remove this.
> > > 
> > 
> > Just want to confirm that: we are going to deprecate support for v5Legacy,
> > right? Or we've already done? If the answer is yes, then it's fine;
> > otherwise there's CONNECTING state along with "datacallstatechange."
> 
> Yes, you are right, there is CONNECTING state, I missed the v5Legacy case.
> Thank you.

Hmm.. the v5Legacy case is a little bit strange, state becomes CONNECTING after 'REQUEST_SETUP_DATA_CALL' returns and then calls 'getDataCallList()' to ensure the state. For backward compability, I will leave it as it is now. Thanks.

Comment 60

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #55)
> (In reply to Edgar Chen [:edgar][:echen] from comment #51)
> > Comment on attachment 8408931 [details] [diff] [review]
> > Part 2: Introduce DataCall, v5.
> > 
> > Review of attachment 8408931 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +4507,5 @@
> > > +
> > > +    // If retry mechanism is running on background, stop it since we are going
> > > +    // to setup data call now.
> > > +    if (this.timer) {
> > > +      this.timer.cancel();
> > 
> > this.timer = null;
> 
> Do we really need to set it to null? Can we reuse it?

Oh yes, let's reuse it. :)

Comment 61

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: use network type as key in NetworkManager, v1.

Turns out that there is not much to be changed!
I have also removed routing stuff in register/unregsiterNetworkInterface() in this patch, register/unregsiterNetworkInterface() should only be used to let NetworkManager know what network types we support.


Note that this is not DSDA compatible yet.

Comment 62

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: set state to DISCONNECTED after REQUEST_DEACTIVATE_DATA_CALL is done.

Currently, we set state to UNKNOWN after REQUEST_DEACTIVATE_DATA_CALL is done. We should set state to DISCONNECTED so that routes are properly removed; state becomes UNKNOWN when we reset the interface.

Comment 63

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: map each apn type with one RILNetworkInterface, v6.

Changes since v5:
  - register network interface when RILNetworkInterface is created and unregister network interface on RILNetworkInterface shutdown
  - check networkInterface.enabled before disconnect()
  - no need to validate apnSetting in updateRILNetworkInterface()
  - rewrite condition checking in handleDataCallError()
  - avoid the use of for-of (merged with previous Part 3)

Comment 64

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: Introduce DataCall, v6.

Changes since v5:
  - bail-out earlier in _deliverDataCallMessage()
  - move cid matching to _deliverDataCallMessage()
  - iterate through _dataCalls when looking for reusable DataCall
  - rename reset() to retry()
  - rename resetDataCall() to reset()
  - rename setupDataCall() to setup()
  - remove setupDataCall() and deactivateDataCall() functions from RadioInterface
  - no need to register/unregister network interface on connected/disconnected now that we register/unregister when RILNetworkInterface is created/shutdown
  - avoid the use of for-of (merged with previous Part 3)


Note that I am not changing the CONNECTING/DISCONNECTING states yet, I think we should revise them after v5 is deprecated (bug 999300).

Comment 65

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #64)
> Created attachment 8410872 [details] [diff] [review]
> Part 4: Introduce DataCall, v6.
> 
> Changes since v5:
>   - bail-out earlier in _deliverDataCallMessage()
>   - move cid matching to _deliverDataCallMessage()
>   - iterate through _dataCalls when looking for reusable DataCall
>   - rename reset() to retry()
>   - rename resetDataCall() to reset()
>   - rename setupDataCall() to setup()
>   - remove setupDataCall() and deactivateDataCall() functions from
> RadioInterface
>   - no need to register/unregister network interface on
> connected/disconnected now that we register/unregister when
> RILNetworkInterface is created/shutdown
>   - avoid the use of for-of (merged with previous Part 3)
    - rename inConnectedTypes() to inRequestedTypes() 
> 
> 
> Note that I am not changing the CONNECTING/DISCONNECTING states yet, I think
> we should revise them after v5 is deprecated (bug 999300).

Comment 66

[Jessica Jong [:jessica] (inactive)]

To avoid bugging you too much, I will ask for hsinyi and vicamo's review after I get f+ from edgar, but feel free to comment anytime. :)

Comment 67

[Edgar Chen [:edgar]]

Comment on attachment 8410813 [details] [diff] [review]
Part 1: use network type as key in NetworkManager, v1.

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #61)
> Created attachment 8410813 [details] [diff] [review]
> Part 1: use network type as key in NetworkManager, v1.
> 
> Note that this is not DSDA compatible yet.

I guess you meant DSDS ;)

This part looks good to me, but I would like to see it again after DSDS is ready. :)

::: dom/system/gonk/nsINetworkManager.idl
@@ +96,5 @@
>  
>  /**
>   * Manage network interfaces.
>   */
> +[scriptable, uuid(01f70698-27d5-423c-85b5-156c72c9ca61)]

Changing the comment doesn't require a UUID bump here

Comment 68

[Edgar Chen [:edgar]]

Comment on attachment 8410821 [details] [diff] [review]
Part 2: set state to DISCONNECTED after REQUEST_DEACTIVATE_DATA_CALL is done.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +4613,5 @@
>        this.prefixLengths = [];
>        this.dnses = [];
>        this.gateways = [];
> +
> +      this.state = RIL.GECKO_NETWORK_STATE_UNKNOWN;

Why set state to UNKNOWN instead of DISCONNECTED?

Comment 69

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #67)
> Comment on attachment 8410813 [details] [diff] [review]
> Part 1: use network type as key in NetworkManager, v1.
> 
> Review of attachment 8410813 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #61)
> > Created attachment 8410813 [details] [diff] [review]
> > Part 1: use network type as key in NetworkManager, v1.
> > 
> > Note that this is not DSDA compatible yet.
> 
> I guess you meant DSDS ;)
> 
> This part looks good to me, but I would like to see it again after DSDS is
> ready. :)

Oh yes, I forgot that even with DSDS, we register network types for every service id at the same time. Will need to revise this, thanks.

> 
> ::: dom/system/gonk/nsINetworkManager.idl
> @@ +96,5 @@
> >  
> >  /**
> >   * Manage network interfaces.
> >   */
> > +[scriptable, uuid(01f70698-27d5-423c-85b5-156c72c9ca61)]
> 
> Changing the comment doesn't require a UUID bump here

Got it, thanks.

Comment 70

[Edgar Chen [:edgar]]

Comment on attachment 8410863 [details] [diff] [review]
Part 3: map each apn type with one RILNetworkInterface, v6.

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

Please see my below comment, thank you. :)

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1153,5 @@
>  
>      // Unregister anything from iface and delete it.
> +    this.dataNetworkInterfaces.forEach(function(networkInterface) {
> +      this.unregisterDataCallCallback(networkInterface);
> +      networkInterface.shutdown();

Unregister networkInterface from NetworkManager here.

@@ +1174,5 @@
> +          continue;
> +        }
> +
> +        this.dataNetworkInterfaces.set(apnType,
> +          new RILNetworkInterface(this, networkType, inputApnSetting));

Register networkInterface to NetworkManager here.
And it is possible being rejected by NetworkManager due to duplicated type.
In such case, we shouldn't add it into |dataNetworkInterfaces|.

try {
 let networkInterface = new RILNetworkInterface(...);
 gNetworkManager.registerNetworkInterface(networkInterface);
 this.dataNetworkInterfaces.set(....);
} catch (e) {
 // Maybe print some log here
}

@@ +1223,3 @@
>        // Clear all existing connections based on APN types.
> +      if (this.getDataCallStateByType(apnType) ==
> +          RIL.GECKO_NETWORK_STATE_CONNECTED) {

if (networkInterfac.state == ...

@@ +1359,3 @@
>    },
>  
>    setupDataCallByType: function(apnType) {

Since this method will be deprecated after bug 928861, so I would like to make it as simple as possible.
So is it possible handling |enabled| and |dataInfo.state| things in |connect()|?

@@ +1381,3 @@
>    },
>  
>    deactivateDataCallByType: function(apnType) {

Same as setupDataCallByType().
Is it possible handling |enabled| things in |disconnect()|?

@@ +1400,5 @@
>    deactivateDataCalls: function() {
>      let dataDisconnecting = false;
> +    this.dataNetworkInterfaces.forEach(function(networkInterface, apnType) {
> +      if (this.getDataCallStateByType(apnType) ==
> +          RIL.GECKO_NETWORK_STATE_CONNECTED) {

if (networkInterface.state == ...

@@ +4288,5 @@
>    this.prefixLengths = [];
>    this.dnses = [];
>    this.gateways = [];
> +
> +  gNetworkManager.registerNetworkInterface(this);

Do register in |_setupApnSettings()|.

@@ +4660,5 @@
>    },
>  
>    shutdown: function() {
> +    if (this.type in gNetworkManager.networkInterfaces) {
> +      gNetworkManager.unregisterNetworkInterface(this);

Do unregister in |_setupApnSettings()|.

Comment 71

[Edgar Chen [:edgar]]

Comment on attachment 8410813 [details] [diff] [review]
Part 1: use network type as key in NetworkManager, v1.

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

::: dom/system/gonk/NetworkManager.js
@@ -404,5 @@
> -#endif
> -    // Remove pre-created default route and let setAndConfigureActive()
> -    // to set default route only on preferred network
> -    gNetworkService.removeDefaultRoute(network);
> -    this.setAndConfigureActive();

When I was looking about part2, I was thinking that do we have any chance that unregister a CONNECTED interface?

Comment 72

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #68)
> Comment on attachment 8410821 [details] [diff] [review]
> Part 2: set state to DISCONNECTED after REQUEST_DEACTIVATE_DATA_CALL is done.
> 
> Review of attachment 8410821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +4613,5 @@
> >        this.prefixLengths = [];
> >        this.dnses = [];
> >        this.gateways = [];
> > +
> > +      this.state = RIL.GECKO_NETWORK_STATE_UNKNOWN;
> 
> Why set state to UNKNOWN instead of DISCONNECTED?

We set state to DISCONNECTED when REQUEST_DEACTIVATE_DATA_CALL returns successfully, then DataCall is in charge of sending out the DISCONNECTED notifications, after that, we reset the DataCall, including setting the state to it's initial value, which is UNKNOWN. Does it make sense?

Comment 73

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: use service id + network type as key in NetworkManager, v2.

Changes since v1:
  - Use network id (service id + network type) instead of network type as key in NetworkManager to support DSDS.

Comment 74

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: map each apn type with one RILNetworkInterface, v7.

Changes since v6:
  - register/unregister network interface in DataConnectionHandler
  - do not add RILNetowrkInterface into dataNetworkInterfaces map if register fails
  - check network state directly instead of calling getDataCallByType() in deactivateDataCalls() and updateApnSettings()
  - leave setupDataCallByType() and deactivateDataCallByType() as simple as possible

Comment 75

[Edgar Chen [:edgar]]

Comment on attachment 8412651 [details] [diff] [review]
Part 1: use service id + network type as key in NetworkManager, v2.

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

Looks good. Thank you.

Comment 76

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: Introduce DataCall, v7.

Per offline discussion, below are the changes since v6:
  - remove network interface from DataCall's requestedNetworkIfaces as soon as disconnect() is called, so DISCONNECTED event is reported right away.
  - do not clean-up requestedNetworkIfaces on reset(), requestedNetworkIfaces will be touched only in connect() and disconnect(), then we can know if this DataCall is requested or not using requestedNetworkIfaces.length
  - move the decision of connect/disconnect data call after state change into DataCall.
  - rewrite DataCall's dataCallStateChanged(), hope it is clearer now.

Comment 77

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: Introduce DataCall, v8.

Sorry, forgot to handle one case...

Changes since v7:
  - consider the case of unexpected data call disconnection in DataCall's dataCallStateChanged().

Comment 78

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: handle DSDS switch default service for data connection, v1.

We should listen to data call state change instead of "network-interface-state-changed".

This patch can be merged with the last one after review.

Comment 79

[Edgar Chen [:edgar]]

Comment on attachment 8413667 [details] [diff] [review]
Part 4: Introduce DataCall, v8.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +4540,5 @@
>      this.timer.initWithCallback(this, apnRetryTimer * 1000,
>                                  Ci.nsITimer.TYPE_ONE_SHOT);
>    },
>  
> +  disconnect: function(networkInterface) {

As offline discussion, please help adjust the logic and also add more comments. :)
Thank you.

@@ +4648,5 @@
> +    return this.apnSetting.port || "";
> +  },
> +
> +  getAddresses: function(ips, prefixLengths) {
> +    let linkInfo = this.dataCall.linkInfo;

Not sure should RILNetworkInterface maintain it own information (state, ip, dns ...). 
But we could discuss about this in another bug. :)

Comment 80

[Edgar Chen [:edgar]]

Comment on attachment 8414371 [details] [diff] [review]
Part 5: handle DSDS switch default service for data connection, v1.

Looks good. Thank you.

Comment 81

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: set state to DISCONNECTED after REQUEST_DEACTIVATE_DATA_CALL is done, v2.

Asking feedback? again since I found a failure while running the tests.

Changes since v1:
  - Remove the case where we set the state to UNKNOWN if the updated data call has no ifname. We should check the state first, and if state is still connected, _compareDataCallLink() [1] below will detect the change and act accordingly.
Without this, unexpected data call disconnection will not be processed correctly due to the UNKNOWN state.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#4142

Comment 82

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: Introduce DataCall, v9.

Changes since v8:
  - per comment 79, adjust the logic on disconnect() and add more comments.
  - fix test_ril_code_quality.py warnings.

Comment 83

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: Introduce DataCall, v10.

Changes since v9:
  - fix for test_ril_code_quality.py warnings are included in this patch and not in v9. :(

Comment 84

[Edgar Chen [:edgar]]

Comment on attachment 8415084 [details] [diff] [review]
Part 4: Introduce DataCall, v10.

Thank you.

Comment 85

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 6: set CONNECTING/DISCONNECTING state in DataCall, v1.

We should set CONNECTING/DISCONNECTING state in DataCall as stated in comment 48.

PS: will merge this patch with the other patches after review.

Comment 86

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 7: changes to test cases, v1.

try results after all these patches:
https://tbpl.mozilla.org/?tree=Try&rev=fa8f2788bd4f

yay~ \o/

Comment 87

[Edgar Chen [:edgar]]

Comment on attachment 8417269 [details] [diff] [review]
Part 7: changes to test cases, v1.

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

Please see my below comments, thank you. :)

::: dom/system/gonk/tests/marionette/test_data_connection.js
@@ +1,5 @@
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  MARIONETTE_TIMEOUT = 60000;
>  MARIONETTE_CONTEXT = "chrome";

Don't need this, it is already included in head.js

@@ +32,1 @@
>    let deferred = Promise.defer();

Don't need to create a new Promise now.
(Promise.all(...).then(...) already returns a resolved promise)

@@ +49,2 @@
>  
>    return deferred.promise;

return Promise.all(...).then(...);

@@ +54,1 @@
>    let deferred = Promise.defer();

ditto.

@@ +67,5 @@
>  
> +    return deferred.resolve();
> +  });
> +
> +  return deferred.promise;

ditto.

@@ +71,5 @@
> +  return deferred.promise;
> +}
> +
> +function setDataEnabledAndWait(enabled) {
> +  let deferred = Promise.defer();

ditto.

@@ +93,2 @@
>  
>    return deferred.promise;

ditto.

@@ +163,5 @@
>      })
>      .then(runNextTest);
>  }
>  
>  let tests = [

Could you help to rewrite it with Promise as well? Thank you.

::: dom/system/gonk/tests/marionette/test_network_active_changed.js
@@ +24,5 @@
>  
>  function testActiveNetworkChangedBySwitchingDataCall(aDataCallEnabled) {
>    log("Test active network by switching dataCallEnabled to " + aDataCallEnabled);
>  
> +  let deferred = Promise.defer();

Don't need to create a new promise.

@@ +47,5 @@
> +
> +    return deferred.resolve();
> +  });
> +
> +  return deferred.promise;

return Promise.all(..).then(...);

Comment 88

[Edgar Chen [:edgar]]

Comment on attachment 8417267 [details] [diff] [review]
Part 6: set CONNECTING/DISCONNECTING state in DataCall, v1.

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

Looks good. Thank you.

Comment 89

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 7: changes to test cases, v2.

Address review comment 87:
- Remove MARIONETTE_CONTEXT, as it is already included in head.js
- Avoid creating new promise whenever possible
- Rewrite test_data_connection.js test cases with promise

Comment 90

[Jessica Jong [:jessica] (inactive)]

Scope is little bit bigger and review process is taking longer than expected.

Comment 91

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8412651 [details] [diff] [review]
Part 1: use service id + network type as key in NetworkManager, v2.

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

::: dom/system/gonk/NetworkManager.js
@@ -138,5 @@
>    this.networkInterfaces = {};
>    Services.obs.addObserver(this, TOPIC_INTERFACE_STATE_CHANGED, true);
> -#ifdef MOZ_B2G_RIL
> -  Services.obs.addObserver(this, TOPIC_INTERFACE_REGISTERED, true);
> -  Services.obs.addObserver(this, TOPIC_INTERFACE_UNREGISTERED, true);

nit: it would be more clear removing these lines in next patch.  It's really not related to network id usage.

@@ +384,1 @@
>                                   Cr.NS_ERROR_INVALID_ARG);

There is a slight difference in where you put the throw statement.  Returning null from |getNetworkId()| means getNetworkId MAY return null by design.  But do we really ever expect a null network id from it?  Certainly not.  Please move this throw statement into getNetworkId as:

  if (this.isNetworkTypeMobile(network.type)) {
    let rilNetwork = network.QueryInterface(Ci.nsIRilNetworkInterface);
    if (!rilNetwork) {
      throw Components.Exception("Mobile network not an nsIRilNetworkInterface",
                                 Cr.NS_ERROR_INVALID_ARG);
    }
    ...

Comment 92

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8412651 [details] [diff] [review]
Part 1: use service id + network type as key in NetworkManager, v2.

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

::: dom/system/gonk/NetworkManager.js
@@ +356,5 @@
>      }
>    },
>  
> +  getNetworkId: function(network) {
> +    let id = '0';

BTW, please use "ril0" or something more explicit.

Comment 93

[Jessica Jong [:jessica] (inactive)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #91)
> Comment on attachment 8412651 [details] [diff] [review]
> Part 1: use service id + network type as key in NetworkManager, v2.
> 
> Review of attachment 8412651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ -138,5 @@
> >    this.networkInterfaces = {};
> >    Services.obs.addObserver(this, TOPIC_INTERFACE_STATE_CHANGED, true);
> > -#ifdef MOZ_B2G_RIL
> > -  Services.obs.addObserver(this, TOPIC_INTERFACE_REGISTERED, true);
> > -  Services.obs.addObserver(this, TOPIC_INTERFACE_UNREGISTERED, true);
> 
> nit: it would be more clear removing these lines in next patch.  It's really
> not related to network id usage.

Mmm, this is kind of a clean-up, will move it to a separate patch. Thank you.

> 
> @@ +384,1 @@
> >                                   Cr.NS_ERROR_INVALID_ARG);
> 
> There is a slight difference in where you put the throw statement. 
> Returning null from |getNetworkId()| means getNetworkId MAY return null by
> design.  But do we really ever expect a null network id from it?  Certainly
> not.  Please move this throw statement into getNetworkId as:
> 
>   if (this.isNetworkTypeMobile(network.type)) {
>     let rilNetwork = network.QueryInterface(Ci.nsIRilNetworkInterface);
>     if (!rilNetwork) {
>       throw Components.Exception("Mobile network not an
> nsIRilNetworkInterface",
>                                  Cr.NS_ERROR_INVALID_ARG);
>     }
>     ...

Right, will do. Thanks.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #92)
> Comment on attachment 8412651 [details] [diff] [review]
> Part 1: use service id + network type as key in NetworkManager, v2.
> 
> Review of attachment 8412651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +356,5 @@
> >      }
> >    },
> >  
> > +  getNetworkId: function(network) {
> > +    let id = '0';
> 
> BTW, please use "ril0" or something more explicit.

Per offline discussion, we will use "rilX" for mobile network types, and "device" for non-mobile network types.

Comment 94

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

Comment on attachment 8412651 [details] [diff] [review]
Part 1: use service id + network type as key in NetworkManager, v2.

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

::: dom/system/gonk/NetworkManager.js
@@ +363,5 @@
> +        debug("Error! Mobile network should be an nsIRilNetworkInterface!");
> +        return null;
> +      }
> +
> +      let rilNetwork = network.QueryInterface(Ci.nsIRilNetworkInterface);

Since we check |network instanceof Ci.nsIRilNetworkInterface| before, we are sure now it's RilNetworkInterface. Wondering if we do need to QueryInterface.

Comment 95

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #94)
> Comment on attachment 8412651 [details] [diff] [review]
> Part 1: use service id + network type as key in NetworkManager, v2.
> 
> Review of attachment 8412651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +363,5 @@
> > +        debug("Error! Mobile network should be an nsIRilNetworkInterface!");
> > +        return null;
> > +      }
> > +
> > +      let rilNetwork = network.QueryInterface(Ci.nsIRilNetworkInterface);
> 
> Since we check |network instanceof Ci.nsIRilNetworkInterface| before, we are
> sure now it's RilNetworkInterface. Wondering if we do need to QueryInterface.

Seems that we don't need to. This will go away after review comment 91.

Comment 96

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #95)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #94)
> > Comment on attachment 8412651 [details] [diff] [review]
> > Part 1: use service id + network type as key in NetworkManager, v2.
> > 
> > Review of attachment 8412651 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/NetworkManager.js
> > @@ +363,5 @@
> > > +        debug("Error! Mobile network should be an nsIRilNetworkInterface!");
> > > +        return null;
> > > +      }
> > > +
> > > +      let rilNetwork = network.QueryInterface(Ci.nsIRilNetworkInterface);
> > 
> > Since we check |network instanceof Ci.nsIRilNetworkInterface| before, we are
> > sure now it's RilNetworkInterface. Wondering if we do need to QueryInterface.
> 
> Seems that we don't need to. This will go away after review comment 91.

Great! Please also keep in mind that don't QueryInterface unless we need to [1]. :)

[1] https://developer.mozilla.org/en-US/docs/JavaScript_Tips

Comment 97

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1.a: use network id as key in NetworkManager, v3.

Changes since v2:
  - move clean-up code to another patch.
  - for sub-id, use "rilX" for mobile network types, and "device" for non-mobile network types.
  - combine review comment 91 and 96: use instanceof to check for nsIRilNetworkInterface and throw error if check fails in getNetworkId().

Comment 98

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1.b: use network id in ril to look for networkInterfaces, v1.

The modified parts will be removed in the upcoming patches, just adding this for consistency.

Comment 99

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

Comment on attachment 8418645 [details] [diff] [review]
Part 1.a: use network id as key in NetworkManager, v3.

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

Looks good.

Comment 100

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2.a: remove routing stuff in register/unregisterNetworkInterface, v1.

Nothing changed, just moved clean-up code to this patch.

Comment 101

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2.b: use DISCONNECTED state instead of UNKNOWN when data is disconnected, v3.

Rebased only.

Comment 102

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: map each apn type with one RILNetworkInterface, v8.

Rebased only.

Comment 103

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4.a: Introduce DataCall, v11.

Rebased only.

Comment 104

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4.b: handle DSDS switch default service for data connection, v2.

Rebased only.

Comment 105

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4.c: set CONNECTING/DISCONNECTING state in DataCall, v2.

Rebased only.

Comment 106

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

Comment on attachment 8419203 [details] [diff] [review]
Part 2.a: remove routing stuff in register/unregisterNetworkInterface, v1.

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

It's really hard to separate a big work into smaller pieces reasonable to every reviewer :(

I know we are going to let RILNetworkInterface(s) be registered to NetworkManager right after they are created in a following patch, and let each network take care of its route. However, I haven't see how those changes would happen though they should be in the following parts. Even this looks good, I'd like to give r+ after I see those changes.

Comment 107

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

Comment on attachment 8419204 [details] [diff] [review]
Part 2.b: use DISCONNECTED state instead of UNKNOWN when data is disconnected, v3.

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

Nice clean-up.

Comment 108

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

Comment on attachment 8419266 [details] [diff] [review]
Part 3: map each apn type with one RILNetworkInterface, v8.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1078,5 @@
> +      gNetworkManager.unregisterNetworkInterface(networkInterface);
> +      networkInterface.shutdown();
> +      networkInterface = null;
> +    });
> +    this.dataNetworkInterfaces.clear();

Don't we need to clear _dataCallbacks[]?

@@ +1212,5 @@
>    anyDataConnected: function() {
> +    let result = false;
> +    this.dataNetworkInterfaces.forEach(function(networkInterface) {
> +      if (networkInterface.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> +        result = true;

We don't really need to visit all interfaces. Can we make it return immediately?

@@ +4553,1 @@
>    connecting: false,

Is it going to be removed in the following patches?

@@ +4574,5 @@
>        return;
>      }
>  
>      if (!this.registeredAsDataCallCallback) {
>        this.dataConnectionHandler.registerDataCallCallback(this);

We unregister callback when shutdown or when apn settings change, we don't unregister when disconnect().

Why don't we register callback when the network interface is created? Then the lifetime management seems clearer and consistent. Any reason?

Comment 109

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

Aha, looks like most of my questions are answered/addressed in part 4.

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #108)
> Comment on attachment 8419266 [details] [diff] [review]
> Part 3: map each apn type with one RILNetworkInterface, v8.
> 
> Review of attachment 8419266 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1078,5 @@
> > +      gNetworkManager.unregisterNetworkInterface(networkInterface);
> > +      networkInterface.shutdown();
> > +      networkInterface = null;
> > +    });
> > +    this.dataNetworkInterfaces.clear();
> 
> Don't we need to clear _dataCallbacks[]?

Done in part 4.

> 
> @@ +1212,5 @@
> >    anyDataConnected: function() {
> > +    let result = false;
> > +    this.dataNetworkInterfaces.forEach(function(networkInterface) {
> > +      if (networkInterface.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> > +        result = true;
> 
> We don't really need to visit all interfaces. Can we make it return
> immediately?

Done in part 4.

> 
> @@ +4553,1 @@
> >    connecting: false,
> 
> Is it going to be removed in the following patches?
> 
> @@ +4574,5 @@
> >        return;
> >      }
> >  
> >      if (!this.registeredAsDataCallCallback) {
> >        this.dataConnectionHandler.registerDataCallCallback(this);
> 
> We unregister callback when shutdown or when apn settings change, we don't
> unregister when disconnect().
> 
> Why don't we register callback when the network interface is created? Then
> the lifetime management seems clearer and consistent. Any reason?

Done in part 4.

Comment 110

[Jessica Jong [:jessica] (inactive)]

Thank you Hsinyi for the review comments. 
Actually, part 3 and part 4.a, .b and .c are all related, but I thought the patch would be too big to read, so I tried to split into smaller patches, but it seems that it isn't helping. :(

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #109)
> Aha, looks like most of my questions are answered/addressed in part 4.
> 
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #108)
> > Comment on attachment 8419266 [details] [diff] [review]
> > Part 3: map each apn type with one RILNetworkInterface, v8.
> > 
> > Review of attachment 8419266 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +1078,5 @@
> > > +      gNetworkManager.unregisterNetworkInterface(networkInterface);
> > > +      networkInterface.shutdown();
> > > +      networkInterface = null;
> > > +    });
> > > +    this.dataNetworkInterfaces.clear();
> > 
> > Don't we need to clear _dataCallbacks[]?
> 
> Done in part 4.
> 
> > 
> > @@ +1212,5 @@
> > >    anyDataConnected: function() {
> > > +    let result = false;
> > > +    this.dataNetworkInterfaces.forEach(function(networkInterface) {
> > > +      if (networkInterface.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> > > +        result = true;
> > 
> > We don't really need to visit all interfaces. Can we make it return
> > immediately?
> 
> Done in part 4.

There is no good way to stop a forEach loop, so I let it finish running.
As you see it, we won't have this problem with _dataCalls array in part 4.

> 
> > 
> > @@ +4553,1 @@
> > >    connecting: false,
> > 
> > Is it going to be removed in the following patches?

Yes, it's replaced in Part 4.c

> > 
> > @@ +4574,5 @@
> > >        return;
> > >      }
> > >  
> > >      if (!this.registeredAsDataCallCallback) {
> > >        this.dataConnectionHandler.registerDataCallCallback(this);
> > 
> > We unregister callback when shutdown or when apn settings change, we don't
> > unregister when disconnect().
> > 
> > Why don't we register callback when the network interface is created? Then
> > the lifetime management seems clearer and consistent. Any reason?
> 
> Done in part 4.

Comment 111

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

Comment on attachment 8419269 [details] [diff] [review]
Part 4.a: Introduce DataCall, v11.

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

Huge work :P

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1321,1 @@
>      let defaultDataCallConnected = defaultDataCallState ==

We don't really care the state of the lower level "dataCall" here. How about simply use "networkInterface.connected"?

@@ +1321,4 @@
>      let defaultDataCallConnected = defaultDataCallState ==
>                                     RIL.GECKO_NETWORK_STATE_CONNECTED;
> +
> +    // We have moved part of the dicision making into DataCall, the rest will be

nit: s/dicision/decision

@@ +1328,5 @@
>           (dataInfo.roaming && !this.dataCallSettings.roamingEnabled))) {
>        if (DEBUG) {
>          this.debug("Data call settings: disconnect data call.");
>        }
>        this.deactivateDataCallByType("default");

Please simply call "networkInterface.disconnect()". And replace "setupDataCallByType("default") in this function with networkInterface.connect().

@@ +1336,5 @@
>      if (defaultDataCallConnected && wifi_active) {
>        if (DEBUG) {
>          this.debug("Disconnect data call when Wifi is connected.");
>        }
>        this.deactivateDataCallByType("default");

ditto.

@@ +4247,4 @@
>    },
>  
>    // TODO: Bug 904514 - [meta] NetworkManager enhancement
>    getDataCallStateByType: function(apntype) {

In bug 904514, I'd like to see the function renamed.
From the perspective of NetworkManager, there's no "dataCall". A higher-level name sounds better.

@@ +4286,3 @@
>  }
>  
> +function DataCall(clientId, apnSetting) {

Duplicate code.

@@ +4357,5 @@
> +          }
> +          this.linkInfo.gateways = datacall.gateways.slice();
> +          this.linkInfo.dnses = datacall.dnses.slice();
> +
> +          if (this.requestedNetworkIfaces.length === 0) {

Can we check this earlier, i.e. right after line 4350? Then we don't need to have line#4352 to #4329.

@@ +4366,5 @@
> +            this.deactivate();
> +            return;
> +          }
> +
> +        } else if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {

No need to check |state == RIL.GECKO_NETWORK_STATE_CONNECTED|, because we are in the case "RIL.GECKO_NETWORK_STATE_CONNECTED".

@@ +4401,5 @@
> +        if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> +          // Notify first on unexpected data call disconnection.
> +          this.state = datacall.state;
> +          for (let i = 0; i < this.requestedNetworkIfaces.length; i++) {
> +            this.requestedNetworkIfaces[i].notifyRILNetworkInterface();

Why notify here and again in line#4423? Redundant?

@@ +4473,5 @@
> +        this.state == RIL.GECKO_NETWORK_STATE_CONNECTED ||
> +        this.state == RIL.GECKO_NETWORK_STATE_DISCONNECTING) {
> +      // DataCall already connected, just do notify.
> +      if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> +        networkInterface.notifyRILNetworkInterface();

Let's write this way for readability and for avoiding redundant check.

if (this.connecting || this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
  return;
}

if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
  networkInterface.notifyRILNetworkInterface();
  return;
}

Comment 112

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

Comment on attachment 8419269 [details] [diff] [review]
Part 4.a: Introduce DataCall, v11.

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

Huge work :P

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1321,1 @@
>      let defaultDataCallConnected = defaultDataCallState ==

We don't really care the state of the lower level "dataCall" here. How about simply use "networkInterface.connected"?

@@ +1321,4 @@
>      let defaultDataCallConnected = defaultDataCallState ==
>                                     RIL.GECKO_NETWORK_STATE_CONNECTED;
> +
> +    // We have moved part of the dicision making into DataCall, the rest will be

nit: s/dicision/decision

@@ +1328,5 @@
>           (dataInfo.roaming && !this.dataCallSettings.roamingEnabled))) {
>        if (DEBUG) {
>          this.debug("Data call settings: disconnect data call.");
>        }
>        this.deactivateDataCallByType("default");

Please simply call "networkInterface.disconnect()". And replace "setupDataCallByType("default") in this function with networkInterface.connect().

@@ +1336,5 @@
>      if (defaultDataCallConnected && wifi_active) {
>        if (DEBUG) {
>          this.debug("Disconnect data call when Wifi is connected.");
>        }
>        this.deactivateDataCallByType("default");

ditto.

@@ +4247,4 @@
>    },
>  
>    // TODO: Bug 904514 - [meta] NetworkManager enhancement
>    getDataCallStateByType: function(apntype) {

In bug 904514, I'd like to see the function renamed.
From the perspective of NetworkManager, there's no "dataCall". A higher-level name sounds better.

@@ +4286,3 @@
>  }
>  
> +function DataCall(clientId, apnSetting) {

Duplicate code.

@@ +4357,5 @@
> +          }
> +          this.linkInfo.gateways = datacall.gateways.slice();
> +          this.linkInfo.dnses = datacall.dnses.slice();
> +
> +          if (this.requestedNetworkIfaces.length === 0) {

Can we check this earlier, i.e. right after line 4350? Then we don't need to have line#4352 to #4329.

@@ +4366,5 @@
> +            this.deactivate();
> +            return;
> +          }
> +
> +        } else if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {

No need to check |state == RIL.GECKO_NETWORK_STATE_CONNECTED|, because we are in the case "RIL.GECKO_NETWORK_STATE_CONNECTED".

@@ +4401,5 @@
> +        if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> +          // Notify first on unexpected data call disconnection.
> +          this.state = datacall.state;
> +          for (let i = 0; i < this.requestedNetworkIfaces.length; i++) {
> +            this.requestedNetworkIfaces[i].notifyRILNetworkInterface();

Why notify here and again in line#4423? Redundant?

@@ +4473,5 @@
> +        this.state == RIL.GECKO_NETWORK_STATE_CONNECTED ||
> +        this.state == RIL.GECKO_NETWORK_STATE_DISCONNECTING) {
> +      // DataCall already connected, just do notify.
> +      if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> +        networkInterface.notifyRILNetworkInterface();

Let's write this way for readability and for avoiding redundant check.

if (this.connecting || this.state == RIL.GECKO_NETWORK_STATE_DISCONNECTING) {
  return;
}

if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
  networkInterface.notifyRILNetworkInterface();
  return;
}

Comment 113

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #111)
> Comment on attachment 8419269 [details] [diff] [review]
> Part 4.a: Introduce DataCall, v11.
> 
> 
> @@ +4366,5 @@
> > +            this.deactivate();
> > +            return;
> > +          }
> > +
> > +        } else if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> 
> No need to check |state == RIL.GECKO_NETWORK_STATE_CONNECTED|, because we
> are in the case "RIL.GECKO_NETWORK_STATE_CONNECTED".

Please ignore this, my bad :)

Comment 114

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

Comment on attachment 8419266 [details] [diff] [review]
Part 3: map each apn type with one RILNetworkInterface, v8.

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

This is good based on comment 110, but I'd like to review it again once part 4 is okay, so keep review? for now :)

Comment 115

[Jessica Jong [:jessica] (inactive)]

Thank you, Hsinyi!

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #112)
> Comment on attachment 8419269 [details] [diff] [review]
> Part 4.a: Introduce DataCall, v11.
> 
> Review of attachment 8419269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Huge work :P
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1321,1 @@
> >      let defaultDataCallConnected = defaultDataCallState ==
> 
> We don't really care the state of the lower level "dataCall" here. How about
> simply use "networkInterface.connected"?

Sounds good! will do.

> 
> @@ +1321,4 @@
> >      let defaultDataCallConnected = defaultDataCallState ==
> >                                     RIL.GECKO_NETWORK_STATE_CONNECTED;
> > +
> > +    // We have moved part of the dicision making into DataCall, the rest will be
> 
> nit: s/dicision/decision

will do.

> 
> @@ +1328,5 @@
> >           (dataInfo.roaming && !this.dataCallSettings.roamingEnabled))) {
> >        if (DEBUG) {
> >          this.debug("Data call settings: disconnect data call.");
> >        }
> >        this.deactivateDataCallByType("default");
> 
> Please simply call "networkInterface.disconnect()". And replace
> "setupDataCallByType("default") in this function with
> networkInterface.connect().

Good idea, will do.

> 
> @@ +1336,5 @@
> >      if (defaultDataCallConnected && wifi_active) {
> >        if (DEBUG) {
> >          this.debug("Disconnect data call when Wifi is connected.");
> >        }
> >        this.deactivateDataCallByType("default");
> 
> ditto.

Will do.

> 
> @@ +4247,4 @@
> >    },
> >  
> >    // TODO: Bug 904514 - [meta] NetworkManager enhancement
> >    getDataCallStateByType: function(apntype) {
> 
> In bug 904514, I'd like to see the function renamed.
> From the perspective of NetworkManager, there's no "dataCall". A
> higher-level name sounds better.

Yup, we should take care of that in bug 904514.

> 
> @@ +4286,3 @@
> >  }
> >  
> > +function DataCall(clientId, apnSetting) {
> 
> Duplicate code.

Sorry about this :( must have been added while rebasing.

> 
> @@ +4357,5 @@
> > +          }
> > +          this.linkInfo.gateways = datacall.gateways.slice();
> > +          this.linkInfo.dnses = datacall.dnses.slice();
> > +
> > +          if (this.requestedNetworkIfaces.length === 0) {
> 
> Can we check this earlier, i.e. right after line 4350? Then we don't need to
> have line#4352 to #4329.

We can check after #4352, we need the cid for disconnecting.

> 
> @@ +4366,5 @@
> > +            this.deactivate();
> > +            return;
> > +          }
> > +
> > +        } else if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> 
> No need to check |state == RIL.GECKO_NETWORK_STATE_CONNECTED|, because we
> are in the case "RIL.GECKO_NETWORK_STATE_CONNECTED".
> 
> @@ +4401,5 @@
> > +        if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> > +          // Notify first on unexpected data call disconnection.
> > +          this.state = datacall.state;
> > +          for (let i = 0; i < this.requestedNetworkIfaces.length; i++) {
> > +            this.requestedNetworkIfaces[i].notifyRILNetworkInterface();
> 
> Why notify here and again in line#4423? Redundant?

If requestedNetworkIfaces.length > 0, it will meet the condition in line #4410, and return directly in #4416; if requestedNetworkIfaces is empty, none of the them is called.

> 
> @@ +4473,5 @@
> > +        this.state == RIL.GECKO_NETWORK_STATE_CONNECTED ||
> > +        this.state == RIL.GECKO_NETWORK_STATE_DISCONNECTING) {
> > +      // DataCall already connected, just do notify.
> > +      if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
> > +        networkInterface.notifyRILNetworkInterface();
> 
> Let's write this way for readability and for avoiding redundant check.
> 
> if (this.connecting || this.state == RIL.GECKO_NETWORK_STATE_DISCONNECTING) {
>   return;
> }
> 
> if (this.state == RIL.GECKO_NETWORK_STATE_CONNECTED) {
>   networkInterface.notifyRILNetworkInterface();
>   return;
> }

Looks much better, thank you. :)

Comment 116

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

Comment on attachment 8419270 [details] [diff] [review]
Part 4.b: handle DSDS switch default service for data connection, v2.

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

Looks good!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +779,5 @@
> +      return this._pendingDataCallRequest !== null;
> +    },
> +
> +    notifyDataCallStateChange: function(clientId) {
> +      if (this.isSwitchingDataClientId() && clientId == this._currentDataClientId) {

nit: bail-out early

Comment 117

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

Comment on attachment 8419271 [details] [diff] [review]
Part 4.c: set CONNECTING/DISCONNECTING state in DataCall, v2.

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

Thank you!

::: dom/system/gonk/ril_worker.js
@@ -2086,5 @@
>      Buf.writeString(options.reason || DATACALL_DEACTIVATE_NO_REASON);
>      Buf.sendParcel();
>  
>      datacall.state = GECKO_NETWORK_STATE_DISCONNECTING;
> -    this.sendChromeMessage(datacall);

This is sort of a workaround, but I am totally fine with this.
Please help leave a TODO comment, Bug 999300 - Remove RIL V5 legacy support, at the beginning of the if-clause.

Comment 118

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8419271 [details] [diff] [review]
Part 4.c: set CONNECTING/DISCONNECTING state in DataCall, v2.

At the end I still didn't finish the review in time and has caused long delay to this task.  I'm very sorry about this.  Happy wedding! :)

Comment 119

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4.a: Introduce DataCall, v12.

Changes since v11:
- Use networkInterface.connected/disconnect()/connect() in updateRILNetworkInterface()
- fix typo: s/dicision/decision
- remove duplicated code
- check for requestedNetworkIfaces.length earlier in dataCallStateChanged()
- refine condition checking in RILNetworkInterface.connect() for readability and for avoiding redundant check

Comment 120

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4.b: handle DSDS switch default service for data connection, v3.

Changes since v2:
- address nit in commment 116: bail-out early in notifyDataCallStateChange()

Comment 121

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4.c: set CONNECTING/DISCONNECTING state in DataCall, v3.

Changes since v2:
- Per offline discussion, remove DISCONNECTING state in ril_worker.js, as it is not needed.

Comment 122

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8417866 [details] [diff] [review]
Part 7: changes to test cases, v2.

This should be "Part 5" after reorganizing.

Comment 123

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

Comment on attachment 8426776 [details] [diff] [review]
Part 4.a: Introduce DataCall, v12.

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

Looks good to me!

Comment 124

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

Comment on attachment 8417866 [details] [diff] [review]
Part 7: changes to test cases, v2.

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

Ya~ thank you for the great work!

Comment 125

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 1.a: use network id as key in NetworkManager. f=edgar r=hsinyi,vicamo

rebased.

Comment 126

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 1.b: use network id in ril to look for networkInterfaces. r=hsinyi,vicamo

rebased.

Comment 127

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 2.a: remove routing stuff in register/unregisterNetworkInterface. f=edgar r=hsinyi,vicamo

rebased.

Comment 128

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 2.b: use DISCONNECTED state instead of UNKNOWN when data is disconnected. f=edgar r=hsinyi,vicamo

rebased.

Comment 129

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 3: Introduce dataNetworkInterfaces map and DataCall. f=edgar r=hsinyi

rebased and squash commits in Part 3, 4.a, 4.b and 4.c.

Comment 130

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 4: changes to test cases after data call enhancement. f=edgar r=hsinyi

rebased.

Comment 131

[Jessica Jong [:jessica] (inactive)]

Per offline discussion, we have decided to land the patches in this bug, since we already have review approval from one of the RIL owners. If there is any concern after this, we can file as follow-ups.

The following scenarios are tested using Buri + CHT sim card:

1. Data enable/disable.
2. Sending mms with mms apn same as default (with data enabled).
3. Sending mms with mms apn different from default (with data enabled).
4. Sending mms with data disabled.
5. Editing default data's apn with data enabled.
6. Airplane mode off -> on and check if data is automatically reconnected.
7. Wifi on -> off and check if data is automatically reconnected.
8. Switching default sim for data on a multisim device (fugu).

Comment 132

[Jessica Jong [:jessica] (inactive)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=ab034750b3ff
All green! Yay~ \o/

Comment 133

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/00aed21d24c5
https://hg.mozilla.org/integration/b2g-inbound/rev/c9ea3d2117bf
https://hg.mozilla.org/integration/b2g-inbound/rev/116dfab6f47c
https://hg.mozilla.org/integration/b2g-inbound/rev/e83a6eee4baf
https://hg.mozilla.org/integration/b2g-inbound/rev/9bf58100cce7
https://hg.mozilla.org/integration/b2g-inbound/rev/8cd3861f8292

Comment 134

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00aed21d24c5
https://hg.mozilla.org/mozilla-central/rev/c9ea3d2117bf
https://hg.mozilla.org/mozilla-central/rev/116dfab6f47c
https://hg.mozilla.org/mozilla-central/rev/e83a6eee4baf
https://hg.mozilla.org/mozilla-central/rev/9bf58100cce7
https://hg.mozilla.org/mozilla-central/rev/8cd3861f8292

Comment 135

[Jessica Jong [:jessica] (inactive)]

Items 1, 2 and 3 in comment 0 have been fixed in this bug, filed Bug 1017470 for item 4.