837488 - B2G RIL: Using array for data call settings and handling multiple sharing on one APN connection

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Ken Chang[:kenkai]]

In current design of data call, there are 2 problems:
1. The apn setting fields of data call are set one by one, which means RIL need to wait until all apn setting fields of data call are configured when RIL want to establish the data call.
2. Two data call types with same apn settings can not share the same network interface.
These problems need to be resolved.

Comment 1

[Ken Chang[:kenkai]]

Attachment: New data call architecture.

Comment 2

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

Comment on attachment 714906 [details] [diff] [review]
New data call architecture.

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

Thanks for the patch, Ken.  

One of the main aims here is not setting data call one by one, so we don't want to keep the old setting scheme in your new patch. Please remove them. We don't need to consider backward compatibility here, but of course, we do need to make sure gaia has corresponding revision.

Besides, I noticed there are several unpleasant trailing whitespaces in the patch. Please pay attention to meeting mozilla's coding style. Thanks!

Comment 3

[Ken Chang[:kenkai]]

Attachment: Removing backward compatibility and trailing whitespaces

Please have a review. Thanks.

Comment 4

[Shian-Yow Wu [:swu]]

Comment on attachment 715952 [details] [diff] [review]
Removing backward compatibility and trailing whitespaces

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

Thank you!

This is a major architecture change, so we need testing like:
- Simultaneously connect/disconnect of multiple APNs, either shared or independent APN name.
- Quickly connect/disconnect
- Co-working with Wifi enable/disable
- Error condition (such as giving a wrong APN name)
- User change APN name
Any other cases, please add. :)

Some coding style things to address, like alignment, capitalization and required space.

