Fix logic for VPN badges and network menu order

chrome/browser/chromeos/system/ash_system_tray_delegate.cc

Index: chrome/browser/chromeos/system/ash_system_tray_delegate.cc
diff --git a/chrome/browser/chromeos/system/ash_system_tray_delegate.cc b/chrome/browser/chromeos/system/ash_system_tray_delegate.cc
index 1facfa46eceea9bc27e232d66ff05d3389eb93dd..b2b78a5013e65e3897b319b146f39685a8f967c3 100644
--- a/chrome/browser/chromeos/system/ash_system_tray_delegate.cc
+++ b/chrome/browser/chromeos/system/ash_system_tray_delegate.cc
@@ -86,21 +86,13 @@ namespace {
 // be no upcoming activity notifications that need to be pushed to UI.
 const int kGDataOperationRecheckDelayMs = 5000;
 
-bool ShouldShowNetworkIconInTray(const Network* network) {
-  if (!network)
-    return true;
-  return !network->connected() || network->type() != TYPE_ETHERNET;
-}
-
 ash::NetworkIconInfo CreateNetworkIconInfo(const Network* network,
-                                           NetworkMenuIcon* network_icon,
                                            NetworkMenu* network_menu) {
   ash::NetworkIconInfo info;
   info.name = UTF8ToUTF16(network->name());
-  info.image = network_icon->GetImage(network, NetworkMenuIcon::COLOR_DARK);
+  info.image = NetworkMenuIcon::GetImage(network, NetworkMenuIcon::COLOR_DARK);
   info.service_path = network->service_path();
   info.highlight = network->connected() || network->connecting();
-  info.tray_icon_visible = ShouldShowNetworkIconInTray(network);
   return info;
 }
 
@@ -178,8 +170,7 @@ class SystemTrayDelegate : public ash::SystemTrayDelegate,
         clock_type_(base::k24HourClock),
         search_key_mapped_to_(input_method::kSearchKey),
         screen_locked_(false),
-        state_(STATE_UNKNOWN),
-        connected_network_(NULL),
+        connected_network_state_(STATE_UNKNOWN),
         data_promo_notification_(new DataPromoNotification()) {
     AudioHandler::GetInstance()->AddVolumeObserver(this);
     DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(this);
@@ -504,131 +495,84 @@ class SystemTrayDelegate : public ash::SystemTrayDelegate,
 
   virtual void GetMostRelevantNetworkIcon(ash::NetworkIconInfo* info,
                                           bool dark) OVERRIDE {
-    NetworkLibrary* crosnet = CrosLibrary::Get()->GetNetworkLibrary();
-    info->image = !dark ? network_icon_->GetIconAndText(&info->description) :
-        network_icon_dark_->GetIconAndText(&info->description);
-    info->tray_icon_visible =
-        ShouldShowNetworkIconInTray(crosnet->connected_network());
+    NetworkMenuIcon* icon =
+        dark ? network_icon_dark_.get() : network_icon_.get();
+    info->image = icon->GetIconAndText(&info->description);
+    info->tray_icon_visible = icon->ShouldShowIconInTray();
   }
 
   virtual void GetAvailableNetworks(
       std::vector<ash::NetworkIconInfo>* list) OVERRIDE {
     NetworkLibrary* crosnet = CrosLibrary::Get()->GetNetworkLibrary();
 
-    int connected_index = 0;
+    std::set<const Network*> added;
+
+    // Add the active network first.
+
+    if (crosnet->active_network()) {
+      AddNetworkToList(list, &added, crosnet->active_network());
+    }
+
+    // Add connected/connecting network(s) second, by type.
+
+    if (crosnet->virtual_network()
+        && crosnet->virtual_network()->connecting_or_connected()) {
+      AddNetworkToList(list, &added, crosnet->virtual_network());
+    }
+    if (crosnet->ethernet_network() &&
+        crosnet->ethernet_network()->connecting_or_connected()) {
+      AddNetworkToList(list, &added, crosnet->ethernet_network());
+    }
+    if (crosnet->cellular_network()
+        && crosnet->cellular_network()->connecting_or_connected()) {
+      AddNetworkToList(list, &added, crosnet->cellular_network());
+    }
+    if (crosnet->wimax_network()
+        && crosnet->wimax_network()->connecting_or_connected()) {
+      AddNetworkToList(list, &added, crosnet->wimax_network());
+    }
+    if (crosnet->wifi_network()
+        && crosnet->wifi_network()->connecting_or_connected()) {
+      AddNetworkToList(list, &added, crosnet->wifi_network());
+    }
+
+    // Add remaining networks by type.
 
     // Ethernet.
     if (crosnet->ethernet_available() && crosnet->ethernet_enabled()) {
       const EthernetNetwork* ethernet_network = crosnet->ethernet_network();
-      if (ethernet_network) {
-        ash::NetworkIconInfo info = CreateNetworkIconInfo(ethernet_network,
-                                                          network_icon_.get(),
-                                                          network_menu_.get());
-        if (info.name.empty())
-          info.name =
-              l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_DEVICE_ETHERNET);
-        if (crosnet->ethernet_connecting()) {
-          info.description = l10n_util::GetStringFUTF16(
-              IDS_STATUSBAR_NETWORK_DEVICE_STATUS,
-              l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_DEVICE_ETHERNET),
-              l10n_util::GetStringUTF16(
-                  IDS_STATUSBAR_NETWORK_DEVICE_CONNECTING));
-        }
-        list->push_back(info);
-        if (ethernet_network->connected() || ethernet_network->connecting())
-          ++connected_index;
-      }
+      if (ethernet_network)
+        AddNetworkToList(list, &added, ethernet_network);
     }
 
-    ash::user::LoginStatus login_status = GetUserLoginStatus();
-
     // VPN (only if logged in).
-    if (login_status != ash::user::LOGGED_IN_NONE &&
+    if (GetUserLoginStatus() != ash::user::LOGGED_IN_NONE &&
         (crosnet->connected_network() ||
          crosnet->virtual_network_connected())) {
       const VirtualNetworkVector& vpns = crosnet->virtual_networks();
-      for (size_t i = 0; i < vpns.size(); ++i) {
-        ash::NetworkIconInfo info = CreateNetworkIconInfo(vpns[i],
-            network_icon_.get(), network_menu_.get());
-        if (vpns[i]->connected() || vpns[i]->connecting())
-          list->insert(list->begin() + connected_index++, info);
-        else
-          list->push_back(info);
-      }
+      for (size_t i = 0; i < vpns.size(); ++i)
+        AddNetworkToList(list, &added, vpns[i]);
     }
 
     // Cellular.
     if (crosnet->cellular_available() && crosnet->cellular_enabled()) {
       const CellularNetworkVector& cell = crosnet->cellular_networks();
-      for (size_t i = 0; i < cell.size(); ++i) {
-        ash::NetworkIconInfo info = CreateNetworkIconInfo(cell[i],
-            network_icon_.get(), network_menu_.get());
-        ActivationState state = cell[i]->activation_state();
-        if (state == ACTIVATION_STATE_NOT_ACTIVATED ||
-            state == ACTIVATION_STATE_PARTIALLY_ACTIVATED) {
-          // If a cellular network needs to be activated,
-          // then do not show it in the lock screen.
-          if (login_status == ash::user::LOGGED_IN_LOCKED)
-            continue;
-
-          info.description = l10n_util::GetStringFUTF16(
-              IDS_STATUSBAR_NETWORK_DEVICE_ACTIVATE,
-              info.name);
-        } else if (state == ACTIVATION_STATE_ACTIVATING) {
-          info.description = l10n_util::GetStringFUTF16(
-              IDS_STATUSBAR_NETWORK_DEVICE_STATUS,
-              info.name, l10n_util::GetStringUTF16(
-                  IDS_STATUSBAR_NETWORK_DEVICE_ACTIVATING));
-        } else if (cell[i]->connecting()) {
-          info.description = l10n_util::GetStringFUTF16(
-              IDS_STATUSBAR_NETWORK_DEVICE_STATUS,
-              info.name, l10n_util::GetStringUTF16(
-                  IDS_STATUSBAR_NETWORK_DEVICE_CONNECTING));
-        }
-
-        if (cell[i]->connected() || cell[i]->connecting())
-          list->insert(list->begin() + connected_index++, info);
-        else
-          list->push_back(info);
-      }
+      for (size_t i = 0; i < cell.size(); ++i)
+        AddNetworkToList(list, &added, cell[i]);
     }
 
     // Wimax.
     if (crosnet->wimax_available() && crosnet->wimax_enabled()) {
       const WimaxNetworkVector& wimax = crosnet->wimax_networks();
-      for (size_t i = 0; i < wimax.size(); ++i) {
-        ash::NetworkIconInfo info = CreateNetworkIconInfo(wimax[i],
-            network_icon_.get(), network_menu_.get());
-        if (wimax[i]->connecting()) {
-          info.description = l10n_util::GetStringFUTF16(
-            IDS_STATUSBAR_NETWORK_DEVICE_STATUS,
-            info.name,
-            l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_DEVICE_CONNECTING));
-        }
-        if (wimax[i]->connecting() || wimax[i]->connected())
-          list->insert(list->begin() + connected_index++, info);
-        else
-          list->push_back(info);
-      }
+      for (size_t i = 0; i < wimax.size(); ++i)
+        AddNetworkToList(list, &added, wimax[i]);
     }
 
     // Wifi.
     if (crosnet->wifi_available() && crosnet->wifi_enabled()) {
       const WifiNetworkVector& wifi = crosnet->wifi_networks();
-      for (size_t i = 0; i < wifi.size(); ++i) {
-        ash::NetworkIconInfo info = CreateNetworkIconInfo(wifi[i],
-            network_icon_.get(), network_menu_.get());
-        if (wifi[i]->connecting()) {
-          info.description = l10n_util::GetStringFUTF16(
-            IDS_STATUSBAR_NETWORK_DEVICE_STATUS,
-            info.name,
-            l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_DEVICE_CONNECTING));
-        }
-        if (wifi[i]->connecting() || wifi[i]->connected())
-          list->insert(list->begin() + connected_index++, info);
-        else
-          list->push_back(info);
-      }
+      for (size_t i = 0; i < wifi.size(); ++i)
+        AddNetworkToList(list, &added, wifi[i]);
     }
   }
 
