911713 - B2G NetworkManager: Move policy control logic to NetworkManager

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

In current B2G, when wifi is connected, we will disconnect mobile data for the sake of battery life [1]. And now this is controlled in RIL itself. When the policy is becoming more and more complicate, it is not easy to maintain in a disperse way. IMO, we should have a central manager to make decision for these kinds of policy control.

[1] Please see bug 817985

Comment 1

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 2

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

It's within the 2.5 scope - As a user, I want to have a robust control in data connection.

Comment 3

[Josh Cheng [:josh]]

Hi John,
Could you help provide a target milestone?
Thanks!

Comment 4

[John Dai[:jdai]]

Attachment: Move wifi check policy code to NetworkManager. v1

- Introducing activate()/deactivate() in nsINetworkInterface.
- Moving wifi check policy code to NetworkManager.

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

Comment 5

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8652700 [details] [diff] [review]
Move wifi check policy code to NetworkManager. v1

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

::: dom/system/gonk/DataCallManager.js
@@ +1682,5 @@
> +      return;
> +    }
> +
> +    if (gNetworkManager.activeNetworkInfo &&
> +        gNetworkManager.activeNetworkInfo.type == NETWORK_TYPE_WIFI) {

This shouldn't be here. All of the network policy should be done in NetworkManager, so we shoudln't check for any specific network state here, we just have to remember whether I'm "allowed" by NetworkManager to connect or not.

@@ +1688,5 @@
> +        this.debug("Don't connect data call when Wifi is connected.");
> +      }
> +      return;
> +    }
> +

For default mobile type, we still need to check for data/roaming settings here, otherwise when NetworkManager calls activate(), it will connect directly even if data/roaming settings is OFF.

::: dom/system/gonk/NetworkManager.js
@@ +474,5 @@
> +          if (this.activeNetworkInfo &&
> +              this.activeNetworkInfo.type == Ci.nsINetworkInfo.NETWORK_TYPE_WIFI) {
> +            wifi_active = true;
> +          }
> +

For now, we can keep the simplicity of the original code: just check if extNetworkInfo is connected or not to decide whether to connect/disconnect mobile network.

@@ +493,5 @@
> +        }
> +      }
> +    }
> +  },
> +

We should also call _checkPolicy() whenever a network gets registered/unregistered, so that NetworkManager can select a network to activate/deactivate accordingly.
You can refer to this sequence diagram: http://goo.gl/j8TDng

::: dom/system/gonk/nsINetworkInterface.idl
@@ +100,5 @@
> +
> +  /*
> +   * Activate network interface.
> +   */
> +  void activate();

I think we can define some consts for activate() result, e.g. STATUS_OK, STATUS_APN_TYPE_NOT_AVAILABLE, etc.

@@ +105,5 @@
> +
> +  /*
> +   * Deactivate network interface.
> +   */
> +  void deactivate();

Ditto.

Comment 6

[John Dai[:jdai]]

Attachment: Move wifi check policy code to NetworkManager. v2

Address comment 5. 

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

Comment 7

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8670713 [details] [diff] [review]
Move wifi check policy code to NetworkManager. v2

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

John, please see my comments below and let me know if you have any questions, thanks.

::: dom/system/gonk/DataCallManager.js
@@ +601,5 @@
>          this.debug("No changes for ril.data.enabled flag. Nothing to do.");
>        }
>        return;
>      }
>  

I think the case where we call disconnect if "network interface is enabled but data settings is off" or "network interface is enabled and we are roaming but data raoming settings is off" is missing here.

@@ +1665,5 @@
> +        this.debug("RIL is not ready for data connection: Phone's not " +
> +                   "registered or doesn't have data connection.");
> +      }
> +      return Ci.nsINetworkInterface.NETWORKINTERFACE_RIL_NOT_READY;
> +    }

Also check if this network interface's service id is the default service id for data call.

@@ +1666,5 @@
> +                   "registered or doesn't have data connection.");
> +      }
> +      return Ci.nsINetworkInterface.NETWORKINTERFACE_RIL_NOT_READY;
> +    }
> +

Below are settings check for default data call. The current behavior is that non-default data calls do not refer to these settings, let's keep the current behavior for now.

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

Is there any reason this was moved here? If so, do we need to move _pendingApnSettings check here too?

::: dom/system/gonk/NetworkManager.js
@@ +296,5 @@
> +
> +    if (network.info.type == Ci.nsINetworkInfo.NETWORK_TYPE_WIFI) {
> +      let extNetworkInfo = new ExtraNetworkInfo(network);
> +      this._checkPolicy(extNetworkInfo);
> +    }

You can call _checkPolicy() directly here, see my comment below.

@@ +300,5 @@
> +    }
> +
> +    if (this.isNetworkTypeMobile(network.info.type)) {
> +      network.activate();
> +    }

This will activate all type of mobile networks, however, default mobile network should be activated in _checkPolicy(); for non-default mobile networks, we will activate them through NetworkManager after bug 928861, so let's still rely on setupDataCallByType() for now.

@@ +469,5 @@
>          break;
>      }
>    },
>  
> +  _checkPolicy: function(extNetworkInfo) {

On second thought, let's make this function a little more generic, so that register/unregisterNetworkInterface() can make use of this function directly, e.g:
If wifi is not registered/connected, activate mobile network, otherwise, deactivate mobile network. Hence, there is no need for a parameter in _checkPolicy().

@@ +503,5 @@
>  
> +    if (network.info.type == Ci.nsINetworkInfo.NETWORK_TYPE_WIFI) {
> +      let extNetworkInfo = new ExtraNetworkInfo(network);
> +      this._checkPolicy(extNetworkInfo);
> +    }

You can call _checkPolicy() directly here, see my comment above.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ -835,5 @@
> -   */
> -  updateRILNetworkInterface: function() {
> -    let connHandler = gDataCallManager.getDataCallHandler(this.clientId);
> -    connHandler.updateRILNetworkInterface();
> -  },

Please remember to remove the method in nsIRadioInterface as well.

::: dom/system/gonk/nsINetworkInterface.idl
@@ +88,5 @@
> +  const long NETWORKINTERFACE_RIL_NOT_READY               = 2;
> +  const long NETWORKINTERFACE_ROAMING_DISABLED            = 3;
> +  const long NETWORKINTERFACE_DATACALL_DISABLED           = 4;
> +  const long NETWORKINTERFACE_DEACTIVATING_DATACALLS      = 5;
> +  const long NETWORKINTERFACE_NETWORKMANAGER_NOT_ALLOWED  = 6;

Would STATUS_* be better?

@@ +113,5 @@
> +
> +  /*
> +   * Activate network interface.
> +   */
> +  string activate();

Should the return type be long?

@@ +118,5 @@
> +
> +  /*
> +   * Deactivate network interface.
> +   */
> +  string deactivate();

Ditto.

Comment 8

[John Dai[:jdai]]

Attachment: Move wifi check policy code to NetworkManager. v3

Address comment 7. 

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

Comment 9

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8673597 [details] [diff] [review]
Move wifi check policy code to NetworkManager. v3

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

Thanks John, I think we are almost there. Please see my comments below.

Also, you can test some scenarios manually first to see if it's causing any regression, e.g. switching default service-id for data call, changing apn, airplane mode on/off, etc. Thanks.

::: dom/system/gonk/DataCallManager.js
@@ +617,5 @@
>        }
>        networkInterface.disconnect();
>        return;
>      }
>  