More comments below.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +881,5 @@
>      // For the data connection, the `connected` flag indicates whether
>      // there's an active data call.
> +    dataInfo.connected = false;
> +    if (apnContexts.byType["default"] && apnContexts.byType["default"].iface &&
> +         this.dataCallSettings.enabled) {

Please align the conditions for if statement here and elsewhere.

For example:
  if (aaa && 
      bbb) {
    ...
  }

@@ +912,5 @@
>     */
>    handleDataCallError: function handleDataCallError(message) {
> +    let dataApn = !this.dataCallSettings.apnContexts.byType["default"]
> +                    ? ""
> +                    : this.dataCallSettings.apnContexts.byType["default"].apn;

Please align the arguments.

@@ +1109,5 @@
>        this.setRadioEnabled(false);
>      }
>    },
>  
> +  createApnContext: function createApnContext(apnSetting) {

Please add comments before this function to describe it.

@@ +1119,5 @@
> +    if (!apnContexts.byAPN[apn]) {
> +      apnContexts.byAPN[apn] = new Object();
> +    }
> +    else if ((apnContexts.byAPN[apn].iface) &&
> +              (apnContexts.byAPN[apn].iface.connected)) {

The parentheses for conditions here are superfluous.

@@ +1120,5 @@
> +      apnContexts.byAPN[apn] = new Object();
> +    }
> +    else if ((apnContexts.byAPN[apn].iface) &&
> +              (apnContexts.byAPN[apn].iface.connected)) {
> +      apnContexts.byAPN[apn].iface.disconnect();

Does this mean we always disconnect the APN when user changes any settings, and expect it to connect again automatically later on?

Currently only default data APN has the automatically reconnection mechanism.

@@ +1122,5 @@
> +    else if ((apnContexts.byAPN[apn].iface) &&
> +              (apnContexts.byAPN[apn].iface.connected)) {
> +      apnContexts.byAPN[apn].iface.disconnect();
> +    }
> +    //Copy apn Settings.

Please add space between // and comment, and use proper capitalization.  Same as comments elsewhere.

@@ +1148,5 @@
> +
> +    return true;
> +  },
> +
> +  checkIfRemovingApn : function checkIfRemovingApn() {

A better name maybe?  Such as removeUnusedApn()?

@@ +1163,5 @@
> +      }
> +    }
> +  },
> +
> +  ensureDataCallSettings: function ensureDataCallSettings(apnSetting) {

validateApnSetting() might be a better name.

@@ +1168,5 @@
> +    if ((!apnSetting)
> +        || (!apnSetting.apn)
> +        || (apnSetting.apn == "")
> +        || (!apnSetting.type)
> +        || (apnSetting.type == "")) {

Put || at the end of the condition for consistency.

@@ +1262,3 @@
>          (!this.dataCallSettings["enabled"] ||
> +         (dataInfo.roaming && !this.dataCallSettings["roamingEnabled"]) ||
> +         (wifi_active && apnSetting.iface.ifaceRefCounter > 1))) {

Does apnSetting.iface.ifaceRefCounter > 1 here means there are at least two active APN connections on APN name of 'default' type?

In wifi active condition,
1. Don't disconnect if there are any active connection opened by 'mms' or 'supl' type on same APN name of 'default' type.  
2. Disconnect if active connection only opened by 'default' type.

@@ +1679,5 @@
> +    let dataApn = !this.dataCallSettings.apnContexts.byType["default"]
> +                    ? ""
> +                    : this.dataCallSettings.apnContexts.byType["default"].apn;
> +    if (datacall.ifname && (datacall.apn == dataApn) &&
> +         this.dataCallSettings.enabled) {

Don't we need to do the following when this.dataCallSettings.enabled is false?

@@ +1694,5 @@
> +    if (((datacall.state == RIL.GECKO_NETWORK_STATE_UNKNOWN) ||
> +         (datacall.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED)) &&
> +         (apnSetting.type.length == 0)) {
> +       delete this.dataCallSettings.apnContexts.byAPN[dataApn];
> +    }

The apnContexts for default APN type is deleted here, other APN types should be checked as well.

@@ +1881,5 @@
>          }
>          // Cancel the timer for the call-ring wake lock.
>          this._cancelCallRingWakeLockTimer();
>          // Shutdown all RIL network interfaces
> +        let apnContexts =this.dataCallSettings.apnContexts;

Add a space after =.

@@ +2832,5 @@
> +    }
> +    else if (this.apnSetting.type.indexOf("mms") != -1) {
> +      return Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS;
> +    }
> +    return Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL;

What if there are other types than "default"/"mms"/"supl"?

@@ +2964,3 @@
>      if (this.connecting || this.connected) {
> +      //There already is connection.
> +      this.ifaceRefCounter++;

In which condition will this happen?  Or this is for safety consideration?

Comment 5

[Ken Chang[:kenkai]]

Attachment: WIP V2

Comment 6

[Ken Chang[:kenkai]]

(In reply to Shian-Yow Wu from comment #4)
> - User change APN name
> Any other cases, please add. :)
> Does this mean we always disconnect the APN when user changes any settings,
> and expect it to connect again automatically later on?
> 
> Currently only default data APN has the automatically reconnection mechanism.
When users change the APN setting data, I think all data connections should be disconnected. The automatically reconnection mechanism of data APN is the error handle mechanism in RIL. 
For other APs using data connection, they should also have error handle to reconnect the data connection.
> Does apnSetting.iface.ifaceRefCounter > 1 here means there are at least two
> active APN connections on APN name of 'default' type?
> 
> In wifi active condition,
> 1. Don't disconnect if there are any active connection opened by 'mms' or
> 'supl' type on same APN name of 'default' type.  
> 2. Disconnect if active connection only opened by 'default' type.
It is a problem. I will modify it.
this.dataCallSettings.apnContexts.byType["default"].apn;
> > +    if (datacall.ifname && (datacall.apn == dataApn) &&
> > +         this.dataCallSettings.enabled) {
> 
> Don't we need to do the following when this.dataCallSettings.enabled is
> false?
Yes, we have to handle it.
> > +    }
> > +    else if (this.apnSetting.type.indexOf("mms") != -1) {
> > +      return Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS;
> > +    }
> > +    return Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL;
> 
> What if there are other types than "default"/"mms"/"supl"?
There are 3 actions needing to be done after establishing data connection.
1. Add default route.
2. Add host route.
3. Get the application porxy.
For "default" APN type, we need to do action 1,action 2, and action3.
For "mms" APN type, we need to do action 2 and action 3 (mms proxy).
For "supl" APN type, we only need to do action 2.
For other type, I think we should only need to do action 2. That is why I use "SUPL" as the default returning value.

> @@ +2964,3 @@
> >      if (this.connecting || this.connected) {
> > +      //There already is connection.
> > +      this.ifaceRefCounter++;
> 
> In which condition will this happen?  Or this is for safety consideration?
It is for safety consideration. If device are trying to establishing 2 data connections with same APN at the same time and the first data connections procedure is failed, the first data connection procedure will enter retrying mechanism and the secondary data connection procedure will do normal data connection establishment procedure. This situation will cause ifaceRefCounter = 1 and connecting = true.

Comment 7

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

Comment on attachment 716427 [details] [diff] [review]
WIP V2

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

This work is huge, thanks for your great effort, Ken!

Below please find my comments and some questions. Shianyow has mentioned some of my concerns as well, and sorry that I cannot comment on the same attachment as Shianyow (don't know what happened to splinter review @@).

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +912,5 @@
>     */
>    handleDataCallError: function handleDataCallError(message) {
> +    let dataApn = !this.dataCallSettings.apnContexts.byType["default"]
> +                    ? ""
> +                    : this.dataCallSettings.apnContexts.byType["default"].apn;

Not big deal here, minor thought: why not
  let dataApn = this.dataCallSettings.apnContexts.byType["default"]
                 ? this.dataCallSettings.apnContexts.byType["default"].apn : "";

I think this looks a little bit more intuitive for me ;)

@@ +1111,5 @@
>    },
>  
> +  createApnContext: function createApnContext(apnSetting) {
> +    if (!this.ensureDataCallSettings(apnSetting)) {
> +      return false;

return; looks better here.

@@ +1118,5 @@
> +    let apnContexts = this.dataCallSettings.apnContexts;
> +    if (!apnContexts.byAPN[apn]) {
> +      apnContexts.byAPN[apn] = new Object();
> +    }
> +    else if ((apnContexts.byAPN[apn].iface) &&

nit: coding style: move 'else if' to right after the previous '}'

@@ +1122,5 @@
> +    else if ((apnContexts.byAPN[apn].iface) &&
> +              (apnContexts.byAPN[apn].iface.connected)) {
> +      apnContexts.byAPN[apn].iface.disconnect();
> +    }
> +    //Copy apn Settings.

nit: coding style. Put a white space after //.

@@ +1126,5 @@
> +    //Copy apn Settings.
> +    for (let apnItem in apnSetting) {
> +      apnContexts.byAPN[apn][apnItem] = apnSetting[apnItem];
> +    }
> +    //Check if the apn of this type is exist

ditto: coding style.
Grammar error: "this type exists" & place a period at the end of the line.

@@ +1128,5 @@
> +      apnContexts.byAPN[apn][apnItem] = apnSetting[apnItem];
> +    }
> +    //Check if the apn of this type is exist
> +    for each (let type in apnContexts.byAPN[apn].type) {
> +      //This type was stored before, remove it and update the new one.

ditto: coding style

@@ +1131,5 @@
> +    for each (let type in apnContexts.byAPN[apn].type) {
> +      //This type was stored before, remove it and update the new one.
> +      if (apnContexts.byType[type] &&
> +           (apnContexts.byType[type] != apnContexts.byAPN[apn])) {
> +        //remove the apn-type link

ditto: coding style.
And s/remove/Remove

@@ +1145,5 @@
> +    if (!apnContexts.byAPN[apn].iface) {
> +      apnContexts.byAPN[apn].iface = new RILNetworkInterface(this, apnSetting);
> +    }
> +
> +    return true;

I don't think we need to return true here.

@@ +1166,5 @@
> +
> +  ensureDataCallSettings: function ensureDataCallSettings(apnSetting) {
> +    if ((!apnSetting)
> +        || (!apnSetting.apn)
> +        || (apnSetting.apn == "")

|apnSetting.apn == ""| seems unnecessary unless we need to check specifically for an empty string over null, but I don't see that need. |!apnSetting.apn| should be enough.

@@ +1210,5 @@
>    },
>  
>    updateRILNetworkInterface: function updateRILNetworkInterface() {
> +    let apnSetting = this.dataCallSettings.apnContexts.byType["default"];
> +    if (!apnSetting || !this.ensureDataCallSettings(apnSetting) ||

We don't need additional |!apnSetting| since we check that in ensureDataCallSettings().

@@ +1211,5 @@
>  
>    updateRILNetworkInterface: function updateRILNetworkInterface() {
> +    let apnSetting = this.dataCallSettings.apnContexts.byType["default"];
> +    if (!apnSetting || !this.ensureDataCallSettings(apnSetting) ||
> +         !apnSetting.iface) {

Don't we need to check whether we've received 'ril.data.enabled' and 'ril.data.roaming_enabled' from settings DB as the original code does?

@@ +1216,2 @@
>        debug("We haven't read completely the APN data from the " +
>              "settings DB yet. Wait for that.");

The comment isn't that right in your patch since, for example, checking apnSetting.iface seems not so strongly related to settings DB.

@@ +1262,3 @@
>          (!this.dataCallSettings["enabled"] ||
> +         (dataInfo.roaming && !this.dataCallSettings["roamingEnabled"]) ||
> +         (wifi_active && apnSetting.iface.ifaceRefCounter > 1))) {

Something wrong in |apnSetting.iface.ifaceRefCounter > 1|?

@@ +1679,5 @@
> +    let dataApn = !this.dataCallSettings.apnContexts.byType["default"]
> +                    ? ""
> +                    : this.dataCallSettings.apnContexts.byType["default"].apn;
> +    if (datacall.ifname && (datacall.apn == dataApn) &&
> +         this.dataCallSettings.enabled) {

Why check 'this.dataCallSettings.enabled' here? We will have 'this.dataCallSettings.enabled == false' but we still need to notify data info changed, because the data info could change from 'enabled' to 'disabled', right?

@@ +1693,5 @@
> +    let apnSetting = this.dataCallSettings.apnContexts.byAPN[dataApn];
> +    if (((datacall.state == RIL.GECKO_NETWORK_STATE_UNKNOWN) ||
> +         (datacall.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED)) &&
> +         (apnSetting.type.length == 0)) {
> +       delete this.dataCallSettings.apnContexts.byAPN[dataApn];

Should be |.apnContexts.byAPN[dataApn.apn]| ??

Maybe we could make the stuff done before delivering callback?

@@ +1881,5 @@
>          }
>          // Cancel the timer for the call-ring wake lock.
>          this._cancelCallRingWakeLockTimer();
>          // Shutdown all RIL network interfaces
> +        let apnContexts =this.dataCallSettings.apnContexts;

nit: space after =

@@ +1962,5 @@
>        case "ril.data.enabled":
> +        debug("'ril.data.enabled' is now " + aResult);
> +        this.dataCallSettings.oldEnabled = this.dataCallSettings.enabled;
> +        this.dataCallSettings.enabled = aResult;
> +        updateDataNetworkInterface = true;

I don't see why we need this flag? Can't we just call this.updateRILNetworkInterface()?

@@ +1968,4 @@
>        case "ril.data.roaming_enabled":
> +        debug("'ril.data.roaming_enabled' is now " + aResult);
> +        this.dataCallSettings.roamingEnabled = aResult;
> +        updateDataNetworkInterface = true;

ditto.

@@ +1977,2 @@
>          }
> +        this.checkIfRemovingApn();

I would like to merge "createApnContext" and "checkIfRemovingApn" into a function naming something like "updateApnContext" and move the 'for each' loop into the updateApnContext function, because the two functions do more than their names represent and they also update apn context.

@@ +1977,3 @@
>          }
> +        this.checkIfRemovingApn();
> +        updateDataNetworkInterface = true;

ditto.

@@ +2700,5 @@
> +    let isRegistered =
> +      dataInfo.state == RIL.GECKO_MOBILE_CONNECTION_STATE_REGISTERED;
> +    let haveDataConnection =
> +      dataInfo.type != RIL.GECKO_MOBILE_CONNECTION_STATE_UNKNOWN;
> +    if (!isRegistered || !haveDataConnection) {

Why not simply
  if (dataInfo.state != RIL.GECKO_MOBILE_CONNECTION_STATE_REGISTERED ||
      dataInfo.type == RIL.GECKO_MOBILE_CONNECTION_STATE_UNKNOWN) {
      return;
  }

so we don't need to create new objects.

@@ +2949,4 @@
>  
>    // APN failed connections. Retry counter
>    apnRetryCounter: 0,
> +  ifaceRefCounter: 0,

Please comment on ifaceRefCounter!

@@ +3017,5 @@
>                                  Ci.nsITimer.TYPE_ONE_SHOT);
>    },
>  
>    disconnect: function disconnect() {
> +    if (--this.ifaceRefCounter != 0) {

no inline operation, please.

Comment 8

[Ken Chang[:kenkai]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> Comment on attachment 716427 [details] [diff] [review]
> WIP V2
> 
> Review of attachment 716427 [details] [diff] [review]:
> -----------------------------------------------------------------
Thanks for your review. I will be based on your comments to modify the code.

> @@ +1211,5 @@
> >  
> >    updateRILNetworkInterface: function updateRILNetworkInterface() {
> > +    let apnSetting = this.dataCallSettings.apnContexts.byType["default"];
> > +    if (!apnSetting || !this.ensureDataCallSettings(apnSetting) ||
> > +         !apnSetting.iface) {
> 
> Don't we need to check whether we've received 'ril.data.enabled' and
> 'ril.data.roaming_enabled' from settings DB as the original code does?
Because 'ril.data.enabled' and 'ril.data.roaming_enabled' are checked 
in following code, I think original codes are redundant.
+    if (!this.dataCallSettings["enabled"] || apnSetting.iface.connected) {
+    if (dataInfo.roaming && !this.dataCallSettings["roamingEnabled"]) {

> > +    if (datacall.ifname && (datacall.apn == dataApn) &&
> > +         this.dataCallSettings.enabled) {
> 
> Why check 'this.dataCallSettings.enabled' here? We will have
> 'this.dataCallSettings.enabled == false' but we still need to notify data
> info changed, because the data info could change from 'enabled' to
> 'disabled', right?
Yes, this condition is not necessary.
 
> @@ +1693,5 @@
> > +    let apnSetting = this.dataCallSettings.apnContexts.byAPN[dataApn];
> > +    if (((datacall.state == RIL.GECKO_NETWORK_STATE_UNKNOWN) ||
> > +         (datacall.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED)) &&
> > +         (apnSetting.type.length == 0)) {
> > +       delete this.dataCallSettings.apnContexts.byAPN[dataApn];
> 
> Should be |.apnContexts.byAPN[dataApn.apn]| ??
dataApn is alreay an 'apn'. 
> 
> Maybe we could make the stuff done before delivering callback?
Because the delivering callback needs the information of apn setting, I think
it is better to delete apn setting after delivering callback.

> @@ +1962,5 @@
> >        case "ril.data.enabled":
> > +        debug("'ril.data.enabled' is now " + aResult);
> > +        this.dataCallSettings.oldEnabled = this.dataCallSettings.enabled;
> > +        this.dataCallSettings.enabled = aResult;
> > +        updateDataNetworkInterface = true;
> 
> I don't see why we need this flag? Can't we just call
> this.updateRILNetworkInterface()?
Yes, we can.

Comment 9

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

(In reply to Ken Chang from comment #8)
> Yes, this condition is not necessary.
>  
> > @@ +1693,5 @@
> > > +    let apnSetting = this.dataCallSettings.apnContexts.byAPN[dataApn];
> > > +    if (((datacall.state == RIL.GECKO_NETWORK_STATE_UNKNOWN) ||
> > > +         (datacall.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED)) &&
> > > +         (apnSetting.type.length == 0)) {
> > > +       delete this.dataCallSettings.apnContexts.byAPN[dataApn];
> > 
> > Should be |.apnContexts.byAPN[dataApn.apn]| ??
> dataApn is alreay an 'apn'. 
Oops... you are right! I might get dazzled. Please ignore this comment :)

Comment 10

[Mozilla RelEng Bot]

Try run for 36e0da7c95ba is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=36e0da7c95ba
Results (out of 356 total builds):
    success: 348
    warnings: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-36e0da7c95ba

Comment 11

[Ken Chang[:kenkai]]

Attachment: WIP V3

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 721097 [details] [diff] [review]
WIP V3

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +267,5 @@
>    // Read preferred network type from the setting DB.
>    lock.get("ril.radio.preferredNetworkType", this);
>  
>    // Read the APN data from the settings DB.
> +  lock.get("ril.data.apnSettings", this);

What's the impact of this change? I mean is the way of storing APN settings in the setting database changing as well? If so, file a follow-up bug for updating the Gaia side as well please.

Comment 13

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #12)
> Comment on attachment 721097 [details] [diff] [review]
> WIP V3
> 
> Review of attachment 721097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +267,5 @@
> >    // Read preferred network type from the setting DB.
> >    lock.get("ril.radio.preferredNetworkType", this);
> >  
> >    // Read the APN data from the settings DB.
> > +  lock.get("ril.data.apnSettings", this);
> 
> What's the impact of this change? I mean is the way of storing APN settings
> in the setting database changing as well? If so, file a follow-up bug for
> updating the Gaia side as well please.

We've already had that, see bug 842252 :)

Comment 14

[Shian-Yow Wu [:swu]]

Comment on attachment 721097 [details] [diff] [review]
WIP V3

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

This patch looks good to me, nice work!
Please address below comments and remove all trailing spaces.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1106,5 @@
>      if (this.rilContext.radioState == RIL.GECKO_RADIOSTATE_READY &&
>          !this._radioEnabled) {
>        this.setRadioEnabled(false);
>      }
>    },

Add an empty line here.

@@ +1109,5 @@
>      }
>    },
> +  /*
> +   * Create or update the apn setting data context.
> +   */

Change /* to /** for consistency, and change 'apn' to 'APN' here and all other comments.

@@ +1120,5 @@
> +    if (!apnContexts.byAPN[apn]) {
> +      apnContexts.byAPN[apn] = new Object();
> +    } else if (apnContexts.byAPN[apn].iface &&
> +               apnContexts.byAPN[apn].iface.connected) {
> +      apnContexts.byAPN[apn].iface.disconnect();

If I understand correctly, the implementation here expects only default APN type to be reconnected later on if data call is enabled.  Other APN types(MMS, SUPL) rely on corresponding modules to reconnect if needed.  This looks reasonable to me.

@@ +1132,5 @@
> +     */
> +    apnContexts.byAPN[apn].connectedTypes = [];
> +    // Check all types of this apnSetting.
> +    for each (let type in apnContexts.byAPN[apn].type) {
> +      /* This type was stored and has different apn, remove it and 

Nit: trailing space.  And please use same style for comment inside function call.

@@ +1686,5 @@
>      }
>  
>      this._deliverDataCallCallback("dataCallStateChanged",
>                                    [datacall]);
> +    // If this state update is from removing apnSeting action, delete this apn.

Nit: s/apnSeting/apnSetting/

@@ +1691,5 @@
> +    apnSetting = this.dataCallSettings.apnContexts.byAPN[datacall.apn];
> +    if (apnSetting && 
> +        (datacall.state == RIL.GECKO_NETWORK_STATE_UNKNOWN ||
> +         datacall.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED) &&
> +        apnSetting.type.length == 0) {

Move apnSetting.type.length to be together with apnSetting.

if (apnSetting && apnSetting.type.length &&
    ......)

@@ +2681,5 @@
>      }
>    },
>  
>    /**
> +   * update data call connection state.

Nit: s/update/Update/

@@ +2711,5 @@
> +    }
> +    if (this.dataCallSettings.apnContexts.byType[apntype] &&
> +        this.dataCallSettings.apnContexts.byType[apntype].iface &&
> +        !this.dataCallSettings.apnContexts.byType[apntype].iface.connected) {
> +      this.dataCallSettings.apnContexts.byType[apntype].iface.connect();

Replace all the above this.dataCallSettings.apnContexts.byType[apntype] with apnSetting.

@@ +2728,5 @@
>      }
> +    if (apnSetting.connectedTypes.length == 0 &&
> +        this.dataCallSettings.apnContexts.byType[apntype] &&
> +        this.dataCallSettings.apnContexts.byType[apntype].iface) {
> +      this.dataCallSettings.apnContexts.byType[apntype].iface.disconnect();

Replace all the above this.dataCallSettings.apnContexts.byType[apntype] with apnSetting.

@@ +2835,5 @@
>  
>    NETWORK_TYPE_WIFI:        Ci.nsINetworkInterface.NETWORK_TYPE_WIFI,
>    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,

Add an empty line here.

@@ +2839,5 @@
>    NETWORK_TYPE_MOBILE_SUPL: Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL,
> +  /* Network interface should not care about the network type. 
> +   * Let the definition of other types to be same as the one of supl type.
> +   */
> +  NETWORK_TYPE_MOBILE_OTHERS: Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL,

Add an empty line here.

Comment 15

[Ken Chang[:kenkai]]

Comment on attachment 721097 [details] [diff] [review]
WIP V3

Here are the test scenarios for this WIP.
A. Precondition: mms and data connections share the same APN Setting.
 1. Quickly enable/disable data connection 20 times.
 2. Quickly enable/disable mms connection 20 times.
 3. Enable mms connection and then enable data connection.
 4. Enable data connection and then enable mms connection.
 5. Disable mms connection and then disable data connection.
 6. Diable data connection and then disable mms connection.
 7. Enable mms, then quickly enable/disable data call 20 times. 
 8. Enable data, then quickly enable/disable mms call 20 times.
 9. Enable data connection, and then quicky enable/disable WIFI 20 times.
10. Enable WIFI, and then quicky enable/disable data connection 20 times. 
11. Enable data and mms connections, and then quicky enable/disable WIFI connection 20 
    times.
12. Enable data connection with the wrong apn setting.
13. Enable mms connection with the wrong apn setting.

B. Precondition: mms and data connections use the different APN Setting.
 Do the same test scenarios with what I have done for A.

Comment 16

[Shian-Yow Wu [:swu]]

Please add this scenario:
- Change APN setting when data call is connected.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 721097 [details] [diff] [review]
WIP V3

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

This patch still has lotsssssssss places to be fixed, even just in comments, styles, ....
And I believe firmly this patch could be much shorter and easier to understand.

For reviewers,
Please don't encourage people to write a patch like this and don't even r+ on it.
It will make our lives in hell.
Everyone in RIL team is trying hard to make their patches look well and test well,
even it's just to remove an empty space.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +215,5 @@
> +    apnContexts: {
> +      byType: {},
> +      byAPN: {}
> +    }
> +  };

These fields must be commented.

@@ +1115,5 @@
> +    if (!this.validateApnSetting(apnSetting)) {
> +      return;
> +    }
> +    let apn = apnSetting["apn"];
> +    let apnContexts = this.dataCallSettings.apnContexts;

Try to make your code shorter
let apnContext = this.dataCallSettins.apnContexts[apn];

@@ +1117,5 @@
> +    }
> +    let apn = apnSetting["apn"];
> +    let apnContexts = this.dataCallSettings.apnContexts;
> +    if (!apnContexts.byAPN[apn]) {
> +      apnContexts.byAPN[apn] = new Object();

.. = {};

@@ +1128,5 @@
> +      apnContexts.byAPN[apn][apnItem] = apnSetting[apnItem];
> +    }
> +    /* The connectedTypes is used to store those types for which apnSetting has
> +     * requested a connection.
> +     */

Please make your comments style consistent,
in this case, please use //

@@ +1151,5 @@
> +            } else {
> +              delete apnContexts.byAPN[apnContexts.byType[type].apn];
> +            }
> +          }
> +        }

4 levels of if.
You must be crazy.

@@ +1161,5 @@
> +      apnContexts.byAPN[apn].iface = new RILNetworkInterface(this, apnSetting);
> +    }
> +
> +    return;
> +  },

add an empty line

@@ +1172,5 @@
> +        !apnSetting.type ||
> +        apnSetting.type.length == 0) {
> +      return false;
> +    }
> +    return true;

return apnSettings && apnSettings.apn && .. ;

@@ +1235,5 @@
>      }
>  
> +    if (apnSetting.iface.state == RIL.GECKO_NETWORK_STATE_CONNECTING ||
> +        apnSetting.iface.state == RIL.GECKO_NETWORK_STATE_DISCONNECTING ||
> +        apnSetting.iface.connecting) {

What's the differece between iface.connecting and iface.state is NETWORK_STATE_CONNECTING?

@@ +1262,3 @@
>          (!this.dataCallSettings["enabled"] ||
> +         (dataInfo.roaming && !this.dataCallSettings["roamingEnabled"]) ||
> +         wifi_active)) {

Can we make this check simpler?

I guess I need to drink 10 coffees to understand when these statements will become true.

@@ +1676,5 @@
>     * Handle data call state changes.
>     */
>    handleDataCallState: function handleDataCallState(datacall) {
>      let data = this.rilContext.data;
> +    let apnSetting = this.dataCallSettings.apnContexts.byType["default"];

If this object is returned from apnContexts,
it should be called 'apnContext',
otherwise we'll confuse with apnSettings and apnContext.

Comment 18

[Ken Chang[:kenkai]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #17)
Thanks for your review.
> @@ +1115,5 @@
> > +    if (!this.validateApnSetting(apnSetting)) {
> > +      return;
> > +    }
> > +    let apn = apnSetting["apn"];
> > +    let apnContexts = this.dataCallSettings.apnContexts;
> 
> Try to make your code shorter
> let apnContext = this.dataCallSettins.apnContexts[apn];
Because it just is apnContexts, if we change to apnContext, it will be confusing to others people. 

> @@ +1151,5 @@
> > +            } else {
> > +              delete apnContexts.byAPN[apnContexts.byType[type].apn];
> > +            }
> > +          }
> > +        }
> 
> 4 levels of if.
> You must be crazy.
I follow the comment #7(as following) to do this.

> > @@ +1977,2 @@
> > >          }
> > > +        this.checkIfRemovingApn();
> > 
> > I would like to merge "createApnContext" and "checkIfRemovingApn" into a
> > function naming something like "updateApnContext" and move the 'for each'
> > loop into the updateApnContext function, because the two functions do more
> > than their names represent and they also update apn context.


> 
> @@ +1235,5 @@
> >      }
> >  
> > +    if (apnSetting.iface.state == RIL.GECKO_NETWORK_STATE_CONNECTING ||
> > +        apnSetting.iface.state == RIL.GECKO_NETWORK_STATE_DISCONNECTING ||
> > +        apnSetting.iface.connecting) {
> 
> What's the differece between iface.connecting and iface.state is
> NETWORK_STATE_CONNECTING?
When dialling up data call, we set apnSetting.iface.connecting to be true. And
apnSetting.iface.state is only set to be RIL.GECKO_NETWORK_STATE_DISCONNECTING 
when RILD update the data call status.
There may be a little time delay between setting apnSetting.iface.connecting to be true and updating the data call status.

> @@ +1262,3 @@
> >          (!this.dataCallSettings["enabled"] ||
> > +         (dataInfo.roaming && !this.dataCallSettings["roamingEnabled"]) ||
> > +         wifi_active)) {
> 
> Can we make this check simpler?
> 
> I guess I need to drink 10 coffees to understand when these statements will
> become true.
>
Ha! Okay, but this modification will change original code flow and add more codes.

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Please address Shainyow's and Hsinyi's comments first,
I'll show you how to make your code shorter and easier to understand.

Comment 20

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

(In reply to Ken Chang from comment #18)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #17)
> 
> > @@ +1151,5 @@
> > > +            } else {
> > > +              delete apnContexts.byAPN[apnContexts.byType[type].apn];
> > > +            }
> > > +          }
> > > +        }
> > 
> > 4 levels of if.
> > You must be crazy.
> I follow the comment #7(as following) to do this.

Yes, I asked Ken to merge his original two functions into one. Thanks for Yoshi's insight here. It looks like that we can still make the code better and clearer. Ken, let's keep improving the code. Sorry and thanks!

Comment 23

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

Comment on attachment 721097 [details] [diff] [review]
WIP V3

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

Thanks for the patch! I believe we can make it better and we sould do it.
Overall, nesting level of if-conditions over the code is generally quite deep that is error-prone, and I did notice some problems there. Let's improve and correct them. We also need to meet the sharing apn scenario as bug 794342 comment#0 mentioned. Moreover, bail out as early as possible.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1108,5 @@
>        this.setRadioEnabled(false);
>      }
>    },
> +  /*
> +   * Create or update the apn setting data context.

Please indicate more about *why* we need byAPN and byType.

@@ +1121,5 @@
> +      apnContexts.byAPN[apn] = new Object();
> +    } else if (apnContexts.byAPN[apn].iface &&
> +               apnContexts.byAPN[apn].iface.connected) {
> +      apnContexts.byAPN[apn].iface.disconnect();
> +    }

Add an empty line.

@@ +1122,5 @@
> +    } else if (apnContexts.byAPN[apn].iface &&
> +               apnContexts.byAPN[apn].iface.connected) {
> +      apnContexts.byAPN[apn].iface.disconnect();
> +    }
> +    // Copy apn Settings.

nit: s/apn Settings/APN settings

@@ +1125,5 @@
> +    }
> +    // Copy apn Settings.
> +    for (let apnItem in apnSetting) {
> +      apnContexts.byAPN[apn][apnItem] = apnSetting[apnItem];
> +    }

Add an empty line here.

@@ +1126,5 @@
> +    // Copy apn Settings.
> +    for (let apnItem in apnSetting) {
> +      apnContexts.byAPN[apn][apnItem] = apnSetting[apnItem];
> +    }
> +    /* The connectedTypes is used to store those types for which apnSetting has

Change the comment format. Use // instead.

@@ +1131,5 @@
> +     * requested a connection.
> +     */
> +    apnContexts.byAPN[apn].connectedTypes = [];
> +    // Check all types of this apnSetting.
> +    for each (let type in apnContexts.byAPN[apn].type) {

Sorry for not indicating this before, but I would like to change the format to byAPN[apn].type*s* (byType[type].type*s*). Such naming makes more sense to me.

@@ +1143,5 @@
> +        if (index != -1) {
> +          apnContexts.byType[type].type.splice(index, 1);
> +          if (apnContexts.byType[type].type.length == 0) {
> +            // If apnContext's network interface is connected, disconnect it.
> +            if (apnContexts.byType[type].iface && 

nit: trailing whitespace

@@ +1151,5 @@
> +            } else {
> +              delete apnContexts.byAPN[apnContexts.byType[type].apn];
> +            }
> +          }
> +        }

We can bail out earlier or use 'continue;' to decrease the level here. Let's enhance the readability that will help maintainace of the code as well!

@@ +1153,5 @@
> +            }
> +          }
> +        }
> +      }
> +      apnContexts.byType[type] = apnContexts.byAPN[apn];

Why doing this even when |apnContexts.byType[type] == apnContexts.byAPN[apn]|?

@@ +1162,5 @@
> +    }
> +
> +    return;
> +  },
> +  /*

Make comment style consistent.

@@ +1212,5 @@
>  
>    updateRILNetworkInterface: function updateRILNetworkInterface() {
> +    let apnSetting = this.dataCallSettings.apnContexts.byType["default"];
> +    if (!this.validateApnSetting(apnSetting) || !apnSetting.iface) {
> +      debug("We haven't gotten completely the APN data.");

Please also check whether we've received'ril.data.enabled' and 'ril.data.roaming_enabled' from settings DB here. Bail out as early as possible.

@@ +1267,3 @@
>        return;
>      }
> +    if (!this.dataCallSettings["enabled"] || 

nit: trailing whitespace

@@ +1688,5 @@
>      this._deliverDataCallCallback("dataCallStateChanged",
>                                    [datacall]);
> +    // If this state update is from removing apnSeting action, delete this apn.
> +    apnSetting = this.dataCallSettings.apnContexts.byAPN[datacall.apn];
> +    if (apnSetting && 

nit: trailing whitespace

@@ +1970,5 @@
> +        break;
> +      case "ril.data.apnSettings":
> +        debug("'ril.data.apnSettings' is now " + JSON.stringify(aResult));
> +        for each (let apnSetting in aResult) {
> +          this.updateApnContext(apnSetting);

Move "for each" into the update function, and rename it 'updateApnContexts.'

And we need to pay more attention to that function implementation, trying to reduce levels and offer good readability.

@@ +2700,3 @@
>    setupDataCallByType: function setupDataCallByType(apntype) {
> +    let dataInfo = this.rilContext.data;
> +    if (dataInfo.state != RIL.GECKO_MOBILE_CONNECTION_STATE_REGISTERED || 

nit: trailing whitespace

@@ +2704,3 @@
>        return;
>      }
> +    let apnSetting = this.dataCallSettings.apnContexts.byType[apntype];

if (!apnSetting) {
  return;
}
Bail out as early as possible and it can make your following if-conditions simpler. Try to do the same thing in other places in the code.

@@ +2713,5 @@
> +        this.dataCallSettings.apnContexts.byType[apntype].iface &&
> +        !this.dataCallSettings.apnContexts.byType[apntype].iface.connected) {
> +      this.dataCallSettings.apnContexts.byType[apntype].iface.connect();
> +    } else {
> +      // Updating data call status to upper layer.

else-condition for updating DataInfo isn't right, but anyway, in considering Bug 794342 comment#0 item3, revision here is needed.

@@ +2728,2 @@
>      }
> +    if (apnSetting.connectedTypes.length == 0 &&

Use |!apnSetting.connectedTypes.length| for here and other places.

@@ +2730,5 @@
> +        this.dataCallSettings.apnContexts.byType[apntype] &&
> +        this.dataCallSettings.apnContexts.byType[apntype].iface) {
> +      this.dataCallSettings.apnContexts.byType[apntype].iface.disconnect();
> +    } else {
> +      // Updating data call status to upper layer.

Again, else-condition for updating DataInfo isn't right, but anyway, in considering Bug 794342 comment#0 item3, revision here is needed.

@@ +2836,5 @@
>    NETWORK_TYPE_WIFI:        Ci.nsINetworkInterface.NETWORK_TYPE_WIFI,
>    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 interface should not care about the network type. 

nit: please make the comment style consistent & remove trailing whitespace.

@@ +2838,5 @@
>    NETWORK_TYPE_MOBILE_MMS:  Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS,
>    NETWORK_TYPE_MOBILE_SUPL: Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL,
> +  /* Network interface should not care about the network type. 
> +   * Let the definition of other types to be same as the one of supl type.
> +   */

