1216468 - B2G NetworkManager: provide a flag to indicate network interface's ready state

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

Per bug 911713 comment 13, we'll need a flag in nsINetworkInterface to let NetworkManager know which of the network interfaces can be activated, and also have a notify function in nsINetworkManager to let it know when to re-check policy. Something like this:

interface nsINetworkInterface {
  ...
  readonly attribute boolean isReady;
  ...
}

interface nsINetworkManager {
  ...
  void onNetworkInterfaceReadyChanged(in nsINetworkInterface network);
  ...
}

For mobile network interfaces, 'isReady' is true when all the following conditions are met:
  - radio on
  - data registration registered
  - is the default service id for data call
  - data settings is enabled
  - data roaming settings is enabled when roaming
  - not in the process of changing apn settings or deactivating data calls

So, in the case of mobile network interface, if network interface is not ready (isReady is false due to data unregistered for example), NetworkManager should not activate it. When data becomes registered, onNetworkInterfaceReadyChanged() is called and NetworkManager will recheck its policy and activate network interface if necessary.

Comment 1

[John Dai[:jdai]]

Attachment: Provide isReady flag to indicate network interface's ready state.

Hi Edgar, May I have your review of this patch? Thanks.

Comment 2

[John Dai[:jdai]]

Attachment: Provide isReady flag to indicate network interface's ready state. (v2)

Per offline discussion, when disconnect network interface, it should notify NetworkManager, so that NetworkManager will check its policy to decide whether to deactivate it.

Hi Edgar, May I have your feedback of this patch? Thanks.

Comment 3

[Edgar Chen [:edgar]]

Comment on attachment 8702212 [details] [diff] [review]
Provide isReady flag to indicate network interface's ready state. (v2)

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

Thanks for the patch, please see my comments below.

