Handle NetworkConnectionHandler errors in NetworkStateListDetailedView

Explicitly handle different error types in NetworkStateListDetailedView
with NetworkConnectionHandler enabled.

Modifies the SystemTrayDelegate interface for new Handler code in
preparation of removing old code.

BUG=235243
R=gspencer@chromium.org

Review URL: https://codereview.chromium.org/14762016

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

8 files changed, 96 insertions, 48 deletions

diff --git a/chromeos/dbus/shill_service_client_stub.cc b/chromeos/dbus/shill_service_client_stub.cc
index 434c4ce..af4245c 100644
--- a/chromeos/dbus/shill_service_client_stub.cc
+++ b/chromeos/dbus/shill_service_client_stub.cc
@@ -355,53 +355,53 @@ void ShillServiceClientStub::SetDefaultProperties() {
 
   if (!CommandLine::ForCurrentProcess()->HasSwitch(
           chromeos::switches::kDisableStubEthernet)) {
-    AddService("stub_ethernet", "eth0",
+    AddService("eth1", "eth1",
                flimflam::kTypeEthernet,
                flimflam::kStateOnline,
                add_to_watchlist);
   }
 
-  AddService("stub_wifi1", "wifi1",
+  AddService("wifi1", "wifi1",
              flimflam::kTypeWifi,
              flimflam::kStateOnline,
              add_to_watchlist);
-  SetServiceProperty("stub_wifi1",
+  SetServiceProperty("wifi1",
                      flimflam::kSecurityProperty,
                      base::StringValue(flimflam::kSecurityWep));
 
-  AddService("stub_wifi2", "wifi2_PSK",
+  AddService("wifi2", "wifi2_PSK",
              flimflam::kTypeWifi,
              flimflam::kStateIdle,
              add_to_watchlist);
-  SetServiceProperty("stub_wifi2",
+  SetServiceProperty("wifi2",
                      flimflam::kSecurityProperty,
                      base::StringValue(flimflam::kSecurityPsk));
   base::FundamentalValue strength_value(80);
-  SetServiceProperty("stub_wifi2",
+  SetServiceProperty("wifi2",
                      flimflam::kSignalStrengthProperty,
                      strength_value);
 
-  AddService("stub_cellular1", "cellular1",
+  AddService("cellular1", "cellular1",
              flimflam::kTypeCellular,
              flimflam::kStateIdle,
              add_to_watchlist);
   base::StringValue technology_value(flimflam::kNetworkTechnologyGsm);
-  SetServiceProperty("stub_cellular1",
+  SetServiceProperty("cellular1",
                      flimflam::kNetworkTechnologyProperty,
                      technology_value);
-  SetServiceProperty("stub_cellular1",
+  SetServiceProperty("cellular1",
                      flimflam::kActivationStateProperty,
                      base::StringValue(flimflam::kActivationStateNotActivated));
-  SetServiceProperty("stub_cellular1",
+  SetServiceProperty("cellular1",
                      flimflam::kRoamingStateProperty,
                      base::StringValue(flimflam::kRoamingStateHome));
 
-  AddService("stub_vpn1", "vpn1",
+  AddService("vpn1", "vpn1",
              flimflam::kTypeVPN,
              flimflam::kStateOnline,
              add_to_watchlist);
 
-  AddService("stub_vpn2", "vpn2",
+  AddService("vpn2", "vpn2",
              flimflam::kTypeVPN,
              flimflam::kStateOffline,
              add_to_watchlist);
diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc
index f8035a5..4b7bdff 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -5,7 +5,9 @@
 #include "chromeos/network/network_connection_handler.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/json/json_reader.h"
+#include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_service_client.h"
@@ -26,12 +28,16 @@ namespace chromeos {
 
 namespace {
 
-void InvokeErrorCallback(
-    const std::string& service_path,
-    const network_handler::ErrorCallback& error_callback,
-    const std::string& error_name) {
-  network_handler::ShillErrorCallbackFunction(
-      service_path, error_callback, error_name, "Connect Error");
+void InvokeErrorCallback(const std::string& service_path,
+                         const network_handler::ErrorCallback& error_callback,
+                         const std::string& error_name) {
+  std::string error_msg = "Connect Error: " + error_name;
+  NET_LOG_ERROR(error_msg, service_path);
+  if (error_callback.is_null())
+    return;
+  scoped_ptr<base::DictionaryValue> error_data(
+      network_handler::CreateErrorData(service_path, error_name, error_msg));
+  error_callback.Run(error_name, error_data.Pass());
 }
 
 bool NetworkMayNeedCredentials(const NetworkState* network) {
@@ -142,6 +148,11 @@ NetworkConnectionHandler* NetworkConnectionHandler::Get() {
 }
 
 NetworkConnectionHandler::NetworkConnectionHandler() {
+  const char* new_handlers_enabled =
+      CommandLine::ForCurrentProcess()->HasSwitch(
+          chromeos::switches::kUseNewNetworkConfigurationHandlers) ?
+      "enabled" : "disabled";
+  NET_LOG_EVENT("NewNetworkConfigurationHandlers", new_handlers_enabled);
 }
 
 NetworkConnectionHandler::~NetworkConnectionHandler() {
@@ -150,7 +161,8 @@ NetworkConnectionHandler::~NetworkConnectionHandler() {
 void NetworkConnectionHandler::ConnectToNetwork(
     const std::string& service_path,
     const base::Closure& success_callback,
-    const network_handler::ErrorCallback& error_callback) {
+    const network_handler::ErrorCallback& error_callback,
+    bool ignore_error_state) {
   const NetworkState* network =
       NetworkStateHandler::Get()->GetNetworkState(service_path);
   if (!network) {
@@ -166,14 +178,32 @@ void NetworkConnectionHandler::ConnectToNetwork(
     InvokeErrorCallback(service_path, error_callback, kErrorConnecting);
     return;
   }
-  if (network->passphrase_required()) {
-    InvokeErrorCallback(service_path, error_callback, kErrorPassphraseRequired);
-    return;
-  }
   if (NetworkRequiresActivation(network)) {
     InvokeErrorCallback(service_path, error_callback, kErrorActivationRequired);
     return;
   }
+  if (!ignore_error_state) {
+    if (network->passphrase_required()) {
+      InvokeErrorCallback(service_path, error_callback,
+                          kErrorPassphraseRequired);
+      return;
+    }
+    if (network->error() == flimflam::kErrorConnectFailed) {
+      InvokeErrorCallback(service_path, error_callback,
+                          kErrorPassphraseRequired);
+      return;
+    }
+    if (network->error() == flimflam::kErrorBadPassphrase) {
+      InvokeErrorCallback(service_path, error_callback,
+                          kErrorPassphraseRequired);
+      return;
+    }
+    if (network->HasAuthenticationError()) {
+      InvokeErrorCallback(service_path, error_callback,
+                          kErrorConfigurationRequired);
+      return;
+    }
+  }
 
   // All synchronous checks passed, add |service_path| to connecting list.
   pending_requests_.insert(service_path);
@@ -289,7 +319,7 @@ void NetworkConnectionHandler::HandleShillSuccess(
   // point (or maybe call it twice, once indicating in-progress, then success
   // or failure).
   pending_requests_.erase(service_path);
-  NET_LOG_EVENT("Connected", service_path);
+  NET_LOG_EVENT("Connect Request Sent", service_path);
   success_callback.Run();
 }
 
diff --git a/chromeos/network/network_connection_handler.h b/chromeos/network/network_connection_handler.h
index afe09e4..cc147be 100644
--- a/chromeos/network/network_connection_handler.h
+++ b/chromeos/network/network_connection_handler.h
@@ -47,6 +47,7 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   static const char kErrorCertificateRequired[];
   static const char kErrorConfigurationRequired[];
   static const char kErrorShillError[];
+  static const char kErrorPreviousConnectFailed[];
 
   // Sets the global instance. Must be called before any calls to Get().
   static void Initialize();
@@ -69,9 +70,12 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   //  kErrorConfigurationRequired if additional configuration is required.
   //  kErrorShillError if a DBus or Shill error occurred.
   // |error_message| will contain an additional error string for debugging.
+  // If |ignore_error_state| is true, error state for the network is ignored
+  //  (e.g. for repeat attempts).
   void ConnectToNetwork(const std::string& service_path,
                         const base::Closure& success_callback,
-                        const network_handler::ErrorCallback& error_callback);
+                        const network_handler::ErrorCallback& error_callback,
+                        bool ignore_error_state);
 
   // DisconnectToNetwork() will send a Disconnect request to Shill.
   // On success, |success_callback| will be called.
diff --git a/chromeos/network/network_connection_handler_unittest.cc b/chromeos/network/network_connection_handler_unittest.cc
index 7b4ab2b..bd3f5a2 100644
--- a/chromeos/network/network_connection_handler_unittest.cc
+++ b/chromeos/network/network_connection_handler_unittest.cc
@@ -66,12 +66,14 @@ class NetworkConnectionHandlerTest : public testing::Test {
   }
 
   void Connect(const std::string& service_path) {
+    const bool ignore_error_state = false;
     NetworkConnectionHandler::Get()->ConnectToNetwork(
         service_path,
         base::Bind(&NetworkConnectionHandlerTest::SuccessCallback,
                    base::Unretained(this)),
         base::Bind(&NetworkConnectionHandlerTest::ErrorCallback,
-                   base::Unretained(this)));
+                   base::Unretained(this)),
+        ignore_error_state);
     message_loop_.RunUntilIdle();
   }
 
diff --git a/chromeos/network/network_handler_callbacks.cc b/chromeos/network/network_handler_callbacks.cc
index 597b8bd..c23dc50 100644
--- a/chromeos/network/network_handler_callbacks.cc
+++ b/chromeos/network/network_handler_callbacks.cc
@@ -44,5 +44,5 @@ void ShillErrorCallbackFunction(const std::string& path,
   error_callback.Run(error_name, error_data.Pass());
 }
 
-} // namespace network_handler
+}  // namespace network_handler
 }  // namespace chromeos
diff --git a/chromeos/network/network_state.cc b/chromeos/network/network_state.cc
index 3c24999..84ff8f3 100644
--- a/chromeos/network/network_state.cc
+++ b/chromeos/network/network_state.cc
@@ -188,6 +188,15 @@ bool NetworkState::IsConnectingState() const {
   return StateIsConnecting(connection_state_);
 }
 
+bool NetworkState::HasAuthenticationError() const {
+  return (error_ == flimflam::kErrorBadPassphrase ||
+          error_ == flimflam::kErrorBadWEPKey ||
+          error_ == flimflam::kErrorPppAuthFailed ||
+          error_ == shill::kErrorEapLocalTlsFailed ||
+          error_ == shill::kErrorEapRemoteTlsFailed ||
+          error_ == shill::kErrorEapAuthenticationFailed);
+}
+
 void NetworkState::UpdateName() {
   if (hex_ssid_.empty()) {
     // Validate name for UTF8.
diff --git a/chromeos/network/network_state.h b/chromeos/network/network_state.h
index d49c5d8..b8b867e 100644
--- a/chromeos/network/network_state.h
+++ b/chromeos/network/network_state.h
@@ -66,6 +66,9 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
   bool IsConnectedState() const;
   bool IsConnectingState() const;
 
+  // Returns true if |error_| contains an authentication error.
+  bool HasAuthenticationError() const;
+
   // Helpers (used e.g. when a state is cached)
   static bool StateIsConnected(const std::string& connection_state);
   static bool StateIsConnecting(const std::string& connection_state);
diff --git a/chromeos/network/network_state_handler_unittest.cc b/chromeos/network/network_state_handler_unittest.cc
index af39bc7..f21d802 100644
--- a/chromeos/network/network_state_handler_unittest.cc
+++ b/chromeos/network/network_state_handler_unittest.cc
@@ -29,10 +29,10 @@ void ErrorCallbackFunction(const std::string& error_name,
   LOG(ERROR) << "Shill Error: " << error_name << " : " << error_message;
 }
 
-const std::string kShillManagerClientStubDefaultService = "stub_ethernet";
-const std::string kShillManagerClientStubDefaultWireless = "stub_wifi1";
-const std::string kShillManagerClientStubWireless2 = "stub_wifi2";
-const std::string kShillManagerClientStubCellular = "stub_cellular";
+const std::string kShillManagerClientStubDefaultService = "eth1";
+const std::string kShillManagerClientStubDefaultWireless = "wifi1";
+const std::string kShillManagerClientStubWireless2 = "wifi2";
+const std::string kShillManagerClientStubCellular = "cellular1";
 
 using chromeos::NetworkState;
 using chromeos::NetworkStateHandler;
@@ -262,46 +262,46 @@ TEST_F(NetworkStateHandlerTest, TechnologyState) {
 
 TEST_F(NetworkStateHandlerTest, ServicePropertyChanged) {
   // Set a service property.
-  const std::string eth0 = kShillManagerClientStubDefaultService;
-  EXPECT_EQ("", network_state_handler_->GetNetworkState(eth0)->security());
-  EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(eth0));
+  const std::string eth1 = kShillManagerClientStubDefaultService;
+  EXPECT_EQ("", network_state_handler_->GetNetworkState(eth1)->security());
+  EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(eth1));
   base::StringValue security_value("TestSecurity");
   DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(eth0),
+      dbus::ObjectPath(eth1),
       flimflam::kSecurityProperty, security_value,
       base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
   message_loop_.RunUntilIdle();
   EXPECT_EQ("TestSecurity",
-            network_state_handler_->GetNetworkState(eth0)->security());
-  EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(eth0));
+            network_state_handler_->GetNetworkState(eth1)->security());
+  EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(eth1));
 
   // Changing a service to the existing value should not trigger an update.
   DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(eth0),
+      dbus::ObjectPath(eth1),
       flimflam::kSecurityProperty, security_value,
       base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
   message_loop_.RunUntilIdle();
-  EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(eth0));
+  EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(eth1));
 }
 
 TEST_F(NetworkStateHandlerTest, NetworkConnectionStateChanged) {
   // Change a network state.
   ShillServiceClient::TestInterface* service_test =
       DBusThreadManager::Get()->GetShillServiceClient()->GetTestInterface();
-  const std::string eth0 = kShillManagerClientStubDefaultService;
+  const std::string eth1 = kShillManagerClientStubDefaultService;
   base::StringValue connection_state_idle_value(flimflam::kStateIdle);
-  service_test->SetServiceProperty(eth0, flimflam::kStateProperty,
+  service_test->SetServiceProperty(eth1, flimflam::kStateProperty,
                                    connection_state_idle_value);
   message_loop_.RunUntilIdle();
   EXPECT_EQ(flimflam::kStateIdle,
-            test_observer_->NetworkConnectionStateForService(eth0));
-  EXPECT_EQ(2, test_observer_->ConnectionStateChangesForService(eth0));
+            test_observer_->NetworkConnectionStateForService(eth1));
+  EXPECT_EQ(2, test_observer_->ConnectionStateChangesForService(eth1));
   // Confirm that changing the connection state to the same value does *not*
   // signal the observer.
-  service_test->SetServiceProperty(eth0, flimflam::kStateProperty,
+  service_test->SetServiceProperty(eth1, flimflam::kStateProperty,
                                    connection_state_idle_value);
   message_loop_.RunUntilIdle();
-  EXPECT_EQ(2, test_observer_->ConnectionStateChangesForService(eth0));
+  EXPECT_EQ(2, test_observer_->ConnectionStateChangesForService(eth1));
 }
 
 TEST_F(NetworkStateHandlerTest, DefaultServiceChanged) {
@@ -313,12 +313,12 @@ TEST_F(NetworkStateHandlerTest, DefaultServiceChanged) {
   ASSERT_TRUE(service_test);
 
   // Change the default network by moving wifi1 to the front of the list
-  // and changing the state of stub_ethernet to Idle.
+  // and changing the state of eth1 to Idle.
   const std::string wifi1 = kShillManagerClientStubDefaultWireless;
   manager_test->MoveServiceToIndex(wifi1, 0, true);
-  const std::string eth0 = kShillManagerClientStubDefaultService;
+  const std::string eth1 = kShillManagerClientStubDefaultService;
   base::StringValue connection_state_idle_value(flimflam::kStateIdle);
-  service_test->SetServiceProperty(eth0, flimflam::kStateProperty,
+  service_test->SetServiceProperty(eth1, flimflam::kStateProperty,
                                    connection_state_idle_value);
   message_loop_.RunUntilIdle();
   EXPECT_EQ(wifi1, test_observer_->default_network());