The comment here doesn't provide more information. What we need is *why* we use 'supl' type as default. Please comment that, thanks!

@@ +2857,5 @@
> +      return this.NETWORK_TYPE_MOBILE;
> +    } else if (this.apnSetting.type.indexOf("mms") != -1) {
> +      return this.NETWORK_TYPE_MOBILE_MMS;
> +    } else if (this.apnSetting.type.indexOf("supl") != -1) {
> +      return this.NETWORK_TYPE_MOBILE_SUPL;

Don't put an else right after a return. Delete the else, it's unnecessary and increases indentation level.

Something wrong here, please see bug 794342 comment#0 item3. I.e.
"3. If MMS activates data APN when data APN disabled by user, we should only enable the routes necessary for MMS, and we should not set navigator.mozMobileConnection.data.connected and navigator.onLine to true."

The implementation violates that.

Also, because of this, setupDatacallByType() and deactiveDatacallByType() should be revised accordingly. We must be careful of sending datainfochanged events.

Comment 24

[Ken Chang[:kenkai]]

Attachment: Using array for data call settings and handling multiple sharing

Comment 25

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

Comment on attachment 728846 [details] [diff] [review]
Using array for data call settings and handling multiple sharing

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

Thanks for the patch! And, let's keep discussing :)

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +220,5 @@
>  
> +
> +  // The "oldEnabled" is the previous data enable flag.
> +  // The "enabled" is the current data enable flag.
> +  // The "roamingEnabled" is the roaming enable flag.

Remove these comments as the code itself is clear enough ;-)

