Use NetworkConnectionObserver to trigger connect failure notifications

Currently we trigger network connect failure notifications when a call
from ui::NetworkConnect::ConnectToNetwork() fails.

The problem with this approach is that connect calls from elsewhere
(e.g. through the networkingPrivate API) will not trigger notifications.

This CL makes NetworkStateNotifier a NetworkConnectionObserver and
triggers the failure notification on ConnectFailed events instead of the
NetworkConnect failure callback so that other failures will also
generate notifications.

This does not affect spawining of new UI (e.g. network configuration UI)
on failure (when appropriate) since that is expected to be handled by any
new UI that uses the networkingPrivate API.

BUG=434112

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

Cr-Commit-Position: refs/heads/master@{#312879}

8 files changed, 272 insertions, 86 deletions

diff --git a/chromeos/chromeos.gyp b/chromeos/chromeos.gyp
index 34022a3..1e85489 100644
--- a/chromeos/chromeos.gyp
+++ b/chromeos/chromeos.gyp
@@ -337,6 +337,8 @@
       'network/network_configuration_observer.h',
       'network/network_connection_handler.cc',
       'network/network_connection_handler.h',
+      'network/network_connection_observer.cc',
+      'network/network_connection_observer.h',
       'network/network_device_handler.cc',
       'network/network_device_handler.h',
       'network/network_device_handler_impl.cc',
diff --git a/chromeos/network/auto_connect_handler.cc b/chromeos/network/auto_connect_handler.cc
index a968ce4..9a84673 100644
--- a/chromeos/network/auto_connect_handler.cc
+++ b/chromeos/network/auto_connect_handler.cc
@@ -16,6 +16,7 @@
 #include "chromeos/dbus/shill_service_client.h"
 #include "chromeos/network/device_state.h"
 #include "chromeos/network/managed_network_configuration_handler.h"
+#include "chromeos/network/network_connection_handler.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_type_pattern.h"
diff --git a/chromeos/network/auto_connect_handler.h b/chromeos/network/auto_connect_handler.h
index 9977107..26444dc 100644
--- a/chromeos/network/auto_connect_handler.h
+++ b/chromeos/network/auto_connect_handler.h
@@ -12,26 +12,25 @@
 #include "chromeos/chromeos_export.h"
 #include "chromeos/login/login_state.h"
 #include "chromeos/network/client_cert_resolver.h"
-#include "chromeos/network/network_connection_handler.h"
+#include "chromeos/network/network_connection_observer.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_policy_observer.h"
 #include "chromeos/network/network_state_handler_observer.h"
 
 namespace chromeos {
 
-class CHROMEOS_EXPORT AutoConnectHandler
-    : public LoginState::Observer,
-      public NetworkPolicyObserver,
-      public NetworkConnectionHandler::Observer,
-      public NetworkStateHandlerObserver,
-      public ClientCertResolver::Observer {
+class CHROMEOS_EXPORT AutoConnectHandler : public LoginState::Observer,
+                                           public NetworkPolicyObserver,
+                                           public NetworkConnectionObserver,
+                                           public NetworkStateHandlerObserver,
+                                           public ClientCertResolver::Observer {
  public:
   ~AutoConnectHandler() override;
 
   // LoginState::Observer
   void LoggedInStateChanged() override;
 
-  // NetworkConnectionHandler::Observer
+  // NetworkConnectionObserver
   void ConnectToNetworkRequested(const std::string& service_path) override;
 
   // NetworkPolicyObserver
diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc
index 5fbdd79..af9f26a 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -32,14 +32,6 @@ namespace chromeos {
 
 namespace {
 
-void InvokeErrorCallback(const std::string& service_path,
-                         const network_handler::ErrorCallback& error_callback,
-                         const std::string& error_name) {
-  NET_LOG_ERROR("Connect Error: " + error_name, service_path);
-  network_handler::RunErrorCallback(
-      error_callback, service_path, error_name, "");
-}
-
 bool IsAuthenticationError(const std::string& error) {
   return (error == shill::kErrorBadWEPKey ||
           error == shill::kErrorPppAuthFailed ||
@@ -108,15 +100,16 @@ const char NetworkConnectionHandler::kErrorConnecting[] = "connecting";
 const char NetworkConnectionHandler::kErrorNotConnected[] = "not-connected";
 const char NetworkConnectionHandler::kErrorPassphraseRequired[] =
     "passphrase-required";
-const char NetworkConnectionHandler::kErrorActivationRequired[] =
-    "activation-required";
+const char NetworkConnectionHandler::kErrorBadPassphrase[] = "bad-passphrase";
 const char NetworkConnectionHandler::kErrorCertificateRequired[] =
     "certificate-required";
 const char NetworkConnectionHandler::kErrorConfigurationRequired[] =
     "configuration-required";
 const char NetworkConnectionHandler::kErrorAuthenticationRequired[] =
     "authentication-required";
-const char NetworkConnectionHandler::kErrorShillError[] = "shill-error";
+const char NetworkConnectionHandler::kErrorConnectFailed[] = "connect-failed";
+const char NetworkConnectionHandler::kErrorDisconnectFailed[] =
+    "disconnect-failed";
 const char NetworkConnectionHandler::kErrorConfigureFailed[] =
     "configure-failed";
 const char NetworkConnectionHandler::kErrorConnectCanceled[] =
@@ -198,11 +191,13 @@ void NetworkConnectionHandler::Init(
     LoggedInStateChanged();
 }
 
-void NetworkConnectionHandler::AddObserver(Observer* observer) {
+void NetworkConnectionHandler::AddObserver(
+    NetworkConnectionObserver* observer) {
   observers_.AddObserver(observer);
 }
 
-void NetworkConnectionHandler::RemoveObserver(Observer* observer) {
+void NetworkConnectionHandler::RemoveObserver(
+    NetworkConnectionObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
@@ -230,14 +225,14 @@ void NetworkConnectionHandler::ConnectToNetwork(
     const network_handler::ErrorCallback& error_callback,
     bool check_error_state) {
   NET_LOG_USER("ConnectToNetwork", service_path);
-  FOR_EACH_OBSERVER(Observer, observers_,
+  FOR_EACH_OBSERVER(NetworkConnectionObserver, observers_,
                     ConnectToNetworkRequested(service_path));
 
   // Clear any existing queued connect request.
   queued_connect_.reset();
   if (HasConnectingNetwork(service_path)) {
     NET_LOG_USER("Connect Request While Pending", service_path);
-    InvokeErrorCallback(service_path, error_callback, kErrorConnecting);
+    InvokeConnectErrorCallback(service_path, error_callback, kErrorConnecting);
     return;
   }
 
@@ -251,23 +246,25 @@ void NetworkConnectionHandler::ConnectToNetwork(
   if (network) {
     // For existing networks, perform some immediate consistency checks.
     if (network->IsConnectedState()) {
-      InvokeErrorCallback(service_path, error_callback, kErrorConnected);
+      InvokeConnectErrorCallback(service_path, error_callback, kErrorConnected);
       return;
     }
     if (network->IsConnectingState()) {
-      InvokeErrorCallback(service_path, error_callback, kErrorConnecting);
+      InvokeConnectErrorCallback(service_path, error_callback,
+                                 kErrorConnecting);
       return;
     }
 
     if (check_error_state) {
       const std::string& error = network->last_error();
       if (error == shill::kErrorBadPassphrase) {
-        InvokeErrorCallback(service_path, error_callback, error);
+        InvokeConnectErrorCallback(service_path, error_callback,
+                                   kErrorBadPassphrase);
         return;
       }
       if (IsAuthenticationError(error)) {
-        InvokeErrorCallback(
-            service_path, error_callback, kErrorAuthenticationRequired);
+        InvokeConnectErrorCallback(service_path, error_callback,
+                                   kErrorAuthenticationRequired);
         return;
       }
     }
@@ -309,14 +306,21 @@ void NetworkConnectionHandler::DisconnectNetwork(
     const base::Closure& success_callback,
     const network_handler::ErrorCallback& error_callback) {
   NET_LOG_USER("DisconnectNetwork", service_path);
+  FOR_EACH_OBSERVER(NetworkConnectionObserver, observers_,
+                    DiconnectRequested(service_path));
+
   const NetworkState* network =
       network_state_handler_->GetNetworkState(service_path);
   if (!network) {
-    InvokeErrorCallback(service_path, error_callback, kErrorNotFound);
+    NET_LOG_ERROR("Disconnect Error: Not Found", service_path);
+    network_handler::RunErrorCallback(error_callback, service_path,
+                                      kErrorNotFound, "");
     return;
   }
   if (!network->IsConnectedState() && !network->IsConnectingState()) {
-    InvokeErrorCallback(service_path, error_callback, kErrorNotConnected);
+    NET_LOG_ERROR("Disconnect Error: Not Connected", service_path);
+    network_handler::RunErrorCallback(error_callback, service_path,
+                                      kErrorNotConnected, "");
     return;
   }
   pending_requests_.erase(service_path);
@@ -535,9 +539,8 @@ void NetworkConnectionHandler::QueueConnectRequest(
   base::TimeDelta dtime = base::TimeTicks::Now() - logged_in_time_;
   if (dtime > base::TimeDelta::FromSeconds(kMaxCertLoadTimeSeconds)) {
     NET_LOG_ERROR("Certificate load timeout", service_path);
-    InvokeErrorCallback(service_path,
-                        request->error_callback,
-                        kErrorCertLoadTimeout);
+    InvokeConnectErrorCallback(service_path, request->error_callback,
+                               kErrorCertLoadTimeout);
     return;
   }
 
@@ -566,9 +569,9 @@ void NetworkConnectionHandler::CheckCertificatesLoaded() {
     return;
   // Otherwise, notify the user.
   NET_LOG_ERROR("Certificate load timeout", queued_connect_->service_path);
-  InvokeErrorCallback(queued_connect_->service_path,
-                      queued_connect_->error_callback,
-                      kErrorCertLoadTimeout);
+  InvokeConnectErrorCallback(queued_connect_->service_path,
+                             queued_connect_->error_callback,
+                             kErrorCertLoadTimeout);
   queued_connect_.reset();
 }
 
@@ -611,8 +614,8 @@ void NetworkConnectionHandler::HandleConfigurationFailure(
   }
   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
-  if (!error_callback.is_null())
-    error_callback.Run(kErrorConfigureFailed, error_data.Pass());
+  InvokeConnectErrorCallback(service_path, error_callback,
+                             kErrorConfigureFailed);
 }
 
 void NetworkConnectionHandler::HandleShillConnectSuccess(
@@ -644,9 +647,17 @@ void NetworkConnectionHandler::HandleShillConnectFailure(
   }
   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
-  network_handler::ShillErrorCallbackFunction(
-      shill::kErrorConnectFailed, service_path, error_callback,
-      dbus_error_name, dbus_error_message);
+  std::string error;
+  if (dbus_error_name == shill::kErrorResultAlreadyConnected) {
+    error = kErrorConnected;
+  } else if (dbus_error_name == shill::kErrorResultInProgress) {
+    error = kErrorConnecting;
+  } else {
+    NET_LOG_ERROR("Connect Failure, Shill error: " + dbus_error_name,
+                  service_path);
+    error = kErrorConnectFailed;
+  }
+  InvokeConnectErrorCallback(service_path, error_callback, error);
 }
 
 void NetworkConnectionHandler::CheckPendingRequest(
@@ -665,7 +676,6 @@ void NetworkConnectionHandler::CheckPendingRequest(
     return;
   }
   if (network->IsConnectedState()) {
-    NET_LOG_EVENT("Connect Request Succeeded", service_path);
     if (!request->profile_path.empty()) {
       // If a profile path was specified, set it on a successful connection.
       configuration_handler_->SetNetworkProfile(
@@ -675,8 +685,8 @@ void NetworkConnectionHandler::CheckPendingRequest(
           base::Bind(&base::DoNothing),
           chromeos::network_handler::ErrorCallback());
     }
-    if (!request->success_callback.is_null())
-      request->success_callback.Run();
+    InvokeConnectSuccessCallback(request->service_path,
+                                 request->success_callback);
     pending_requests_.erase(service_path);
     return;
   }
@@ -693,7 +703,7 @@ void NetworkConnectionHandler::CheckPendingRequest(
     // Another connect request canceled this one.
     error_name = kErrorConnectCanceled;
   } else {
-    error_name = shill::kErrorConnectFailed;
+    error_name = kErrorConnectFailed;
     if (network->connection_state() != shill::kStateFailure) {
       NET_LOG_ERROR("Unexpected State: " + network->connection_state(),
                     service_path);
@@ -702,11 +712,7 @@ void NetworkConnectionHandler::CheckPendingRequest(
 
   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
-  if (error_callback.is_null()) {
-    NET_LOG_ERROR("Connect Error, no callback: " + error_name, service_path);
-    return;
-  }
-  InvokeErrorCallback(service_path, error_callback, error_name);
+  InvokeConnectErrorCallback(service_path, error_callback, error_name);
 }
 
 void NetworkConnectionHandler::CheckAllPendingRequests() {
@@ -716,6 +722,18 @@ void NetworkConnectionHandler::CheckAllPendingRequests() {
   }
 }
 
+// Connect callbacks
+
+void NetworkConnectionHandler::InvokeConnectSuccessCallback(
+    const std::string& service_path,
+    const base::Closure& success_callback) {
+  NET_LOG_EVENT("Connect Request Succeeded", service_path);
+  if (!success_callback.is_null())
+    success_callback.Run();
+  FOR_EACH_OBSERVER(NetworkConnectionObserver, observers_,
+                    ConnectSucceeded(service_path));
+}
+
 void NetworkConnectionHandler::ErrorCallbackForPendingRequest(
     const std::string& service_path,
     const std::string& error_name) {
@@ -728,7 +746,18 @@ void NetworkConnectionHandler::ErrorCallbackForPendingRequest(
   // Remove the entry before invoking the callback in case it triggers a retry.
   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
-  InvokeErrorCallback(service_path, error_callback, error_name);
+  InvokeConnectErrorCallback(service_path, error_callback, error_name);
+}
+
+void NetworkConnectionHandler::InvokeConnectErrorCallback(
+    const std::string& service_path,
+    const network_handler::ErrorCallback& error_callback,
+    const std::string& error_name) {
+  NET_LOG_ERROR("Connect Failure: " + error_name, service_path);
+  network_handler::RunErrorCallback(error_callback, service_path, error_name,
+                                    "");
+  FOR_EACH_OBSERVER(NetworkConnectionObserver, observers_,
+                    ConnectFailed(service_path, error_name));
 }
 
 // Disconnect
@@ -743,7 +772,7 @@ void NetworkConnectionHandler::CallShillDisconnect(
       base::Bind(&NetworkConnectionHandler::HandleShillDisconnectSuccess,
                  AsWeakPtr(), service_path, success_callback),
       base::Bind(&network_handler::ShillErrorCallbackFunction,
-                 kErrorShillError, service_path, error_callback));
+                 kErrorDisconnectFailed, service_path, error_callback));
 }
 
 void NetworkConnectionHandler::HandleShillDisconnectSuccess(
diff --git a/chromeos/network/network_connection_handler.h b/chromeos/network/network_connection_handler.h
index 7cdb19e..aef25b9 100644
--- a/chromeos/network/network_connection_handler.h
+++ b/chromeos/network/network_connection_handler.h
@@ -19,6 +19,7 @@
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/login/login_state.h"
+#include "chromeos/network/network_connection_observer.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_handler_callbacks.h"
 #include "chromeos/network/network_state_handler_observer.h"
@@ -49,19 +50,6 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
       public NetworkStateHandlerObserver,
       public base::SupportsWeakPtr<NetworkConnectionHandler> {
  public:
-  class Observer {
-   public:
-    // Called if a connection to network |service_path| was requested, by
-    // calling ConnectToNetwork.
-    virtual void ConnectToNetworkRequested(const std::string& service_path) = 0;
-
-   protected:
-    virtual ~Observer() {}
-
-   private:
-    DISALLOW_ASSIGN(Observer);
-  };
-
   // Constants for |error_name| from |error_callback| for Connect.
 
   //  No network matching |service_path| is found (hidden networks must be
@@ -77,7 +65,8 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   // The passphrase is missing or invalid.
   static const char kErrorPassphraseRequired[];
 
-  static const char kErrorActivationRequired[];
+  // The passphrase is incorrect.
+  static const char kErrorBadPassphrase[];
 
   // The network requires a cert and none exists.
   static const char kErrorCertificateRequired[];
@@ -92,8 +81,11 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   // Configuration failed during the configure stage of the connect flow.
   static const char kErrorConfigureFailed[];
 
-  // For Disconnect or Activate, an unexpected DBus or Shill error occurred.
-  static const char kErrorShillError[];
+  // An unexpected DBus or Shill error occurred while connecting.
+  static const char kErrorConnectFailed[];
+
+  // An unexpected DBus or Shill error occurred while disconnecting.
+  static const char kErrorDisconnectFailed[];
 
   // A new network connect request canceled this one.
   static const char kErrorConnectCanceled[];
@@ -106,15 +98,13 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
 
   ~NetworkConnectionHandler() override;
 
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
+  void AddObserver(NetworkConnectionObserver* observer);
+  void RemoveObserver(NetworkConnectionObserver* observer);
 
   // ConnectToNetwork() will start an asynchronous connection attempt.
   // On success, |success_callback| will be called.
   // On failure, |error_callback| will be called with |error_name| one of the
-  //   constants defined above, or shill::kErrorConnectFailed or
-  //   shill::kErrorBadPassphrase if the Shill Error property (from a
-  //   previous connect attempt) was set to one of those.
+  //   constants defined above.
   // |error_message| will contain an additional error string for debugging.
   // If |check_error_state| is true, the current state of the network is
   //  checked for errors, otherwise current state is ignored (e.g. for recently
@@ -129,7 +119,7 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   // On failure, |error_callback| will be called with |error_name| one of:
   //  kErrorNotFound if no network matching |service_path| is found.
   //  kErrorNotConnected if not connected to the network.
-  //  kErrorShillError if a DBus or Shill error occurred.
+  //  kErrorDisconnectFailed if a DBus or Shill error occurred.
   // |error_message| will contain and additional error string for debugging.
   void DisconnectNetwork(const std::string& service_path,
                          const base::Closure& success_callback,
@@ -206,9 +196,22 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   void CheckPendingRequest(const std::string service_path);
   void CheckAllPendingRequests();
 
+  // Notify caller and observers that the connect request succeeded.
+  void InvokeConnectSuccessCallback(const std::string& service_path,
+                                    const base::Closure& success_callback);
+
+  // Look up the ConnectRequest for |service_path| and call
+  // InvokeConnectErrorCallback.
   void ErrorCallbackForPendingRequest(const std::string& service_path,
                                       const std::string& error_name);
 
+  // Notify caller and observers that the connect request failed.
+  // |error_name| will be one of the kError* messages defined above.
+  void InvokeConnectErrorCallback(
+      const std::string& service_path,
+      const network_handler::ErrorCallback& error_callback,
+      const std::string& error_name);
+
   // Calls Shill.Manager.Disconnect asynchronously.
   void CallShillDisconnect(
       const std::string& service_path,
@@ -219,7 +222,7 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   void HandleShillDisconnectSuccess(const std::string& service_path,
                                     const base::Closure& success_callback);
 
-  ObserverList<Observer> observers_;
+  ObserverList<NetworkConnectionObserver> observers_;
 
   // Local references to the associated handler instances.
   CertLoader* cert_loader_;
diff --git a/chromeos/network/network_connection_handler_unittest.cc b/chromeos/network/network_connection_handler_unittest.cc
index ab4b00c..8e059c9 100644
--- a/chromeos/network/network_connection_handler_unittest.cc
+++ b/chromeos/network/network_connection_handler_unittest.cc
@@ -4,6 +4,9 @@
 
 #include "chromeos/network/network_connection_handler.h"
 
+#include <map>
+#include <set>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/files/file_util.h"
@@ -20,6 +23,7 @@
 #include "chromeos/dbus/shill_service_client.h"
 #include "chromeos/network/managed_network_configuration_handler_impl.h"
 #include "chromeos/network/network_configuration_handler.h"
+#include "chromeos/network/network_connection_observer.h"
 #include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/onc/onc_utils.h"
@@ -34,6 +38,8 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
+namespace chromeos {
+
 namespace {
 
 const char* kSuccessResult = "success";
@@ -46,9 +52,48 @@ void ConfigureErrorCallback(const std::string& error_name,
                             const std::string& error_message) {
 }
 
-}  // namespace
+class TestNetworkConnectionObserver : public NetworkConnectionObserver {
+ public:
+  TestNetworkConnectionObserver() {}
+  ~TestNetworkConnectionObserver() override {}
 
-namespace chromeos {
+  // NetworkConnectionObserver
+  void ConnectToNetworkRequested(const std::string& service_path) override {
+    requests_.insert(service_path);
+  }
+
+  void ConnectSucceeded(const std::string& service_path) override {
+    results_[service_path] = kSuccessResult;
+  }
+
+  void ConnectFailed(const std::string& service_path,
+                     const std::string& error_name) override {
+    results_[service_path] = error_name;
+  }
+
+  void DiconnectRequested(const std::string& service_path) override {
+    requests_.insert(service_path);
+  }
+
+  bool GetRequested(const std::string& service_path) {
+    return requests_.count(service_path) != 0;
+  }
+
+  std::string GetResult(const std::string& service_path) {
+    auto iter = results_.find(service_path);
+    if (iter == results_.end())
+      return "";
+    return iter->second;
+  }
+
+ private:
+  std::set<std::string> requests_;
+  std::map<std::string, std::string> results_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestNetworkConnectionObserver);
+};
+
+}  // namespace
 
 class NetworkConnectionHandlerTest : public testing::Test {
  public:
@@ -106,6 +151,9 @@ class NetworkConnectionHandlerTest : public testing::Test {
     network_connection_handler_->Init(network_state_handler_.get(),
                                       network_config_handler_.get(),
                                       managed_config_handler_.get());
+    network_connection_observer_.reset(new TestNetworkConnectionObserver);
+    network_connection_handler_->AddObserver(
+        network_connection_observer_.get());
 
     base::RunLoop().RunUntilIdle();
   }
@@ -113,6 +161,9 @@ class NetworkConnectionHandlerTest : public testing::Test {
   void TearDown() override {
     managed_config_handler_.reset();
     network_profile_handler_.reset();
+    network_connection_handler_->RemoveObserver(
+        network_connection_observer_.get());
+    network_connection_observer_.reset();
     network_connection_handler_.reset();
     network_config_handler_.reset();
     network_state_handler_.reset();
@@ -248,6 +299,7 @@ class NetworkConnectionHandlerTest : public testing::Test {
   scoped_ptr<NetworkStateHandler> network_state_handler_;
   scoped_ptr<NetworkConfigurationHandler> network_config_handler_;
   scoped_ptr<NetworkConnectionHandler> network_connection_handler_;
+  scoped_ptr<TestNetworkConnectionObserver> network_connection_observer_;
   scoped_ptr<ManagedNetworkConfigurationHandlerImpl> managed_config_handler_;
   scoped_ptr<NetworkProfileHandler> network_profile_handler_;
   ShillManagerClient::TestInterface* test_manager_client_;
@@ -263,6 +315,12 @@ class NetworkConnectionHandlerTest : public testing::Test {
 
 namespace {
 
+const char* kNoNetwork = "no-network";
+const char* kWifi0 = "wifi0";
+const char* kWifi1 = "wifi1";
+const char* kWifi2 = "wifi2";
+const char* kWifi3 = "wifi3";
+
 const char* kConfigConnectable =
     "{ \"GUID\": \"wifi0\", \"Type\": \"wifi\", \"State\": \"idle\", "
     "  \"Connectable\": true }";
@@ -278,30 +336,45 @@ const char* kConfigRequiresPassphrase =
 
 TEST_F(NetworkConnectionHandlerTest, NetworkConnectionHandlerConnectSuccess) {
   EXPECT_TRUE(Configure(kConfigConnectable));
-  Connect("wifi0");
+  Connect(kWifi0);
   EXPECT_EQ(kSuccessResult, GetResultAndReset());
   EXPECT_EQ(shill::kStateOnline,
-            GetServiceStringProperty("wifi0", shill::kStateProperty));
+            GetServiceStringProperty(kWifi0, shill::kStateProperty));
+  // Observer expectations
+  EXPECT_TRUE(network_connection_observer_->GetRequested(kWifi0));
+  EXPECT_EQ(kSuccessResult, network_connection_observer_->GetResult(kWifi0));
 }
 
 // Handles basic failure cases.
 TEST_F(NetworkConnectionHandlerTest, NetworkConnectionHandlerConnectFailure) {
-  Connect("no-network");
+  Connect(kNoNetwork);
   EXPECT_EQ(NetworkConnectionHandler::kErrorConfigureFailed,
             GetResultAndReset());
+  EXPECT_TRUE(network_connection_observer_->GetRequested(kNoNetwork));
+  EXPECT_EQ(NetworkConnectionHandler::kErrorConfigureFailed,
+            network_connection_observer_->GetResult(kNoNetwork));
 
   EXPECT_TRUE(Configure(kConfigConnected));
-  Connect("wifi1");
+  Connect(kWifi1);
   EXPECT_EQ(NetworkConnectionHandler::kErrorConnected, GetResultAndReset());
+  EXPECT_TRUE(network_connection_observer_->GetRequested(kWifi1));
+  EXPECT_EQ(NetworkConnectionHandler::kErrorConnected,
+            network_connection_observer_->GetResult(kWifi1));
 
   EXPECT_TRUE(Configure(kConfigConnecting));
-  Connect("wifi2");
+  Connect(kWifi2);
   EXPECT_EQ(NetworkConnectionHandler::kErrorConnecting, GetResultAndReset());
+  EXPECT_TRUE(network_connection_observer_->GetRequested(kWifi2));
+  EXPECT_EQ(NetworkConnectionHandler::kErrorConnecting,
+            network_connection_observer_->GetResult(kWifi2));
 
   EXPECT_TRUE(Configure(kConfigRequiresPassphrase));
-  Connect("wifi3");
+  Connect(kWifi3);
   EXPECT_EQ(NetworkConnectionHandler::kErrorPassphraseRequired,
             GetResultAndReset());
+  EXPECT_TRUE(network_connection_observer_->GetRequested(kWifi3));
+  EXPECT_EQ(NetworkConnectionHandler::kErrorPassphraseRequired,
+            network_connection_observer_->GetResult(kWifi3));
 }
 
 namespace {
@@ -382,18 +455,19 @@ TEST_F(NetworkConnectionHandlerTest,
 TEST_F(NetworkConnectionHandlerTest,
        NetworkConnectionHandlerDisconnectSuccess) {
   EXPECT_TRUE(Configure(kConfigConnected));
-  Disconnect("wifi1");
+  Disconnect(kWifi1);
+  EXPECT_TRUE(network_connection_observer_->GetRequested(kWifi1));
   EXPECT_EQ(kSuccessResult, GetResultAndReset());
 }
 
 TEST_F(NetworkConnectionHandlerTest,
        NetworkConnectionHandlerDisconnectFailure) {
-  Connect("no-network");
+  Connect(kNoNetwork);
   EXPECT_EQ(NetworkConnectionHandler::kErrorConfigureFailed,
             GetResultAndReset());
 
   EXPECT_TRUE(Configure(kConfigConnectable));
-  Disconnect("wifi0");
+  Disconnect(kWifi0);
   EXPECT_EQ(NetworkConnectionHandler::kErrorNotConnected, GetResultAndReset());
 }
 
diff --git a/chromeos/network/network_connection_observer.cc b/chromeos/network/network_connection_observer.cc
new file mode 100644
index 0000000..4c4f5ae
--- /dev/null
+++ b/chromeos/network/network_connection_observer.cc
@@ -0,0 +1,31 @@
+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/network/network_connection_observer.h"
+
+namespace chromeos {
+
+NetworkConnectionObserver::NetworkConnectionObserver() {
+}
+
+void NetworkConnectionObserver::ConnectToNetworkRequested(
+    const std::string& service_path) {
+}
+
+void NetworkConnectionObserver::ConnectSucceeded(
+    const std::string& service_path) {
+}
+
+void NetworkConnectionObserver::ConnectFailed(const std::string& service_path,
+                                              const std::string& error_name) {
+}
+
+void NetworkConnectionObserver::DiconnectRequested(
+    const std::string& service_path) {
+}
+
+NetworkConnectionObserver::~NetworkConnectionObserver() {
+}
+
+}  // namespace chromeos
diff --git a/chromeos/network/network_connection_observer.h b/chromeos/network/network_connection_observer.h
new file mode 100644
index 0000000..d7aae7c
--- /dev/null
+++ b/chromeos/network/network_connection_observer.h
@@ -0,0 +1,47 @@
+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_NETWORK_NETWORK_CONNECTION_OBSERVER_H_
+#define CHROMEOS_NETWORK_NETWORK_CONNECTION_OBSERVER_H_
+
+#include <string>
+
+#include "base/macros.h"
+#include "chromeos/chromeos_export.h"
+
+namespace chromeos {
+
+// Observer class for network connection events.
+class CHROMEOS_EXPORT NetworkConnectionObserver {
+ public:
+  NetworkConnectionObserver();
+
+  // Called when a connection to network |service_path| is requested by
+  // calling NetworkConnectionHandler::ConnectToNetwork.
+  virtual void ConnectToNetworkRequested(const std::string& service_path);
+
+  // Called when a connection request succeeds.
+  virtual void ConnectSucceeded(const std::string& service_path);
+
+  // Called when a connection request fails. Valid error names are defined in
+  // NetworkConnectionHandler.
+  virtual void ConnectFailed(const std::string& service_path,
+                             const std::string& error_name);
+
+  // Called when a disconnect to network |service_path| is requested by
+  // calling NetworkConnectionHandler::DisconnectNetwork. Success or failure
+  // for disconnect is not tracked here, observe NetworkStateHandler for state
+  // changes instead.
+  virtual void DiconnectRequested(const std::string& service_path);
+
+ protected:
+  virtual ~NetworkConnectionObserver();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NetworkConnectionObserver);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_NETWORK_NETWORK_CONNECTION_OBSERVER_H_