928861 - B2G NetworkManager: Provide a more generic function for connecting

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Description

[Gene Lian (I already quit Mozilla)]

+++ This bug was initially created as a clone of Bug #854326 +++

As title. In the long run, we hope to provide a more generic function for connecting to all kinds of connection type like mobile, MMS or SUPL. Note that this function can be called in a way of re-entry and its callback will return the nsINetworkInterface when successfully connected.

Comment 1

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, IDL changes

Comment 2

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

Comment on attachment 819652 [details] [diff] [review]
Part 1, IDL changes

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

As we had a short discuss before, the thing in my mind is:

  // Represent a handle to the connection that the caller want's to setup.
  interface nsINetworkConnection {
    void disconnect(); // Throws on secondary invocation.
  };

  interface nsINetworkInterfaceCallback {
    // Called with |reason| = "connected", "disconnected", "changed", and maybe other status string.  |iface| as the current binding network interface to the |conn|.
    void callback(nsINetworkConnection conn, nsINetworkInterface iface, ACString reason);
  };

  partial nsINetworkManager {
    void activateConnection(ACString rule, nsINetworkInterfaceCallback);
  }

While |rule| is of format "<feature>[:<device-info>][,<feature2>[:device-info2][,...]]".  For example, it can be "inet" for any connection that provides internet connectivity, and "mms:ril-0" for mms connection provided by the first RadioInterface.

Comment 3

[Gene Lian (I already quit Mozilla)]

Per off-line discussion with Vicamo, in the initiative step, we may provide a simple connect function which takes care of the reentry and timeout issues. We've already had pretty much done in the MmsService.js. Please see MmsConnection.acquire(...) and MmsConnection.release(...). We have to move those logic from MmsService.js to NetworkManager.js.

Reassign this task to :skao. Please feel free to come by at any time to have discussions.

Comment 4

[Shao Hang Kao [:skao]]

Attachment: Part 1, IDL changes

Comment 5

[Shao Hang Kao [:skao]]

Attachment: Part 1, IDL changes

Comment 6

[Shao Hang Kao [:skao]]

Hi Gene, Vicamo,

Could you check if the revised idl matches your previous discussions?

Comment 7

[Gene Lian (I already quit Mozilla)]

Hi Vicamo, note that what skao did is trying to provide a framework which can properly handle:

  1. multi-sim
  2. re-entry
  3. timeout
  4. callback
  5. disconnect
  6. ref count... etc.

which still takes use of the current RIL.setupDataCallByType(...) but can help a lot for the future work. We'll revisit the design when we want to handle multi-preference and strip out the tight connection with RIL.

Comment 8

[Gene Lian (I already quit Mozilla)]

Comment on attachment 829067 [details] [diff] [review]
Part 1, IDL changes

Just having some discussions with Vicamo, he is helping summarize what we discussed. Thanks Vicamo!

Comment 9

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

Comment on attachment 829067 [details] [diff] [review]
Part 1, IDL changes

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

A lot changes after some discussions. :)