@@ -825,39 +769,36 @@ class SystemTrayDelegate : public ash::SystemTrayDelegate,
     if (observer) {
       ash::NetworkIconInfo info;
       info.image = network_icon_->GetIconAndText(&info.description);
-      info.tray_icon_visible =
-          ShouldShowNetworkIconInTray(crosnet->connected_network());
+      info.tray_icon_visible = network_icon_->ShouldShowIconInTray();
       observer->OnNetworkRefresh(info);
     }
 
-    // Determine whether or not we need to update the icon.
-    const Network* connected_network = crosnet->connected_network();
+    // Update Accessibility.
+
+    std::string connected_network_path;
+    ConnectionState connected_network_state(STATE_UNKNOWN);
+    if (crosnet->connected_network()) {
+      connected_network_path = crosnet->connected_network()->service_path();
+      connected_network_state = crosnet->connected_network()->state();
+    }
     if (accessibility::IsSpokenFeedbackEnabled()) {
       bool speak = false;
-      if (connected_network_ != connected_network) {
+      if ((connected_network_path_ != connected_network_path) ||
+          (Network::IsConnectedState(connected_network_state_) &&
+           !Network::IsConnectedState(connected_network_state)) ||
+          (Network::IsConnectingState(connected_network_state_) &&
+           !Network::IsConnectingState(connected_network_state)) ||
+          (Network::IsDisconnectedState(connected_network_state_) &&
+           !Network::IsDisconnectedState(connected_network_state))) {
         speak = true;
-      } else if (connected_network) {
-        if ((Network::IsConnectedState(state_) &&
-             !connected_network->connected()) ||
-            (Network::IsConnectingState(state_) &&
-             !connected_network->connecting()) ||
-            (Network::IsDisconnectedState(state_) &&
-             !connected_network->disconnected())) {
-          speak = true;
-        }
       }
 
-      if (speak) {
-        AccessibilitySpeak(connected_network);
-      }
+      if (speak)
+        AccessibilitySpeak(crosnet->connected_network());
     }
 
-    connected_network_ = connected_network;
-    if (connected_network) {
-      state_ = connected_network->state();
-    } else {
-      state_ = STATE_UNKNOWN;
-    }
+    connected_network_path_ = connected_network_path_;
+    connected_network_state_ = connected_network_state;
   }
 
   void NotifyRefreshBluetooth() {
@@ -932,6 +873,67 @@ class SystemTrayDelegate : public ash::SystemTrayDelegate,
     accessibility::Speak(connection_string.c_str());
   }
 
