Better support for failed network connection attempts (chromiumos:8594).

This includes some NetworkMenu refactoring to reduce code duplication.

BUG=http://code.google.com/p/chromium-os/issues/detail?id=8594
TEST=Test network menu, particularly failed connection attempts.

Review URL: http://codereview.chromium.org/4598002

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

5 files changed, 114 insertions, 70 deletions

diff --git a/chrome/browser/chromeos/cros/mock_network_library.h b/chrome/browser/chromeos/cros/mock_network_library.h
index ef72702..796f735 100644
--- a/chrome/browser/chromeos/cros/mock_network_library.h
+++ b/chrome/browser/chromeos/cros/mock_network_library.h
@@ -50,17 +50,17 @@ class MockNetworkLibrary : public NetworkLibrary {
   MOCK_METHOD0(RequestWifiScan, void(void));
   MOCK_METHOD0(UpdateSystemInfo, void(void));
   MOCK_METHOD1(GetWifiAccessPoints, bool(WifiAccessPointVector*));
-  MOCK_METHOD4(ConnectToWifiNetwork, void(const WifiNetwork*,
+  MOCK_METHOD4(ConnectToWifiNetwork, bool(const WifiNetwork*,
                                           const std::string&,
                                           const std::string&,
                                           const std::string&));
-  MOCK_METHOD6(ConnectToWifiNetwork, void(ConnectionSecurity security,
+  MOCK_METHOD6(ConnectToWifiNetwork, bool(ConnectionSecurity security,
                                           const std::string&,
                                           const std::string&,
                                           const std::string&,
                                           const std::string&,
                                           bool));
-  MOCK_METHOD1(ConnectToCellularNetwork, void(const CellularNetwork*));
+  MOCK_METHOD1(ConnectToCellularNetwork, bool(const CellularNetwork*));
   MOCK_METHOD1(RefreshCellularDataPlans, void(const CellularNetwork* network));
 
   MOCK_METHOD1(DisconnectFromWirelessNetwork, void(const WirelessNetwork*));
diff --git a/chrome/browser/chromeos/cros/network_library.cc b/chrome/browser/chromeos/cros/network_library.cc
index a12d537..fc7cec6 100644
--- a/chrome/browser/chromeos/cros/network_library.cc
+++ b/chrome/browser/chromeos/cros/network_library.cc
@@ -570,6 +570,7 @@ WifiNetwork::WifiNetwork(const WifiNetwork& network)
     : WirelessNetwork(network) {
   encryption_ = network.encryption();
   passphrase_ = network.passphrase();
+  passphrase_required_ = network.passphrase_required();
   identity_ = network.identity();
   cert_path_ = network.cert_path();
 }
@@ -578,6 +579,13 @@ WifiNetwork::WifiNetwork(const ServiceInfo* service)
     : WirelessNetwork(service) {
   encryption_ = service->security;
   passphrase_ = SafeString(service->passphrase);
+  // TODO(stevenjb): Remove this once flimflam is setting passphrase_required
+  // correctly: http://crosbug.com/8830.
+  if (service->state == chromeos::STATE_FAILURE &&
+      service->security != chromeos::SECURITY_NONE)
+    passphrase_required_ = true;
+  else
+    passphrase_required_ = service->passphrase_required;
   identity_ = SafeString(service->identity);
   cert_path_ = SafeString(service->cert_path);
   type_ = TYPE_WIFI;
@@ -833,13 +841,13 @@ class NetworkLibraryImpl : public NetworkLibrary  {
     return true;
   }
 
-  virtual void ConnectToWifiNetwork(const WifiNetwork* network,
+  virtual bool ConnectToWifiNetwork(const WifiNetwork* network,
                                     const std::string& password,
                                     const std::string& identity,
                                     const std::string& certpath) {
     DCHECK(network);
     if (!EnsureCrosLoaded())
-      return;
+      return true;  // No library loaded, don't trigger a retry attempt.
     // TODO(ers) make wifi the highest priority service type
     if (ConnectToNetworkWithCertInfo(network->service_path().c_str(),
         password.empty() ? NULL : password.c_str(),
@@ -856,18 +864,20 @@ class NetworkLibraryImpl : public NetworkLibrary  {
         wifi_ = wifi;
       }
       NotifyNetworkManagerChanged();
+      return true;
+    } else {
+      return false;  // Immediate failure.
     }
   }
 
-  virtual void ConnectToWifiNetwork(ConnectionSecurity security,
+  virtual bool ConnectToWifiNetwork(ConnectionSecurity security,
                                     const std::string& ssid,
                                     const std::string& password,
                                     const std::string& identity,
                                     const std::string& certpath,
                                     bool auto_connect) {
     if (!EnsureCrosLoaded())
-      return;
-
+      return true;  // No library loaded, don't trigger a retry attempt.
     // First create a service from hidden network.
     ServiceInfo* service = GetWifiService(ssid.c_str(), security);
     if (service) {
@@ -875,23 +885,26 @@ class NetworkLibraryImpl : public NetworkLibrary  {
       SetAutoConnect(service->service_path, auto_connect);
       // Now connect to that service.
       // TODO(ers) make wifi the highest priority service type
-      ConnectToNetworkWithCertInfo(service->service_path,
+      bool res = ConnectToNetworkWithCertInfo(
+          service->service_path,
           password.empty() ? NULL : password.c_str(),
           identity.empty() ? NULL : identity.c_str(),
           certpath.empty() ? NULL : certpath.c_str());
 
       // Clean up ServiceInfo object.
       FreeServiceInfo(service);
+      return res;
     } else {
       LOG(WARNING) << "Cannot find hidden network: " << ssid;
       // TODO(chocobo): Show error message.
+      return false;  // Immediate failure.
     }
   }
 
-  virtual void ConnectToCellularNetwork(const CellularNetwork* network) {
+  virtual bool ConnectToCellularNetwork(const CellularNetwork* network) {
     DCHECK(network);
     if (!EnsureCrosLoaded())
-      return;
+      return true;  // No library loaded, don't trigger a retry attempt.
     // TODO(ers) make cellular the highest priority service type
     if (network && ConnectToNetwork(network->service_path().c_str(), NULL)) {
       // Update local cache and notify listeners.
@@ -902,6 +915,9 @@ class NetworkLibraryImpl : public NetworkLibrary  {
         cellular_ = cellular;
       }
       NotifyNetworkManagerChanged();
+      return true;
+    } else {
+      return false;  // Immediate failure.
     }
   }
 
@@ -1622,17 +1638,23 @@ class NetworkLibraryStubImpl : public NetworkLibrary {
     return false;
   }
 
-  virtual void ConnectToWifiNetwork(const WifiNetwork* network,
+  virtual bool ConnectToWifiNetwork(const WifiNetwork* network,
                                     const std::string& password,
                                     const std::string& identity,
-                                    const std::string& certpath) {}
-  virtual void ConnectToWifiNetwork(ConnectionSecurity security,
+                                    const std::string& certpath) {
+    return true;
+  }
+  virtual bool ConnectToWifiNetwork(ConnectionSecurity security,
                                     const std::string& ssid,
                                     const std::string& password,
                                     const std::string& identity,
                                     const std::string& certpath,
-                                    bool auto_connect) {}
-  virtual void ConnectToCellularNetwork(const CellularNetwork* network) {}
+                                    bool auto_connect) {
+    return true;
+  }
+  virtual bool ConnectToCellularNetwork(const CellularNetwork* network) {
+    return true;
+  }
   virtual void RefreshCellularDataPlans(const CellularNetwork* network) {}
   virtual void DisconnectFromWirelessNetwork(const WirelessNetwork* network) {}
   virtual void SaveCellularNetwork(const CellularNetwork* network) {}
diff --git a/chrome/browser/chromeos/cros/network_library.h b/chrome/browser/chromeos/cros/network_library.h
index 80f1f94..de81636 100644
--- a/chrome/browser/chromeos/cros/network_library.h
+++ b/chrome/browser/chromeos/cros/network_library.h
@@ -304,6 +304,7 @@ class WifiNetwork : public WirelessNetwork {
   bool encrypted() const { return encryption_ != SECURITY_NONE; }
   ConnectionSecurity encryption() const { return encryption_; }
   const std::string& passphrase() const { return passphrase_; }
+  bool passphrase_required() const { return passphrase_required_; }
   const std::string& identity() const { return identity_; }
   const std::string& cert_path() const { return cert_path_; }
 
@@ -333,6 +334,7 @@ class WifiNetwork : public WirelessNetwork {
  protected:
   ConnectionSecurity encryption_;
   std::string passphrase_;
+  bool passphrase_required_;
   std::string identity_;
   std::string cert_path_;
 };
@@ -497,13 +499,15 @@ class NetworkLibrary {
   virtual void UpdateSystemInfo() = 0;
 
   // Connect to the specified wireless network with password.
-  virtual void ConnectToWifiNetwork(const WifiNetwork* network,
+  // Returns false if the attempt fails immediately (e.g. passphrase too short).
+  virtual bool ConnectToWifiNetwork(const WifiNetwork* network,
                                     const std::string& password,
                                     const std::string& identity,
                                     const std::string& certpath) = 0;
 
   // Connect to the specified network with security, ssid, and password.
-  virtual void ConnectToWifiNetwork(ConnectionSecurity security,
+  // Returns false if the attempt fails immediately (e.g. passphrase too short).
+  virtual bool ConnectToWifiNetwork(ConnectionSecurity security,
                                     const std::string& ssid,
                                     const std::string& password,
                                     const std::string& identity,
@@ -511,7 +515,8 @@ class NetworkLibrary {
                                     bool auto_connect) = 0;
 
   // Connect to the specified cellular network.
-  virtual void ConnectToCellularNetwork(const CellularNetwork* network) = 0;
+  // Returns false if the attempt fails immediately.
+  virtual bool ConnectToCellularNetwork(const CellularNetwork* network) = 0;
 
   // Initiates cellular data plan refresh. Plan data will be passed through
   // Network::Observer::CellularDataPlanChanged callback.
diff --git a/chrome/browser/chromeos/options/wifi_config_view.cc b/chrome/browser/chromeos/options/wifi_config_view.cc
index b34e82b..7cdad5b 100644
--- a/chrome/browser/chromeos/options/wifi_config_view.cc
+++ b/chrome/browser/chromeos/options/wifi_config_view.cc
@@ -191,6 +191,7 @@ bool WifiConfigView::Login() {
   if (identity_textfield_ != NULL) {
     identity_string = UTF16ToUTF8(identity_textfield_->text());
   }
+  bool connected = false;
   if (other_network_) {
     ConnectionSecurity sec = SECURITY_UNKNOWN;
     int index = security_combobox_->selected_item();
@@ -202,17 +203,17 @@ bool WifiConfigView::Login() {
       sec = SECURITY_WPA;
     else if (index == INDEX_RSN)
       sec = SECURITY_RSN;
-    CrosLibrary::Get()->GetNetworkLibrary()->ConnectToWifiNetwork(
+    connected =  CrosLibrary::Get()->GetNetworkLibrary()->ConnectToWifiNetwork(
         sec, GetSSID(), GetPassphrase(),
         identity_string, certificate_path_,
         autoconnect_checkbox_ ? autoconnect_checkbox_->checked() : true);
   } else {
     Save();
-    CrosLibrary::Get()->GetNetworkLibrary()->ConnectToWifiNetwork(
+    connected = CrosLibrary::Get()->GetNetworkLibrary()->ConnectToWifiNetwork(
         wifi_.get(), GetPassphrase(),
         identity_string, certificate_path_);
   }
-  return true;
+  return connected;
 }
 
 bool WifiConfigView::Save() {
diff --git a/chrome/browser/chromeos/status/network_menu.cc b/chrome/browser/chromeos/status/network_menu.cc
index e6aaa36..9ca372f 100644
--- a/chrome/browser/chromeos/status/network_menu.cc
+++ b/chrome/browser/chromeos/status/network_menu.cc
@@ -210,39 +210,82 @@ bool NetworkMenu::ConnectToNetworkAt(int index,
       // Connect or reconnect.
       if (remember >= 0)
         wifi->set_favorite(remember ? true : false);
+      if (cros->wifi_network() &&
+          wifi->service_path() == cros->wifi_network()->service_path()) {
+        // Show the config settings for the active network.
+        ShowWifi(wifi, false);
+        return true;
+      }
+      bool connected = false;
       if (wifi->encrypted()) {
         if (wifi->IsCertificateLoaded()) {
-          cros->ConnectToWifiNetwork(wifi, std::string(), std::string(),
-                                     wifi->cert_path());
+          connected = cros->ConnectToWifiNetwork(
+              wifi, std::string(), std::string(), wifi->cert_path());
         } else if (wifi->encryption() == SECURITY_8021X) {
-          // Show the wifi settings/dialog to load/select a certificate.
+          // Always show the wifi settings/dialog to load/select a certificate.
           ShowWifi(wifi, true);
+          return true;
         } else {
-          if (MenuUI::IsEnabled()) {
-            cros->ConnectToWifiNetwork(wifi, passphrase, std::string(),
-                                       std::string());
-          } else {
-            ShowWifi(wifi, true);
+          if (MenuUI::IsEnabled() || !wifi->passphrase_required()) {
+            connected = cros->ConnectToWifiNetwork(
+                wifi, passphrase, std::string(), std::string());
           }
         }
       } else {
-        cros->ConnectToWifiNetwork(wifi, std::string(), std::string(),
-                                   std::string());
+        connected = cros->ConnectToWifiNetwork(
+            wifi, std::string(), std::string(), std::string());
+      }
+      if (!connected) {
+        if (!MenuUI::IsEnabled()) {
+          // Show the wifi dialog on a failed attempt for non DOM UI menus.
+          ShowWifi(wifi, true);
+          return true;
+        } else {
+          // If the connection attempt failed immediately (e.g. short password)
+          // keep the menu open so that a retry can be attempted.
+          return false;
+        }
       }
+    } else {
+      // If we are attempting to connect to a network that no longer exists,
+      // display a notification.
+      // TODO(stevenjb): Show notification.
     }
   } else if (flags & FLAG_CELLULAR) {
     CellularNetwork* cellular = cros->FindCellularNetworkByPath(
         menu_items_[index].wireless_path);
     if (cellular) {
-      // TODO(zelidrag): Start activation process if needed.
-      // Connect or reconnect.
-      cros->ConnectToCellularNetwork(cellular);
+      if (cellular->activation_state() != ACTIVATION_STATE_ACTIVATED) {
+        ActivateCellular(cellular);
+        return true;
+      } else if (cros->cellular_network() &&
+                 (cellular->service_path() ==
+                  cros->cellular_network()->service_path())) {
+        // Show the config settings for the cellular network.
+        ShowCellular(cellular, false);
+        return true;
+      } else {
+        bool connected = cros->ConnectToCellularNetwork(cellular);
+        if (!connected) {
+          ShowCellular(cellular, true);
+        }
+      }
+    } else {
+      // If we are attempting to connect to a network that no longer exists,
+      // display a notification.
+      // TODO(stevenjb): Show notification.
     }
   } else if (flags & FLAG_OTHER_NETWORK) {
-    bool favorite = remember == 0 ? false : true;  // default is true
-    cros->ConnectToWifiNetwork(
-        passphrase.empty() ? SECURITY_NONE : SECURITY_UNKNOWN,
-        ssid, passphrase, std::string(), std::string(), favorite);
+    bool connected = false;
+    if (MenuUI::IsEnabled()) {
+      bool favorite = remember == 0 ? false : true;  // default is true
+      connected = cros->ConnectToWifiNetwork(
+          passphrase.empty() ? SECURITY_NONE : SECURITY_UNKNOWN,
+          ssid, passphrase, std::string(), std::string(), favorite);
+    }
+    if (!connected) {
+      ShowOther();
+    }
   }
   return true;
 }
@@ -302,43 +345,16 @@ void NetworkMenu::ActivatedAt(int index) {
     cros->EnableCellularNetworkDevice(!cros->cellular_enabled());
   } else if (flags & FLAG_TOGGLE_OFFLINE) {
     cros->EnableOfflineMode(!cros->offline_mode());
-  } else if (flags & FLAG_OTHER_NETWORK) {
-    ShowOther();
   } else if (flags & FLAG_ETHERNET) {
     if (cros->ethernet_connected()) {
       ShowEthernet(cros->ethernet_network());
     }
   } else if (flags & FLAG_WIFI) {
-    WifiNetwork* wifi = cros->FindWifiNetworkByPath(
-        menu_items_[index].wireless_path);
-    if (!wifi) {
-      // If we are attempting to connect to a network that no longer exists,
-      // display a notification.
-      // TODO(stevenjb): Show notification.
-    } else if (cros->wifi_network() &&
-               wifi->service_path() == cros->wifi_network()->service_path()) {
-      // Show the config settings for the active network.
-      ShowWifi(wifi, false);
-    } else {
-      ConnectToNetworkAt(index, std::string(), std::string(), -1);
-    }
+    ConnectToNetworkAt(index, std::string(), std::string(), -1);
+  } else if (flags & FLAG_OTHER_NETWORK) {
+    ConnectToNetworkAt(index, std::string(), std::string(), -1);
   } else if (flags & FLAG_CELLULAR) {
-    CellularNetwork* cellular = cros->FindCellularNetworkByPath(
-        menu_items_[index].wireless_path);
-    if (!cellular) {
-      // If we are attempting to connect to a network that no longer exists,
-      // display a notification.
-      // TODO(stevenjb): Show notification.
-    } else if (cellular->activation_state() != ACTIVATION_STATE_ACTIVATED) {
-      ActivateCellular(cellular);
-    } else if (cros->cellular_network() &&
-               (cellular->service_path() ==
-                cros->cellular_network()->service_path())) {
-      // Show the config settings for the cellular network.
-      ShowCellular(cellular, false);
-    } else {
-      ConnectToNetworkAt(index, std::string(), std::string(), -1);
-    }
+    ConnectToNetworkAt(index, std::string(), std::string(), -1);
   }
 }