Push change for stevenjb:

Issue 2848016: Store wifi network in network_menu_button.cc as a path instead of as a struct.
Store wifi network in network_menu_button.cc as a path instead of as a struct. 
This should address a potential edge case where wifi network information gets updated after the menu is opened and before a network is selected. 

BUG=None 
TEST=None
Review URL: http://codereview.chromium.org/2817036

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@51064 0039d316-1c4b-4281-b951-d872f2087c98

.gitignore

@@ -12,6 +12,7 @@
 *~
 .*.sw?
 .cproject
+.gdbinit
 .DS_Store
 .metadata
 .project

chrome/browser/chromeos/cros/mock_network_library.h

@@ -41,6 +41,11 @@ class MockNetworkLibrary : public NetworkLibrary {
   MOCK_CONST_METHOD0(remembered_cellular_networks,
                      const CellularNetworkVector&(void));
 
+  MOCK_CONST_METHOD2(FindWifiNetworkByPath, bool(const std::string&,
+                                                 WifiNetwork*));
+  MOCK_CONST_METHOD2(FindCellularNetworkByPath, bool(const std::string&,
+                                                     CellularNetwork*));
+
   MOCK_METHOD0(RequestWifiScan, void(void));
   MOCK_METHOD0(ConnectToPreferredNetworkIfAvailable, bool(void));
   MOCK_METHOD0(PreferredNetworkConnected, bool(void));

chrome/browser/chromeos/cros/network_library.cc

@@ -263,6 +263,32 @@ void NetworkLibraryImpl::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+
+bool NetworkLibraryImpl::FindWifiNetworkByPath(
+    const std::string& path, WifiNetwork* result) const {
+  const WifiNetwork* wifi =
+      GetWirelessNetworkByPath(wifi_networks_, path);
+  if (wifi) {
+    if (result)
+      *result = *wifi;
+    return true;
+  }
+  return false;
+}
+
+bool NetworkLibraryImpl::FindCellularNetworkByPath(
+    const std::string& path, CellularNetwork* result) const {
+  const CellularNetwork* cellular =
+      GetWirelessNetworkByPath(cellular_networks_, path);
+  if (cellular) {
+    if (result)
+      *result = *cellular;
+    return true;
+  }
+  return false;
+}
+
 void NetworkLibraryImpl::RequestWifiScan() {
   if (CrosLibrary::Get()->EnsureLoaded()) {
     RequestScan(TYPE_WIFI);
@@ -300,7 +326,7 @@ bool NetworkLibraryImpl::ConnectToPreferredNetworkIfAvailable() {
     for (int i = 0; i < 30; i++) {
       // Update the system and refetch the network.
       UpdateSystemInfo();
-      wifi = GetWifiNetworkByPath(wifi_path);
+      wifi = GetWirelessNetworkByPath(wifi_networks_, wifi_path);
       // See if identity and certpath are available.
       if (wifi && !wifi->identity().empty() && !wifi->cert_path().empty()) {
         LOG(INFO) << "Google wifi set up after " << (i*0.1) << " seconds.";
@@ -386,7 +412,8 @@ void NetworkLibraryImpl::DisconnectFromWirelessNetwork(
 
 void NetworkLibraryImpl::SaveWifiNetwork(const WifiNetwork& network) {
   // Update the wifi network in the local cache.
-  WifiNetwork* wifi = GetWifiNetworkByPath(network.service_path());
+  WifiNetwork* wifi = GetWirelessNetworkByPath(wifi_networks_,
+                                               network.service_path());
   if (wifi)
     *wifi = network;
 
@@ -619,13 +646,21 @@ WifiNetwork* NetworkLibraryImpl::GetWifiNetworkByName(const std::string& name) {
   return NULL;
 }
 
-WifiNetwork* NetworkLibraryImpl::GetWifiNetworkByPath(const std::string& path) {
-  for (size_t i = 0; i < wifi_networks_.size(); ++i) {
-    if (wifi_networks_[i].service_path().compare(path) == 0) {
-      return &wifi_networks_[i];
-    }
-  }
-  return NULL;
+template<typename T> T* NetworkLibraryImpl::GetWirelessNetworkByPath(
+    std::vector<T>& networks, const std::string& path) {
+  typedef typename std::vector<T>::iterator iter_t;
+  iter_t iter = std::find_if(networks.begin(), networks.end(),
+                             WirelessNetwork::ServicePathEq(path));
+  return (iter != networks.end()) ? &(*iter) : NULL;
+}
+
+// const version
+template<typename T> const T* NetworkLibraryImpl::GetWirelessNetworkByPath(
+    const std::vector<T>& networks, const std::string& path) const {
+  typedef typename std::vector<T>::const_iterator iter_t;
+  iter_t iter = std::find_if(networks.begin(), networks.end(),
+                             WirelessNetwork::ServicePathEq(path));
+  return (iter != networks.end()) ? &(*iter) : NULL;
 }
 
 void NetworkLibraryImpl::EnableNetworkDeviceType(ConnectionType device,

chrome/browser/chromeos/cros/network_library.h

@@ -77,6 +77,15 @@ class WirelessNetwork : public Network {
     return name_ < other.name();
   }
 
+  // We frequently want to compare networks by service path.
+  struct ServicePathEq {
+    explicit ServicePathEq(const std::string& path_in) : path(path_in) {}
+    bool operator()(const WirelessNetwork& a) {
+      return a.service_path().compare(path) == 0;
+    }
+    const std::string& path;
+  };
+
   const std::string& name() const { return name_; }
   int strength() const { return strength_; }
   bool auto_connect() const { return auto_connect_; }
@@ -289,6 +298,13 @@ class NetworkLibrary {
   // Returns the list of remembered cellular networks.
   virtual const CellularNetworkVector& remembered_cellular_networks() const = 0;
 
+  // Search the current list of networks by path and if the network
+  // is available, copy the result and return true.
+  virtual bool FindWifiNetworkByPath(const std::string& path,
+                                     WifiNetwork* result) const = 0;
+  virtual bool FindCellularNetworkByPath(const std::string& path,
+                                         CellularNetwork* result) const = 0;
+
   // Request a scan for new wifi networks.
   virtual void RequestWifiScan() = 0;
 
@@ -416,6 +432,11 @@ class NetworkLibraryImpl : public NetworkLibrary,
     return remembered_cellular_networks_;
   }
 
+  virtual bool FindWifiNetworkByPath(const std::string& path,
+                                     WifiNetwork* network) const;
+  virtual bool FindCellularNetworkByPath(const std::string& path,
+                                         CellularNetwork* network) const;
+
   virtual void RequestWifiScan();
   virtual bool ConnectToPreferredNetworkIfAvailable();
   virtual bool PreferredNetworkConnected();
@@ -493,10 +514,16 @@ class NetworkLibraryImpl : public NetworkLibrary,
   WifiNetwork* GetPreferredNetwork();
 
   // Gets the WifiNetwork with the given name. Returns NULL if not found.
+  // Only used by GetPreferredNetwork() to lookup "Google" and "GoogleA" (hack)
   WifiNetwork* GetWifiNetworkByName(const std::string& name);
 
-  // Gets the WifiNetwork with the given path. Returns NULL if not found.
-  WifiNetwork* GetWifiNetworkByPath(const std::string& path);
+  // Gets the WirelessNetwork (WifiNetwork or CellularNetwork) by path
+  template<typename T>
+  T* GetWirelessNetworkByPath(std::vector<T>& networks,
+                              const std::string& path);
+  template<typename T>
+  const T* GetWirelessNetworkByPath(const std::vector<T>& networks,
+                                    const std::string& path) const;
 
   // Enables/disables the specified network device.
   void EnableNetworkDeviceType(ConnectionType device, bool enable);

chrome/browser/chromeos/status/network_menu_button.cc

@@ -116,18 +116,24 @@ void NetworkMenuButton::ActivatedAt(int index) {
       window->Show();
     }
   } else if (flags & FLAG_WIFI) {
-    WifiNetwork wifi = menu_items_[index].wifi_network;
-
-    // If clicked on a network that we are already connected to or we are
-    // currently trying to connect to, then open config dialog.
-    if (wifi.name() == cros->wifi_name()) {
+    WifiNetwork wifi;
+    bool wifi_exists = cros->FindWifiNetworkByPath(
+        menu_items_[index].wireless_path, &wifi);
+    if (!wifi_exists) {
+      // If we are attempting to connect to a network that no longer exists,
+      // display a notification.
+      // TODO(stevenjb): Show notification.
+    } else if (wifi.name() == cros->wifi_name()) {
       if (cros->wifi_connected()) {
+        // If we are already connected, open the config dialog.
         NetworkConfigView* view = new NetworkConfigView(wifi, false);
         view->set_browser_mode(host_->IsBrowserMode());
         views::Window* window = views::Window::CreateChromeWindow(
             host_->GetNativeWindow(), gfx::Rect(), view);
         window->SetIsAlwaysOnTop(true);
         window->Show();
+      } else {
+        // TODO(stevenjb): Connection in progress. Show dialog?
       }
     } else {
       // If wifi network is not encrypted, then directly connect.
@@ -145,11 +151,17 @@ void NetworkMenuButton::ActivatedAt(int index) {
       }
     }
   } else if (flags & FLAG_CELLULAR) {
-    CellularNetwork cellular = menu_items_[index].cellular_network;
-
-    // If clicked on a network that we are already connected to or we are
-    // currently trying to connect to, then open config dialog.
-    if (cellular.name() == cros->cellular_name()) {
+    CellularNetwork cellular;
+    bool cellular_exists = cros->FindCellularNetworkByPath(
+        menu_items_[index].wireless_path, &cellular);
+
+    if (!cellular_exists) {
+      // If we are attempting to connect to a network that no longer exists,
+      // display a notification.
+      // TODO(stevenjb): Show notification.
+    } else if (cellular.name() == cros->cellular_name()) {
+      // If clicked on a network that we are already connected to or we are
+      // currently trying to connect to, then open config dialog.
       if (cros->cellular_connected()) {
         NetworkConfigView* view = new NetworkConfigView(cellular);
         view->set_browser_mode(host_->IsBrowserMode());
@@ -157,6 +169,8 @@ void NetworkMenuButton::ActivatedAt(int index) {
             host_->GetNativeWindow(), gfx::Rect(), view);
         window->SetIsAlwaysOnTop(true);
         window->Show();
+      } else {
+        // TODO(stevenjb): Connection in progress. Show dialog?
       }
     } else {
       cros->ConnectToCellularNetwork(cellular);
@@ -422,7 +436,7 @@ void NetworkMenuButton::InitMenuItems() {
   int flag = (cros->ethernet_connecting() || cros->ethernet_connected()) ?
       FLAG_ETHERNET | FLAG_ASSOCIATED : FLAG_ETHERNET;
   menu_items_.push_back(MenuItem(menus::MenuModel::TYPE_COMMAND, label,
-      IconForDisplay(icon, badge), WifiNetwork(), CellularNetwork(), flag));
+      IconForDisplay(icon, badge), std::string(), flag));
 
   // Wifi
   const WifiNetworkVector& wifi_networks = cros->wifi_networks();
@@ -435,8 +449,7 @@ void NetworkMenuButton::InitMenuItems() {
     flag = (wifi_networks[i].name() == cros->wifi_name()) ?
         FLAG_WIFI | FLAG_ASSOCIATED : FLAG_WIFI;
     menu_items_.push_back(MenuItem(menus::MenuModel::TYPE_COMMAND, label,
-        IconForDisplay(icon, badge), wifi_networks[i], CellularNetwork(),
-        flag));
+        IconForDisplay(icon, badge), wifi_networks[i].service_path(), flag));
   }
 
   // Cellular
@@ -451,7 +464,7 @@ void NetworkMenuButton::InitMenuItems() {
     flag = (cell_networks[i].name() == cros->cellular_name()) ?
         FLAG_CELLULAR | FLAG_ASSOCIATED : FLAG_CELLULAR;
     menu_items_.push_back(MenuItem(menus::MenuModel::TYPE_COMMAND, label,
-        IconForDisplay(icon, badge), WifiNetwork(), cell_networks[i], flag));
+        IconForDisplay(icon, badge), cell_networks[i].service_path(), flag));
   }
 
   // No networks available message.
@@ -459,7 +472,7 @@ void NetworkMenuButton::InitMenuItems() {
     label = l10n_util::GetStringFUTF16(IDS_STATUSBAR_NETWORK_MENU_ITEM_INDENT,
                 l10n_util::GetStringUTF16(IDS_STATUSBAR_NO_NETWORKS_MESSAGE));
     menu_items_.push_back(MenuItem(menus::MenuModel::TYPE_COMMAND, label,
-        SkBitmap(), WifiNetwork(), CellularNetwork(), FLAG_DISABLED));
+        SkBitmap(), std::string(), FLAG_DISABLED));
   }
 
   // Other networks
@@ -467,7 +480,7 @@ void NetworkMenuButton::InitMenuItems() {
       l10n_util::GetStringUTF16(IDS_OPTIONS_SETTINGS_OTHER_NETWORKS),
       IconForDisplay(*rb.GetBitmapNamed(IDR_STATUSBAR_NETWORK_BARS0),
                      SkBitmap()),
-      WifiNetwork(), CellularNetwork(), FLAG_OTHER_NETWORK));
+      std::string(), FLAG_OTHER_NETWORK));
 
   if (cros->wifi_available() || cros->cellular_available()) {
     // Separator.
@@ -480,7 +493,7 @@ void NetworkMenuButton::InitMenuItems() {
       label = l10n_util::GetStringFUTF16(id,
           l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_DEVICE_WIFI));
       menu_items_.push_back(MenuItem(menus::MenuModel::TYPE_COMMAND, label,
-          SkBitmap(), WifiNetwork(), CellularNetwork(), FLAG_TOGGLE_WIFI));
+          SkBitmap(), std::string(), FLAG_TOGGLE_WIFI));
     }
 
     // Turn Cellular Off. (only if cellular available)
@@ -490,7 +503,7 @@ void NetworkMenuButton::InitMenuItems() {
       label = l10n_util::GetStringFUTF16(id,
           l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_DEVICE_CELLULAR));
       menu_items_.push_back(MenuItem(menus::MenuModel::TYPE_COMMAND, label,
-          SkBitmap(), WifiNetwork(), CellularNetwork(), FLAG_TOGGLE_CELLULAR));
+          SkBitmap(), std::string(), FLAG_TOGGLE_CELLULAR));
     }
   }
 