One more case missing here: return directly when data call settings is enabled and default data call enabled (same case for data call settings is disabled and data call is disabled). There are many cases where updateRILNetworkInterface() is called, e.g., data registration change, we'd like to avoid calling connect() multiple times.

@@ +1691,5 @@
> +      if (DEBUG) {
> +        this.debug("RIL is not ready for data connection: Phone's not " +
> +                   "registered or doesn't have data connection.");
> +      }
> +      return Ci.nsINetworkInterface.STATUS_RIL_NOT_READY;

RIL_NOT_READY is not precise enough, maybe STATUS_DATA_NOT_REGISTERED?

@@ +1716,5 @@
> +      if (dataInfo.roaming && !this.dataCallHandler.dataCallSettings.roamingEnabled) {
> +        if (DEBUG) {
> +          this.debug("We're roaming, but data roaming is disabled.");
> +        }
> +        return Ci.nsINetworkInterface.STATUS_ROAMING_DISABLED;

Maybe DATA_ROAMING_DISABLED?

@@ +1723,5 @@
> +      if (!this.activated) {
> +        if (DEBUG) {
> +          this.debug("NetwrokManager not allowed.");
> +        }
> +        return Ci.nsINetworkInterface.STATUS_NETWORKMANAGER_NOT_ALLOWED;

Maybe STATUS_POLICY_NOT_ALLOWED?

I'd prefer to move this check to the top of the function, that means, we don't need to do additional checks if policy is not allowed. And for non-default data calls, we should still check for activated flag, but since we don't support to activate them through NetworkManager, we can call activate()/deactivate() in setup/deactivateDataCallByType() and leave a TODO note there.

::: dom/system/gonk/NetworkManager.js
@@ +300,5 @@
> +
> +    if (network.info.type === Ci.nsINetworkInfo.NETWORK_TYPE_MOBILE) {
> +      network.activate();
> +    }
> +

Just call this._checkPolicy() here if network type is WIFI or MOBILE. No need to activate here, remember, _checkPolicy() is where we decide which network to activate/deactivate.

And please move it before notifyObservers.

@@ +480,5 @@
> +        if (wifi_active &&
> +          this.networkInterfaces[networkId].info.state === Ci.nsINetworkInfo.NETWORK_STATE_CONNECTED) {
> +          debug("Disconnect data call when Wifi is connected.");
> +          this.networkInterfaces[networkId].deactivate();
> +          return;

You must handle all NETWORK_TYPE_MOBILEs (from different service ids), otherwise, you'll have problems in multi-sim case.

@@ +486,5 @@
> +        if (!wifi_active &&
> +            this.networkInterfaces[networkId].info.state !== Ci.nsINetworkInfo.NETWORK_STATE_CONNECTED) {
> +          debug("Connect data call when Wifi is disconnected.");
> +          this.networkInterfaces[networkId].activate();
> +          return;

Ditto.

@@ +509,5 @@
> +    }
> +    if (network.info.type === Ci.nsINetworkInfo.NETWORK_TYPE_MOBILE) {
> +      network.deactivate();
> +    }
> +

Ditto.

And move this after deleting from networkInterfaces, when calling _checkPolicy(), network should be removed from this.networkInterfaces already.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8673597 [details] [diff] [review]
Move wifi check policy code to NetworkManager. v3

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

::: dom/system/gonk/DataCallManager.js
@@ +617,5 @@
>        }
>        networkInterface.disconnect();
>        return;
>      }
>  

After testing multi-sim case, we may need to move the following to connect(), please have a try and reconsider. Thanks.

Comment 11

[John Dai[:jdai]]

Attachment: Move wifi check policy code to NetworkManager. v4

Addressed comment 9 and comment 10. 

Try link:
https://treeherder.allizom.org/#/jobs?repo=try&revision=b462d1b8507a

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

Comment 12

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8675544 [details] [diff] [review]
Move wifi check policy code to NetworkManager. v4

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

Thanks John. Overall looks good, just some minor changes to be refined.
And please help add 3G/wifi switch test cases as discussed.

::: dom/system/gonk/DataCallManager.js
@@ +677,5 @@
>        }
>        return;
>      }
> +    // TODO: Bug 928861 - B2G NetworkManager: Provide a more generic function
> +    //                    for connecting

To be clearer, you can say:
// TODO: activate() should be called from NetworkManager after Bug 928861 -
// B2G NetworkManager: Provide a more generic function for connecting.

@@ +699,5 @@
>        }
>        return;
>      }
> +    // TODO: Bug 928861 - B2G NetworkManager: Provide a more generic function
> +    //                    for connecting

Ditto.

