Settings: Network: Merge Tether networks into Mobile subpage

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

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview Polymer element for displaying information about WiFi,
  * WiMAX, or virtual networks.
  */
 
 Polymer({
@@ skipping 12 matching lines @@
      * @type {?CrOnc.NetworkStateProperties|undefined}
      */
     defaultNetwork: Object,
 
     /**
      * Device state for the network type.
      * @type {!CrOnc.DeviceStateProperties|undefined}
      */
     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,
 
     /**
      * List of third party VPN providers.
      * @type
      *     {!Array<!chrome.networkingPrivate.ThirdPartyVPNProperties>|undefined}
      */
     thirdPartyVpnProviders: Array,
 
@@ skipping 121 matching lines @@
 
   /** @private */
   getNetworkStateList_: function() {
     if (!this.deviceState)
       return;
     var filter = {
       networkType: this.deviceState.Type,
       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];
         var providerType = state.VPN && state.VPN.ThirdPartyVPN &&
             state.VPN.ThirdPartyVPN.ProviderName;
         if (providerType) {
           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;
   },
 
   /**
    * @param {!CrOnc.DeviceStateProperties|undefined} deviceState
    * @return {boolean} Whether or not the device state is enabled.
    * @private
    */
   deviceIsEnabled_: function(deviceState) {
     return !!deviceState && deviceState.State == CrOnc.DeviceState.ENABLED;
   },
@@ skipping 31 matching lines @@
   /**
    * @param {!CrOnc.DeviceStateProperties|undefined} deviceState
    * @return {string}
    * @private
    */
   getToggleA11yString_: function(deviceState) {
     if (!this.enableToggleIsVisible_(deviceState))
       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:
         return this.i18n('internetToggleWiFiA11yLabel');
       case CrOnc.Type.WI_MAX:
         return this.i18n('internetToggleWiMAXA11yLabel');
     }
     assertNotReached();
     return '';
   },
@@ skipping 138 matching lines @@
     }
     if (state.Type == CrOnc.Type.VPN &&
         (!defaultNetwork ||
          defaultNetwork.ConnectionState != CrOnc.ConnectionState.CONNECTED)) {
       return false;
     }
     return true;
   },
 
   /**
+   * @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}
    */
   isEqual_: function(lhs, rhs) {
     return lhs === rhs;
   },
 });