Differentiate between 'connect-failed' and 'bad-passphrase'

chromeos/network/network_connection_handler.cc

Index: chromeos/network/network_connection_handler.cc
diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc
index f27ff4f8a1a25e74cc6bb8cbc03744a3d91d7d5f..35ba30a820497436d6cc455800d7dc8a0e9fc682 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -118,16 +118,10 @@ const char NetworkConnectionHandler::kErrorConfigurationRequired[] =
 const char NetworkConnectionHandler::kErrorAuthenticationRequired[] =
     "authentication-required";
 const char NetworkConnectionHandler::kErrorShillError[] = "shill-error";
-const char NetworkConnectionHandler::kErrorConnectFailed[] = "connect-failed";
 const char NetworkConnectionHandler::kErrorConfigureFailed[] =
     "configure-failed";
-const char NetworkConnectionHandler::kErrorActivateFailed[] =
-    "activate-failed";
-const char NetworkConnectionHandler::kErrorMissingProvider[] =
-    "missing-provider";
 const char NetworkConnectionHandler::kErrorConnectCanceled[] =
     "connect-canceled";
-const char NetworkConnectionHandler::kErrorUnknown[] = "unknown-error";
 
 struct NetworkConnectionHandler::ConnectRequest {
   ConnectRequest(const std::string& service_path,
@@ -254,14 +248,8 @@ void NetworkConnectionHandler::ConnectToNetwork(
 
     if (check_error_state) {
       const std::string& error = network->error();
-      if (error == flimflam::kErrorConnectFailed) {

[pneubeck (no reviews), 2013/08/26 08:14:11]

for non-VPN this looks fine, as it's handled later in
VerifyConfiguredAndConnect.

But I'm a bit concerned that we silently ignore a connect-failed error in case
of VPN. See my comment in VerifyConfiguredAndConnect.

[stevenjb, 2013/08/27 15:52:08]

The purpose of this code was to try to guess the user's intent:
* If we know that the network is misconfigured (i.e. Error == bad-passphrase),
then we should show the configure UI instead of attempting to connect.
* Sometimes, when Error == connect-failed, it was because of a configuration
error, in which case we would like to show the configure UI. However, unless we
know that the failure was due to a configuration error, showing the configure UI
will be confusing (e.g for WiMax).
* If we try to connect and it fails (again), we will show the configure UI
anyway, it just may take a little longer.

[pneubeck (no reviews), 2013/08/28 10:02:35]

My bad.. I should have placed this comment to the new code and not the old... I
meant: Removing this looks fine for non-VPN.

On 2013/08/27 15:52:08, stevenjb (chromium) wrote:

-        InvokeErrorCallback(
-            service_path, error_callback, kErrorPassphraseRequired);
-        return;
-      }
       if (error == flimflam::kErrorBadPassphrase) {
-        InvokeErrorCallback(
-            service_path, error_callback, kErrorPassphraseRequired);
+        InvokeErrorCallback(service_path, error_callback, error);
         return;
       }
       if (IsAuthenticationError(error)) {
@@ -405,7 +393,7 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
           flimflam::kHostProperty, &vpn_provider_host);
     }
     if (vpn_provider_type.empty() || vpn_provider_host.empty()) {
-      ErrorCallbackForPendingRequest(service_path, kErrorMissingProvider);
+      ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
       return;
     }
     // VPN requires a host and username to be set.
@@ -578,7 +566,7 @@ void NetworkConnectionHandler::HandleShillConnectFailure(
   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
   network_handler::ShillErrorCallbackFunction(
-      kErrorConnectFailed, service_path, error_callback,
+      flimflam::kErrorConnectFailed, service_path, error_callback,
       dbus_error_name, dbus_error_message);
 }
 
@@ -618,7 +606,7 @@ void NetworkConnectionHandler::CheckPendingRequest(
     error_name = kErrorConnectCanceled;
     error_detail = "";
   } else {
-    error_name = kErrorConnectFailed;
+    error_name = flimflam::kErrorConnectFailed;
     error_detail = network->error();
     if (error_detail.empty()) {
       if (network->connection_state() == flimflam::kStateFailure)