@@ +1646,5 @@
>  
>      gNetworkManager.updateNetworkInterface(this);
>    },
>  
> +  activate: function() {

Should we return directly if it's already activated?

@@ +1663,5 @@
> +    this.activated = false;
> +    if (DEBUG) {
> +        this.debug("deactivate:" + this.activated);
> +    }
> +    this.disconnect();

deactivate() should return one of the STATUS_* values.

@@ +1702,5 @@
> +      return Ci.nsINetworkInterface.STATUS_RADIO_NOT_READY;
> +    }
> +
> +    // Make sure this network interface's service id is the default service id for data call.
> +    if (this.info.serviceId !== this.dataCallHandler.dataCallManager.dataDefaultServiceId) {

nit:
let dataDefaultServiceId = this.dataCallHandler.dataCallManager.dataDefaultServiceId;
if (this.info.serviceId !== dataDefaultServiceId) {
  ...
}

::: dom/system/gonk/NetworkManager.js
@@ +465,5 @@
>          break;
>      }
>    },
>  
> +  _checkPolicy: function() {

Also leave a comment here saying that we are hard-coding the priority of wifi and 3G for now, but there should be a more generic way of deciding which network to activate/deactivate after bug 928861.

::: dom/system/gonk/nsINetworkInterface.idl
@@ +89,5 @@
> +  const long STATUS_POLICY_NOT_ALLOWED          = 3;
> +  const long STATUS_DATA_NOT_REGISTERED         = 4;
> +  const long STATUS_DATA_ROAMING_DISABLED       = 5;
> +  const long STATUS_CHANGING_APN_SETTINGS       = 6;
> +  const long STATUS_DEACTIVATING_DATACALLS      = 7;

Hmmm.. I wonder if we need to expose that much detail to NetworkManager on fail cases, but let's leave it like this for now.

@@ +112,5 @@
>     */
>    readonly attribute long mtu;
> +
> +  /*
> +   * Activate network interface.

nit: leave one empty line here.

@@ +113,5 @@
>    readonly attribute long mtu;
> +
> +  /*
> +   * Activate network interface.
> +   * @returns activate status.

nit: @return activate status, one of the STATUS_* values.

@@ +118,5 @@
> +   */
> +  long activate();
> +
> +  /*
> +   * Deactivate network interface.

Ditto.

@@ +119,5 @@
> +  long activate();
> +
> +  /*
> +   * Deactivate network interface.
> +   * @returns deactivate status.

Ditto.

Comment 13

[Jessica Jong [:jessica] (inactive)]

In this bug, after calling activate() on a RILNetworkInterface, it will remain "activated" and try to connect itself when possible.

However, per offline discussion, this will also be moved to NetworkManager in the future, if network interface is not able to be activated at the moment, e.g. due to radio off, when radio becomes on, we'll notify NetworkManager about a "condition changed" or "ready changed", so that NetworkManager will check its policy to decide whether to activate it again or not. This can be done in bug 928861, but to narrow the scope of the bug, I'll file a separate bug for this.

Comment 14

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #13)
> In this bug, after calling activate() on a RILNetworkInterface, it will
> remain "activated" and try to connect itself when possible.
> 
> However, per offline discussion, this will also be moved to NetworkManager
> in the future, if network interface is not able to be activated at the
> moment, e.g. due to radio off, when radio becomes on, we'll notify
> NetworkManager about a "condition changed" or "ready changed", so that
> NetworkManager will check its policy to decide whether to activate it again
> or not. This can be done in bug 928861, but to narrow the scope of the bug,
> I'll file a separate bug for this.

Filed bug 1216468 for this.

Comment 15

[John Dai[:jdai]]

Attachment: Part1: Move wifi check policy code to NetworkManager. v5

Addressed comment 12.

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

Comment 16

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase. v1

Addressed comment 12.

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

Comment 17

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8677383 [details] [diff] [review]
Part1: Move wifi check policy code to NetworkManager. v5

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

Thanks John, just need a final check after revising these comments.

::: dom/system/gonk/DataCallManager.js
@@ +606,2 @@
>        return;
>      }

nit: keep the empty line here.

@@ +1651,5 @@
> +    }
> +
> +    this.activated = true;
> +    if (DEBUG) {
> +        this.debug("activate:" + this.activated);

nit: indentation

And you can move this to the top, since this.activated would be always true here.

@@ +1663,5 @@
> +    }
> +
> +    this.activated = false;
> +    if (DEBUG) {
> +        this.debug("deactivate:" + this.activated);

Ditto.

@@ +1666,5 @@
> +    if (DEBUG) {
> +        this.debug("deactivate:" + this.activated);
> +    }
> +    this.disconnect();
> +    return Ci.nsINetworkInterface.STATUS_OK;

nit: just to be symmetric with activate(), return this.disconnect().

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

Missing return value here. You can consider merging some of the statuses, e.g. STATUS_GENERIC_ERROR.

@@ +1710,5 @@
> +    if (this.info.serviceId !== dataDefaultServiceId) {
> +      if (DEBUG) {
> +        this.debug("Not dataDefaultServiceId network interface for default data.");
> +      }
> +      return Ci.nsINetworkInterface.STATUS_NETWORKMANAGER_NOT_ALLOWED;

This return value doesn't seem right?

Comment 18

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8677386 [details] [diff] [review]
Part2: Marionette testcase. v1

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

Please see my comments below, thanks.

::: dom/system/gonk/tests/marionette/head.js
@@ +215,5 @@
> + *        A matching function returns true or false to filter the event.
> + *
> + * @return A deferred promise.
> + */
> +function waitForManagerEvent(aEventName, aServiceId, aMatchFun) {

Since this is not in the mobileconnection folder, please use a more specific function name, e.g.  "waitForMobileManagerEvent".

@@ +322,5 @@
>    waitFor(finish, function() {
>      return _pendingEmulatorShellCmdCount === 0;
>    });
>  }
>  

Please add function description like rest of the file. Thanks.

@@ +324,5 @@
>    });
>  }
>  
> +function ensureWifiEnabled(aEnabled) {
> +  if (wifiManager.enabled === aEnabled) {

I can't find where wifiManager is defined?

@@ +330,5 @@
> +    return Promise.resolve();
> +  }
> +  return requestWifiEnabled(aEnabled);
> +}
> +

Please add function description like rest of the file. Thanks.

@@ +343,5 @@
> +
> +  return Promise.all(promises);
> +}
> +
> +/*********Copy from gecko/dom/wifi/test/marionette/head.js ********************/

You can divide head.js into sections using the following format:


/******************************************************************************/
/***                            Wifi Helper Functions                       ***/
/******************************************************************************/

@@ +641,5 @@
> +    });
> +}
> +/******************************************************************************/
> +
> +function testAllNetworkInfo(aAnyConnected) {

Do not put "test cases" here, you should put common helper functions instead in head.js.

@@ +664,5 @@
> +
> +  is(aAnyConnected, connected, "NetworkManager.allNetworkInfo any connected");
> +}
> +
> +function testDefaultNetworkInfo(aServiceId, aConnected) {

Ditto.

::: dom/system/gonk/tests/marionette/test_dsds_switch_wifi_3g.js
@@ +6,5 @@
> +
> +var networkManager =
> +  Cc["@mozilla.org/network/manager;1"].getService(Ci.nsINetworkManager);
> +ok(networkManager,
> +   "networkManager.constructor is " + networkManager.constructor);

Move this to head.js, you can add a helper function, e.g. getNetworkManager().

@@ +9,5 @@
> +ok(networkManager,
> +   "networkManager.constructor is " + networkManager.constructor);
> +
> +var wifiManager = window.navigator.mozWifiManager;
> +ok(wifiManager, "wifiManager.constructor is " + wifiManager.constructor);

Ditto.

@@ +13,5 @@
> +ok(wifiManager, "wifiManager.constructor is " + wifiManager.constructor);
> +
> +var connections;
> +var numOfRadioInterfaces;
> +var currentDataDefaultId = 0;

Are the above three variables used somewhere?

@@ +53,5 @@
> +    .then(() => waitForManagerEvent(TOPIC_DATA_CHANGED, aServiceId))
> +    .then(() => waitForDataState(aServiceId, aConnected));
> +}
> +
> +function switchDataDefaultId(aServiceId, aConnected) {

"switchDataDefaultIdAndWait"?

@@ +58,5 @@
> +  log("switchDataDefaultId: " + aServiceId);
> +  aServiceId = aServiceId || 0;
> +  let promises = [];
> +
> +  //promises.push(waitForMobileConnectionEventOnce(TOPIC_DATA_CHANGED, aServiceId)

remove this line.

@@ +59,5 @@
> +  aServiceId = aServiceId || 0;
> +  let promises = [];
> +
> +  //promises.push(waitForMobileConnectionEventOnce(TOPIC_DATA_CHANGED, aServiceId)
> +  promises.push(waitForDataState(aServiceId, aConnected))

Since we are in chrome context and in gonk folder, you can listen for "network-connection-state-changed" or "network-active-changed" instead.

@@ +80,5 @@
> +    })
> +    .then(() => setEmulatorAPN())
> +    // 3G enabled
> +    .then(() => setDataEnabledAndWait(true))
> +    .then(() => testAllNetworkInfo(true))

You can listen to "network-active-changed" or check for activeNetworkInfo instead.

@@ +82,5 @@
> +    // 3G enabled
> +    .then(() => setDataEnabledAndWait(true))
> +    .then(() => testAllNetworkInfo(true))
> +    // switch to sim2, check sim2 is connected
> +    .then(() => switchDataDefaultId(1, true))

Please use a variable to store current default id for data, to avoid magic numbers.

@@ +85,5 @@
> +    // switch to sim2, check sim2 is connected
> +    .then(() => switchDataDefaultId(1, true))
> +    // wifi connected, 3G should disconnected.
> +    .then(setWifiConnectedAndWait)
> +    .then(() => testDefaultNetworkInfo(1, false))

Ditto.

