Add error message for immediate failure on short passwords.

BUG=chromium-os:9100
TEST=See issue. Test repeatedly connecting to a variety of secure and 'other' wifi networks.

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

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

6 files changed, 64 insertions, 33 deletions

diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
index a0db961..9be6fb7 100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -9360,6 +9360,9 @@ Keep your key file in a safe place. You will need it to create new versions of y
       <message name="IDS_OPTIONS_SETTINGS_INTERNET_OPTIONS_MORE_INFO" desc="In settings Internet options, the title for button for getting more information about the current cellular data plan.">
         More info...
       </message>
+      <message name="IDS_OPTIONS_SETTINGS_INTERNET_OPTIONS_BAD_CREDENTIALS" desc="In settings Internet options, the label for invalid passphrase or identity input.">
+        Password is too short or invalid.
+      </message>
       <message name="IDS_OPTIONS_SETTINGS_INTERNET_CELL_PLAN_NAME" desc="In settings Internet options, the label displayed next to cellular plan name.">
         Plan name:
       </message>
diff --git a/chrome/browser/chromeos/cros/mock_network_library.h b/chrome/browser/chromeos/cros/mock_network_library.h
index 7f8bdcb..cfa161f 100644
--- a/chrome/browser/chromeos/cros/mock_network_library.h
+++ b/chrome/browser/chromeos/cros/mock_network_library.h
@@ -49,7 +49,7 @@ class MockNetworkLibrary : public NetworkLibrary {
 
   MOCK_METHOD0(RequestWifiScan, void(void));
   MOCK_METHOD1(GetWifiAccessPoints, bool(WifiAccessPointVector*));
-  MOCK_METHOD4(ConnectToWifiNetwork, bool(const WifiNetwork*,
+  MOCK_METHOD4(ConnectToWifiNetwork, bool(WifiNetwork*,
                                           const std::string&,
                                           const std::string&,
                                           const std::string&));
diff --git a/chrome/browser/chromeos/cros/network_library.cc b/chrome/browser/chromeos/cros/network_library.cc
index 13fa2a8..ca56b63 100644
--- a/chrome/browser/chromeos/cros/network_library.cc
+++ b/chrome/browser/chromeos/cros/network_library.cc
@@ -1036,7 +1036,7 @@ class NetworkLibraryImpl : public NetworkLibrary  {
     return true;
   }
 
-  virtual bool ConnectToWifiNetwork(const WifiNetwork* network,
+  virtual bool ConnectToWifiNetwork(WifiNetwork* network,
                                     const std::string& password,
                                     const std::string& identity,
                                     const std::string& certpath) {
@@ -1061,6 +1061,10 @@ class NetworkLibraryImpl : public NetworkLibrary  {
       NotifyNetworkManagerChanged();
       return true;
     } else {
+      // The only likely cause for an immediate failure is a badly formatted
+      // passphrase. TODO(stevenjb): get error information from libcros
+      // and call set_error correctly. crosbug.com/9538.
+      network->set_error(ERROR_BAD_PASSPHRASE);
       return false;  // Immediate failure.
     }
   }
@@ -1926,7 +1930,7 @@ class NetworkLibraryStubImpl : public NetworkLibrary {
     return false;
   }
 
-  virtual bool ConnectToWifiNetwork(const WifiNetwork* network,
+  virtual bool ConnectToWifiNetwork(WifiNetwork* network,
                                     const std::string& password,
                                     const std::string& identity,
                                     const std::string& certpath) {
diff --git a/chrome/browser/chromeos/cros/network_library.h b/chrome/browser/chromeos/cros/network_library.h
index 1b7e9af..41868f2 100644
--- a/chrome/browser/chromeos/cros/network_library.h
+++ b/chrome/browser/chromeos/cros/network_library.h
@@ -98,6 +98,7 @@ class Network {
   void set_state(ConnectionState state) { state_ = state; }
   void set_connectable(bool connectable) { connectable_ = connectable; }
   void set_active(bool is_active) { is_active_ = is_active; }
+  void set_error(ConnectionError error) { error_ = error; }
 
   // Initialize the IP address field
   void InitIPAddress();
@@ -552,8 +553,9 @@ class NetworkLibrary {
   // TODO(joth): Add GetCellTowers to retrieve a CellTowerVector.
 
   // Connect to the specified wireless network with password.
-  // Returns false if the attempt fails immediately (e.g. passphrase too short).
-  virtual bool ConnectToWifiNetwork(const WifiNetwork* network,
+  // Returns false if the attempt fails immediately (e.g. passphrase too short)
+  // and sets network->error().
+  virtual bool ConnectToWifiNetwork(WifiNetwork* network,
                                     const std::string& password,
                                     const std::string& identity,
                                     const std::string& certpath) = 0;
diff --git a/chrome/browser/chromeos/options/wifi_config_view.cc b/chrome/browser/chromeos/options/wifi_config_view.cc
index c02315b..e920109 100644
--- a/chrome/browser/chromeos/options/wifi_config_view.cc
+++ b/chrome/browser/chromeos/options/wifi_config_view.cc
@@ -69,7 +69,8 @@ WifiConfigView::WifiConfigView(NetworkConfigView* parent,
       security_combobox_(NULL),
       passphrase_textfield_(NULL),
       passphrase_visible_button_(NULL),
-      autoconnect_checkbox_(NULL) {
+      autoconnect_checkbox_(NULL),
+      error_label_(NULL) {
   Init();
 }
 
@@ -84,7 +85,8 @@ WifiConfigView::WifiConfigView(NetworkConfigView* parent)
       security_combobox_(NULL),
       passphrase_textfield_(NULL),
       passphrase_visible_button_(NULL),
-      autoconnect_checkbox_(NULL) {
+      autoconnect_checkbox_(NULL),
+      error_label_(NULL) {
   Init();
 }
 
@@ -132,6 +134,29 @@ void WifiConfigView::UpdateCanViewPassword() {
   }
 }
 
+void WifiConfigView::UpdateErrorLabel(bool failed) {
+  static const int kNoError = -1;
+  int id = kNoError;
+  if (wifi_.get()) {
+    // Right now, only displaying bad_passphrase and bad_wepkey errors.
+    if (wifi_->error() == ERROR_BAD_PASSPHRASE)
+      id = IDS_OPTIONS_SETTINGS_INTERNET_OPTIONS_BAD_PASSPHRASE;
+    else if (wifi_->error() == ERROR_BAD_WEPKEY)
+      id = IDS_OPTIONS_SETTINGS_INTERNET_OPTIONS_BAD_WEPKEY;
+  }
+  if (id == kNoError && failed) {
+    // We don't know what the error was. For now assume bad identity or
+    // passphrase. See TODO comment in Login() and crosbug.com/9538.
+    id = IDS_OPTIONS_SETTINGS_INTERNET_OPTIONS_BAD_CREDENTIALS;
+  }
+  if (id != kNoError) {
+    error_label_->SetText(l10n_util::GetString(id));
+    error_label_->SetVisible(true);
+  } else {
+    error_label_->SetVisible(false);
+  }
+}
+
 void WifiConfigView::ContentsChanged(views::Textfield* sender,
                                      const string16& new_contents) {
   UpdateCanLogin();
@@ -211,22 +236,21 @@ bool WifiConfigView::Login() {
         sec, GetSSID(), GetPassphrase(),
         identity_string, certificate_path_,
         autoconnect_checkbox_ ? autoconnect_checkbox_->checked() : true);
-    // TODO(stevenjb): Modify libcros to set an error code and return 'false'
-    // only on an invalid password or other UI failure.
   } else {
     Save();
     connected = cros->ConnectToWifiNetwork(
         wifi_.get(), GetPassphrase(),
         identity_string, certificate_path_);
-    if (!connected) {
-      // Assume this failed due to an invalid password.
-      // TODO(stevenjb): Modify libcros to set an error code and return 'false'
-      // only on an invalid password or other recoverable failure.
-      wifi_->set_passphrase(std::string());
-      cros->SaveWifiNetwork(wifi_.get());
-    }
   }
-  return connected;
+  if (!connected) {
+    // Assume this failed due to an invalid password.
+    // TODO(stevenjb): Modify libcros to set an error code. Return 'false'
+    // only on invalid password or other recoverable failure. crosbug.com/9538.
+    // Update any error message and return false (keep dialog open).
+    UpdateErrorLabel(true);
+    return false;
+  }
+  return true;  // dialog will be closed
 }
 
 bool WifiConfigView::Save() {
@@ -417,22 +441,16 @@ void WifiConfigView::Init() {
     layout->AddPaddingRow(0, kRelatedControlVerticalSpacing);
   }
 
-  // Error label
-  // If there's an error, add an error message label.
-  // Right now, only displaying bad_passphrase and bad_wepkey errors.
-  if (wifi_.get() && (wifi_->error() == ERROR_BAD_PASSPHRASE ||
-      wifi_->error() == ERROR_BAD_WEPKEY)) {
-    layout->StartRow(0, column_view_set_id);
-    layout->SkipColumns(1);
-    int id = IDS_OPTIONS_SETTINGS_INTERNET_OPTIONS_BAD_PASSPHRASE;
-    if (wifi_->error() == ERROR_BAD_WEPKEY)
-      id = IDS_OPTIONS_SETTINGS_INTERNET_OPTIONS_BAD_WEPKEY;
-    views::Label* label_error = new views::Label(l10n_util::GetString(id));
-    label_error->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
-    label_error->SetColor(SK_ColorRED);
-    layout->AddView(label_error);
-    layout->AddPaddingRow(0, kRelatedControlVerticalSpacing);
-  }
+  // Create an error label.
+  layout->StartRow(0, column_view_set_id);
+  layout->SkipColumns(1);
+  error_label_ = new views::Label();
+  error_label_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
+  error_label_->SetColor(SK_ColorRED);
+  layout->AddView(error_label_);
+  layout->AddPaddingRow(0, kRelatedControlVerticalSpacing);
+  // Set or hide the error text.
+  UpdateErrorLabel(false);
 
   // Autoconnect checkbox
   // Only show if this network is already remembered (a favorite).
diff --git a/chrome/browser/chromeos/options/wifi_config_view.h b/chrome/browser/chromeos/options/wifi_config_view.h
index c8beb38..120fd27 100644
--- a/chrome/browser/chromeos/options/wifi_config_view.h
+++ b/chrome/browser/chromeos/options/wifi_config_view.h
@@ -98,6 +98,9 @@ class WifiConfigView : public views::View,
   // Updates state of the "view password" button.
   void UpdateCanViewPassword();
 
+  // Updates the error text label.
+  void UpdateErrorLabel(bool failed);
+
   NetworkConfigView* parent_;
 
   bool other_network_;
@@ -117,6 +120,7 @@ class WifiConfigView : public views::View,
   views::Textfield* passphrase_textfield_;
   views::ImageButton* passphrase_visible_button_;
   views::Checkbox* autoconnect_checkbox_;
+  views::Label* error_label_;
 
   DISALLOW_COPY_AND_ASSIGN(WifiConfigView);
 };