::: dom/system/gonk/nsINetworkManager.idl
@@ +131,5 @@
>  };
>  
> +[scriptable, function, uuid(07095b5c-f3c1-4277-bb43-5c11261b6e6d)]
> +interface nsINetworkConnection : nsISupports {
> +  void disconnect();

Since we're not literally disconnecting a network connection in this method call, the name could be very misleading.  It's actually more like releasing a request to networking ability, so "release" will be a better name here.

@@ +141,5 @@
> +  const long NETWORK_CONNECT_STATUS_UNKNOWN = -1;
> +  const long NETWORK_CONNECT_STATUS_CONNECTED = 0;
> +  const long NETWORK_CONNECT_STATUS_TIMEOUT = 1;
> +  const long NETWORK_CONNECT_STATUS_DISCONNECTED = 2;
> +  const long NETWORK_CONNECT_STATUS_CHANGED = 3;

Don't know whether should we report "changed" events to the API users yet.  Let's moving it to further enhancements and skip it now.

@@ +155,5 @@
> +   *        One of the NETWORK_CONNECT_STATUS_* constants.
> +   */
> +  void notifyConnectResult(in nsINetworkConnection conn,
> +                           in nsINetworkInterface network,
> +                           in long status);

Let nsINetworkConnection own two additional attributes "network" and "status" and remove the two in the back from argument list.

@@ +321,5 @@
> +   *
> +   * @param callback
> +   *        Callback to notify the connect result.
> +   */
> +  void connect(in unsigned long serviceId,

Like the "release" method, it will be more appropriate with "acquireConnection" instead.

@@ +322,5 @@
> +   * @param callback
> +   *        Callback to notify the connect result.
> +   */
> +  void connect(in unsigned long serviceId,
> +               in long networkType,

I'd really want to avoid including RIL specific arguments in common APIs like this.  Let's have a dictionary parameter here.

Comment 10

[Shao Hang Kao [:skao]]

Attachment: Part 1, IDL Changes

Comment 11

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

Comment on attachment 830041 [details] [diff] [review]
Part 1, IDL Changes

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

::: dom/system/gonk/nsINetworkManager.idl
@@ +13,5 @@
> + * @param serviceId (optional)
> + *        The ID of the RIL service.
> + *
> + */
> +dictionary NetworkParams {

NetworkConnAcquisitionParams?

@@ +149,5 @@
> +interface nsINetworkConnection : nsISupports {
> +  void release();
> +  readonly attribute nsINetworkInterface network;
> +  readonly attribute long status;
> +}

nit: semi-colon missed.

@@ +152,5 @@
> +  readonly attribute long status;
> +}
> +
> +[scriptable, function, uuid(c71e4336-3a2c-11e3-be3a-abc134733c1f)]
> +interface nsINetworkConnectCallback : nsISupports

nsINetworkConnStatusCallback?

@@ +167,5 @@
> +   *        The connection interface which used to disconnect.
> +   * @param nsINetworkInterface
> +   *        The connected network interface (null if failed to connect).
> +   * @param status
> +   *        One of the NETWORK_CONNECT_STATUS_* constants.

nit: surely we have only nsINetworkConnection now.

@@ +169,5 @@
> +   *        The connected network interface (null if failed to connect).
> +   * @param status
> +   *        One of the NETWORK_CONNECT_STATUS_* constants.
> +   */
> +  void notifyConnectResult(in nsINetworkConnection conn);

I'm lazy.  How about just call it |notify|?

@@ +333,5 @@
> +   * @param callback
> +   *        Callback to notify the connect result.
> +   */
> +  void acquireConnection(/* NetworkParams */
> +                         in jsval networkParams,

void acquireConnection(in jsval params, // NetworkParams
                       in nsINetworkConnectCallback callback);

Comment 12

[Shao Hang Kao [:skao]]

Attachment: Part 1, IDL Changes

fixed nit & naming.

Comment 13

[Shao Hang Kao [:skao]]

Attachment: Part 2, NetworkManager & MmsService

Comment 14

[Gene Lian (I already quit Mozilla)]

Will try to catch up the review by the end of tomorrow.

Comment 15

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8334404 [details] [diff] [review]
Part 2, NetworkManager & MmsService

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

I just removed the NetworkManager.js part and hope to take another review. Thanks!

::: dom/system/gonk/NetworkManager.js
@@ +128,4 @@
>    });
>  }
>  
> +function NetworkConnection() {

Let's construct NetworkConnection with params:

function NetworkConnection(params) {
  this.params = params;
  this.radioInterface = ...;
}

@@ +134,5 @@
> +  network: null,
> +  status: Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_UNKNOWN,
> +  params: null,
> +  callbacks: [],
> +  uuids: [],

Please do:

callbacks: {},

and use:

callbacks[uuid] = callback

and

delete callbacks[uuid]

to manage the callbacks.

@@ +135,5 @@
> +  status: Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_UNKNOWN,
> +  params: null,
> +  callbacks: [],
> +  uuids: [],
> +  refCount: 0,

Is that possible to directly count the number of callbacks?

@@ +150,5 @@
> +  },
> +
> +  removeCallbacks: function removeCallbacks() {
> +    this.callbacks.splice(0, this.callbacks.length);
> +    this.uuids.splice(0, this.uuids.length);

You cannot just remove callbacks. You have to run these callbacks with NETWORK_CONNECT_STATUS_TIMEOUT.

@@ +158,5 @@
> +    if (this.status ==
> +        Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_CONNECTED) {
> +      debug("acquire: already connected");
> +      let lastCallback = this.callbacks[this.callbacks.length-1];
> +      let lastUuid = this.uuids[this.uuids.length-1];

Add spaces around "-".

@@ +161,5 @@
> +      let lastCallback = this.callbacks[this.callbacks.length-1];
> +      let lastUuid = this.uuids[this.uuids.length-1];
> +      lastCallback.notifyConnectResult(this.expose(lastUuid));
> +    }
> +    else {

Combine "else {" into the above line.

@@ +169,5 @@
> +          this.radioInterface.setupDataCallByType("mms");
> +          break;
> +        default:
> +          debug("Unsupported network type in acquireConnection: "
> +            + this.params.networkType);

debug("Unsupported network type in acquireConnection: " +
      this.params.networkType);

@@ +185,5 @@
> +  update: function update(network, status) {
> +    this.network = network;
> +    debug ("status: " + this.status + " => " + status);
> +    if (this.status === status)
> +      return;

Add {} even for one-line condition.

@@ +188,5 @@
> +    if (this.status === status)
> +      return;
> +    this.connectTimer.cancel();
> +    this.status = status;
> +    for (let i=0; i<this.callbacks.length; i++) {

nit: add spaces around operators.

@@ +199,5 @@
> +      network: this.network,
> +      status: this.status,
> +      release: function release() {
> +        let uuidFound = false;
> +        for (let i=0; i<this.callbacks.length; i++) {

nit: add spaces.

@@ +232,5 @@
>   * adjusts routes etc. accordingly.
>   */
>  function NetworkManager() {
>    this.networkInterfaces = {};
> +  this.networkConnections = [];

I'd prefer using an object which is indexed by the params so that you don't need to iterate through the network connections to search.

@@ +962,5 @@
>      }
>    },
>  
> +  acquireConnection: function acquireConnection(params, callback) {
> +    let networkConnection = null;

let networkConnection;

@@ +963,5 @@
>    },
>  
> +  acquireConnection: function acquireConnection(params, callback) {
> +    let networkConnection = null;
> +    for (let i=0; i<this.networkConnections.length; i++) {

nit: please add some spaces in between:

for (let i = 0; i < this.networkConnections.length; i++) {

@@ +971,5 @@
> +        break;
> +      }
> +    }
> +
> +    if (networkConnection == null) {

if (!networkConnection) {
  ...
}

@@ +972,5 @@
> +      }
> +    }
> +
> +    if (networkConnection == null) {
> +      networkConnection = new NetworkConnection();

networkConnection = new NetworkConnection(params);

because params is unique and will be determined one time.

@@ +973,5 @@
> +    }
> +
> +    if (networkConnection == null) {
> +      networkConnection = new NetworkConnection();
> +      networkConnection.params = params;

and drop this line.

@@ +977,5 @@
> +      networkConnection.params = params;
> +#ifdef MOZ_B2G_RIL
> +      if (typeof params.serviceId != 'undefined') {
> +        networkConnection.radioInterface =
> +          this.mRil.getRadioInterface(params.serviceId);

Please put this into the constructor as well. The reason is the same as params.

@@ +984,5 @@
> +      networkConnection.addCallback(callback);
> +      this.networkConnections.push(networkConnection);
> +    }
> +
> +    networkConnection.acquire();

Can we input callback as the parameter of acquire(...) so that you don't need addCallback(...)?

Comment 16

[Shao Hang Kao [:skao]]

Attachment: bug928861.part2.patch

Comment 17

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8337495 [details] [diff] [review]
bug928861.part2.patch

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

I'd like to take another review.

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +165,4 @@
>    mmsProxy: "",
>    mmsPort:  -1,
>  
> +  mmsConn: null,

s/mmsConn/networkConnection

because you're in the MmsConnection object.

@@ -321,5 @@
>     */
>    acquire: function acquire(callback) {
> -    this.refCount++;
> -    this.connectTimer.cancel();
> -    this.disconnectTimer.cancel();

Please keep this line.

@@ +348,5 @@
> +          serviceId: this.serviceId,
> +          networkType: Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS
> +        },
> +        {
> +        notifyConnectResult: function notifyConnectResult(conn) {

I think you can directly input |function notifyConnectResult(...)| as the second parameter.

s/conn/networkConn

@@ +360,5 @@
> +                debug("Got the MMS network connected! Resend the buffered " +
> +                      "MMS requests: number: " + this.pendingCallbacks.length);
> +              }
> +              this.flushPendingCallbacks(
> +                _HTTP_STATUS_ACQUIRE_CONNECTION_SUCCESS);

Don't need to wrap this line.

::: dom/system/gonk/NetworkManager.js
@@ +103,4 @@
>  
>  const MANUAL_PROXY_CONFIGURATION = 1;
>  
> +const NETWORK_CONNECTION_TIME_OUT      = 30000;

Just leave one space before "=".

@@ +129,5 @@
>  }
>  
> +function NetworkConnection(params) {
> +  this.params = params;
> +

this.radioInterface = null;

@@ +131,5 @@
> +function NetworkConnection(params) {
> +  this.params = params;
> +
> +#ifdef MOZ_B2G_RIL
> +  if (typeof params.serviceId != 'undefined') {

if (params.serviceId != null)

or

if (params.serviceId != undefined)

@@ +139,5 @@
> +}
> +NetworkConnection.prototype = {
> +  network: null,
> +  status: Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_UNKNOWN,
> +  params: null,

Drop this line.

@@ +142,5 @@
> +  status: Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_UNKNOWN,
> +  params: null,
> +  callbacks: {},
> +  connectTimer: Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer),
> +  radioInterface: null,

Drop this line.

@@ +149,5 @@
> +    let uuidGen = Cc["@mozilla.org/uuid-generator;1"]
> +                    .getService(Ci.nsIUUIDGenerator);
> +    let uuid = uuidGen.generateUUID();
> +    this.callbacks[uuid] = callback;
> +    debug("added callback with uuid: " + uuid);

debug("addCallback: added callback with uuid: " + uuid);

@@ +153,5 @@
> +    debug("added callback with uuid: " + uuid);
> +    return uuid;
> +  },
> +
> +  removeCallbacks: function removeCallbacks() {

It's not worth making a function if only one point calls it.

@@ +166,5 @@
> +      callback.notifyConnectResult(this.expose(uuid));
> +    } else {
> +      switch (this.params.networkType) {
> +        case Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS:
> +          debug("setup mms data call");

debug("acquire: setup MMS data call");

@@ +171,5 @@
> +          this.radioInterface.setupDataCallByType("mms");
> +          break;
> +        default:
> +          debug("Unsupported network type in acquireConnection: " +
> +            this.params.networkType);

debug("acquire: unsupported network type: " + this.params.networkType);

@@ +178,5 @@
> +
> +      this.connectTimer.
> +        initWithCallback(this.update.bind(this, null, Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_TIMEOUT),
> +                     NETWORK_CONNECTION_TIME_OUT,
> +                     Ci.nsITimer.TYPE_ONE_SHOT);

this.connectTimer.initWithCallback(
  this.update.bind(this, null,
    Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_TIMEOUT),
  NETWORK_CONNECTION_TIME_OUT,
  Ci.nsITimer.TYPE_ONE_SHOT);

@@ +184,5 @@
> +  },
> +
> +  update: function update(network, status) {
> +    this.network = network;
> +    debug ("status: " + this.status + " => " + status);

debug ("update: change status from " + this.status + " to " + status);

@@ +187,5 @@
> +    this.network = network;
> +    debug ("status: " + this.status + " => " + status);
> +    if (this.status === status) {
> +      return;
> +    }

Add one blank line for each early-returned if-block.

@@ +189,5 @@
> +    if (this.status === status) {
> +      return;
> +    }
> +    this.connectTimer.cancel();
> +    this.status = status;

Move this line to the above of the above line.

@@ +192,5 @@
> +    this.connectTimer.cancel();
> +    this.status = status;
> +    for (let uuid in this.callbacks) {
> +      this.callbacks[uuid].notifyConnectResult(this.expose(uuid));
> +    }

if (status = Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_DISCONNECTED) {
  this.callbacks = {};
}

@@ +196,5 @@
> +    }
> +  },
> +
> +  expose: function expose(uuid) {
> +    let networkConnection = {

return {
  ...
};

@@ +200,5 @@
> +    let networkConnection = {
> +      network: this.network,
> +      status: this.status,
> +      release: function release() {
> +        if (typeof this.callbacks[uuid] != 'undefined') {

if (this.callbacks[uuid])

@@ +203,5 @@
> +      release: function release() {
> +        if (typeof this.callbacks[uuid] != 'undefined') {
> +          delete this.callbacks[uuid];
> +        } else {
> +          throw Components.Exception("Connection already released.",

s/Connection/Network connection/

@@ +208,5 @@
> +                                     Cr.NS_ERROR_FAILURE);
> +        }
> +
> +        debug("release: refCount = " + Object.keys(this.callbacks).length);
> +        if (Object.keys(this.callbacks).length == 0) {

let refCount = Object.keys(this.callbacks).length;
debug("release: refCount = " + refCount);
if (refCount == 0) {
  ...
}

@@ +214,5 @@
> +        }
> +      }.bind(this),
> +      QueryInterface: Ci.nsINetworkConnection
> +    }
> +    return networkConnection;

Drop this line.

@@ +336,4 @@
>                  network.type == Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL) {
>                this.removeHostRoutes(network.name);
>                this.addHostRoute(network);
> +              network = subject.QueryInterface(Ci.nsIRilNetworkInterface);

s/network/rilNetwork/

@@ +336,5 @@
>                  network.type == Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL) {
>                this.removeHostRoutes(network.name);
>                this.addHostRoute(network);
> +              network = subject.QueryInterface(Ci.nsIRilNetworkInterface);
> +              for (let i=0; i<this.networkConnections.length; i++) {

Add spaces around operators.

@@ +369,5 @@
>              CaptivePortalDetectionHelper
>                .notify(CaptivePortalDetectionHelper.EVENT_CONNECT, this.active);
>              break;
> +          case Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTING:
> +            // work around: now we won't receive DISCONNECTED

// Work around because NETWORK_STATE_DISCONNECTED is not working.

@@ +371,5 @@
>              break;
> +          case Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTING:
> +            // work around: now we won't receive DISCONNECTED
> +
> +            for (let i=0; i<this.networkConnections.length; i++) {

Add spaces around operators.

@@ +380,5 @@
> +                  if (networkConn.params.serviceId != network.serviceId) {
> +                    break;
> +                  }
> +                  if (networkConn.status !=
> +                      Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_CONNECTED) {

Merge the above two conditions by "||".

@@ +389,5 @@
> +                      Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +                    networkConn.update(network,
> +                      Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_DISCONNECTED);
> +                    networkConn.removeCallbacks();
> +                  }

if (this.mRil.getRadioInterface(network.serviceId).
      getDataCallStateByType("mms") != Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
  networkConn.update(network,
    Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_DISCONNECTED);
}

@@ +954,5 @@
>      }
>    },
>  
> +  acquireConnection: function acquireConnection(params, callback) {
> +    let networkConnection;

s/networkConnection/networkConn/

Comment 18

[Shao Hang Kao [:skao]]

Attachment: Part 2, NetworkManager & MmsService

Comment 19

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8337651 [details] [diff] [review]
Part 2, NetworkManager & MmsService

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +224,5 @@
>     */
>    onDisconnectTimerTimeout: function onDisconnectTimerTimeout() {
>      if (DEBUG) debug("onDisconnectTimerTimeout: deactivate the MMS data call.");
> +    if (this.networkConnection != null) {
> +      debug("call release");

Please drop this debug which is a bit redundant. Also, you have to use:

if (DEBUG) debug(...)

@@ +324,4 @@
>      this.disconnectTimer.cancel();
> +    if (this.networkConnection == null ||
> +        this.networkConnection.status !=
> +          Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_CONNECTED) {

Ditto. Use shorter const names.

@@ +347,5 @@
> +        function notifyConnectResult(conn) {
> +          debug("notifyConnectResult: " + JSON.stringify(conn));
> +          this.networkConnection = conn;
> +
> +          switch (conn.status) {

Just making sure you don't need to handle disconnected case here?

@@ +358,5 @@
> +              this.flushPendingCallbacks(_HTTP_STATUS_ACQUIRE_CONNECTION_SUCCESS);
> +              break;
> +            case Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_TIMEOUT:
> +              if (DEBUG) {
> +                debug("MMS connection time out");

debug("MMS network connection is time out. Drop the pending callbacks.");

@@ +383,3 @@
>      }
> +
> +    this.disconnectTimer.cancel();

Why do you add this? May we have a discussion? Thanks!

@@ -413,5 @@
> -          this.radioInterface.getDataCallStateByType("mms") ==
> -            Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED;
> -
> -        // Return if the MMS network state doesn't change, where the network
> -        // state change can come from other non-MMS networks.

Did you handle this case in the NetworkConnection?

::: dom/system/gonk/NetworkManager.js
@@ +131,5 @@
> +function NetworkConnection(params) {
> +  this.params = params;
> +  this.radioInterface = null;
> +
> +#ifdef MOZ_B2G_RIL

Change the check to:

if (params.networkYype == Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE ||
    params.networkYype == Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS ||
    params.networkYype == Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL) {
  if (params.serviceId == undefined) {
    throw Components.Exception("NetworkConnection constructor: serviceId is not valid",
                               Cr.NS_ERROR_FAILURE);
  }

  this.radioInterface = this.mRil.getRadioInterface(params.serviceId);
}

@@ +132,5 @@
> +  this.params = params;
> +  this.radioInterface = null;
> +
> +#ifdef MOZ_B2G_RIL
> +  if (params.serviceId != undefined) {

And drop this check.

@@ +139,5 @@
> +#endif
> +}
> +NetworkConnection.prototype = {
> +  network: null,
> +  status: Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_UNKNOWN,

nit: please define

const NETWORK_CONNECT_STATUS_XXX = Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_XXX,

on top of this file and replace all the current uses by the shorter names.

@@ +155,5 @@
> +
> +  acquire: function acquire(callback) {
> +    let uuid = this.addCallback(callback);
> +    if (this.status ==
> +        Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_CONNECTED) {

Ditto and one line should be enough:

if (this.status == NETWORK_CONNECT_STATUS_CONNECTED)

@@ +162,5 @@
> +    } else {
> +      switch (this.params.networkType) {
> +        case Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS:
> +          debug("acquire: setup mms data call");
> +          this.radioInterface.setupDataCallByType("mms");

With that check in the constructor, the |this.radioInterface| must be valid.

@@ +166,5 @@
> +          this.radioInterface.setupDataCallByType("mms");
> +          break;
> +        default:
> +          debug("acquire: unsupported network type in acquireConnection: " +
> +            this.params.networkType);

Just dump:

debug("acquire: unsupported network type: " + this.params.networkType);

acquireConnection is the caller not this function.

@@ +172,5 @@
> +      }
> +
> +      this.connectTimer.initWithCallback(
> +        this.update.bind(this, null,
> +          Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_TIMEOUT),

this.update.bind(this, null, NETWORK_CONNECT_STATUS_TIMEOUT),

@@ +192,5 @@
> +      this.callbacks[uuid].notifyConnectResult(this.expose(uuid));
> +    }
> +
> +    if (status ==
> +      Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_DISCONNECTED) {

Ditto.

@@ +206,5 @@
> +        if (this.callbacks[uuid]) {
> +          delete this.callbacks[uuid];
> +        } else {
> +          throw Components.Exception("Network connection already released.",
> +                                     Cr.NS_ERROR_FAILURE);

throw Components.Exception("expose: connection is already released.",
		           Cr.NS_ERROR_FAILURE);

@@ +341,5 @@
> +              let rilNetwork =
> +                subject.QueryInterface(Ci.nsIRilNetworkInterface);
> +              for (let i = 0; i < this.networkConnections.length; i++) {
> +                let networkConn = this.networkConnections[i];
> +                if (networkConn.params.networkType == rilNetwork.type &&

I don't think you can properly handle the shared APN case here. Please come by to discuss.

@@ +344,5 @@
> +                let networkConn = this.networkConnections[i];
> +                if (networkConn.params.networkType == rilNetwork.type &&
> +                    networkConn.params.serviceId == rilNetwork.serviceId) {
> +                  networkConn.update(rilNetwork,
> +                    Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_CONNECTED);

Ditto. Please use the shorter consts.

@@ +381,5 @@
> +#ifdef MOZ_B2G_RIL
> +                case Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS:
> +                  if (networkConn.params.serviceId != network.serviceId ||
> +                      networkConn.status !=
> +                        Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_CONNECTED) {

Ditto.

@@ +383,5 @@
> +                  if (networkConn.params.serviceId != network.serviceId ||
> +                      networkConn.status !=
> +                        Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_CONNECTED) {
> +                    break;
> +                  }

Add one blank line here.

@@ +386,5 @@
> +                    break;
> +                  }
> +                  if (this.mRil.getRadioInterface(network.serviceId).
> +                      getDataCallStateByType("mms") !=
> +                      Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {

Align this line by two more spaces.

@@ +388,5 @@
> +                  if (this.mRil.getRadioInterface(network.serviceId).
> +                      getDataCallStateByType("mms") !=
> +                      Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +                    networkConn.update(network,
> +                      Ci.nsINetworkConnStatusCallback.NETWORK_CONNECT_STATUS_DISCONNECTED);

Ditto. One line.

Comment 20

[Shao Hang Kao [:skao]]

Attachment: Part 2, NetworkManager & MmsService

Comment 21

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8338389 [details] [diff] [review]
Part 2, NetworkManager & MmsService

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

Looks good to me now but I hope Vicamo can take another look. Please fix the following nits and ask for Vicamo's review. Thanks!

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +212,4 @@
>    disconnectTimer: Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer),
>  
>    /**
> +   * Callback when network status changes or cancelled by shutdown.

Callback when network status is changed or cancelled by shutdown.

::: dom/system/gonk/NetworkManager.js
@@ +146,5 @@
> +      params.networkType == Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS ||
> +      params.networkType == Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL) {
> +    if (params.serviceId == undefined) {
> +      throw Components.Exception("NetworkConnection constructor: serviceId is not valid",
> +        Cr.NS_ERROR_FAILURE);

Align the second param with the first one.

@@ +172,5 @@
> +  acquire: function acquire(callback) {
> +    let uuid = this.addCallback(callback);
> +    if (this.status == NETWORK_CONNECT_STATUS_CONNECTED) {
> +      debug("acquire: already connected");
> +      callback.notifyConnectResult(this.expose(uuid));

Please use early return and drop the else.

@@ +181,5 @@
> +          this.radioInterface.setupDataCallByType("mms");
> +          break;
> +        default:
> +          debug("acquire: unsupported network type: " +
> +            this.params.networkType);

Don't need to wrap this line.

@@ +195,5 @@
> +
> +  update: function update(network, status) {
> +    if (this.status === status) {
> +      return;
> +    }

Add one blank line here.

@@ +365,5 @@
> +                    }
> +
> +                    if (this.mRil.getRadioInterface(rilNetwork.serviceId).
> +                        getDataCallStateByType("mms") ==
> +                        Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {

Please add two more spaces for this line:

getDataCallStateByType("mms") ==
  Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {

@@ +367,5 @@
> +                    if (this.mRil.getRadioInterface(rilNetwork.serviceId).
> +                        getDataCallStateByType("mms") ==
> +                        Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +                        networkConn.update(rilNetwork,
> +                          NETWORK_CONNECT_STATUS_CONNECTED);

Please don't wrap this line.

@@ +413,5 @@
> +                  }
> +
> +                  if (this.mRil.getRadioInterface(network.serviceId).
> +                      getDataCallStateByType("mms") !=
> +                      Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {

Ditto. Two more spaces.

@@ +415,5 @@
> +                  if (this.mRil.getRadioInterface(network.serviceId).
> +                      getDataCallStateByType("mms") !=
> +                      Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +                      networkConn.update(network,
> +                        NETWORK_CONNECT_STATUS_DISCONNECTED);

Don't need to wrap this line.

Comment 22

[Gene Lian (I already quit Mozilla)]

Please fix the nits and ask for Vicamo's review again. Thanks!

r=gene

Comment 23

[Shao Hang Kao [:skao]]

Attachment: Part 2, NetworkManager & MmsService

Comment 24

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

Comment on attachment 8339038 [details] [diff] [review]
Part 2, NetworkManager & MmsService

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

Please follow https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F to generate your patch.  These patches don't come with 8-line context, and I can't locate its parent HEAD 7421e8be044d807bdc95ec438641091795f7fc21.

Comment 25

[Shao Hang Kao [:skao]]

Attachment: Part 1, IDL Changes

Comment 26

[Shao Hang Kao [:skao]]

Attachment: Part 2, NetworkManager & MmsService

Comment 27

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

Comment on attachment 8339186 [details] [diff] [review]
Part 2, NetworkManager & MmsService

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

::: dom/system/gonk/NetworkManager.js
@@ +176,5 @@
> +
> +    this.status = status;
> +    this.connectTimer.cancel();
> +    for (let uuid in this.callbacks) {
> +      this.callbacks[uuid].notifyConnectResult(this.expose(uuid));

This means the nsINetworkConnection instances you pass to the callback vary every time the callback is invoked.  A NetworkManager client may keep reference to such an instance, but the calls to nsINetworkConnection::release takes place on different ones.  This also follows |nsINetworkConnection::{network,status}| may not be updated on those local copies.

The nsINetworkConnStatusCallback and nsINetworkConnection should be an 1-to-1 mapping.  Everytime you invoke a callback, it uses the same nsINetworkConnection instance.

@@ +188,5 @@
> +  expose: function expose(uuid) {
> +    return {
> +      network: this.network,
> +      status: this.status,
> +      release: function release() {

Please extract this release function to be a direct member method of the NetworkConnection class.

@@ +202,5 @@
> +        if (refCount == 0) {
> +          this.radioInterface.deactivateDataCallByType("mms");
> +        }
> +      }.bind(this),
> +      QueryInterface: Ci.nsINetworkConnection

How could this even work?  |QueryInterface| should be a function that accepts iid as the only argument and return |this| if input iid matches implemented interfaces.  Please use |XPCOMUtils.generateQI()| for this.

In combined:

  function NetworkConnectionLock(connection, callback) {
    this.connection = connection;
    this.callback = callback;
  }
  NetworkConnectionLock.prototype = {
    QueryInterface: XPCOMUtils.generateQI([Ci.nsINetworkConnection]),
    get network() {
      return this.connection.network;
    },
    get status() {
      return this.connection.status;
    },
    release: function release() {
      let connection = this.connection;
      delete this.connection;
      connection.release(this);
    }
  };

  NetworkConnection.prototype = {
    acquire: function acquire(callback) {
      let lock = new NetworkConnectionLock(this, callback);
      this.locks.push(lock);

      if (this.status == NETWORK_CONNECT_STATUS_CONNECTED) {
        callback.notifyConnectResult(lock);
        return;
      }

      if (this.locks > 1) {
        return;
      }

      this.activate();
    },
    update: function update(network, status) {
      this.network = network;
      this.status = status;

      if (status == NETWORK_STATE_CONNECTED &&
          !thi.locks.length) {
        // All clients release before ever connected.
        this.deactivate();
      } else if (status == NETWORK_STATE_DISCONNECTED &&
          this.locks.length) {
        // New acquisitions before completely disconnected.
        this.activate();
      }

      for (let lock of this.locks.slice()) {
        lock.callback(lock);
      }
    },
    release: function release(lock) {
      let index = this.locks.indexOf(lock);
      if (index < 0) {
        throw Cr.NS_ERROR_UNEXPECTED;
      }
      this.locks.splice(index, 1);

      if (this.locks.length ||
          this.status != NETWORK_STATE_CONNECTED) {
        return;
      }

      this.deactivate();
    },
    activate: function activate() {
      ...
    },
    deactivate: function deactivate() {
      this.radioInterface.deactivateDataCallByType("mms");
    }
  };

Comment 28

[Shao Hang Kao [:skao]]

Attachment: Part 2, NetworkManager & MmsService

Comment 29

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

Comment on attachment 8339836 [details] [diff] [review]
Part 2, NetworkManager & MmsService

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ -201,5 @@
>    //if the MMS network fails to be connected within a timer.
>    pendingCallbacks: [],
>  
> -  /** MMS network connection reference count. */
> -  refCount: 0,

There is one MmsConnection instance per radio interface.  So all mms transactions of the same radio interface shares the same MmsConnection object.  Before every transaction has its own MmsConnection instance, removing refCount here is probably wrong.

@@ +384,5 @@
>    release: function release() {
> +    // The waiting is too small, just skip the timer creation.
> +    if (PREF_TIME_TO_RELEASE_MMS_CONNECTION < 1000) {
> +      this.onDisconnectTimerTimeout();
> +      return;

And it is wrong.  Any transaction releases a MmsConnection can cause onDisconnectTimerTimeout being called.  Basically if you're not going to remove |this.pendingCallbacks| as well, then you still have to keep nearly every line in MmsConnection.

::: dom/system/gonk/NetworkManager.js
@@ +118,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsINetworkConnection]),
> +  get network() {
> +    return this.connection.network;
> +  },
> +  get status() {

nit: empty line between class methods.

@@ +153,5 @@
> +}
> +NetworkConnectionInternal.prototype = {
> +  network: null,
> +  status: NETWORK_CONNECT_STATUS_UNKNOWN,
> +  cbConns: [],

non-primary-type data member initial in class prototype object become "static" data members.  This line means all NetworkConnectionInternal instances share the same |this.cbConns| array object here.  Please initialize it in ctor.

And please just rename it to |connections|.

@@ +154,5 @@
> +NetworkConnectionInternal.prototype = {
> +  network: null,
> +  status: NETWORK_CONNECT_STATUS_UNKNOWN,
> +  cbConns: [],
> +  connectTimer: Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer),

Same here.  |this.connectTimer| becomes a "static" data member.  Please delay its instantiation to |this.activate()|.

Besides, a connectTimer is not every connection acquisition need.  Have to go some day.

@@ +166,5 @@
> +      callback.notifyConnectResult(cbConn);
> +      return;
> +    }
> +
> +    this.activate();

Only call |this.activate()| once.  That is:

  if (this.cbConns.length == 1) {
    this.activate();
  }

However, there is a more tricky issue.  When the last NetworkConnection is released:

  1) call to this.radioInterface.deactivateDataCallByType("mms");
  2) ril_worker replies data call state change "disconnecting"
  3) ril_worker replies data call state change "disconnected"

We can always have a new |acquire| call during 1)-2) or 2)-3), and that results in a flow error.  So, to be more complete:

  if (this.cbConns.length == 1 &&
      (this.status == NETWORK_CONNECT_STATUS_UNKNOWN ||
       this.status == NETWORK_CONNECT_STATUS_DISCONNECTED)) {
    this.activate();
  }

@@ +183,5 @@
> +
> +    if (status == NETWORK_CONNECT_STATUS_CONNECTED && !this.cbConns.length) {
> +      // All clients release before ever connected.
> +      this.deactivate();
> +      return;

Actually I don't think we really need this return statement.  A NetworkConnection doesn't necessarily be released when a disconnected event is dispatched.  So all state changes should lead to a notification to each callbacks.

@@ +185,5 @@
> +      // All clients release before ever connected.
> +      this.deactivate();
> +      return;
> +    } else if (status == NETWORK_CONNECT_STATUS_DISCONNECTED &&
> +        this.cbConns.length) {

nit: align to |status|

@@ +188,5 @@
> +    } else if (status == NETWORK_CONNECT_STATUS_DISCONNECTED &&
> +        this.cbConns.length) {
> +      // New acquisitions before completely disconnected.
> +      this.activate();
> +      return;

ditto.

@@ +204,5 @@
> +    }
> +    this.cbConns.splice(index, 1);
> +
> +    if (this.cbConns.length ||
> +      this.status != NETWORK_CONNECT_STATUS_CONNECTED) {

nit: align to |this.cbConns.length|

@@ +221,5 @@
> +        break;
> +#endif
> +      default:
> +        debug("activate: unsupported network type: " + this.params.networkType);
> +        break;

Accept only NETWORK_TYPE_MOBILE_MMS in NetworkManager::acquireConnection and remove this default label.

@@ +240,5 @@
> +        break;
> +#endif
> +      default:
> +        debug("deactivate: unsupported network type: " + this.params.networkType);
> +        break;

ditto

@@ +393,5 @@
>              CaptivePortalDetectionHelper
>                .notify(CaptivePortalDetectionHelper.EVENT_CONNECT, this.active);
>              break;
> +          case Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTING:
> +            // Work around because NETWORK_STATE_DISCONNECTED is not working.

To be discussed tomorrow.

Comment 30

[Shao Hang Kao [:skao]]

Comment on attachment 8339836 [details] [diff] [review]
Part 2, NetworkManager & MmsService

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ -201,5 @@
>    //if the MMS network fails to be connected within a timer.
>    pendingCallbacks: [],
>  
> -  /** MMS network connection reference count. */
> -  refCount: 0,

we use this.cbConns.length in NetworkManager to replace this.

@@ +384,5 @@
>    release: function release() {
> +    // The waiting is too small, just skip the timer creation.
> +    if (PREF_TIME_TO_RELEASE_MMS_CONNECTION < 1000) {
> +      this.onDisconnectTimerTimeout();
> +      return;

I didn't see the problem? could you give me an example?

::: dom/system/gonk/NetworkManager.js
@@ +188,5 @@
> +    } else if (status == NETWORK_CONNECT_STATUS_DISCONNECTED &&
> +        this.cbConns.length) {
> +      // New acquisitions before completely disconnected.
> +      this.activate();
> +      return;

in this case if we don't early return, someone would receive a DISCONNECTED and than a CONNECTED event after calling acquireConnection. This sounds strange to me.

@@ +221,5 @@
> +        break;
> +#endif
> +      default:
> +        debug("activate: unsupported network type: " + this.params.networkType);
> +        break;

I'll add the checking in acquireConnection. For me it's always good to keep a default condition in a switch even you think the default condition would never reach, but if you insist I can remote it.

Comment 31

[Shao Hang Kao [:skao]]

Attachment: Part 2, NetworkManager & MmsService

Comment 32

[Shao Hang Kao [:skao]]

Attachment: Part 2, NetworkManager & MmsService

Actually this patch now doesn't work on my phone, 

I got this:
E/GeckoConsole( 7798): [JavaScript Error: "gNetworkManager is not defined" {file: "jar:file:///system/b2g/omni.ja!/components/MmsService.js" line: 364}]

I'll do a clean build to see if it helps.

@Gene, if this patch doesn't work for you either. You can try to get 1 or 2 previously obsoleted patches, I tested them and should work. Sorry for the inconvenience.

Comment 33

[Shao Hang Kao [:skao]]

Attachment: Part 2, NetworkManager & MmsService

Now it works

Comment 34

[Shao Hang Kao [:skao]]

Thank you Gene :)

Comment 35

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

Allow me.

Comment 36

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

Comment on attachment 8340267 [details] [diff] [review]
Part 2, NetworkManager & MmsService

To be rewrited.

Comment 37

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 38

[Jessica Jong [:jessica] (inactive)]

Per offline discussion with Edgar, we decided to mark this bug as a meta bug, and file sub-bugs to:
- put all information needed by other modules into a separate interface - nsINetworkInfo
- Add activate()/deactivate in nsINetworkInterface and RIL implementation (probably bug 911713)
- Implement nsINetworkInterface.active()/deactive in wifi network interface
and other bugs that may come to mind later...

Comment 39

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #38)
> Per offline discussion with Edgar, we decided to mark this bug as a meta
> bug, and file sub-bugs to:
> - put all information needed by other modules into a separate interface -
> nsINetworkInfo
> - Add activate()/deactivate in nsINetworkInterface and RIL implementation
> (probably bug 911713)
> - Implement nsINetworkInterface.active()/deactive in wifi network interface
> and other bugs that may come to mind later...

Sorry, please ignore this, commented in wrong bug :(

Comment 40

[Jessica Jong [:jessica] (inactive)]

Attachment: [WIP] Part 1: idl, v1.

After finishing bug 911713 and bug 1207066, we'd like to move on with this bug.
We've gone through the previous design discussed in this bug, and attached is our current proposal. 

There are a few things that we need to consider on the implementation part:
-  In NetworkManager, some networks should be ref-counted, e.g non-default data calls, which means that we should only deactivate it when no one requests it. What about default networks? e.g. wifi and default data call. These networks are either "enabled" or "disabled" based on user selection.

  IMO, NetworkManager will not activate a default network due to requestConnection(), but will activate it only when policy allows it. So, if requestConnection() requests a default network that is not *CONNECTED,* maybe we should just reject or still return the network connection handle, and notify when it is available.

  So, the flow for default network connection would look like this: http://goo.gl/vntKGE. And for non-default network connection: http://goo.gl/q5ZZym

- After calling requestConnection(), should the request live until it is released via nsINetworkConnection.release() or is there any chance we'll let it become "dead"? Consider the following scenarios:

  * airplane mode
  * data registration lost (for mobile)
  * signal lost (for wifi)
  * apn changed (for mobile)
  * service id changed (for mobile)
  * other cases not listed here...

  Should we try to re-connect network connections automatically after the above scenarios? If yes, we should remember their states and routes as well and handle them after the above scenarios. If not, we will need a special state, e.g. "unavailable", to report to API consumers that network connection is not coming back and should be released immediately.

Please note that this is still a early draft, and we are open to discussion/suggestions.

Comment 41

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #40)
> 
> - After calling requestConnection(), should the request live until it is
> released via nsINetworkConnection.release() or is there any chance we'll let
> it become "dead"? Consider the following scenarios:
> 
>   * airplane mode
>   * data registration lost (for mobile)
>   * signal lost (for wifi)
>   * apn changed (for mobile)
>   * service id changed (for mobile)
>   * other cases not listed here...
> 
>   Should we try to re-connect network connections automatically after the
> above scenarios? If yes, we should remember their states and routes as well
> and handle them after the above scenarios. If not, we will need a special
> state, e.g. "unavailable", to report to API consumers that network
> connection is not coming back and should be released immediately.
> 

I think even we are going to try to re-connect, we still need to define a special state because it's likely that reconnection fails eventually.

Comment 42

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #41)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #40)
> > 
> > - After calling requestConnection(), should the request live until it is
> > released via nsINetworkConnection.release() or is there any chance we'll let
> > it become "dead"? Consider the following scenarios:
> > 
> >   * airplane mode
> >   * data registration lost (for mobile)
> >   * signal lost (for wifi)
> >   * apn changed (for mobile)
> >   * service id changed (for mobile)
> >   * other cases not listed here...
> > 
> >   Should we try to re-connect network connections automatically after the
> > above scenarios? If yes, we should remember their states and routes as well
> > and handle them after the above scenarios. If not, we will need a special
> > state, e.g. "unavailable", to report to API consumers that network
> > connection is not coming back and should be released immediately.
> > 
> 
> I think even we are going to try to re-connect, we still need to define a
> special state because it's likely that reconnection fails eventually.

Thanks Hsinyi for bringing up this case. I think if reconnect fails after a certain number of retries, the state remains DISCONNECTED. But there is still chance for it to reconnect, for example, when data registration becomes registered again, NetworkManager would re-check its policy and re-activate it if there is still requests for it. So, I think if we are going to try to re-connect, we can just reflect the state of nsINetworkInfo.

In the case of permanent failure reported by network, we should only reconnect it if apn has changed.

Comment 43

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #42)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #41)
> > (In reply to Jessica Jong [:jjong] [:jessica] from comment #40)
> > > 
> > > - After calling requestConnection(), should the request live until it is
> > > released via nsINetworkConnection.release() or is there any chance we'll let
> > > it become "dead"? Consider the following scenarios:
> > > 
> > >   * airplane mode
> > >   * data registration lost (for mobile)
> > >   * signal lost (for wifi)
> > >   * apn changed (for mobile)
> > >   * service id changed (for mobile)
> > >   * other cases not listed here...
> > > 
> > >   Should we try to re-connect network connections automatically after the
> > > above scenarios? If yes, we should remember their states and routes as well
> > > and handle them after the above scenarios. If not, we will need a special
> > > state, e.g. "unavailable", to report to API consumers that network
> > > connection is not coming back and should be released immediately.
> > > 
> > 
> > I think even we are going to try to re-connect, we still need to define a
> > special state because it's likely that reconnection fails eventually.
> 
> Thanks Hsinyi for bringing up this case. I think if reconnect fails after a
> certain number of retries, the state remains DISCONNECTED. But there is
> still chance for it to reconnect, for example, when data registration
> becomes registered again, NetworkManager would re-check its policy and
> re-activate it if there is still requests for it. So, I think if we are
> going to try to re-connect, we can just reflect the state of nsINetworkInfo.
> 
> In the case of permanent failure reported by network, we should only
> reconnect it if apn has changed.

I got your point that makes sense to me! :)

Comment 44

[BMO Automation]

Firefox OS is not being worked on