@@ +88,5 @@
> +    .then(setWifiConnectedAndWait)
> +    .then(() => testDefaultNetworkInfo(1, false))
> +    // wifi disconnected, 3G should connected.
> +    .then(setWifiDisconnected)
> +    .then(() => testDefaultNetworkInfo(1, true))

Ditto.

@@ +93,5 @@
> +    // switch to sim1, check sim1 is connected
> +    .then(() => switchDataDefaultId(0, true))
> +    // 3G disabled
> +    .then(() => setDataEnabledAndWait(false))
> +    .then(() => testAllNetworkInfo(false))

Ditto.

::: dom/system/gonk/tests/marionette/test_switch_wifi_3g.js
@@ +6,5 @@
> +
> +var networkManager =
> +  Cc["@mozilla.org/network/manager;1"].getService(Ci.nsINetworkManager);
> +ok(networkManager,
> +   "networkManager.constructor is " + networkManager.constructor);

Move this to head.js, you can add a helper function, e.g. getNetworkManager().

@@ +9,5 @@
> +ok(networkManager,
> +   "networkManager.constructor is " + networkManager.constructor);
> +
> +var wifiManager = window.navigator.mozWifiManager;
> +ok(wifiManager, "wifiManager.constructor is " + wifiManager.constructor);

Ditto.

@@ +51,5 @@
> +    })
> +    .then(() => setEmulatorAPN())
> +    // 3G enabled
> +    .then(() => setDataEnabledAndWait(true))
> +    .then(() => testAllNetworkInfo(true))

You can listen to "network-active-changed" or check for activeNetworkInfo instead.

@@ +54,5 @@
> +    .then(() => setDataEnabledAndWait(true))
> +    .then(() => testAllNetworkInfo(true))
> +    // wifi connected, 3G should disconnected.
> +    .then(setWifiConnectedAndWait)
> +    .then(() => testDefaultNetworkInfo(0, false))

Ditto.

@@ +57,5 @@
> +    .then(setWifiConnectedAndWait)
> +    .then(() => testDefaultNetworkInfo(0, false))
> +    // wifi disconnected, 3G should connected.
> +    .then(setWifiDisconnected)
> +    .then(() => testDefaultNetworkInfo(0, true))

Ditto.

@@ +60,5 @@
> +    .then(setWifiDisconnected)
> +    .then(() => testDefaultNetworkInfo(0, true))
> +    // 3G disabled
> +    .then(() => setDataEnabledAndWait(false))
> +    .then(() => testAllNetworkInfo(false))

Ditto.

Comment 19

[John Dai[:jdai]]

Attachment: Part1: Move wifi check policy code to NetworkManager. v6

Addressed comment 17.

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

Comment 20

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase. v2

Addressed comment 18.

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

Comment 21

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8678766 [details] [diff] [review]
Part1: Move wifi check policy code to NetworkManager. v6

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

Thanks John for the work!

::: dom/system/gonk/DataCallManager.js
@@ +1712,5 @@
> +    if (this.info.serviceId !== dataDefaultServiceId) {
> +      if (DEBUG) {
> +        this.debug("Not default client id for data call.");
> +      }
> +      return Ci.nsINetworkInterface.STATUS_POLICY_NOT_ALLOWED;

nit: since this status is rather significant, I suggest to use "Ci.nsINetworkInterface.STATUS_NOT_DEFAULT_SERVICE_ID", thanks.

Comment 22

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8678768 [details] [diff] [review]
Part2: Marionette testcase. v2

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

Hi John, please see my comments below.
For wifi part, we can ask :hchang to see if what we are doing is okay and if there is any better approach? Thanks.

::: dom/system/gonk/tests/marionette/head.js
@@ +348,5 @@
> +    .then(function(networks) {
> +      firstNetwork = networks[0];
> +      return testAssociate(firstNetwork);
> +    })
> +    .then(() => waitForObserverEvent(TOPIC_CONNECTION_STATE_CHANGED))

To avoid timing issues, we should call waitForObserverEvent() before associating network. You may need to use Promise.all() instead.

@@ +354,5 @@
> +      is(aSubject.type, NETWORK_TYPE_WIFI,
> +         "subject.type should be " + NETWORK_TYPE_WIFI);
> +      is(aSubject.state, NETWORK_STATE_CONNECTED);
> +
> +      return aSubject;

Do we need to return this? If yes, please correct Fulfill params in function description.

@@ +654,5 @@
> + *        An object of MozWifiNetwork.
> + *
> + * @return A deferred promise.
> + */
> +function testAssociate(aNetwork) {

Since this is an internal helper function instead of a test, please rename the function name to "associateNetwork()".

@@ +655,5 @@
> + *
> + * @return A deferred promise.
> + */
> +function testAssociate(aNetwork) {
> +  //setPasswordIfNeeded(aNetwork);

remove this line?

::: dom/system/gonk/tests/marionette/test_all_network_info.js
@@ +61,1 @@
>    let allNetworkInfo = networkManager.allNetworkInfo;

Since we now have getNetworkManager() and getWifiManager() in head.js, please help switch to use them in this and other files. Thanks.

::: dom/system/gonk/tests/marionette/test_dsds_switch_wifi_3g.js
@@ +112,5 @@
> +      let wifiManager = getWifiManager();
> +      origWifiEnabled = wifiManager.enabled;
> +    })
> +    .then(() => verifyInitialState())
> +    .then(() => killAllHostapd())

We can move this before startHostapds() in head.js.

@@ +120,5 @@
> +    })
> +    .then(() => setEmulatorAPN())
> +    // 3G enabled.
> +    .then(() => setDataEnabledAndWait(true))
> +    .then(() => testAllNetworkInfoAndWait(true))

Per offline discussion, please use verifyActiveNetworkInfo(aType, aServiceId /* optional */)
and check networkManager.activeNetworkInfo in that function.

@@ +122,5 @@
> +    // 3G enabled.
> +    .then(() => setDataEnabledAndWait(true))
> +    .then(() => testAllNetworkInfoAndWait(true))
> +    // switch to sim2, check sim2 is connected.
> +    .then(() => switchDataDefaultIdAndWait(true))

Ditto.

@@ +125,5 @@
> +    // switch to sim2, check sim2 is connected.
> +    .then(() => switchDataDefaultIdAndWait(true))
> +    // wifi connected, 3G should disconnected.
> +    .then(setWifiConnectedAndWait)
> +    .then(() => testDefaultNetworkInfo(false))

Ditto.

@@ +128,5 @@
> +    .then(setWifiConnectedAndWait)
> +    .then(() => testDefaultNetworkInfo(false))
> +    // wifi disconnected, 3G should connected.
> +    .then(setWifiDisconnected)
> +    .then(() => testDefaultNetworkInfo(true))

Ditto.

@@ +133,5 @@
> +    // switch to sim1, check sim1 is connected.
> +    .then(() => switchDataDefaultIdAndWait(true))
> +    // 3G disabled.
> +    .then(() => setDataEnabledAndWait(false))
> +    .then(() => testAllNetworkInfoAndWait(false))

Ditto.

::: dom/system/gonk/tests/marionette/test_switch_wifi_3g.js
@@ +97,5 @@
> +      let wifiManager = getWifiManager();
> +      origWifiEnabled = wifiManager.enabled;
> +    })
> +    .then(() => verifyInitialState())
> +    .then(() => killAllHostapd())

We can move this before startHostapds() in head.js.