::: dom/system/gonk/DataCallManager.js
@@ +225,5 @@
>            this._currentDataClientId = this._dataDefaultClientId;
>  
>            this._setDataRegistration(newIface, true).then(() => {
> +            gNetworkManager.onNetworkInterfaceReadyChanged(
> +              newConnHandler.dataNetworkInterfaces.get(NETWORK_TYPE_MOBILE));

Most of the readiness changes are only notified for default interface. I have some questions for all of them. Take this as example, 
1. Does it need to be notified for the non-default interface, too?
2. And what if the newConnHandler doesn't have default interface?

@@ +1590,5 @@
> +    if (this.info.type !== NETWORK_TYPE_MOBILE) {
> +      if (DEBUG) {
> +        this.debug("No network interface for default data.");
> +      }
> +      return false;

isReady() always returns false for non-default interface, it seems to me that it should not behave like this, right?

@@ +1595,5 @@
> +    }
> +
> +    if (this.dataCallHandler._pendingApnSettings) {
> +      if (DEBUG) {
> +        this.debug("We're changing apn settings, ignore any changes.");

Since we don't really "ignore any changes" after we move these code to isReady(). Please revise the logging a bit.

@@ +1602,5 @@
> +    }
> +
> +    if (this.dataCallHandler._deactivatingDataCalls) {
> +      if (DEBUG) {
> +        this.debug("We're deactivating all data calls, ignore any changes.");

Hmm, these codes are from |updateRILNetworkInterface()| which was used for preventing timing problem during deactivating. Do you intent to report false as readiness during deactivating? and could you explain why?

@@ +1624,5 @@
> +    // Make sure this network interface's service id is the default service id for data call.
> +    let dataDefaultServiceId = this.dataCallHandler.dataCallManager.dataDefaultServiceId;
> +    if (this.info.serviceId !== dataDefaultServiceId) {
> +      if (DEBUG) {
> +        this.debug("Not default client id for data call.");

This is only for default data call, right?

@@ +1645,5 @@
> +    // disconnecting the data call. If the value of "ril.data.enabled" is
> +    // true and any of the remaining flags change the setting application
> +    // should turn this flag to false and then to true in order to reload
> +    // the new values and reconnect the data call.
> +    if (this.dataCallHandler.dataCallSettings.oldEnabled === this.dataCallHandler.dataCallSettings.enabled) {

Do we really need this condition for readiness checking?

@@ +1649,5 @@
> +    if (this.dataCallHandler.dataCallSettings.oldEnabled === this.dataCallHandler.dataCallSettings.enabled) {
> +      if (DEBUG) {
> +        this.debug("No changes for ril.data.enabled flag. Nothing to do.");
> +      }
> +      return;

We should return either true or false.

@@ +1653,5 @@
> +      return;
> +    }
> +
> +    if ((this.dataCallHandler.dataCallSettings.enabled && this.connected) ||
> +        (!this.dataCallHandler.dataCallSettings.enabled && !this.connected)) {

Looks like we don't really need this condition for readiness checking.

::: dom/system/gonk/NetworkManager.js
@@ +478,5 @@
>        if (this.networkInterfaces.hasOwnProperty(networkId) &&
>            this.networkInterfaces[networkId].info.type === Ci.nsINetworkInfo.NETWORK_TYPE_MOBILE) {
> +        if (!this.networkInterfaces[networkId].isReady) {
> +          debug("Disconnect data call when isReady is false.");
> +          this.networkInterfaces[networkId].deactivate();

What about non-default data calls, should we also apply this policy to them?

@@ +485,3 @@
>          if (wifi_active) {
>            debug("Disconnect data call when Wifi is connected.");
>            this.networkInterfaces[networkId].deactivate();

Bail-out early?

@@ +486,5 @@
>            debug("Disconnect data call when Wifi is connected.");
>            this.networkInterfaces[networkId].deactivate();
> +        }
> +
> +        if (!wifi_active && this.networkInterfaces[networkId].isReady) {

If we bail-out early in both wifi_active and isReady checking, we don't have to check them again here.

::: dom/system/gonk/nsINetworkManager.idl
@@ +18,5 @@
> +   *
> +   * @param network
> +   *        Network interface to notify.
> +   */
> +  void onNetworkInterfaceReadyChanged(in nsINetworkInterface network);

Is it possible that we reuse |updateNetworkInterface()| to notify the readiness of an interface is changed?

Comment 4

[John Dai[:jdai]]

Attachment: Provide isReady flag to indicate network interface's ready state. (v3)

In this patch, I add a skip state to isReady(), which is to bail-out early in checkPolicy().

Hi Edgar, May I have your feedback of this patch? Thanks.

Comment 5

[Edgar Chen [:edgar]]

Comment on attachment 8704583 [details] [diff] [review]
Provide isReady flag to indicate network interface's ready state. (v3)

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

As per discussion: in your patch, activating/de-activating for changing apn setting and switching default service id are still handled in DataCallManager, it will be nicer if we could move all decision of activating/de-activating to NetworkManager. The advantage of that is we don't need to worry about the consistence of activating/de-activating control between two modules (DataCallManager and NetworkManager). Thank you.

::: dom/system/gonk/NetworkManager.js
@@ +450,5 @@
>              if (extNetworkInfo.type == Ci.nsINetworkInfo.NETWORK_TYPE_WIFI) {
>                this._checkPolicy();
>              }
>            })
> +          .then(() => this.setAndConfigureActive())

Could you explain why we need this change?

::: dom/system/gonk/nsINetworkInterface.idl
@@ +110,5 @@
>    /*
> +   * The flag that notice network interfaces can be activated/deactivated.
> +   * -1 is to skip activate and deactivate network interfaces.
> +   */
> +  readonly attribute long isReady;

It seems we don't have to introduce a state for `SKIP`. If networkInterface doesn't want networkmanager to do anything, maybe we could consider just don't inform isReady is changed.

Comment 6

[John Dai[:jdai]]

Attachment: Provide isReady flag to indicate network interface's ready state. (v4)

Addressed comment #5.
1. Removed checkPolicy() 'SKIP' state.
2. Moved changing apn setting and switching default service id decision of activating/de-activating to NetworkManager.

Hi Edgar, May I have your feedback of this patch? Thanks.

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8707264 [details] [diff] [review]
Provide isReady flag to indicate network interface's ready state. (v4)

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

::: dom/system/gonk/DataCallManager.js
@@ +317,5 @@
>          if (this._dataDefaultClientId === -1) {
>            // We haven't got the default id for data from db.
>            break;
>          }
> +        this._connectionHandlers[this._dataDefaultClientId].notifyNetworkInterfaceReadyChanged();

Nit: Indentation.

@@ +651,5 @@
>        return;
>      }
>      // TODO: deactivate() should be called from NetworkManager after Bug 928861 -
>      // B2G NetworkManager: Provide a more generic function for connecting.
> +    if (networkInterface.isReady) {

Why check |isReady| for deactivating?

@@ +658,5 @@
>    },
>  
>    _deactivatingDataCalls: false,
>  
>    deactivateDataCalls: function(aCallback) {

DataCallManager won't `deactivate` data call directly any more, please rename it to a proper one (maybe come with some refactor). Same for other deactivate* functions or variables which probably has a confusing name.

@@ +687,5 @@
>    },
>  
> +  ready: false,
> +  notifyNetworkInterfaceReadyChanged: function() {
> +    let networkInterface = this.dataNetworkInterfaces.get(NETWORK_TYPE_MOBILE);

I think we should also handle non-default type.

@@ +1594,5 @@
>      // Value provided by network has higher priority than apn settings.
>      return this.dataCall.linkInfo.mtu || this.apnSetting.mtu || -1;
>    },
>  
> +  get isReady() {

Since most of the condition is from DataCallHandler, how about using boolean to cache the `ready` state instead of a getter function.
1. Move the condition checking to DataCallHandler.
2. Call some update function to update the cache value in NetworkInterface when state is changed.

::: dom/system/gonk/nsIDataCallManager.idl
@@ +65,2 @@
>     */
> +  void notifyNetworkInterfaceReadyChanged();

This function now is internally used by DataCallManager, we don't need to expose it on idl interface.
Actually, |updateRILNetworkInterface()| probably could be removed in bug 911713.

Comment 8

[John Dai[:jdai]]

Attachment: Provide isReady flag to indicate network interface's ready state. (v5)

Addressed comment #7.
1.Move the ready state condition checking to DataCallHandler.
2.Call some update function to update the cache value in NetworkInterface when state is changed.

Hi Edgar, May I have your feedback of this patch? Thanks.

Comment 9

[John Dai[:jdai]]

Attachment: Provide isReady flag to indicate network interface's ready state. (v6)

I uploaded a wrong patch. Please ignore previous one.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8708914 [details] [diff] [review]
Provide isReady flag to indicate network interface's ready state. (v6)

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

Please see my comments below, thank you.

::: dom/system/gonk/DataCallManager.js
@@ +197,5 @@
>          let settings = connHandler.dataCallSettings;
>          settings.oldEnabled = settings.enabled;
>          settings.enabled = true;
> +        connHandler.updateReadyChanged(
> +          connHandler.dataNetworkInterfaces.get(NETWORK_TYPE_MOBILE));

Just pass |NETWORK_TYPE_MOBILE| as argument and get corresponding NetworkInterface in |connHandler.updateReadyChanged()|.

e.g.
connHandler.updateReadyChanged(NETWORK_TYPE_MOBILE);


void updateReadyChanged(aNetworkType) {
  networkInterface = this.dataNetworkInterfaces.get(aNetworkType);
  ....
}

And there is some other problems,
1. What if there is no NETWORK_TYPE_MOBILE type in this dataCallHandler?
2. You only update the ready state for NETWORK_TYPE_MOBILE type, do we need to do update for other types?

@@ +322,5 @@
>            // We haven't got the default id for data from db.
>            break;
>          }
> +        this._connectionHandlers[this._dataDefaultClientId].updateReadyChanged(
> +          this._connectionHandlers[this._dataDefaultClientId].dataNetworkInterfaces.get(NETWORK_TYPE_MOBILE));

Nit: Use a local variable to cache `this._connectionHandlers[this._dataDefaultClientId]` to make this line shorter.

@@ +668,5 @@
>      this.dataNetworkInterfaces.forEach(function(networkInterface) {
>        if (networkInterface.enabled) {
>          if (networkInterface.info.state != NETWORK_STATE_UNKNOWN &&
>              networkInterface.info.state != NETWORK_STATE_DISCONNECTED) {
> +          readyStateChanging = true;

We still need to update readyState to a network even if it is not connected, right?

@@ +673,3 @@
>          }
> +
> +        networkInterface.dataCallHandler.updateReadyChanged(networkInterface);

this.updateReadyChanged(networkInterface);

@@ +837,5 @@
>      if (aUpdatedDataCall.state == NETWORK_STATE_DISCONNECTED ||
>          aUpdatedDataCall.state == NETWORK_STATE_UNKNOWN &&
> +        this.allDataDisconnected() && this._changingReadyState) {
> +      this._changingReadyState = false;
> +      this._notifyListeners("notifyAllReadyStateChanged", {

It is a notification for "all data disconnected", I think we should not rename it as "notifyAllReadyStateChanged".

@@ +1664,5 @@
> +  get isReady() {
> +    return this.ready;
> +  },
> +
> +  set isReady(val) {

We should not give a setter for isReady as it is defined as a read-only attribute in IDL.

Use `updateReadyState: function() { ... }` instead.

@@ +1665,5 @@
> +    return this.ready;
> +  },
> +
> +  set isReady(val) {
> +    this.ready = val;

Notify to NetworkManager when ready is changed then we don't need |notifyReadyChanged()|.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +304,5 @@
>        _deactivatingDeferred = {};
>  
>        let promise = Promise.resolve();
>        for (let i = 0, N = _ril.numRadioInterfaces; i < N; ++i) {
> +       promise = promise.then(this._deactivateDataCallsForClient(i));

I guess you miss deleting a space here.

@@ +314,5 @@
>      _deactivateDataCallsForClient: function(clientId) {
>        return function() {
> +        return new Promise((aResolve, aReject) => {
> +          let dataCallHandler = gDataCallManager.getDataCallHandler(clientId);
> +          dataCallHandler.updateNetworkInterfacesReadyChanged(function() {

Per offline discussion, this doesn't really trigger deactivating data call since |isReady| is still true. One possible solution is updating radioState to "disabling" which will trigger |isReady| change, and we need to review the flow of disabling radio power to see if it is workable.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8708914 [details] [diff] [review]
Provide isReady flag to indicate network interface's ready state. (v6)

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +304,5 @@
>        _deactivatingDeferred = {};
>  
>        let promise = Promise.resolve();
>        for (let i = 0, N = _ril.numRadioInterfaces; i < N; ++i) {
> +       promise = promise.then(this._deactivateDataCallsForClient(i));

A potential error is found in original implementation, the parameter of then() is a function instead of a returned promise:
promise = promise.then(() => this._deactivateDataCallsForClient(i));

@@ -304,5 @@
>        _deactivatingDeferred = {};
>  
>        let promise = Promise.resolve();
>        for (let i = 0, N = _ril.numRadioInterfaces; i < N; ++i) {
> -        promise = promise.then(this._deactivateDataCallsForClient(i));

A potential error is found in original implementation.
The parameter of then() is a function instead of a returned promise:
promise = promise.then(() => this._deactivateDataCallsForClient(i));

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8708914 [details] [diff] [review]
Provide isReady flag to indicate network interface's ready state. (v6)

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +304,5 @@
>        _deactivatingDeferred = {};
>  
>        let promise = Promise.resolve();
>        for (let i = 0, N = _ril.numRadioInterfaces; i < N; ++i) {
> +       promise = promise.then(this._deactivateDataCallsForClient(i));

Sorry, I missed line#315, _eactiveDataCallsForClient did return a callback for promise.then().

Comment 13

[John Dai[:jdai]]

Attachment: Provide isReady flag to indicate network interface's ready state. (v7)

Addressed comment #10.

Hi Edgar, May I have your feedback of this patch? Thanks.

Comment 14

[Edgar Chen [:edgar]]

Comment on attachment 8711603 [details] [diff] [review]
Provide isReady flag to indicate network interface's ready state. (v7)

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

Overall architecture looks good, so give f+.
Please address or answer my comments below, thank you.

::: dom/system/gonk/DataCallManager.js
@@ +558,5 @@
>      return true;
>    },
>  
> +  _changingReadyState: false,
> +  updateReadyChangedAndWait: function() {

s/updateReadyChangedAndWait/updateReadyStateAndWaitDataDisconnected/

@@ +573,5 @@
>        });
> +
> +      this._changingReadyState = readyStateChanging;
> +      if (!readyStateChanging) {
> +        aResolve();

Could you explain why we don't update readyState if network is disconnected?

@@ +711,5 @@
>  
> +      let dataInfo = connection && connection.data;
> +      // We have moved part of the decision making into DataCall, the rest will be
> +      // moved after Bug 904514 - [meta] NetworkManager enhancement.
> +      if (networkInterface.enabled &&

Why checking networkInterface.enable for readiness?

@@ +715,5 @@
> +      if (networkInterface.enabled &&
> +         (!this.dataCallSettings.enabled ||
> +          (dataInfo.roaming && !this.dataCallSettings.roamingEnabled))) {
> +        if (DEBUG) {
> +          this.debug("Data call settings: disconnect data call.");

We don't disconnect here, please revise the logging.

@@ +722,5 @@
> +      }
> +
> +      if (!this.dataCallSettings.enabled) {
> +        if (DEBUG) {
> +          this.debug("Data call settings: nothing to do.");

"nothing to do" is confused, please revise the logging.

@@ +737,5 @@
>      }
> +    return true;
> +  },
> +
> +  updateReadyChanged: function() {

s/updateReadyChanged/updateNetworkInterfacesReadyState/

@@ +823,5 @@
>      // Process pending radio power off request after all data calls
>      // are disconnected.
>      if (aUpdatedDataCall.state == NETWORK_STATE_DISCONNECTED ||
>          aUpdatedDataCall.state == NETWORK_STATE_UNKNOWN &&
> +        this.allDataDisconnected() && this._changingReadyState) {

notifyAllDataDisconnected is a listener event now, I think it should be triggered once all data is disconnected, even not in processing ready state.

@@ +908,5 @@
> +    });
> +
> +    this._changingReadyState = readyStateChanging;
> +    if (!readyStateChanging) {
> +      this._notifyListeners("notifyAllDataDisconnected", {

I know this is for gRadioEnabledController, but notifyAllDataDisconnected here is weird to me, especially we notify AllDataDisconnected for every radioState change.

@@ +1653,5 @@
>     */
>  
>    info: null,
>  
> +  ready: null,

give 'false' as default value and put it with |info| instead of |get httpProxyHost|.

@@ +1673,5 @@
> +  },
> +
> +  updateReadyState: function(val) {
> +    this.ready = val;
> +    gNetworkManager.onNetworkInterfaceReadyChanged(this);

Notify only when readyState is changed.

@@ +1737,5 @@
>        }
>        return Ci.nsINetworkInterface.STATUS_GENERIC_ERROR;
>      }
>  
> +    if (this.dataCallHandler._changingReadyState) {

I think this probably could be covered by radioState checking below, right? Or did I miss any other cases?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +272,5 @@
>  
>          // In some DSDS architecture with only one modem, toggling one radio may
>          // toggle both. Therefore, for safely turning off, we should first
>          // explicitly deactivate all data calls from all clients.
> +        let dataCallHandler = gDataCallManager.getDataCallHandler(clientId);

Original code deactivates all data calls from all clients, so please handle other client as well.

::: dom/system/gonk/nsINetworkManager.idl
@@ +18,5 @@
> +   *
> +   * @param network
> +   *        Network interface to notify.
> +   */
> +  void onNetworkInterfaceReadyChanged(in nsINetworkInterface network);

s/onNetworkInterfaceReadyChanged/updateNetworkInterfaceReadyState/ and put this function between `updateNetworkInterface` and `unregisterNetworkInterface`.

Comment 15

[John Dai[:jdai]]

Comment on attachment 8711603 [details] [diff] [review]
Provide isReady flag to indicate network interface's ready state. (v7)

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

::: dom/system/gonk/DataCallManager.js
@@ +558,5 @@
>      return true;
>    },
>  
> +  _changingReadyState: false,
> +  updateReadyChangedAndWait: function() {

Will do.

@@ +573,5 @@
>        });
> +
> +      this._changingReadyState = readyStateChanging;
> +      if (!readyStateChanging) {
> +        aResolve();

I think we should also update readyState when network is disconnected.

@@ +711,5 @@
>  
> +      let dataInfo = connection && connection.data;
> +      // We have moved part of the decision making into DataCall, the rest will be
> +      // moved after Bug 904514 - [meta] NetworkManager enhancement.
> +      if (networkInterface.enabled &&

I think we don't need to check |networkInterface.enable| anymore.

@@ +715,5 @@
> +      if (networkInterface.enabled &&
> +         (!this.dataCallSettings.enabled ||
> +          (dataInfo.roaming && !this.dataCallSettings.roamingEnabled))) {
> +        if (DEBUG) {
> +          this.debug("Data call settings: disconnect data call.");

Will do.

@@ +722,5 @@
> +      }
> +
> +      if (!this.dataCallSettings.enabled) {
> +        if (DEBUG) {
> +          this.debug("Data call settings: nothing to do.");

Will do.

@@ +737,5 @@
>      }
> +    return true;
> +  },
> +
> +  updateReadyChanged: function() {

Will do.

@@ +823,5 @@
>      // Process pending radio power off request after all data calls
>      // are disconnected.
>      if (aUpdatedDataCall.state == NETWORK_STATE_DISCONNECTED ||
>          aUpdatedDataCall.state == NETWORK_STATE_UNKNOWN &&
> +        this.allDataDisconnected() && this._changingReadyState) {

Will do.

@@ +908,5 @@
> +    });
> +
> +    this._changingReadyState = readyStateChanging;
> +    if (!readyStateChanging) {
> +      this._notifyListeners("notifyAllDataDisconnected", {

Per offline discussion, I will move this logic to RadioInterfaceLayer.

@@ +1653,5 @@
>     */
>  
>    info: null,
>  
> +  ready: null,

Will do. I guess your meaning is put in with |info| instead of |get isReady|.

@@ +1673,5 @@
> +  },
> +
> +  updateReadyState: function(val) {
> +    this.ready = val;
> +    gNetworkManager.onNetworkInterfaceReadyChanged(this);

Will do.

@@ +1737,5 @@
>        }
>        return Ci.nsINetworkInterface.STATUS_GENERIC_ERROR;
>      }
>  
> +    if (this.dataCallHandler._changingReadyState) {

Yes, we can get rid of this check.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +272,5 @@
>  
>          // In some DSDS architecture with only one modem, toggling one radio may
>          // toggle both. Therefore, for safely turning off, we should first
>          // explicitly deactivate all data calls from all clients.
> +        let dataCallHandler = gDataCallManager.getDataCallHandler(clientId);

Will do.

::: dom/system/gonk/nsINetworkManager.idl
@@ +18,5 @@
> +   *
> +   * @param network
> +   *        Network interface to notify.
> +   */
> +  void onNetworkInterfaceReadyChanged(in nsINetworkInterface network);

Will do.

Comment 16

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar][:echen] from comment #14)
> Comment on attachment 8711603 [details] [diff] [review]
> Provide isReady flag to indicate network interface's ready state. (v7)
> 
> Review of attachment 8711603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall architecture looks good, so give f+.
> Please address or answer my comments below, thank you.
> 
> ::: dom/system/gonk/DataCallManager.js
> @@ +558,5 @@
> >      return true;
> >    },
> >  
> > +  _changingReadyState: false,
> > +  updateReadyChangedAndWait: function() {
> 
> s/updateReadyChangedAndWait/updateReadyStateAndWaitDataDisconnected/
Will do.
> 
> @@ +573,5 @@
> >        });
> > +
> > +      this._changingReadyState = readyStateChanging;
> > +      if (!readyStateChanging) {
> > +        aResolve();
> 
> Could you explain why we don't update readyState if network is disconnected?
I think we should also update readyState when network is disconnected.
> 
> @@ +711,5 @@
> >  
> > +      let dataInfo = connection && connection.data;
> > +      // We have moved part of the decision making into DataCall, the rest will be
> > +      // moved after Bug 904514 - [meta] NetworkManager enhancement.
> > +      if (networkInterface.enabled &&
> 
> Why checking networkInterface.enable for readiness?
I think we don't need to check |networkInterface.enable| anymore.
> 
> @@ +715,5 @@
> > +      if (networkInterface.enabled &&
> > +         (!this.dataCallSettings.enabled ||
> > +          (dataInfo.roaming && !this.dataCallSettings.roamingEnabled))) {
> > +        if (DEBUG) {
> > +          this.debug("Data call settings: disconnect data call.");
> 
> We don't disconnect here, please revise the logging.
> 
> @@ +722,5 @@
> > +      }
> > +
> > +      if (!this.dataCallSettings.enabled) {
> > +        if (DEBUG) {
> > +          this.debug("Data call settings: nothing to do.");
> 
> "nothing to do" is confused, please revise the logging.
Will do.
> 
> @@ +737,5 @@
> >      }
> > +    return true;
> > +  },
> > +
> > +  updateReadyChanged: function() {
> 
> s/updateReadyChanged/updateNetworkInterfacesReadyState/
Will do.
> 
> @@ +823,5 @@
> >      // Process pending radio power off request after all data calls
> >      // are disconnected.
> >      if (aUpdatedDataCall.state == NETWORK_STATE_DISCONNECTED ||
> >          aUpdatedDataCall.state == NETWORK_STATE_UNKNOWN &&
> > +        this.allDataDisconnected() && this._changingReadyState) {
> 
> notifyAllDataDisconnected is a listener event now, I think it should be
> triggered once all data is disconnected, even not in processing ready state.
Will do.
> 
> @@ +908,5 @@
> > +    });
> > +
> > +    this._changingReadyState = readyStateChanging;
> > +    if (!readyStateChanging) {
> > +      this._notifyListeners("notifyAllDataDisconnected", {
> 
> I know this is for gRadioEnabledController, but notifyAllDataDisconnected
> here is weird to me, especially we notify AllDataDisconnected for every
> radioState change.
Per offline discussion, I will move this logic to RadioInterfaceLayer.
> 
> @@ +1653,5 @@
> >     */
> >  
> >    info: null,
> >  
> > +  ready: null,
> 
> give 'false' as default value and put it with |info| instead of |get
> httpProxyHost|.
Will do. I guess your meaning is put in with |info| instead of |get isReady|.
> 
> @@ +1673,5 @@
> > +  },
> > +
> > +  updateReadyState: function(val) {
> > +    this.ready = val;
> > +    gNetworkManager.onNetworkInterfaceReadyChanged(this);
> 
> Notify only when readyState is changed.
Will do.
> 
> @@ +1737,5 @@
> >        }
> >        return Ci.nsINetworkInterface.STATUS_GENERIC_ERROR;
> >      }
> >  
> > +    if (this.dataCallHandler._changingReadyState) {
> 
> I think this probably could be covered by radioState checking below, right?
> Or did I miss any other cases?
Yes, we can get rid of this check.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +272,5 @@
> >  
> >          // In some DSDS architecture with only one modem, toggling one radio may
> >          // toggle both. Therefore, for safely turning off, we should first
> >          // explicitly deactivate all data calls from all clients.
> > +        let dataCallHandler = gDataCallManager.getDataCallHandler(clientId);
> 
> Original code deactivates all data calls from all clients, so please handle
> other client as well.
Will do.
> 
> ::: dom/system/gonk/nsINetworkManager.idl
> @@ +18,5 @@
> > +   *
> > +   * @param network
> > +   *        Network interface to notify.
> > +   */
> > +  void onNetworkInterfaceReadyChanged(in nsINetworkInterface network);
> 
> s/onNetworkInterfaceReadyChanged/updateNetworkInterfaceReadyState/ and put
> this function between `updateNetworkInterface` and
> `unregisterNetworkInterface`.
Will do.

Comment 17

[John Dai[:jdai]]

Attachment: Provide isReady flag to indicate network interface's ready state. (v8)

Addressed comment #14.

Hi Edgar, May I have your review of this patch? Thanks.

Comment 18

[Edgar Chen [:edgar]]

Since there are some tests failed in try after applying bug 911713 [1], I'd hold the review until those failures are clarified. Thanks.

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

Comment 19

[John Dai[:jdai]]

No longer work on this.

Comment 20

[Edgar Chen [:edgar]]

Comment on attachment 8713066 [details] [diff] [review]
Provide isReady flag to indicate network interface's ready state. (v8)

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

::: dom/system/gonk/DataCallManager.js
@@ +687,5 @@
> +     return false;
> +    }
> +
> +    let connection =
> +    gMobileConnectionService.getItemByServiceId(this.clientId);

Nit: indentation

let connection =
  gMobileConnectionService.getItemByServiceId(this.clientId);

@@ +723,1 @@
>      }

Nit: one empty line here.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +279,5 @@
>          });
>  
>          this._createTimer();
>        }
>      },

Nit: one empty line here.

@@ +307,5 @@
> +
> +        dataCallHandler.registerListener(callback);
> +      });
> +
> +    },

Nit: one empty line here.

Comment 21

[BMO Automation]

Firefox OS is not being worked on