+  void AddNetworkToList(std::vector<ash::NetworkIconInfo>* list,
+                        std::set<const Network*>* added,
+                        const Network* network) {
+    // Only add networks to the list once.
+    if (added->find(network) != added->end())
+      return;
+
+    ash::NetworkIconInfo info = CreateNetworkIconInfo(network,
+                                                      network_menu_.get());
+    switch (network->type()) {
+      case TYPE_ETHERNET:
+        if (info.name.empty()) {
+          info.name =
+              l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_DEVICE_ETHERNET);
+        }
+        break;
+      case TYPE_CELLULAR: {
+        const CellularNetwork* cellular =
+            static_cast<const CellularNetwork*>(network);
+        ActivationState state = cellular->activation_state();
+        if (state == ACTIVATION_STATE_NOT_ACTIVATED ||
+            state == ACTIVATION_STATE_PARTIALLY_ACTIVATED) {
+          // If a cellular network needs to be activated,
+          // then do not show it in the lock screen.
+          if (GetUserLoginStatus() == ash::user::LOGGED_IN_LOCKED)
+            return;
+
+          info.description = l10n_util::GetStringFUTF16(
+              IDS_STATUSBAR_NETWORK_DEVICE_ACTIVATE,
+              info.name);
+        } else if (state == ACTIVATION_STATE_ACTIVATING) {
+          info.description = l10n_util::GetStringFUTF16(
+              IDS_STATUSBAR_NETWORK_DEVICE_STATUS,
+              info.name, l10n_util::GetStringUTF16(
+                  IDS_STATUSBAR_NETWORK_DEVICE_ACTIVATING));
+        } else if (network->connecting()) {
+          info.description = l10n_util::GetStringFUTF16(
+              IDS_STATUSBAR_NETWORK_DEVICE_STATUS,
+              info.name, l10n_util::GetStringUTF16(
+                  IDS_STATUSBAR_NETWORK_DEVICE_CONNECTING));
+        }
+        break;
+      }
+      case TYPE_VPN:
+      case TYPE_WIFI:
+      case TYPE_WIMAX:
+      case TYPE_BLUETOOTH:
+      case TYPE_UNKNOWN:
+        break;
+    }
+    if (network->connecting()) {
+      info.description = l10n_util::GetStringFUTF16(
+          IDS_STATUSBAR_NETWORK_DEVICE_STATUS,
+          info.name,
+          l10n_util::GetStringUTF16(
+              IDS_STATUSBAR_NETWORK_DEVICE_CONNECTING));
+    }
+    added->insert(network);
+    list->push_back(info);
+  }
+
   // Overridden from AudioHandler::VolumeObserver.
   virtual void OnVolumeChanged() OVERRIDE {
     float level = AudioHandler::GetInstance()->GetVolumePercent() / 100.f;
@@ -1239,8 +1241,8 @@ class SystemTrayDelegate : public ash::SystemTrayDelegate,
   base::HourClockType clock_type_;
   int search_key_mapped_to_;
   bool screen_locked_;
-  ConnectionState state_;
-  const Network* connected_network_;
+  ConnectionState connected_network_state_;
+  std::string connected_network_path_;
 
   scoped_refptr<BluetoothAdapter> bluetooth_adapter_;