@@ +105,5 @@
> +    })
> +    .then(() => setEmulatorAPN())
> +    // 3G enabled.
> +    .then(() => setDataEnabledAndWait(true))
> +    .then(() => testAllNetworkInfoAndWait(true))

Per offline discussion, please use verifyActiveNetworkInfo(aType, aServiceId)
and check networkManager.activeNetworkInfo in that function.

@@ +108,5 @@
> +    .then(() => setDataEnabledAndWait(true))
> +    .then(() => testAllNetworkInfoAndWait(true))
> +    // wifi connected, 3G should disconnected.
> +    .then(setWifiConnectedAndWait)
> +    .then(() => testDefaultNetworkInfo(false))

Ditto.

@@ +111,5 @@
> +    .then(setWifiConnectedAndWait)
> +    .then(() => testDefaultNetworkInfo(false))
> +    // wifi disconnected, 3G should connected.
> +    .then(setWifiDisconnected)
> +    .then(() => testDefaultNetworkInfo(true))

Ditto.

@@ +114,5 @@
> +    .then(setWifiDisconnected)
> +    .then(() => testDefaultNetworkInfo(true))
> +    // 3G disabled.
> +    .then(() => setDataEnabledAndWait(false))
> +    .then(() => testAllNetworkInfoAndWait(false))

Ditto.

Comment 23

[John Dai[:jdai]]

Attachment: Part1: Move wifi check policy code to NetworkManager. r=jjong

Addressed comment 21.

Comment 24

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase. v3

Addressed comment 22.

Since Jessica has been taking maternity leave, Edgar would you help me review this patch? Thanks.

Comment 25

[Edgar Chen [:edgar]]

Comment on attachment 8680543 [details] [diff] [review]
Part2: Marionette testcase. v3

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

Thanks for adding the tests.

Could you also add one more dsds test to ensure switching defaultId works as expected even when wifi is connected?
The test flow is something like,
1. Enable wifi
2. Switch default id to x
3. Disable wifi
4. We expect the data connection will be established on service x.