@@ +227,5 @@
> +    enabled: false,
> +    roamingEnabled: false
> +  };
> +
> +  // The "apnSettings" is used to keep all apn settings.

s/The "apnSettings"/"apnSettings"

@@ +228,5 @@
> +    roamingEnabled: false
> +  };
> +
> +  // The "apnSettings" is used to keep all apn settings.
> +  // The byApn[] makes it is easier to get the apn setting via apn name.

"byApn[] makes it easier to get the apn settings via APN names."

@@ +229,5 @@
> +  };
> +
> +  // The "apnSettings" is used to keep all apn settings.
> +  // The byApn[] makes it is easier to get the apn setting via apn name.
> +  // The byType[] makes it is easier to get the apn setting via apn type.

"byType[] makes it easier to get the apn settings via APN types."

And wrap these lines at the 80th character.

@@ +968,3 @@
>      }
>  
>      this._deliverDataCallCallback("dataCallError", [message]);

Shouldn't we update apnSetting.connectedTypes?

B.t.w, should be out of the scope of this issue: but, where is the callback 'dataCallError' defined? It is supposed to be defined in "nsIRILCallback" but I don't see there. If this is indeed an issue, please file a bug.

@@ +1158,5 @@
>      }
>    },
>  
> +  /**
> +   * This function will do the following steps

Put a colon at the end of this line.

@@ +1160,5 @@
>  
> +  /**
> +   * This function will do the following steps
> +   * 1. Clear old APN settings.
> +   * 1. Use apn name as the index to store apn setting into the byApn{}.

Correct the item number.

s/apn/APN and grammatic error

@@ +1163,5 @@
> +   * 1. Clear old APN settings.
> +   * 1. Use apn name as the index to store apn setting into the byApn{}.
> +   *    This make it easy to get the apn setting via apn name.
> +   * 2. Use apn type as the index to update the link between byType{} and
> +   *    byApn{}. This make it easy to get the apn setting via apn type.

nit: grammatic error

@@ +1187,2 @@
>        for (let apnIndex = 0; thisSimApnSettings[apnIndex]; apnIndex++) {
> +        let InputApnSetting = thisSimApnSettings[apnIndex];

s/InputApnSetting/inputApnSetting.
Doing the same for all the followings.

@@ +1191,4 @@
>          }
>  
> +        let apn = InputApnSetting["apn"];
> +        // To use apn name to get a element of byApn{}.

"Use APN name to get the corresponding APN setting."

@@ +1249,5 @@
>        this.worker.postMessage({rilMessageType: "setCallWaiting", enabled: value});
>      }
>    },
>  
>    updateRILNetworkInterface: function updateRILNetworkInterface() {

I wonder whether the name 'updateRILNetworkInterface' is proper. This function focuses only on updating data call of 'default' type. So, how does renaming sound?

@@ +1296,5 @@
>          gNetworkManager.active.type == Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {
>        wifi_active = true;
>      }
>  
> +    let dataConnected = apnSetting.connectedTypes.indexOf("default") != -1;

I don't like the name 'dataConnected' since 'dataConnected' could imply data call of mms as well. The name should be more targeted on 'default' type. 

Let's see if we can have a better name here and all related places over the file.

@@ +1306,4 @@
>        return;
>      }
> +
> +    if (dataConnected && wifi_active) {

When will this happen?

@@ +1768,1 @@
>        this._sendMobileConnectionMessage("RIL:DataInfoChanged", data);

If this.apnSettings.byType["default"].connectedType.indexOf("default") == -1, then why do we still need to send "RIL:DataInfoChanged"?

@@ +1989,5 @@
>    // 'ril.callwaiting.enbled' setting from the UI.
>    _callWaitingEnabled: null,
>  
> +  // Data calls setting.
> +  dataCallSettings: null,

We also need 'apnSettings' here, no?

@@ +2811,5 @@
> +    let dataInfo = this.rilContext.data;
> +    let apnSetting = this.apnSettings.byType[apntype];
> +    if (dataInfo.state != RIL.GECKO_MOBILE_CONNECTION_STATE_REGISTERED ||
> +        dataInfo.type == RIL.GECKO_MOBILE_CONNECTION_STATE_UNKNOWN ||
> +        !apnSetting) {

How about this?

let apnSetting = this.apnSettings.byType[apntype];
if (!apnSetting) {
  return;
}

let dataInfo = this.rilContext.data;
if (dataInfo.state != RIL.GECKO_MOBILE_CONNECTION_STATE_REGISTERED ||
    dataInfo.type == RIL.GECKO_MOBILE_CONNECTION_STATE_UNKNOWN) {
  return;
}

@@ +2815,5 @@
> +        !apnSetting) {
> +      return;
> +    }
> +
> +    // Check if this types had connected.

Part of me has a feeling that we could remove this comment.

@@ +2819,5 @@
> +    // Check if this types had connected.
> +    if (apnSetting.connectedTypes.indexOf(apntype) == -1) {
> +       apnSetting.connectedTypes.push(apntype);
> +    }
> +    apnSetting.iface.apnSetting.connectedTypes = apnSetting.connectedTypes;

This is very garrulous. Can we have a way to improving it @@?

@@ +2823,5 @@
> +    apnSetting.iface.apnSetting.connectedTypes = apnSetting.connectedTypes;
> +
> +    if (apnSetting.iface.connected) {
> +      // Updating data call status to upper layer.
> +      if (apntype == "default" && dataInfo.connected != true) {

s/dataInfo.connected != true/!dataInfo.connected

@@ +2824,5 @@
> +
> +    if (apnSetting.iface.connected) {
> +      // Updating data call status to upper layer.
> +      if (apntype == "default" && dataInfo.connected != true) {
> +        this.rilContext.data.connected = true;

s/this.rilContext.data.connected/dataInfo.connected

@@ +2828,5 @@
> +        this.rilContext.data.connected = true;
> +        this._sendTargetMessage("mobileconnection", "RIL:DataInfoChanged", dataInfo);
> +      }
> +
> +      // Updating network interface.

The comment looks not so right to me. Seems like you miss some explanation; Please complement it :)

@@ +2831,5 @@
> +
> +      // Updating network interface.
> +      Services.obs.notifyObservers(apnSetting.iface,
> +                                   kNetworkInterfaceStateChangedTopic,
> +                                   null);

Forgot "return;" here?

@@ +2833,5 @@
> +      Services.obs.notifyObservers(apnSetting.iface,
> +                                   kNetworkInterfaceStateChangedTopic,
> +                                   null);
> +    }
> +    this.apnSettings.byType[apntype].iface.connect();

s/this.apnSettings.byType[apntype]/apnSetting

@@ +2843,5 @@
> +      return;
> +    }
> +    // Check if this types had connected.
> +    let index = apnSetting.connectedTypes.indexOf(apntype);
> +    if (index != -1) {

If index == -1, i.e. connection of this type isn't established, then we should just return, right?

@@ +2846,5 @@
> +    let index = apnSetting.connectedTypes.indexOf(apntype);
> +    if (index != -1) {
> +      apnSetting.connectedTypes.splice(index, 1);
> +    }
> +    apnSetting.iface.apnSetting.connectedTypes = apnSetting.connectedTypes;

This is very garrulous. Can we have a way to improving it @@?

@@ +2851,5 @@
> +
> +    if (apnSetting.connectedTypes.length) {
> +      let dataInfo = this.rilContext.data;
> +      // Updating data call status to upper layer.
> +      if (apntype == "default" && dataInfo.connected != false) {

s/dataInfo.connected != false/dataInfo.connected

@@ +2856,5 @@
> +        dataInfo.connected = false;
> +        this._sendTargetMessage("mobileconnection", "RIL:DataInfoChanged", dataInfo);
> +      }
> +
> +      // Updating network interface.

How can we update network interface? It looks like you only send out a notification. Please describe more here.

@@ +2970,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,
> +  // The network manager should only need to add the host route for "other"
> +  // types, which is same handling method as the supl type. So let the

s/same/the same

@@ +2971,5 @@
>    NETWORK_TYPE_MOBILE_MMS:  Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS,
>    NETWORK_TYPE_MOBILE_SUPL: Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL,
> +  // The network manager should only need to add the host route for "other"
> +  // types, which is same handling method as the supl type. So let the
> +  // definition of other types to be same as the one of supl type.

s/same/the same

@@ +2986,5 @@
>    // Event timer for connection retries
>    timer: null,
>  
> +  get type() {
> +    let returnType = this.NETWORK_TYPE_MOBILE_OTHERS;

s/returnType/type

@@ +2990,5 @@
> +    let returnType = this.NETWORK_TYPE_MOBILE_OTHERS;
> +    if (this.apnSetting.connectedTypes.indexOf("default") != -1) {
> +      returnType = this.NETWORK_TYPE_MOBILE;
> +    } else if (this.apnSetting.connectedTypes.indexOf("mms") != -1) {
> +      returnType = this.NETWORK_TYPE_MOBILE_MMS;

Don't put an else right after a return. Delete the else, it's unnecessary and increases indentation level.

@@ +3061,5 @@
>        if (!this.registeredAsNetworkInterface) {
>          gNetworkManager.registerNetworkInterface(this);
>          this.registeredAsNetworkInterface = true;
> +        let mRILapnSetting = this.mRIL.apnSettings.byAPN[this.apnSetting.apn];
> +        mRILapnSetting.connectedTypes = this.apnSetting.connectedTypes;

I don't quite understand why we need this.

RILNetworkInterface.apnSetting.connectedTypes is determined in RadioInterfaceLayer.setupDataCallByType() and .deactivateDataCallByType(). Then if data call is setup successfully, this, RILNetworkInterface.dataCallStateChanged(), is called. In the flow I don't see anything that makes RILNetworkInterface.apnSetting.connectedTypes and mRILapnSetting.connectedTypes become inconsistant. So, why we need this?

@@ +3086,5 @@
>        this.mRIL.updateRILNetworkInterface();
>      }
>  
>      if (this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN &&
>         this.registeredAsNetworkInterface) {

nit: indention

@@ +3090,5 @@
>         this.registeredAsNetworkInterface) {
>        gNetworkManager.unregisterNetworkInterface(this);
>        this.registeredAsNetworkInterface = false;
>        this.cid = null;
> +      this.mRIL.apnSettings.byAPN[this.apnSetting.apn].connectedTypes = [];

Why do we need this?

If we do need this, don't we need this.apnSetting.connectedTypes = [] as well?

@@ +3151,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.mRIL.apnSettings.byAPN[this.apnSetting.apn].connectedTypes = [];

Again, why this here? 
Shouldn't this be handled in RadioInterfaceLayer.handleDataCallError?

Also, shouldn't we update RILNetworkInterface.apnSetting.connectedTypes as well?

Comment 26

[Ken Chang[:kenkai]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #25)
Thanks for your review.
> 
> @@ +968,3 @@
> >      }
> >  
> >      this._deliverDataCallCallback("dataCallError", [message]);
> 
> Shouldn't we update apnSetting.connectedTypes?
Because the data call error will trigger the re-establishing data call procedure, we can not
directly update apnSetting.connectedTypes here. Or when the re-establishing data call procedure is successful, there is the different connectedTypes between apnSetting and network 
interface.  

> 
> B.t.w, should be out of the scope of this issue: but, where is the callback
> 'dataCallError' defined? It is supposed to be defined in "nsIRILCallback"
> but I don't see there. If this is indeed an issue, please file a bug.
Okay, I will look into it.

> >  
> >    updateRILNetworkInterface: function updateRILNetworkInterface() {
> 
> I wonder whether the name 'updateRILNetworkInterface' is proper. This
> function focuses only on updating data call of 'default' type. So, how does
> renaming sound?
> 
Because the network manager also use this function, I think it is better to 
create another bug for this change.

> @@ +1306,4 @@
> >        return;
> >      }
> > +
> > +    if (dataConnected && wifi_active) {
> 
> When will this happen?
When the data call has been connected and then user turns on wifi, 
this condition is fulfilled.

> 
> @@ +1768,1 @@
> >        this._sendMobileConnectionMessage("RIL:DataInfoChanged", data);
> 
> If this.apnSettings.byType["default"].connectedType.indexOf("default") ==
> -1, then why do we still need to send "RIL:DataInfoChanged"?
>
This function is for updating the default data connection status. In the circumstance what
you said, we should notify that the default data connection is disconnected.

> 
> @@ +3061,5 @@
> >        if (!this.registeredAsNetworkInterface) {
> >          gNetworkManager.registerNetworkInterface(this);
> >          this.registeredAsNetworkInterface = true;
> > +        let mRILapnSetting = this.mRIL.apnSettings.byAPN[this.apnSetting.apn];
> > +        mRILapnSetting.connectedTypes = this.apnSetting.connectedTypes;
> 
> I don't quite understand why we need this.
> 
> RILNetworkInterface.apnSetting.connectedTypes is determined in
> RadioInterfaceLayer.setupDataCallByType() and .deactivateDataCallByType().
> Then if data call is setup successfully, this,
> RILNetworkInterface.dataCallStateChanged(), is called. In the flow I don't
> see anything that makes RILNetworkInterface.apnSetting.connectedTypes and
> mRILapnSetting.connectedTypes become inconsistant. So, why we need this?
> 
> 
> @@ +3090,5 @@
> >         this.registeredAsNetworkInterface) {
> >        gNetworkManager.unregisterNetworkInterface(this);
> >        this.registeredAsNetworkInterface = false;
> >        this.cid = null;
> > +      this.mRIL.apnSettings.byAPN[this.apnSetting.apn].connectedTypes = [];
> 
> Why do we need this?
> 
> If we do need this, don't we need this.apnSetting.connectedTypes = [] as
> well?
> 
> @@ +3151,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.mRIL.apnSettings.byAPN[this.apnSetting.apn].connectedTypes = [];
> 
> Again, why this here? 
> Shouldn't this be handled in RadioInterfaceLayer.handleDataCallError?
> 
> Also, shouldn't we update RILNetworkInterface.apnSetting.connectedTypes as
> well?
Here are my responses for proceeding questions.
I want to handle the following race condition.
1. The data call procedure is failed, the re-establishing data call procedure is running in
   background.
2. And another data call is triggered for different type at the same time.
3. Then there are a few chances that connectedTypes will different between interface
   and apnSetting.
In order to resolve this race condition, we have to let apnSetting know the exact
connectedTypes of network interface when the network interface is connected.

If the network interface is down, we don't have to update the connectedTypes of the network
interface. Since the connectedTypes of the network interface is meaningless.
And setupDataCallByType() will be called and provide the new connectedTypes for the network
interface.

Comment 27

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

(In reply to Ken Chang from comment #26)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #25)
> Thanks for your review.
> > 
> > @@ +968,3 @@
> > >      }
> > >  
> > >      this._deliverDataCallCallback("dataCallError", [message]);
> > 
> > Shouldn't we update apnSetting.connectedTypes?
> Because the data call error will trigger the re-establishing data call
> procedure, we can not
> directly update apnSetting.connectedTypes here. Or when the re-establishing
> data call procedure is successful, there is the different connectedTypes
> between apnSetting and network 
> interface.  
> 

Got it. Thanks.

> 
> > @@ +1306,4 @@
> > >        return;
> > >      }
> > > +
> > > +    if (dataConnected && wifi_active) {
> > 
> > When will this happen?
> When the data call has been connected and then user turns on wifi, 
> this condition is fulfilled.
> 

Per my understanding, while the wifi is turned on in the situation you described, the data connection of default type will be forced to disconnected.
That's why I doubted the condition. I got something wrong?

> > 
> > @@ +1768,1 @@
> > >        this._sendMobileConnectionMessage("RIL:DataInfoChanged", data);
> > 
> > If this.apnSettings.byType["default"].connectedType.indexOf("default") ==
> > -1, then why do we still need to send "RIL:DataInfoChanged"?
> >
> This function is for updating the default data connection status. In the
> circumstance what
> you said, we should notify that the default data connection is disconnected.
> 

I couldn't find my original comment that you quoted, so I am not very sure whether I expressed myself clearly enough. I am sorry if not.

However, the problem I am seeing here is that:
Per your implementation, you will send "RIL:DataInfoChanged" even when |this.apnSettings.byType["default"].connectedType.indexOf("default") == -1|, i.e. this data info isn't for "default" type. The data state of the default type isn't changed at all, so we don't have to send this dummy info, right? 

Or please kindly remind me if there are some edgy cases we discussed before and we agree that there's a need to send this message out.

> > 
> > @@ +3061,5 @@
> > >        if (!this.registeredAsNetworkInterface) {
> > >          gNetworkManager.registerNetworkInterface(this);
> > >          this.registeredAsNetworkInterface = true;
> > > +        let mRILapnSetting = this.mRIL.apnSettings.byAPN[this.apnSetting.apn];
> > > +        mRILapnSetting.connectedTypes = this.apnSetting.connectedTypes;
> > 
> > I don't quite understand why we need this.
> > 
> > RILNetworkInterface.apnSetting.connectedTypes is determined in
> > RadioInterfaceLayer.setupDataCallByType() and .deactivateDataCallByType().
> > Then if data call is setup successfully, this,
> > RILNetworkInterface.dataCallStateChanged(), is called. In the flow I don't
> > see anything that makes RILNetworkInterface.apnSetting.connectedTypes and
> > mRILapnSetting.connectedTypes become inconsistant. So, why we need this?
> > 
> > 
> > @@ +3090,5 @@
> > >         this.registeredAsNetworkInterface) {
> > >        gNetworkManager.unregisterNetworkInterface(this);
> > >        this.registeredAsNetworkInterface = false;
> > >        this.cid = null;
> > > +      this.mRIL.apnSettings.byAPN[this.apnSetting.apn].connectedTypes = [];
> > 
> > Why do we need this?
> > 
> > If we do need this, don't we need this.apnSetting.connectedTypes = [] as
> > well?
> > 
> > @@ +3151,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.mRIL.apnSettings.byAPN[this.apnSetting.apn].connectedTypes = [];
> > 
> > Again, why this here? 
> > Shouldn't this be handled in RadioInterfaceLayer.handleDataCallError?
> > 
> > Also, shouldn't we update RILNetworkInterface.apnSetting.connectedTypes as
> > well?
> Here are my responses for proceeding questions.
> I want to handle the following race condition.
> 1. The data call procedure is failed, the re-establishing data call
> procedure is running in
>    background.
> 2. And another data call is triggered for different type at the same time.
> 3. Then there are a few chances that connectedTypes will different between
> interface
>    and apnSetting.
> In order to resolve this race condition, we have to let apnSetting know the
> exact
> connectedTypes of network interface when the network interface is connected.

I see. Thank you.
However, I have a feeling that we update connectedTypes in too many divergent places, some in RadioInterfaceLayer, some in RILNetworkInterface. I don't have a better idea right now, but would like to see if we can improve it. :) 

> 
> If the network interface is down, we don't have to update the connectedTypes
> of the network
> interface. Since the connectedTypes of the network interface is meaningless.
> And setupDataCallByType() will be called and provide the new connectedTypes
> for the network
> interface.

I don't think updating connectedTypes of the network interface when it is down is meaningless. We need to make all attributes of that network interface consistent with its state, because we will never know what this interface object is gonna be used by others or in the future. If we make all attribute consistent with the  interface state, we can help prevent weird situation or error-prone code, especially this is really an easy fix. Not convincible saying that it's meaningless because the connectedTypes will be updated anyway once someone will setup a new connection in the future. So let's fix it.

One more thing -- we now don't handle the error of requesting deactivating data connection. This patch is gonna remove a type from connectedTypes before receiving the request result from ril_worker.js. Once the request fails, the data state stays wrong. We need to file a bug for addressing that.

Comment 28

[Ken Chang[:kenkai]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #27)
> 
> Per my understanding, while the wifi is turned on in the situation you
> described, the data connection of default type will be forced to
> disconnected.
> That's why I doubted the condition. I got something wrong?
Yes, you are right. what I did here just handles what you said...:)

> 
> I couldn't find my original comment that you quoted, so I am not very sure
> whether I expressed myself clearly enough. I am sorry if not.
> 
> However, the problem I am seeing here is that:
> Per your implementation, you will send "RIL:DataInfoChanged" even when
> |this.apnSettings.byType["default"].connectedType.indexOf("default") == -1|,
> i.e. this data info isn't for "default" type. The data state of the default
> type isn't changed at all, so we don't have to send this dummy info, right? 
> 
> Or please kindly remind me if there are some edgy cases we discussed before
> and we agree that there's a need to send this message out.
> 
I don't change the logic; it is the original design. Because all information is cleaned 
when we call disconnect procedure, we don't know which type trigged this disconnection
procedure and can only decide to send this message by checking the APN.

> > Here are my responses for proceeding questions.
> > I want to handle the following race condition.
> > 1. The data call procedure is failed, the re-establishing data call
> > procedure is running in
> >    background.
> > 2. And another data call is triggered for different type at the same time.
> > 3. Then there are a few chances that connectedTypes will different between
> > interface
> >    and apnSetting.
> > In order to resolve this race condition, we have to let apnSetting know the
> > exact
> > connectedTypes of network interface when the network interface is connected.
> 
> I see. Thank you.
> However, I have a feeling that we update connectedTypes in too many
> divergent places, some in RadioInterfaceLayer, some in RILNetworkInterface.
> I don't have a better idea right now, but would like to see if we can
> improve it. :) 
The followings are the logic. Hope it is helpful.
1. RIL update "wanted" connectedTypes when calling setupDataCallByType() or 
   deactivateDataCallByType().
2. RIL network interface update the "real" connectedTypes when finishing connection or 
   disconnection procedure.
> 
> > 
> > If the network interface is down, we don't have to update the connectedTypes
> > of the network
> > interface. Since the connectedTypes of the network interface is meaningless.
> > And setupDataCallByType() will be called and provide the new connectedTypes
> > for the network
> > interface.
> 
> I don't think updating connectedTypes of the network interface when it is
> down is meaningless. We need to make all attributes of that network
> interface consistent with its state, because we will never know what this
> interface object is gonna be used by others or in the future. If we make all
> attribute consistent with the  interface state, we can help prevent weird
> situation or error-prone code, especially this is really an easy fix. Not
> convincible saying that it's meaningless because the connectedTypes will be
> updated anyway once someone will setup a new connection in the future. So
> let's fix it.
Let's think in this way. When the network interface is create, what is the default value of
connectedTypes? We don't know and don't care because the network interface is down. And we
will give value to the connectedTypes of network when calling setupDataCallByType().
And If we add those codes, we will increase the running time.
But if you insist to add those codes, I will add them.

> 
> One more thing -- we now don't handle the error of requesting deactivating
> data connection. This patch is gonna remove a type from connectedTypes
> before receiving the request result from ril_worker.js. Once the request
> fails, the data state stays wrong. We need to file a bug for addressing that.
OK. No problem.

Comment 29

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

(In reply to Ken Chang from comment #28)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #27)
> > 
> > Per my understanding, while the wifi is turned on in the situation you
> > described, the data connection of default type will be forced to
> > disconnected.
> > That's why I doubted the condition. I got something wrong?
> Yes, you are right. what I did here just handles what you said...:)
> 
> > 
> > I couldn't find my original comment that you quoted, so I am not very sure
> > whether I expressed myself clearly enough. I am sorry if not.
> > 
> > However, the problem I am seeing here is that:
> > Per your implementation, you will send "RIL:DataInfoChanged" even when
> > |this.apnSettings.byType["default"].connectedType.indexOf("default") == -1|,
> > i.e. this data info isn't for "default" type. The data state of the default
> > type isn't changed at all, so we don't have to send this dummy info, right? 
> > 
> > Or please kindly remind me if there are some edgy cases we discussed before
> > and we agree that there's a need to send this message out.
> > 
> I don't change the logic; it is the original design. Because all information
> is cleaned 
> when we call disconnect procedure, we don't know which type trigged this
> disconnection
> procedure and can only decide to send this message by checking the APN.
> 

I see.  Please comment on that, thanks!

> > 
> > I see. Thank you.
> > However, I have a feeling that we update connectedTypes in too many
> > divergent places, some in RadioInterfaceLayer, some in RILNetworkInterface.
> > I don't have a better idea right now, but would like to see if we can
> > improve it. :) 
> The followings are the logic. Hope it is helpful.
> 1. RIL update "wanted" connectedTypes when calling setupDataCallByType() or 
>    deactivateDataCallByType().
> 2. RIL network interface update the "real" connectedTypes when finishing
> connection or 
>    disconnection procedure.
> > 

Thanks for your explanation. I do understand the code flow. What I was trying to express is that we should try to find a way to improving that, because you can see now this single |connectedTypes| variable indicaties both 'wanted' and 'real' values. It's always not a good idea to have a variable presenting two different meanings and purposes.

> > > 
> > > If the network interface is down, we don't have to update the connectedTypes
> > > of the network
> > > interface. Since the connectedTypes of the network interface is meaningless.
> > > And setupDataCallByType() will be called and provide the new connectedTypes
> > > for the network
> > > interface.
> > 
> > I don't think updating connectedTypes of the network interface when it is
> > down is meaningless. We need to make all attributes of that network
> > interface consistent with its state, because we will never know what this
> > interface object is gonna be used by others or in the future. If we make all
> > attribute consistent with the  interface state, we can help prevent weird
> > situation or error-prone code, especially this is really an easy fix. Not
> > convincible saying that it's meaningless because the connectedTypes will be
> > updated anyway once someone will setup a new connection in the future. So
> > let's fix it.
> Let's think in this way. When the network interface is create, what is the
> default value of
> connectedTypes? We don't know and don't care because the network interface
> is down. And we
> will give value to the connectedTypes of network when calling
> setupDataCallByType().
> And If we add those codes, we will increase the running time.
> But if you insist to add those codes, I will add them.
> 

When we create the interface, its connectedTypes is [] for sure. The patch indeed ensures that. 

Even the interface is down, the network interface object is still there. And since the interface is down, there should definitely no connectedTypes at all. Better make all attributes consistant. Yes, always having tradeoff between running time and accuracy, in this case, I do vote for the latter. So please fix it.

Comment 30

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

B.t.w, please specify the version of your patch sequencely, thank you.

Comment 31

[Ken Chang[:kenkai]]

Attachment: Using array for data call settings and handling multiple sharing - WIP V5

Comment 32

[Ken Chang[:kenkai]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #27)
> One more thing -- we now don't handle the error of requesting deactivating
> data connection. This patch is gonna remove a type from connectedTypes
> before receiving the request result from ril_worker.js. Once the request
> fails, the data state stays wrong. We need to file a bug for addressing that.
I have created bug 865986 for this issue.

Comment 33

[Ken Chang[:kenkai]]

Attachment: 742207: Using array for data call settings and handling multiple sharing - WIP V6

Comment 34

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

Comment on attachment 745060 [details] [diff] [review]
742207: Using array for data call settings and handling multiple sharing - WIP V6

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

Nice work. Thank you!

Since the patch changes the architecture, we need to pass a thorough test (Comment 15 and comment 16) before pushing to m-c.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2831,5 @@
> +    // data call type. 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 (apnSetting.iface.connected) {
> +      // Update data call status to upper layer.

nit: we can remove this line.

@@ +2847,2 @@
>      }
> +    return;

nit: we can remove 'return;' here, right?

@@ +2863,5 @@
> +    // necessary data call and interface information to RILContentHelper
> +    // and network manager for current data call type.
> +    if (apnSetting.iface.connectedTypes.length && apnSetting.iface.connected) {
> +      let dataInfo = this.rilContext.data;
> +      // Update data call status to upper layer.

nit: we can remove this line.

@@ +2876,5 @@
> +      Services.obs.notifyObservers(apnSetting.iface,
> +                                   kNetworkInterfaceStateChangedTopic,
> +                                   null);
> +    }
> +    return;

nit: we can remove 'return;' here, right?

@@ +3137,5 @@
> +  connect: function connect(apntype) {
> +    if (this.connectedTypes.indexOf(apntype) != -1) {
> +      return;
> +    }
> +    this.connectedTypes.push(apntype);

nit: add an empty line here

Comment 35

[Ken Chang[:kenkai]]

Attachment: 742207: Using array for data call settings and handling multiple sharing r=htsai

Comment 36

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

(In reply to Ken Chang from comment #35)
> Created attachment 745090 [details] [diff] [review]
> 742207: Using array for data call settings and handling multiple sharing 
> r=htsai

Ken, please use r=hsinyi as what we have been doing. ;-)

Comment 37

[Julien Wajsberg [:julienw]]

blocks a leo+ bug -> leo ?

Comment 38

[Ken Chang[:kenkai]]

Attachment: 745060: 742207: Using array for data call settings and handling multiple sharing - WIP V7

Comment 39

[Ken Chang[:kenkai]]

Attachment: Using array for data call settings and handling multiple sharing - WIP V8

Comment 40

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

Comment on attachment 765290 [details] [diff] [review]
Using array for data call settings and handling multiple sharing - WIP V8

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

Overall the patch looks good except some small questions below. Thank you!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +983,5 @@
>      dataInfo.emergencyCallsOnly = newInfo.emergencyCallsOnly;
>      dataInfo.type = newInfo.type;
>      // For the data connection, the `connected` flag indicates whether
>      // there's an active data call.
> +    let apnSetting = this.apnSettings.byType["default"];

s/byType["default"]/byType.default

and please use this commented style all over the RIL code.

@@ +1241,5 @@
> +      let thisSimApnSettings = allApnSettings[simId];
> +      if (!thisSimApnSettings) {
> +        return;
> +      }
> +      //Notify the upper layer, the data connection doesn't work now.

nit: add a ws between // and Notify

@@ +1242,5 @@
> +      if (!thisSimApnSettings) {
> +        return;
> +      }
> +      //Notify the upper layer, the data connection doesn't work now.
> +      this.rilContext.data.connected = false;

Sorry I don't get it. How does this line notifies the upper layer? And why do we need to automatically change this flag to false? this.rilContext.data.connected should be updated once clearing old APN settings, no?

@@ +1259,5 @@
> +        }
> +        delete apnSetting.iface;
> +      }
> +      this.apnSettings.byAPN = {};
> +      this.apnSettings.byType = {};

put an extra line here.

@@ +1270,5 @@
> +
> +        // Combine APN, user name, and password as the key of byAPN{} to get
> +        // the corresponding APN setting.
> +        let apnKey = inputApnSetting["apn"] + (inputApnSetting["user"] || '') +
> +                  + (inputApnSetting["password"] || '');

Redundant '+' here.

@@ +1272,5 @@
> +        // the corresponding APN setting.
> +        let apnKey = inputApnSetting["apn"] + (inputApnSetting["user"] || '') +
> +                  + (inputApnSetting["password"] || '');
> +        if (!this.apnSettings.byAPN[apnKey] ||
> +            !this.validateApnSetting(this.apnSettings.byAPN[apnKey])) {

I think we don't need |!this.validateApnSetting(this.apnSettings.byAPN[apnKey]| since only validated inputApnSetting will be pushed to this.apnSettings.byAPN.

@@ +1323,5 @@
>        return;
>      }
>  
> +    if (this.getDataCallStateByType("default") ==
> +        RIL.GECKO_NETWORK_STATE_CONNECTING ||

nit: indent

@@ +1325,5 @@
>  
> +    if (this.getDataCallStateByType("default") ==
> +        RIL.GECKO_NETWORK_STATE_CONNECTING ||
> +        this.getDataCallStateByType("default") ==
> +        RIL.GECKO_NETWORK_STATE_DISCONNECTING) {

ditto.

@@ +2109,5 @@
>  
> +  // Flag to determine whether we reject a waiting call directly or we
> +  // notify the UI of a waiting call. It corresponds to the
> +  // 'ril.callwaiting.enbled' setting from the UI.
> +  _callWaitingEnabled: null,

Why this? @@

Comment 41

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

Comment on attachment 765271 [details] [diff] [review]
745060: 742207: Using array for data call settings and handling multiple sharing - WIP V7

This could be obsolete. :)

Comment 42

[Ken Chang[:kenkai]]

Attachment: Using array for data call settings and handling multiple sharing - WIP V9

Comment 43

[Ken Chang[:kenkai]]

Attachment: Using array for data call settings and handling multiple sharing - WIP V9

Comment 44

[Ken Chang[:kenkai]]

Attachment: Using array for data call settings and handling multiple sharing - WIP V9

Comment 45

[Ken Chang[:kenkai]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #40)
> 
> @@ +1242,5 @@
> > +      if (!thisSimApnSettings) {
> > +        return;
> > +      }
> > +      //Notify the upper layer, the data connection doesn't work now.
> > +      this.rilContext.data.connected = false;
> 
> Sorry I don't get it. How does this line notifies the upper layer? And why
> do we need to automatically change this flag to false?
> this.rilContext.data.connected should be updated once clearing old APN
> settings, no?
Thanks for your review, here is the reason of doing this,
We compare the apn name of datacallstatechange message with the apn name of default apnSetting to decide if we need to update data.connected according to data call state of datacallstatechange message. But after user changes the apn settings, the apn name of default apnSetting may be changed. Then we may be not able to update data.connected according to data call state of datacallstatechange message later and the data.connected will always be true.

Because we are 100% sure that we have to disconnect all data connections and delete all network interfaces, I think it is right to update data.connected to be false here.

Comment 46

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

Comment on attachment 765796 [details] [diff] [review]
Using array for data call settings and handling multiple sharing - WIP V9

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1245,5 @@
> +      // Notify the upper layer, the data connection doesn't work now.
> +      let dataInfo = this.rilContext.data;
> +      if (dataInfo.connected) {
> +        dataInfo.connected = false;
> +        this._sendMobileConnectionMessage("RIL:DataInfoChanged", dataInfo);

Thanks for the reply. Now I understand your attempt, though I still don't think we need this here. The reason is in line#1258 we call |this.deactivateDataCallByType(type)|, while in deactivateDataCallByType() line#3024 does exactly the same thing as what we are trying to do here. So, it should be safe to remove the redundancy, right?

@@ +3274,5 @@
>      return this.state == RIL.GECKO_NETWORK_STATE_CONNECTED;
>    },
>  
> +  connect: function connect(apntype) {
> +    if (apntype && !this.inConnectedTypes(apntype)) {

Let's do this:

if (!apntype) {
  return;
}

if (!this.inConnectedTypes(apntype)) {
  this.connectedTypes.push(apntype);
}

@@ +3278,5 @@
> +    if (apntype && !this.inConnectedTypes(apntype)) {
> +      this.connectedTypes.push(apntype);
> +    }
> +
> +    if (this.connecting || this.connected || !this.connectedTypes.length) {

And then, remove this condition, !this.connectedTypes.length.

Comment 47

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

Comment on attachment 765796 [details] [diff] [review]
Using array for data call settings and handling multiple sharing - WIP V9

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2969,5 @@
>        }
>      }
>    },
>  
> +  setupDataCallByType: function setupDataCallByType(apntype) {

We should bail out if anptype is not specified. Then we don't need to worry too much about empty anptype in RILNetworkInterface.connect().

@@ +3274,5 @@
>      return this.state == RIL.GECKO_NETWORK_STATE_CONNECTED;
>    },
>  
> +  connect: function connect(apntype) {
> +    if (apntype && !this.inConnectedTypes(apntype)) {

Sorry, according to my above sentence, here we could just do:

if (!this.inConnectedTypes(apntype)) {
  this.connectedTypes.push(apntype);
}

rather than my comment #46.

@@ +3278,5 @@
> +    if (apntype && !this.inConnectedTypes(apntype)) {
> +      this.connectedTypes.push(apntype);
> +    }
> +
> +    if (this.connecting || this.connected || !this.connectedTypes.length) {

And then, remove this condition, !this.connectedTypes.length.

Comment 48

[Ken Chang[:kenkai]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #46)
> Comment on attachment 765796 [details] [diff] [review]
> Using array for data call settings and handling multiple sharing - WIP V9
> 
> Review of attachment 765796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1245,5 @@
> > +      // Notify the upper layer, the data connection doesn't work now.
> > +      let dataInfo = this.rilContext.data;
> > +      if (dataInfo.connected) {
> > +        dataInfo.connected = false;
> > +        this._sendMobileConnectionMessage("RIL:DataInfoChanged", dataInfo);
> 
> Thanks for the reply. Now I understand your attempt, though I still don't
> think we need this here. The reason is in line#1258 we call
> |this.deactivateDataCallByType(type)|, while in deactivateDataCallByType()
> line#3024 does exactly the same thing as what we are trying to do here. So,
> it should be safe to remove the redundancy, right?
> 
If default type is the last one in the connectedTypes, this condition will not be fullfill.

> @@ +3274,5 @@
> >      return this.state == RIL.GECKO_NETWORK_STATE_CONNECTED;
> >    },
> >  
> > +  connect: function connect(apntype) {
> > +    if (apntype && !this.inConnectedTypes(apntype)) {
> 
> Let's do this:
> 
> if (!apntype) {
>   return;
> }
We cannot return it because the retry mechanism will directly call connect(). 

> if (!this.inConnectedTypes(apntype)) {
>   this.connectedTypes.push(apntype);
> }
> 
> @@ +3278,5 @@
> > +    if (apntype && !this.inConnectedTypes(apntype)) {
> > +      this.connectedTypes.push(apntype);
> > +    }
> > +
> > +    if (this.connecting || this.connected || !this.connectedTypes.length) {
> 
> And then, remove this condition, !this.connectedTypes.length.
We cannot remove it because we have chances to have this.connectedTypes.length ==0 when the retry mechanism is running in background and someone call disconnect().

Comment 49

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

(In reply to Ken Chang from comment #48)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #46)
> > Comment on attachment 765796 [details] [diff] [review]
> > Using array for data call settings and handling multiple sharing - WIP V9
> > 
> > Review of attachment 765796 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +1245,5 @@
> > > +      // Notify the upper layer, the data connection doesn't work now.
> > > +      let dataInfo = this.rilContext.data;
> > > +      if (dataInfo.connected) {
> > > +        dataInfo.connected = false;
> > > +        this._sendMobileConnectionMessage("RIL:DataInfoChanged", dataInfo);
> > 
> > Thanks for the reply. Now I understand your attempt, though I still don't
> > think we need this here. The reason is in line#1258 we call
> > |this.deactivateDataCallByType(type)|, while in deactivateDataCallByType()
> > line#3024 does exactly the same thing as what we are trying to do here. So,
> > it should be safe to remove the redundancy, right?
> > 
> If default type is the last one in the connectedTypes, this condition will
> not be fullfill.
> 

I see. So the problem seems that we don't record the exact apn for that data call that causes difficulty in matching rilContext.data in handleDataCallState(). How about we doing the following:

  1 if (apnSetting && datacall.ifname) {
  2   if (dataCallConnected && datacall.apn == apnSetting.apn) {
  3     data.connected = dataCallConnected;
  4     this._sendMobileConnectionMessage("RIL:DataInfoChanged", data);
  5     data.apn = datacall.apn;
  6   } else if (!dataCallConnected && datacall.apn == data.apn) {
  7     data.connected = dataCallConnected;
  8     delete data.apn;
  9     this._sendMobileConnectionMessage("RIL:DataInfoChanged", data);
 10   }
 11 }


> > @@ +3274,5 @@
> > >      return this.state == RIL.GECKO_NETWORK_STATE_CONNECTED;
> > >    },
> > >  
> > > +  connect: function connect(apntype) {
> > > +    if (apntype && !this.inConnectedTypes(apntype)) {
> > 
> > Let's do this:
> > 
> > if (!apntype) {
> >   return;
> > }
> We cannot return it because the retry mechanism will directly call
> connect(). 
> 

Agree. 

> > if (!this.inConnectedTypes(apntype)) {
> >   this.connectedTypes.push(apntype);
> > }
> > 
> > @@ +3278,5 @@
> > > +    if (apntype && !this.inConnectedTypes(apntype)) {
> > > +      this.connectedTypes.push(apntype);
> > > +    }
> > > +
> > > +    if (this.connecting || this.connected || !this.connectedTypes.length) {
> > 
> > And then, remove this condition, !this.connectedTypes.length.
> We cannot remove it because we have chances to have
> this.connectedTypes.length ==0 when the retry mechanism is running in
> background and someone call disconnect().

I see. Then please separate |!this.connectedTypes.length| to another if-condition and comment on that. Thank you!

Comment 50

[Ken Chang[:kenkai]]

Attachment: Using array for data call settings and handling multiple sharing - WIP V10

Comment 51

[Ken Chang[:kenkai]]

Attachment: Using array for data call settings and handling multiple sharing - WIP V10

Comment 52

[Ken Chang[:kenkai]]

Comment on attachment 767692 [details] [diff] [review]
Using array for data call settings and handling multiple sharing - WIP V10

Have done what you suggested.

Comment 53

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

Comment on attachment 767692 [details] [diff] [review]
Using array for data call settings and handling multiple sharing - WIP V10

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

Good job. Let's land it!

Thank you :)

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1833,5 @@
> +    let dataCallConnected =
> +        (datacall.state == RIL.GECKO_NETWORK_STATE_CONNECTED);
> +    if (apnSetting && datacall.ifname) {
> +      if (dataCallConnected && datacall.apn == apnSetting.apn) {
> +          data.connected = dataCallConnected;

nit: indention

@@ +1837,5 @@
> +          data.connected = dataCallConnected;
> +          this._sendMobileConnectionMessage("RIL:DataInfoChanged", data);
> +          data.apn = datacall.apn;
> +      } else if (!dataCallConnected && datacall.apn == data.apn) {
> +          data.connected = dataCallConnected;

ditto

Comment 55

[Ken Chang[:kenkai]]

The patch of bug 884829 can fix those leo+ bugs needing the patch of this bug 837488. So I remove leo+ and some blocked bugs.

Comment 56

[Ken Chang[:kenkai]]

In order to avoid the confusion, I'd like to have further comments.
The patch of this bug,837488, includes 3 fixes.
1. Getting the apn settings of all data types at the same time, which also needs 
   to modify Gaia. This is why I created the bug 842252 for Gaia.
2. Storing all apn settings in an array.
3. Using a network interface for the shared APN setting. This modification 
   can also fix the bug of being not able to send MMS when data optin isn't 
   enable.
The main reason of nominating this bug as leo+ is only to have the 3rd fix.
Therefore, it's unnecessary to land all 3 fixes into b2g18.

The 3rd fix of this bug is also the solution for bug 884829, so I provided a patch only including the 3rd fix for bug 884829. By doing this, we can use the patch of bug 884829 to solve bug 862764 and bug 880680.

After the patch for bug 884829 is provided, bug 837488 and bug 842252 are no longer the leo+ bugs and will not block bug 862764 and bug 880680.

Comment 57

[Ken Chang[:kenkai]]

Attachment: WIP V11 - Rebase

Comment 58

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

Comment on attachment 770039 [details] [diff] [review]
WIP V11 - Rebase

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

Simply rebase. r=me!

Comment 59

[Ken Chang[:kenkai]]

Attachment: Rebase r=hsinyi

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/f05d729359b5

Comment 61

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 774459 [details] [diff] [review]
Rebase r=hsinyi

You can just set the checkin-needed keyword on the bug next time.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f05d729359b5

Comment 63

[:gerard-majax]

Looks like a small typo got into this patch, leading to invalid APN settings' authentication type retrieved from settings, as described in bug 901227.

Comment 64

[Ken Chang[:kenkai]]

There isn't the typo. Please compare the previous version and the current version
The following is previous version,
let authType = RIL.RIL_DATACALL_AUTH_TO_GECKO.indexOf(this.dataCallSettings["authtype"]);
The following is current version,
let authType = RIL.RIL_DATACALL_AUTH_TO_GECKO.indexOf(this.apnSetting.authtype);

Comment 65

[:gerard-majax]

(In reply to Ken Chang from comment #64)
> There isn't the typo. Please compare the previous version and the current
> version
> The following is previous version,
> let authType =
> RIL.RIL_DATACALL_AUTH_TO_GECKO.indexOf(this.dataCallSettings["authtype"]);
> The following is current version,
> let authType =
> RIL.RIL_DATACALL_AUTH_TO_GECKO.indexOf(this.apnSetting.authtype);

yet the this.apnSetting.authtype value returns undefined, while this.apnSetting.authType returns something valid.

Comment 66

[Ken Chang[:kenkai]]

(In reply to Alexandre LISSY :gerard-majax from comment #65)
> yet the this.apnSetting.authtype value returns undefined, while
> this.apnSetting.authType returns something valid.
Please check comment 2 of Bug 901227, the root cause should be the different keys was sent from Gaia.