@@ -499,7 +512,7 @@ void NetworkMenuButton::InitMenuItems() {
 //  menu_items_.push_back(MenuItem(cros->offline_mode() ?
 //      menus::MenuModel::TYPE_CHECK : menus::MenuModel::TYPE_COMMAND,
 //      l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_OFFLINE_MODE),
-//      SkBitmap(), WifiNetwork(), CellularNetwork(), FLAG_TOGGLE_OFFLINE));
+//      SkBitmap(), std::string(), FLAG_TOGGLE_OFFLINE));
 
   if (cros->Connected() || host_->ShouldOpenButtonOptions(this)) {
     // Separator.
@@ -509,7 +522,7 @@ void NetworkMenuButton::InitMenuItems() {
     if (cros->Connected()) {
       menu_items_.push_back(MenuItem(menus::MenuModel::TYPE_COMMAND,
           ASCIIToUTF16(cros->IPAddress()), SkBitmap(),
-          WifiNetwork(), CellularNetwork(), FLAG_DISABLED));
+          std::string(), FLAG_DISABLED));
     }
 
     // Network settings.
@@ -517,7 +530,7 @@ void NetworkMenuButton::InitMenuItems() {
       label =
           l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_OPEN_OPTIONS_DIALOG);
       menu_items_.push_back(MenuItem(menus::MenuModel::TYPE_COMMAND, label,
-          SkBitmap(), WifiNetwork(), CellularNetwork(), FLAG_OPTIONS));
+          SkBitmap(), std::string(), FLAG_OPTIONS));
     }
   }
 }

chrome/browser/chromeos/status/network_menu_button.h

@@ -125,20 +125,17 @@ class NetworkMenuButton : public StatusAreaButton,
         : type(menus::MenuModel::TYPE_SEPARATOR),
           flags(0) {}
     MenuItem(menus::MenuModel::ItemType type, string16 label, SkBitmap icon,
-             WifiNetwork wifi_network, CellularNetwork cellular_network,
-             int flags)
+             const std::string& wireless_path, int flags)
         : type(type),
           label(label),
           icon(icon),
-          wifi_network(wifi_network),
-          cellular_network(cellular_network),
+          wireless_path(wireless_path),
           flags(flags) {}
 
     menus::MenuModel::ItemType type;
     string16 label;
     SkBitmap icon;
-    WifiNetwork wifi_network;
-    CellularNetwork cellular_network;
+    std::string wireless_path;
     int flags;
   };
   typedef std::vector<MenuItem> MenuItemVector;