Settings: Network: Merge Tether networks into Mobile subpage

chrome/browser/resources/settings/internet_page/internet_subpage.js

Index: chrome/browser/resources/settings/internet_page/internet_subpage.js
diff --git a/chrome/browser/resources/settings/internet_page/internet_subpage.js b/chrome/browser/resources/settings/internet_page/internet_subpage.js
index 71ef10933330e0153d3944127f6c3f224deb941c..8f3d568293a01772cc3308c1ed764d63cc301b1d 100644
--- a/chrome/browser/resources/settings/internet_page/internet_subpage.js
+++ b/chrome/browser/resources/settings/internet_page/internet_subpage.js
@@ -30,6 +30,13 @@ Polymer({
      */
     deviceState: Object,
 
+    /**
+     * If both Cellular and Tether technologies exist, we combine the subpages
+     * and set this to the device state for Tether.
+     * @type {!CrOnc.DeviceStateProperties|undefined}
+     */
+    tetherDeviceState: Object,
+
     /** @type {!chrome.networkingPrivate.GlobalPolicy|undefined} */
     globalPolicy: Object,
 
@@ -171,15 +178,34 @@ Polymer({
       visible: true,
       configured: false
     };
-    this.networkingPrivate.getNetworks(filter, function(networkStates) {
-      if (!this.deviceState)
-        return;
-      if (this.deviceState.Type != CrOnc.Type.VPN) {
-        this.networkStateList_ = networkStates;
-        return;
-      }
-      // For VPNs, separate out third party VPNs.
-      var networkStateList = [];
+    this.networkingPrivate.getNetworks(filter, this.onGetNetworks_.bind(this));
+  },
+
+  /**
+   * @param {!Array<!CrOnc.NetworkStateProperties>} networkStates
+   * @private
+   */
+  onGetNetworks_: function(networkStates) {
+    if (!this.deviceState)
+      return;  // Edge case when device states change before this callback.
+
+    // For the Cellular/Mobile subpage, request Tether networks if available.
+    if (this.deviceState.Type == CrOnc.Type.CELLULAR &&

[Kyle Horimoto, 2017/06/01 20:15:21]

What about the opposite case when type == TETHER? Don't we have to do the
reverse and look up the CELLULAR networks?

For reference, we open that page here:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/net/tether_notif...

Alternatively, we could change the entry point to go Cellular instead of Tether.

[stevenjb, 2017/06/01 21:55:03]

Ah, good catch. Rather than complicate this code, I added a small change to
always pass the Cellular device state to the subpage if both technologies are
available and we navigate directly to the Tether subpage. (That data binding is
in internet_page.html.)

I also update the comment for |deviceState|.

+        this.tetherDeviceState) {
+      var filter = {
+        networkType: CrOnc.Type.TETHER,
+        visible: true,
+        configured: false
+      };
+      this.networkingPrivate.getNetworks(filter, function(tetherNetworkStates) {
+        this.networkStateList_ = networkStates.concat(tetherNetworkStates);
+      }.bind(this));
+      return;
+    }
+
+    // For VPNs, separate out third party VPNs.
+    if (this.deviceState.Type == CrOnc.Type.VPN) {
+      var builtinNetworkStates = [];
       var thirdPartyVpns = {};
       for (var i = 0; i < networkStates.length; ++i) {
         var state = networkStates[i];
@@ -189,12 +215,14 @@ Polymer({
           thirdPartyVpns[providerType] = thirdPartyVpns[providerType] || [];
           thirdPartyVpns[providerType].push(state);
         } else {
-          networkStateList.push(state);
+          builtinNetworkStates.push(state);
         }
+        networkStates = builtinNetworkStates;
+        this.thirdPartyVpns_ = thirdPartyVpns;
       }
-      this.networkStateList_ = networkStateList;
-      this.thirdPartyVpns_ = thirdPartyVpns;
-    }.bind(this));
+    }
+
+    this.networkStateList_ = networkStates;
   },
 
   /**
@@ -246,7 +274,6 @@ Polymer({
       return '';
     switch (deviceState.Type) {
       case CrOnc.Type.TETHER:
-        return this.i18n('internetToggleTetherA11yLabel');
       case CrOnc.Type.CELLULAR:
         return this.i18n('internetToggleMobileA11yLabel');
       case CrOnc.Type.WI_FI:
@@ -405,6 +432,30 @@ Polymer({
   },
 
   /**
+   * @param {!CrOnc.DeviceStateProperties|undefined} deviceState
+   * @param {!CrOnc.DeviceStateProperties|undefined} tetherDeviceState
+   * @return {boolean}
+   * @private
+   */
+  tetherToggleIsVisible_: function(deviceState, tetherDeviceState) {

[Kyle Horimoto, 2017/06/01 20:15:21]

Edge case I thought of: What if cellular is not present and there are no tether
networks in the list? Wouldn't this result in a top toggle and a bottom toggle
with no networks in between? Should we still show the tether toggle in this
case?

[stevenjb, 2017/06/01 21:55:03]

Correct. I believe we still need access to both toggles. (If we decide to change
this or add some additional text we can do that later).

I think there may be some additional edge cases, but I'm not actually set up to
test on a device, so I leave those as an exercise for the reader :)

+    return !!deviceState && deviceState.Type == CrOnc.Type.CELLULAR &&
+        !!tetherDeviceState &&
+        tetherDeviceState.State != CrOnc.DeviceState.UNINITIALIZED;

[Ryan Hansberry, 2017/06/01 18:13:00]

[Apologies in advance for being a noob here] 

Does this mean that if the Tether technology state is UNINITIALIZED that the
toggle will not be visible? Right now Tether technology state is UNINITIALIZED
when Bluetooth or Cellular are disabled, but it's expected that the Tether
toggle be greyed out as opposed to invisible when that's the case.

[stevenjb, 2017/06/01 21:55:03]

OK. That is not made clear in the UX design doc I have to reference. Is there
another document where this is mentioned that we can link to the issue?

I will add a tetherToggleIsEnabled_ and add the UNINITIALIZED test there.

Is there some other message we should show in that case to explain that? Again,
a design doc would be nice.

Note: There are actually *TWO* places where we might show this toggle. Here in
the subpage, and in network-summary-item (on the main page) when there is no
Cellular technology available. I will modify both.

[Kyle Horimoto, 2017/06/01 22:21:46]

Did you make this change? I don't think I see it, but maybe I'm missing it?

[stevenjb, 2017/06/01 22:37:02]

I did... this function no longer tests 'etherDeviceState.State !=
CrOnc.DeviceState.UNINITIALIZED' and see tetherToggleIsEnabled_ in this file...

+  },
+
+  /**
+   * @param {!Event} event
+   * @private
+   */
+  onTetherEnabledTap_: function(event) {
+    this.fire('device-enabled-toggled', {
+      enabled: !this.deviceIsEnabled_(this.tetherDeviceState),
+      type: CrOnc.Type.TETHER,
+    });
+    event.stopPropagation();
+  },
+
+  /**
    * @param {*} lhs
    * @param {*} rhs
    * @return {boolean}