::: dom/system/gonk/tests/marionette/head.js
@@ +369,5 @@
>  /**
> + * Wait for "network-connection-state-changed" event and
> + * verify state is connected.
> + */
> +function waitForExpectedEvent(aType) {

s/waitForExpectedEvent/waitForConnectedEvent/

@@ +374,5 @@
> +  return waitForObserverEvent(TOPIC_CONNECTION_STATE_CHANGED)
> +    .then(function(aSubject) {
> +      if (aSubject === null) {
> +        log('Not expected event. Wait again!');
> +        return waitForExpectedEvent(aType);

This will do addObserver() and removeObserver() multiple times to wait the target event, but what if the event comes right at the moment that we remove the observer and not yet add it back?

I think we should rewrite waitForObserverEvent(), maybe take an additional matching function which helps to filter out the event, something like https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/tests/marionette/head.js#214.

@@ +414,5 @@
> +            let activeNetworkInfo = networkManager.activeNetworkInfo;
> +            let isActivateNetworkInfo = activeNetworkInfo &&
> +                activeNetworkInfo.state == NETWORK_STATE_CONNECTED &&
> +                activeNetworkInfo.type == NETWORK_TYPE_WIFI;
> +            ok(isActivateNetworkInfo, "ActivateNetworkInfo type should be WIFI and state should be connected.");

Nit: This utility function is to enable the wifi and wait it to get connected, we should just resolve the promise once wifi is connected and leave the checking things to the caller.

@@ +436,5 @@
> + * @return A deferred promise.
> + */
> +function getNetworkInfo(aType, aServiceId) {
> +  if (aServiceId === undefined) {
> +    aServiceId = 0;

function getNetworkInfo(aType, aServiceId = 0) {
  ...
}

@@ +447,5 @@
> +      try {
> +        if (networkInfo instanceof Ci.nsIRilNetworkInfo) {
> +          let rilNetwork = networkInfo.QueryInterface(Ci.nsIRilNetworkInfo);
> +          if (rilNetwork.serviceId != aServiceId) {
> +            continue;

if (rilNetwork.serviceId === aServiceId) {
  return networkInfo;
}

@@ +519,5 @@
> +  let promises = [];
> +  let wifiManager = getWifiManager();
> +  promises.push(waitForTargetEvent(wifiManager, aEnabled ? 'enabled' : 'disabled',
> +    function() {
> +      return wifiManager.enabled === aEnabled ? true : false;

return wifiManager.enabled === aEnabled;

@@ +537,5 @@
> + */
> +function connectToFirstNetwork() {
> +  let firstNetwork;
> +  return requestWifiScan()
> +    .then(function (networks) {

Nit: remove the space between `function` and `(`.

@@ +649,5 @@
> +  function startOneHostapd(aIndex) {
> +    let configFileName = HOSTAPD_CONFIG_PATH + 'ap' + aIndex + '.conf';
> +    return writeHostapdConfFile(configFileName, createConfigFromCommon(aIndex))
> +      .then(() => runEmulatorShellCmdSafe(['hostapd', '-B', configFileName]))
> +      .then(function (reply) {

Ditto.

@@ +650,5 @@
> +    let configFileName = HOSTAPD_CONFIG_PATH + 'ap' + aIndex + '.conf';
> +    return writeHostapdConfFile(configFileName, createConfigFromCommon(aIndex))
> +      .then(() => runEmulatorShellCmdSafe(['hostapd', '-B', configFileName]))
> +      .then(function (reply) {
> +        // It may fail at the first time due to the previous ungracefully terminated one.

Does this comment belongs to `if (reply[0].indexOf('bind(PF_UNIX): Address already in use') !== -1) {'?

@@ +658,5 @@
> +        }
> +
> +        if (reply[0].indexOf('bind(PF_UNIX): Address already in use') !== -1) {
> +          return startOneHostapd(aIndex);
> +        }

Reject the promise for other errors?

@@ +749,5 @@
> +  log("= requestWifiScan =");
> +  let wifiManager = getWifiManager();
> +  let request = wifiManager.getNetworks();
> +  return wrapDomRequestAsPromise(request)
> +    .then(event => event.target.result);

DOMRequest is `then-able` [1] now, you could just: `return request.then(....);`

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/DOMRequest.webidl#24

@@ +783,5 @@
> +  return Promise.all(promises);
> +}
> +
> +function waitForConnected(aExpectedNetwork) {
> +  return waitForWifiManagerEventOnce('statuschange')

1. Use `waitForTargetEvent(...)` instead.
2. Use matching function to filter the event.

@@ +810,5 @@
> + *        A string event name.
> + *
> + * @return A deferred promise.
> + */
> +function waitForWifiManagerEventOnce(aEventName) {

Ditto: This equals to `waitForTargetEvent(getWifiManager(), aEventName)` without passing matching function.

::: dom/system/gonk/tests/marionette/manifest.ini
@@ +18,5 @@
>  [test_network_interface_mtu.js]
>  skip-if = android_version < '19'
>  [test_timezone_changes.js]
> +[test_switch_wifi_3g.js]
> +skip-if = android_version < '19'

Couldn't `test_switch_wifi_3g.js` get passed on emulator-ics? If it could, we don't need this condition.

::: dom/system/gonk/tests/marionette/test_dsds_switch_wifi_3g.js
@@ +34,5 @@
> +
> +  let serviceId = currentDataDefaultId ? 0 : 1;
> +  let promises = [];
> +
> +  promises.push(waitForExpectedEvent(NETWORK_TYPE_MOBILE));

Should we wait for connected event from specific service id?

@@ +127,5 @@
> +    // 3G enabled.
> +    .then(() => setDataEnabledAndWait(true))
> +    .then(() => verifyMobileActiveNetworkInfo())
> +    // Switch to sim2, check sim2 is connected.
> +    .then(() => switchDataDefaultIdAndWait(true))

Pass the serviceId which we want to switch to, instead of hard coding in switchDataDefaultIdAndWait().

@@ +140,5 @@
> +    .then(() => switchDataDefaultIdAndWait(true))
> +    // 3G disabled.
> +    .then(() => setDataEnabledAndWait(false))
> +    // Verify all networkinfo are disconnected.
> +    .then(() => testAllNetworkInfoAndWait(false))

We already wait for TOPIC_CONNECTION_STATE_CHANGED in setDataEnabledAndWait(), why wait again here?

@@ +141,5 @@
> +    // 3G disabled.
> +    .then(() => setDataEnabledAndWait(false))
> +    // Verify all networkinfo are disconnected.
> +    .then(() => testAllNetworkInfoAndWait(false))
> +    // Restore original apn settings and wifi state.

Should we also restore the default id of data connection?

::: dom/system/gonk/tests/marionette/test_switch_wifi_3g.js
@@ +28,5 @@
> +    })
> +    .then(() => ensureWifiEnabled(false));
> +}
> +
> +function testAllNetworkInfoAndWait(aAnyConnected) {

s/testAllNetworkInfoAndWait/testAllNetworkInfo/

@@ +44,5 @@
> +    if (allNetworkInfo.hasOwnProperty(networkId)) {
> +      let networkInfo = allNetworkInfo[networkId];
> +      if (networkInfo.state == NETWORK_STATE_CONNECTED) {
> +        connected = true;
> +        break;

Nit: you could use `let connected = allNetworkInfo.some(...)` [1].

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some

@@ +81,5 @@
> +  aConnected = aConnected ? NETWORK_STATE_CONNECTED : NETWORK_STATE_DISCONNECTED;
> +
> +  let isMobileConnected = (networkInfo &&
> +      networkInfo.state == aConnected &&
> +      networkInfo.type == NETWORK_TYPE_MOBILE);

Nit: indentation 

let isMobileConnected = (networkInfo &&
                         networkInfo.state == aConnected &&
                         networkInfo.type == NETWORK_TYPE_MOBILE);

@@ +88,5 @@
> +
> +// Start test
> +startTestBase(function() {
> +
> +  let origApnSettings, origWifiEnabled;

Nit: One declaration per line.

Comment 26

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase. v4

Addressed comment 25.

Hi Edgar, Would you help me review this patch? Thanks.

Comment 27

[Edgar Chen [:edgar]]

Comment on attachment 8683021 [details] [diff] [review]
Part2: Marionette testcase. v4

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

s/test_dsds_switch_wifi_enable.js/test_dsds_switch_data_default_id_wifi_enabled.js/

::: dom/system/gonk/tests/marionette/head.js
@@ +219,5 @@
>   *        A string topic name.
>   *
>   * @return A deferred promise.
>   */
> +function waitForObserverEvent(aTopic, aMatchFun) {

Please remember to update the documentation.

@@ +358,5 @@
>  /**
> + * Wait for "network-connection-state-changed" event and
> + * verify state is connected.
> + */
> +function waitForConnectedEvent(aState, aType, aServiceId = 0) {

Looks like this utility function isn't only for `connected` state anymore. Please have a proper name and also update the documentation.

@@ +360,5 @@
> + * verify state is connected.
> + */
> +function waitForConnectedEvent(aState, aType, aServiceId = 0) {
> +  return waitForObserverEvent(TOPIC_CONNECTION_STATE_CHANGED, (aSubject) => {
> +    let eventServiceId;

let eventServiceId = 0;

@@ +363,5 @@
> +  return waitForObserverEvent(TOPIC_CONNECTION_STATE_CHANGED, (aSubject) => {
> +    let eventServiceId;
> +    if (aSubject === null) {
> +      log('Subject is null. Wait again!');
> +      return waitForConnectedEvent(aState, aType, aServiceId);

return false;

@@ +381,5 @@
> +      return true;
> +    }
> +
> +    log('Not expected event. Wait again!');
> +    return waitForConnectedEvent(aState, aType, aServiceId);

return false;

@@ +393,5 @@
> + * Fulfill params: (none)
> + *
> + * @return A deferred promise.
> + */
> +function setWifiConnectedAndWait() {

Should this belong to wifi helper function?

@@ +440,5 @@
> + * Fulfill params: (none)
> + *
> + * @return A deferred promise.
> + */
> +function setWifiDisconnected() {

Should this belong to wifi helper function?

@@ +634,5 @@
> +        }
> +        // It may fail at the first time due to the previous ungracefully terminated one.
> +        if (reply[0].indexOf('bind(PF_UNIX): Address already in use') !== -1) {
> +          return startOneHostapd(aIndex);
> +        }

Throw an exception to reject the promise for other errors?

@@ +730,5 @@
> +
> +  return Promise.all(promises);
> +}
> +
> +function waitForConnected(aExpectedNetwork) {

Since this function is only used in associateNetwork, just merge into associateNetwork(aNetwork).

::: dom/system/gonk/tests/marionette/test_dsds_switch_wifi_3g.js
@@ +41,5 @@
> +  let promises = [];
> +
> +  promises.push(waitForConnectedEvent(aConnected, NETWORK_TYPE_MOBILE, aServiceId));
> +  promises.push(setSettings(SETTINGS_KEY_DATA_DEFAULT_ID, aServiceId));
> +  promises.push(muxModem(aServiceId));

It seems we didn't use console command here, why we need this?

@@ +84,5 @@
> +
> +  let isActivateNetworkInfo = activeNetworkInfo &&
> +                              activeNetworkInfo.state == NETWORK_STATE_CONNECTED &&
> +                              activeNetworkInfo.type == aType &&
> +                              aServiceId == currentDataDefaultId;

Should this be activeNetworkInfo.serviceId == aServiceId?
But only nsIRilNetworkInfo has serviceId [1], you should also consider wifi case.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIDataCallManager.idl#9

@@ +125,5 @@
> +    // Switch to sim2, check sim2 is connected.
> +    .then(() => switchDataDefaultIdAndWait(NETWORK_STATE_CONNECTED, SIM2))
> +    // Wifi connected, 3G should disconnected.
> +    .then(setWifiConnectedAndWait)
> +    .then(() => verifyActiveNetworkInfo(NETWORK_TYPE_WIFI, SIM2))

Passing SIM2 here is kind of weird.

@@ +142,5 @@
> +    .then(() => setSettings(SETTINGS_KEY_DATA_APN_SETTINGS, origApnSettings))
> +    .then(() => ensureWifiEnabled(origWifiEnabled))
> +    .then(() => {
> +      if (currentDataDefaultId != SIM1) {
> +        return switchDataDefaultIdAndWait(NETWORK_STATE_DISCONNECTED, SIM1);

Could you explain why checking currentDataDefaultId and waiting DISCONNECTED here?

::: dom/system/gonk/tests/marionette/test_dsds_switch_wifi_enable.js
@@ +40,5 @@
> +  log("= switchDataDefaultIdAndWait = ");
> +  let promises = [];
> +
> +  promises.push(setSettings(SETTINGS_KEY_DATA_DEFAULT_ID, aServiceId));
> +  promises.push(muxModem(aServiceId));

Ditto

@@ +126,5 @@
> +    .then(setWifiConnectedAndWait)
> +    .then(() => verifyActiveNetworkInfo(NETWORK_TYPE_WIFI))
> +    .then(() => testMobileNetworkInfo(false))
> +    // Switch to sim2, check sim2 is disconnected.
> +    .then(() => switchDataDefaultIdAndWait(SIM2))

Just `setSettings(SETTINGS_KEY_DATA_DEFAULT_ID, SIM2)`

@@ +133,5 @@
> +    .then(setWifiDisconnected)
> +    .then(() => verifyActiveNetworkInfo(NETWORK_TYPE_MOBILE, SIM2))
> +    .then(() => testMobileNetworkInfo(true))
> +
> +    // Restore original apn settings and wifi state.

And also disable mobile data?

::: dom/system/gonk/tests/marionette/test_switch_wifi_3g.js
@@ +3,5 @@
> +
> +MARIONETTE_TIMEOUT = 120000;
> +MARIONETTE_HEAD_JS = "head.js";
> +
> +var currentDataDefaultId = 0;

Nit: Get default data service id from setting.

@@ +26,5 @@
> +    .then(value => {
> +      is(value, false, "Data must be off");
> +    })
> +    .then(() => ensureWifiEnabled(false))
> +    .then(killAllHostapd);

setWifiDisconnected() ?

@@ +107,5 @@
> +    .then(() => testMobileNetworkInfo(false))
> +    // Wifi disconnected, 3G should connected.
> +    .then(setWifiDisconnected)
> +    .then(() => verifyActiveNetworkInfo(NETWORK_TYPE_MOBILE))
> +    .then(() => testMobileNetworkInfo(true))

The `testMobileNetworkInfo()` shows in multiple place, I think it worth to move to head.js and becomes to a utility function, e.g. isAnyNetworkConnected()
and do test here like `ok(true, isAnyNetworkConnected(), "test is there any network connected")`

Comment 28

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase. v5

Addressed comment 27.

Hi Edgar, Would you help me review this patch? Thanks.

Comment 29

[Edgar Chen [:edgar]]

Comment on attachment 8683632 [details] [diff] [review]
Part2: Marionette testcase. v5

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

r=me with comment addressed. Thank you.

::: dom/system/gonk/tests/marionette/head.js
@@ +426,5 @@
> + *        A numeric DSDS service id. Default: 0.
> + *
> + * @return true if state is connected.
> + */
> +function isAnyNetworkConnected(aType, aServiceId = 0) {

The function name doesn't match with the code and comment.

@@ +584,5 @@
> + * Resolve when all the hostpads are requested to start. It is not guaranteed
> + * that all the hostapds will be up and running successfully.
> + *
> + * Fulfill params: (none)
> + *Reject params: (none)

Nit: one space after *. And the reject params aren't (none) actually, but error message with string type.

::: dom/system/gonk/tests/marionette/test_dsds_switch_wifi_3g.js
@@ +129,5 @@
> +    // 3G disabled.
> +    .then(() => setDataEnabledAndWait(false))
> +    // Verify all networkinfo are disconnected.
> +    .then(() => testAllNetworkInfo(false))
> +    .then(setWifiDisconnected)

This seems not necessary, Wifi is already disconnected in line #123.

@@ +133,5 @@
> +    .then(setWifiDisconnected)
> +    .then(() => ensureWifiEnabled(origWifiEnabled))
> +    // Disabled mobile data.
> +    .then(() => setSettings(SETTINGS_KEY_DATA_ENABLED, false))
> +    // Restore original apn settings and wifi state.

Move this comment to line #134.

::: dom/system/gonk/tests/marionette/test_dsds_switch_wifi_enable.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Could you rename this test to `test_dsds_switch_data_default_id_wifi_enabled.js`?

@@ +104,5 @@
> +    .then(() => setWifiConnectedAndWait())
> +    .then(() => verifyActiveNetworkInfo(NETWORK_TYPE_WIFI))
> +    .then(() => ok(!isAnyNetworkConnected(NETWORK_TYPE_MOBILE),
> +                   "Mobile network should be disconnected."))
> +    // Switch to sim2, check sim2 is disconnected.

We don't check sim2 actually, please revise the comments to match with the code.

@@ +111,5 @@
> +    // Wifi disconnected, 3G should connected.
> +    .then(setWifiDisconnected)
> +    .then(() => verifyActiveNetworkInfo(NETWORK_TYPE_MOBILE, SIM2))
> +    .then(() => ok(isAnyNetworkConnected(NETWORK_TYPE_MOBILE, SIM2),
> +                "SIM 2 Mobile network should be connected."))

Nit: Indentation, align the arguments.

@@ +117,5 @@
> +    .then(setWifiDisconnected)
> +    .then(() => ensureWifiEnabled(origWifiEnabled))
> +    // Disabled mobile data.
> +    .then(() => setSettings(SETTINGS_KEY_DATA_ENABLED, false))
> +    // Restore original apn settings and wifi state.

Ditto: Move this comment to line #118.

::: dom/system/gonk/tests/marionette/test_switch_wifi_3g.js
@@ +27,5 @@
> +    .then(setWifiDisconnected);
> +}
> +
> +function testAllNetworkInfo(aAnyConnected) {
> +  log("= testAllNetworkInfo = " + aAnyConnected);

(In reply to Edgar Chen [:edgar][:echen] from comment #27)
> Comment on attachment 8683021 [details] [diff] [review]
> Part2: Marionette testcase. v4
> 
> Review of attachment 8683021 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/system/gonk/tests/marionette/test_switch_wifi_3g.js
> 
> @@ +107,5 @@
> > +    .then(() => testMobileNetworkInfo(false))
> > +    // Wifi disconnected, 3G should connected.
> > +    .then(setWifiDisconnected)
> > +    .then(() => verifyActiveNetworkInfo(NETWORK_TYPE_MOBILE))
> > +    .then(() => testMobileNetworkInfo(true))
> 
> The `testMobileNetworkInfo()` shows in multiple place, I think it worth to
> move to head.js and becomes to a utility function, e.g.
> isAnyNetworkConnected()
> and do test here like `ok(true, isAnyNetworkConnected(), "test is there any
> network connected")`

In fact, I was trying to say `testAllNetworkInfo`, not `testMobileNetworkInfo`. Sorry for the wrong comments on previous review. :(

@@ +99,5 @@
> +    // Wifi disconnected, 3G should connected.
> +    .then(setWifiDisconnected)
> +    .then(() => verifyActiveNetworkInfo(NETWORK_TYPE_MOBILE))
> +    .then(() => ok(isAnyNetworkConnected(NETWORK_TYPE_MOBILE),
> +                  "Mobile network should be connected."))

Ditto: Indentation, align the arguments.

Comment 30

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase. r=echen

Addressed comment 29 and carried on r+.

Comment 31

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase. r=echen

Add forgetAllKnownNetworks() when testcases doing cleanup.

Hi Edgar,
Would you help me review this patch? Thanks.

Try link:
https://treeherder.allizom.org/#/jobs?repo=try&revision=787ddddf1fd6

Comment 32

[Edgar Chen [:edgar]]

Comment on attachment 8687790 [details] [diff] [review]
Part2: Marionette testcase. r=echen

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

::: dom/system/gonk/tests/marionette/head.js
@@ +390,5 @@
> +  });
> +}
> +
> +
> +

Nit: remove redundant blank lines

@@ +421,5 @@
> +
> +/**
> + * Determinated Any network state is connected.
> + *
> + * @return true if state is connected. Default: false.

@return true if any network state is connected.

@@ +431,5 @@
> +  let allNetworkInfo = networkManager.allNetworkInfo;
> +  ok(allNetworkInfo, "NetworkManager.allNetworkInfo");
> +
> +  let count = Object.keys(allNetworkInfo).length;
> +  ok(count > 0, "NetworkManager.allNetworkInfo count");

From an utility function point of view, it could be possible that there is no any network registered in NetworkManager. IMO, we should not treat it as failure. And we will return false for such case in following code, removing this checking seems okay.

@@ +761,5 @@
> +  let wifiManager = getWifiManager();
> +  function createForgetNetworkChain(aNetworks) {
> +    let chain = Promise.resolve();
> +
> +    aNetworks.forEach(function (aNetwork) {

Nit: remove the space between `function` and `(`.

@@ +765,5 @@
> +    aNetworks.forEach(function (aNetwork) {
> +      chain = chain.then(() => wifiManager.forget(aNetwork));
> +    });
> +
> +    return chain;

We could consider using Promise.all() to parallel processing.

::: dom/system/gonk/tests/marionette/test_all_network_info.js
@@ +65,5 @@
>      })
>      .then(() => setEmulatorAPN())
>      .then(() => setDataEnabledAndWait(true))
> +    .then(() => ok(isAnyNetworkConnected(),
> +                   "All network should be connected."))

Logging doesn't match with the condition.

::: dom/system/gonk/tests/marionette/test_dsds_switch_data_default_id_wifi_enabled.js
@@ +39,5 @@
> +
> +  let isMobileConnected = (networkInfo &&
> +      networkInfo.state == aConnected &&
> +      networkInfo.type == NETWORK_TYPE_MOBILE);
> +  ok(isMobileConnected, "Mobile network should be " + aConnected ? "CONNECTED" : "DISCONNECTED");

1. |isMobileConnected| is confused, please having better naming.
2. Nit: indentation,

let isMobileConnected = (networkInfo &&
                         networkInfo.state == aConnected &&
                         networkInfo.type == NETWORK_TYPE_MOBILE);

@@ +102,5 @@
> +    .then(() => verifyActiveNetworkInfo(NETWORK_TYPE_MOBILE, SIM2))
> +    .then(() => testMobileNetworkInfo(true, SIM2))
> +    // Restore original apn settings and wifi state.
> +    .then(() => ensureWifiEnabled(true))
> +    .then(forgetAllKnownNetworks)

What about doing this in |setWifiDisconnected()|?

::: dom/system/gonk/tests/marionette/test_dsds_switch_wifi_3g.js
@@ +42,5 @@
> +    log("Data default id switched to: " + aServiceId);
> +  });
> +}
> +
> +function testMobileNetworkInfo(aConnected, aServiceId = 0) {

test_dsds_switch_data_default_id_wifi_enabled.js also uses this, maybe put this in head.js.

@@ +120,5 @@
> +    .then(() => ok(!isAnyNetworkConnected(),
> +                   "All network should be disconnected."))
> +    // Restore original apn settings and wifi state.
> +    .then(() => ensureWifiEnabled(true))
> +    .then(forgetAllKnownNetworks)

Ditto, what about doing this in |setWifiDisconnected()|?

::: dom/system/gonk/tests/marionette/test_switch_wifi_3g.js
@@ +26,5 @@
> +    })
> +    .then(setWifiDisconnected);
> +}
> +
> +function testMobileNetworkInfo(aConnected, aServiceId = 0) {

Ditto, maybe put this in head.js.

@@ +39,5 @@
> +      networkInfo.type == NETWORK_TYPE_MOBILE);
> +  ok(isMobileConnected, "Mobile network should be " + aConnected ? "CONNECTED" : "DISCONNECTED");
> +}
> +
> +function verifyActiveNetworkInfo(aType, aServiceId = 0) {

Put this in head.js since test_dsds_switch_wifi_3g.js also use this.

@@ +99,5 @@
> +    .then(() => setSettings(SETTINGS_KEY_DATA_ENABLED, false))
> +    // Restore original apn settings and wifi state.
> +    .then(() => setSettings(SETTINGS_KEY_DATA_APN_SETTINGS, origApnSettings))
> +    .then(() => ensureWifiEnabled(true))
> +    .then(forgetAllKnownNetworks)

Ditto, what about doing this in |setWifiDisconnected()|?

Comment 33

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase. v6

Addressed comment 32.

Hi Edgar, Would you help me review this patch? Thanks.

Comment 34

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase. v7

The previous patch is not correct. Update a correct patch for review.

Comment 35

[Edgar Chen [:edgar]]

Comment on attachment 8702810 [details] [diff] [review]
Part2: Marionette testcase. v7

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

Looks good to me, thank you.

Comment 36

[John Dai[:jdai]]

Try result:
https://treeherder.allizom.org/#/jobs?repo=try&revision=cbf21374069b&filter-tier=1&filter-tier=2&filter-tier=3&exclusion_profile=false

Comment 37

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase. v8

In comment #36 there are some tests failed in try. The problm is found in setWifiDisconnected(), the parameter of then() is a function instead of a returned promise:
return Promise.resolve().then(setWifiDisconnected)

After resolve this error, MNW returns green.

https://treeherder.allizom.org/#/jobs?repo=try&revision=be27cdf81fd2&exclusion_profile=false&filter-tier=1&filter-tier=2&filter-tier=3

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

Comment 38

[Edgar Chen [:edgar]]

Comment on attachment 8714278 [details] [diff] [review]
Part2: Marionette testcase. v8

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

::: dom/system/gonk/tests/marionette/test_dsds_switch_data_default_id_wifi_enabled.js
@@ +97,5 @@
> +    // Switch to sim2.
> +    .then(() => setSettings(SETTINGS_KEY_DATA_DEFAULT_ID, SIM2))
> +    .then(() => verifyActiveNetworkInfo(NETWORK_TYPE_WIFI))
> +    // Wifi disconnected, 3G should connected.
> +    .then(() => setWifiDisconnected())

`.then(setWifiDisconnected)` gives the then() the reference of function setWifiDisconnected(), not the result of it.
It should have the same result as `.then(() => setWifiDisconnected())`.

So it doesn't look like a necessary change and fix anything, or did I miss something?
Or actually the failure shown in comment #36 was caused by other issue, and it doesn't exist any more?

Comment 39

[John Dai[:jdai]]

Attachment: Part2: Marionette testcase. v9

The failure shown in comment #36 was caused by other issue, which is inside setWifiDisconnected(), the parameter of then() is a function instead of a returned promise:

return forgetAllKnownNetworks().then(ensureWifiEnabled(false))


https://treeherder.allizom.org/#/jobs?repo=try&revision=fbdc45e203ea

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

Comment 40

[John Dai[:jdai]]

No longer work on this.

Comment 41

[BMO Automation]

Firefox OS is not being worked on