Merged Tether and cellular network types in System Tray.

ash/system/network/network_list.cc

Index: ash/system/network/network_list.cc
diff --git a/ash/system/network/network_list.cc b/ash/system/network/network_list.cc
index db5cc7c85fb19140ae3748c8fd373538b24620b1..ef1d534715f103298330c6b719642fbe3131d26c 100644
--- a/ash/system/network/network_list.cc
+++ b/ash/system/network/network_list.cc
@@ -221,29 +221,14 @@ class CellularHeaderRowView : public NetworkListView::SectionHeaderRowView {
         NetworkHandler::Get()->network_state_handler();
     handler->SetTechnologyEnabled(NetworkTypePattern::Cellular(), is_on,
                                   chromeos::network_handler::ErrorCallback());
+    handler->SetTechnologyEnabled(NetworkTypePattern::Tether(), is_on,

[Ryan Hansberry, 2017/05/17 20:46:18]

This works fine in the case that the device has no cellular modem -- the first
SetTechnologyEnabled call will be a no-op and the toggle UI will behave fine.

However, I'm concerned about the case of the device having a cellular modem.
After glancing at the mocks, it looks like only the toggle specific to Tether
inside Settings should specifically enable/disable Tether. When the device does
have a cellular modem, this toggle should only enable/disable the broad cellular
settings section, and leave alone the Tether enabled/disabled user preference
(sorry if I'm being unclear, please let me know if I need to reword this).

I think the best way to account for both cases is to first check if a cellular
modem exists (NetworkStateHandler::IsTechnologyAvailable) and then:
1. If no cellular modem exists, only set the Tether technology as
enabled/disabled.
2. If a cellular modem exists, only set the cellular technology as
enabled/disabled. We will now need to listen for cellular technology changes.
 
Leslie, to accommodate #2, in TetherService::DeviceListChanged, please also
listen for a change to the cellular device enabled/disabled event, as it already
does for the Tether device. IMO, the Tether technology state should be
'TECHNOLOGY_UNINITIALIZED' when cellular is disabled. Make that change in this
CL; it should be small.

+                                  chromeos::network_handler::ErrorCallback());
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(CellularHeaderRowView);
 };
 
-class TetherHeaderRowView : public NetworkListView::SectionHeaderRowView {
- public:
-  TetherHeaderRowView()
-      : SectionHeaderRowView(IDS_ASH_STATUS_TRAY_NETWORK_TETHER) {}
-
-  ~TetherHeaderRowView() override {}
-
-  const char* GetClassName() const override { return "TetherHeaderRowView"; }
-
- protected:
-  void OnToggleToggled(bool is_on) override {
-    // TODO (hansberry): Persist toggle to settings/preferences.
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TetherHeaderRowView);
-};
 
 class WifiHeaderRowView : public NetworkListView::SectionHeaderRowView {
  public:
@@ -326,10 +311,8 @@ NetworkListView::NetworkListView(SystemTrayItem* owner, LoginStatus login)
       no_wifi_networks_view_(nullptr),
       no_cellular_networks_view_(nullptr),
       cellular_header_view_(nullptr),
-      tether_header_view_(nullptr),
       wifi_header_view_(nullptr),
       cellular_separator_view_(nullptr),
-      tether_separator_view_(nullptr),
       wifi_separator_view_(nullptr),
       connection_warning_(nullptr) {}
 
@@ -457,12 +440,12 @@ void NetworkListView::UpdateNetworkIcons() {
         prohibited_by_policy;
     info->connected = network->IsConnectedState();
     info->connecting = network->IsConnectingState();
-    if (network->Matches(NetworkTypePattern::WiFi()))
+    if (network->Matches(NetworkTypePattern::WiFi())) {
       info->type = NetworkInfo::Type::WIFI;
-    else if (network->Matches(NetworkTypePattern::Cellular()))
+    } else if (network->Matches(NetworkTypePattern::Cellular()) ||
+               network->Matches(NetworkTypePattern::Tether())) {
       info->type = NetworkInfo::Type::CELLULAR;
-    else if (network->Matches(NetworkTypePattern::Tether()))
-      info->type = NetworkInfo::Type::TETHER;
+    }
     if (prohibited_by_policy) {
       info->tooltip =
           l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_NETWORK_PROHIBITED);
@@ -543,11 +526,13 @@ NetworkListView::UpdateNetworkListEntries() {
       UpdateNetworkChildren(NetworkInfo::Type::UNKNOWN, index);
   index += new_guids->size();
 
-  if (handler->IsTechnologyAvailable(NetworkTypePattern::Cellular())) {
+  if (handler->IsTechnologyAvailable(NetworkTypePattern::Cellular()) ||
+      handler->IsTechnologyAvailable(NetworkTypePattern::Tether())) {
     index = UpdateSectionHeaderRow(
         NetworkTypePattern::Cellular(),
-        handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()), index,
-        &cellular_header_view_, &cellular_separator_view_);
+        handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()) ||
+            handler->IsTechnologyEnabled(NetworkTypePattern::Tether()),

[Ryan Hansberry, 2017/05/17 20:46:18]

Given my comment above, please change this to:

handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()) ||         	       
     (!handler->IsTechnologyAvailable(NetworkTypePattern::Cellular()) &&
handler->IsTechnologyEnabled(NetworkTypePattern::Tether()))

This accounts for the case of not having a cellular modem.

+        index, &cellular_header_view_, &cellular_separator_view_);
   }
 
   // Cellular initializing.
@@ -561,30 +546,13 @@ NetworkListView::UpdateNetworkListEntries() {
   if (cellular_message_id)
     ++index;
 
-  // Add cellular networks.
+  // Add cellular and Tether networks.
+  // TODO (hansberry): Audit existing usage of NonVirtual and consider changing
+  // it to include Tether. See crbug.com/693647.
   std::unique_ptr<std::set<std::string>> new_cellular_guids =
       UpdateNetworkChildren(NetworkInfo::Type::CELLULAR, index);
-  index += new_cellular_guids->size();
   new_guids->insert(new_cellular_guids->begin(), new_cellular_guids->end());

[Ryan Hansberry, 2017/05/17 20:46:18]

Why did you move this line? (previously below 'index +=
new_cellular_guids->size();')

[lesliewatkins, 2017/05/22 22:57:37]

An artifact of intermediate changes. It doesn't affect the functionality, but I
moved it back.

-
-  // TODO (hansberry): Audit existing usage of NonVirtual and consider changing
-  // it to include Tether. See crbug.com/693647.
-  if (handler->IsTechnologyAvailable(NetworkTypePattern::Tether())) {
-    index = UpdateSectionHeaderRow(
-        NetworkTypePattern::Tether(),
-        handler->IsTechnologyEnabled(NetworkTypePattern::Tether()), index,
-        &tether_header_view_, &tether_separator_view_);
-
-    // TODO (hansberry): Should a message similar to
-    // IDS_ASH_STATUS_TRAY_NO_CELLULAR_NETWORKS be shown if Tether technology is
-    // enabled but no networks are around?
-
-    // Add Tether networks.
-    std::unique_ptr<std::set<std::string>> new_tether_guids =
-        UpdateNetworkChildren(NetworkInfo::Type::TETHER, index);
-    index += new_tether_guids->size();
-    new_guids->insert(new_tether_guids->begin(), new_tether_guids->end());
-  }
+  index += new_cellular_guids->size();
 
   index = UpdateSectionHeaderRow(
       NetworkTypePattern::WiFi(),
@@ -748,10 +716,9 @@ int NetworkListView::UpdateSectionHeaderRow(NetworkTypePattern pattern,
                                             SectionHeaderRowView** view,
                                             views::Separator** separator_view) {
   if (!*view) {
-    if (pattern.Equals(NetworkTypePattern::Cellular()))
+    if (pattern.Equals(NetworkTypePattern::Cellular()) ||
+        pattern.Equals(NetworkTypePattern::Tether()))
       *view = new CellularHeaderRowView();
-    else if (pattern.Equals(NetworkTypePattern::Tether()))
-      *view = new TetherHeaderRowView();
     else if (pattern.Equals(NetworkTypePattern::WiFi()))
       *view = new WifiHeaderRowView();
     else