Trigger NetworkConnectionHandler callbacks on success or failure

This CL triggers waits for the network state to change before triggering
success or failure callbacks in NetworkConnectionHandler.

Notifications are triggered on success or failure without requiring the
NetworkStateHandler connecting_network() hack.

It also separates the enable flag from NetworkConfigurationHandler since
enabling that has different artifacts.

BUG=243899
For ash.gyp:
TBR=gspencer@chromium.org

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

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

5 files changed, 303 insertions, 104 deletions

diff --git a/chromeos/chromeos_switches.cc b/chromeos/chromeos_switches.cc
index 95d8376..1452a39 100644
--- a/chromeos/chromeos_switches.cc
+++ b/chromeos/chromeos_switches.cc
@@ -191,5 +191,8 @@ const char kStubCrosSettings[]              = "stub-cros-settings";
 const char kUseNewNetworkConfigurationHandlers[] =
     "use-new-network-configuration-handlers";
 
+const char kUseNewNetworkConnectionHandler[] =
+    "use-new-network-connection-handler";
+
 }  // namespace switches
 }  // namespace chromeos
diff --git a/chromeos/chromeos_switches.h b/chromeos/chromeos_switches.h
index f4fd779..d4ad7a3 100644
--- a/chromeos/chromeos_switches.h
+++ b/chromeos/chromeos_switches.h
@@ -72,6 +72,7 @@ CHROMEOS_EXPORT extern const char kSkipHWIDCheck[];
 CHROMEOS_EXPORT extern const char kSmsTestMessages[];
 CHROMEOS_EXPORT extern const char kStubCrosSettings[];
 CHROMEOS_EXPORT extern const char kUseNewNetworkConfigurationHandlers[];
+CHROMEOS_EXPORT extern const char kUseNewNetworkConnectionHandler[];
 
 }  // namespace switches
 }  // namespace chromeos
diff --git a/chromeos/dbus/shill_service_client_stub.cc b/chromeos/dbus/shill_service_client_stub.cc
index 0c0d2f93..17055c8 100644
--- a/chromeos/dbus/shill_service_client_stub.cc
+++ b/chromeos/dbus/shill_service_client_stub.cc
@@ -102,9 +102,12 @@ void ShillServiceClientStub::SetProperty(const dbus::ObjectPath& service_path,
   base::DictionaryValue* dict = NULL;
   if (!stub_services_.GetDictionaryWithoutPathExpansion(
           service_path.value(), &dict)) {
+    LOG(ERROR) << "Service not found:  " << service_path.value();
     error_callback.Run("Error.InvalidService", "Invalid Service");
     return;
   }
+  VLOG(1) << "Service.SetProperty: " << name << " = " << value
+          << " For: " << service_path.value();
   if (name == flimflam::kStateProperty) {
     // If the service went into a connected state, then move it to the top of
     // the list in the manager client.
@@ -186,19 +189,21 @@ void ShillServiceClientStub::ClearProperties(
 void ShillServiceClientStub::Connect(const dbus::ObjectPath& service_path,
                                      const base::Closure& callback,
                                      const ErrorCallback& error_callback) {
+  VLOG(1) << "ShillServiceClientStub::Connect: " << service_path.value();
   base::Value* service;
   if (!stub_services_.Get(service_path.value(), &service)) {
+    LOG(ERROR) << "Service not found:  " << service_path.value();
     error_callback.Run("Error.InvalidService", "Invalid Service");
     return;
   }
   base::TimeDelta delay;
+  // Set Associating
+  base::StringValue associating_value(flimflam::kStateAssociation);
+  SetServiceProperty(service_path.value(),
+                     flimflam::kStateProperty,
+                     associating_value);
   if (CommandLine::ForCurrentProcess()->HasSwitch(
           chromeos::switches::kEnableStubInteractive)) {
-    // Set Associating
-    base::StringValue associating_value(flimflam::kStateAssociation);
-    SetServiceProperty(service_path.value(),
-                       flimflam::kStateProperty,
-                       associating_value);
     const int kConnectDelaySeconds = 5;
     delay = base::TimeDelta::FromSeconds(kConnectDelaySeconds);
   }
diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc
index 10f619d..97cc9f8 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -40,6 +40,12 @@ void InvokeErrorCallback(const std::string& service_path,
   error_callback.Run(error_name, error_data.Pass());
 }
 
+bool NetworkConnectable(const NetworkState* network) {
+  if (network->type() == flimflam::kTypeVPN)
+    return false;  // TODO(stevenjb): Shill needs to properly set Connectable.
+  return network->connectable();
+}
+
 bool NetworkMayNeedCredentials(const NetworkState* network) {
   if (network->type() == flimflam::kTypeWifi &&
       (network->security() == flimflam::kSecurity8021x ||
@@ -56,34 +62,62 @@ bool NetworkRequiresActivation(const NetworkState* network) {
            network->cellular_out_of_credits()));
 }
 
-bool VPNIsConfigured(const base::DictionaryValue& properties) {
+bool VPNIsConfigured(const std::string& service_path,
+                     const base::DictionaryValue& service_properties) {
+  const DictionaryValue* properties;
+  if (!service_properties.GetDictionary(flimflam::kProviderProperty,
+                                        &properties)) {
+    NET_LOG_ERROR("VPN Provider Dictionary not present", service_path);
+    return false;
+  }
   std::string provider_type;
   // Note: we use Value path expansion to extract Provider.Type.
-  properties.GetString(flimflam::kProviderTypeProperty, &provider_type);
+  if (!properties->GetString(flimflam::kTypeProperty, &provider_type)) {
+    NET_LOG_ERROR("VPN Provider Type not present", service_path);
+    return false;
+  }
   if (provider_type == flimflam::kProviderOpenVpn) {
     std::string hostname;
-    properties.GetString(flimflam::kProviderHostProperty, &hostname);
-    if (hostname.empty())
+    properties->GetString(flimflam::kHostProperty, &hostname);
+    if (hostname.empty()) {
+      NET_LOG_EVENT("OpenVPN: No hostname", service_path);
       return false;
+    }
     std::string username;
-    properties.GetStringWithoutPathExpansion(
+    properties->GetStringWithoutPathExpansion(
         flimflam::kOpenVPNUserProperty, &username);
-    if (username.empty())
+    if (username.empty()) {
+      NET_LOG_EVENT("OpenVPN: No username", service_path);
       return false;
+    }
+    bool passphrase_required = false;
+    properties->GetBooleanWithoutPathExpansion(
+        flimflam::kPassphraseRequiredProperty, &passphrase_required);
+    std::string passphrase;
+    properties->GetStringWithoutPathExpansion(
+        flimflam::kOpenVPNPasswordProperty, &passphrase);
+    if (passphrase_required && passphrase.empty()) {
+      NET_LOG_EVENT("OpenVPN: No passphrase", service_path);
+      return false;
+    }
     std::string client_cert_id;
-    properties.GetStringWithoutPathExpansion(
+    properties->GetStringWithoutPathExpansion(
         flimflam::kOpenVPNClientCertIdProperty, &client_cert_id);
-    if (client_cert_id.empty())
+    if (client_cert_id.empty()) {
+      NET_LOG_EVENT("OpenVPN: No cert id", service_path);
       return false;
+    }
+    NET_LOG_EVENT("OpenVPN Is Configured", service_path);
   } else {
     bool passphrase_required = false;
     std::string passphrase;
-    properties.GetBooleanWithoutPathExpansion(
+    properties->GetBooleanWithoutPathExpansion(
         flimflam::kL2tpIpsecPskRequiredProperty, &passphrase_required);
-    properties.GetStringWithoutPathExpansion(
+    properties->GetStringWithoutPathExpansion(
         flimflam::kL2tpIpsecPskProperty, &passphrase);
     if (passphrase_required && passphrase.empty())
       return false;
+    NET_LOG_EVENT("VPN Is Configured", service_path);
   }
   return true;
 }
@@ -124,24 +158,48 @@ const char NetworkConnectionHandler::kErrorCertificateRequired[] =
 const char NetworkConnectionHandler::kErrorConfigurationRequired[] =
     "configuration-required";
 const char NetworkConnectionHandler::kErrorShillError[] = "shill-error";
+const char NetworkConnectionHandler::kErrorConnectFailed[] = "connect-failed";
+const char NetworkConnectionHandler::kErrorUnknown[] = "unknown-error";
+
+struct NetworkConnectionHandler::ConnectRequest {
+  ConnectRequest(const base::Closure& success,
+                 const network_handler::ErrorCallback& error)
+      : connect_state(CONNECT_REQUESTED),
+        success_callback(success),
+        error_callback(error) {
+  }
+  enum ConnectState {
+    CONNECT_REQUESTED = 0,
+    CONNECT_STARTED = 1,
+    CONNECT_CONNECTING = 2
+  };
+  ConnectState connect_state;
+  base::Closure success_callback;
+  network_handler::ErrorCallback error_callback;
+};
 
 NetworkConnectionHandler::NetworkConnectionHandler()
     : network_state_handler_(NULL),
       network_configuration_handler_(NULL) {
-  const char* new_handlers_enabled =
+  const char* new_handler_enabled =
       CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kUseNewNetworkConfigurationHandlers) ?
+          chromeos::switches::kUseNewNetworkConnectionHandler) ?
       "enabled" : "disabled";
-  NET_LOG_EVENT("NewNetworkConfigurationHandlers", new_handlers_enabled);
+  NET_LOG_EVENT("NewNetworkConnectionHandler", new_handler_enabled);
 }
 
 NetworkConnectionHandler::~NetworkConnectionHandler() {
+  if (network_state_handler_)
+    network_state_handler_->RemoveObserver(this);
 }
 
 void NetworkConnectionHandler::Init(
     NetworkStateHandler* network_state_handler,
     NetworkConfigurationHandler* network_configuration_handler) {
-  network_state_handler_ = network_state_handler;
+  if (network_state_handler) {
+    network_state_handler_ = network_state_handler;
+    network_state_handler_->AddObserver(this);
+  }
   network_configuration_handler_ = network_configuration_handler;
 }
 
@@ -157,12 +215,16 @@ void NetworkConnectionHandler::ConnectToNetwork(
     InvokeErrorCallback(service_path, error_callback, kErrorNotFound);
     return;
   }
+  if (HasConnectingNetwork(service_path)) {
+    NET_LOG_USER("Connect Request While Pending", service_path);
+    InvokeErrorCallback(service_path, error_callback, kErrorConnecting);
+    return;
+  }
   if (network->IsConnectedState()) {
     InvokeErrorCallback(service_path, error_callback, kErrorConnected);
     return;
   }
-  if (network->IsConnectingState() ||
-      pending_requests_.find(service_path) != pending_requests_.end()) {
+  if (network->IsConnectingState()) {
     InvokeErrorCallback(service_path, error_callback, kErrorConnecting);
     return;
   }
@@ -194,20 +256,21 @@ void NetworkConnectionHandler::ConnectToNetwork(
   }
 
   // All synchronous checks passed, add |service_path| to connecting list.
-  pending_requests_.insert(service_path);
+  pending_requests_.insert(std::make_pair(
+      service_path, ConnectRequest(success_callback, error_callback)));
 
-  if (!network->connectable() && NetworkMayNeedCredentials(network)) {
+  if (!NetworkConnectable(network) && NetworkMayNeedCredentials(network)) {
     // Request additional properties to check.
     network_configuration_handler_->GetProperties(
         network->path(),
         base::Bind(&NetworkConnectionHandler::VerifyConfiguredAndConnect,
-                   AsWeakPtr(), success_callback, error_callback),
+                   AsWeakPtr()),
         base::Bind(&NetworkConnectionHandler::HandleConfigurationFailure,
-                   AsWeakPtr(), network->path(), error_callback));
+                   AsWeakPtr(), network->path()));
     return;
   }
   // All checks passed, send connect request.
-  CallShillConnect(service_path, success_callback, error_callback);
+  CallShillConnect(service_path);
 }
 
 void NetworkConnectionHandler::DisconnectNetwork(
@@ -228,50 +291,51 @@ void NetworkConnectionHandler::DisconnectNetwork(
   CallShillDisconnect(service_path, success_callback, error_callback);
 }
 
-void NetworkConnectionHandler::CallShillConnect(
-    const std::string& service_path,
-    const base::Closure& success_callback,
-    const network_handler::ErrorCallback& error_callback) {
-  // TODO(stevenjb): Remove SetConnectingNetwork and use this class to maintain
-  // the connecting network(s) once NetworkLibrary path is eliminated.
-  network_state_handler_->SetConnectingNetwork(service_path);
-  NET_LOG_EVENT("Connect Request", service_path);
-  DBusThreadManager::Get()->GetShillServiceClient()->Connect(
-      dbus::ObjectPath(service_path),
-      base::Bind(&NetworkConnectionHandler::HandleShillSuccess,
-                 AsWeakPtr(), service_path, success_callback),
-      base::Bind(&NetworkConnectionHandler::HandleShillFailure,
-                 AsWeakPtr(), service_path, error_callback));
+bool NetworkConnectionHandler::HasConnectingNetwork(
+    const std::string& service_path) {
+  return pending_requests_.count(service_path) != 0;
 }
 
-void NetworkConnectionHandler::CallShillDisconnect(
-    const std::string& service_path,
-    const base::Closure& success_callback,
-    const network_handler::ErrorCallback& error_callback) {
-  NET_LOG_EVENT("Disconnect Request", service_path);
-  DBusThreadManager::Get()->GetShillServiceClient()->Disconnect(
-      dbus::ObjectPath(service_path),
-      base::Bind(&NetworkConnectionHandler::HandleShillSuccess,
-                 AsWeakPtr(), service_path, success_callback),
-      base::Bind(&NetworkConnectionHandler::HandleShillFailure,
-                 AsWeakPtr(), service_path, error_callback));
+void NetworkConnectionHandler::NetworkListChanged() {
+  CheckAllPendingRequests();
+}
+
+void NetworkConnectionHandler::NetworkPropertiesUpdated(
+    const NetworkState* network) {
+  if (HasConnectingNetwork(network->path()))
+    CheckPendingRequest(network->path());
+}
+
+NetworkConnectionHandler::ConnectRequest*
+NetworkConnectionHandler::pending_request(
+    const std::string& service_path) {
+  std::map<std::string, ConnectRequest>::iterator iter =
+      pending_requests_.find(service_path);
+  return iter != pending_requests_.end() ? &(iter->second) : NULL;
 }
 
+// ConnectToNetwork implementation
+
 void NetworkConnectionHandler::VerifyConfiguredAndConnect(
-    const base::Closure& success_callback,
-    const network_handler::ErrorCallback& error_callback,
     const std::string& service_path,
     const base::DictionaryValue& properties) {
+  NET_LOG_EVENT("VerifyConfiguredAndConnect", service_path);
+  ConnectRequest* request = pending_request(service_path);
+  DCHECK(request);
+  network_handler::ErrorCallback error_callback = request->error_callback;
+
   const NetworkState* network =
       network_state_handler_->GetNetworkState(service_path);
   if (!network) {
+    pending_requests_.erase(service_path);
     InvokeErrorCallback(service_path, error_callback, kErrorNotFound);
     return;
   }
 
   // VPN requires a host and username to be set.
   if (network->type() == flimflam::kTypeVPN &&
-      !VPNIsConfigured(properties)) {
+      !VPNIsConfigured(service_path, properties)) {
+    pending_requests_.erase(service_path);
     InvokeErrorCallback(service_path, error_callback,
                         kErrorConfigurationRequired);
     return;
@@ -281,44 +345,152 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
   scoped_ptr<NetworkUIData> ui_data =
       ManagedNetworkConfigurationHandler::GetUIData(properties);
   if (ui_data && !CertificateIsConfigured(ui_data.get())) {
+    pending_requests_.erase(service_path);
     InvokeErrorCallback(service_path, error_callback,
                         kErrorCertificateRequired);
     return;
   }
 
-  CallShillConnect(service_path, success_callback, error_callback);
+  CallShillConnect(service_path);
+}
+
+void NetworkConnectionHandler::CallShillConnect(
+    const std::string& service_path) {
+  NET_LOG_EVENT("Connect Request", service_path);
+  DBusThreadManager::Get()->GetShillServiceClient()->Connect(
+      dbus::ObjectPath(service_path),
+      base::Bind(&NetworkConnectionHandler::HandleShillConnectSuccess,
+                 AsWeakPtr(), service_path),
+      base::Bind(&NetworkConnectionHandler::HandleShillConnectFailure,
+                 AsWeakPtr(), service_path));
 }
 
 void NetworkConnectionHandler::HandleConfigurationFailure(
     const std::string& service_path,
-    const network_handler::ErrorCallback& error_callback,
     const std::string& error_name,
     scoped_ptr<base::DictionaryValue> error_data) {
+  ConnectRequest* request = pending_request(service_path);
+  DCHECK(request);
+  network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
   if (!error_callback.is_null())
     error_callback.Run(error_name, error_data.Pass());
 }
 
-void NetworkConnectionHandler::HandleShillSuccess(
+void NetworkConnectionHandler::HandleShillConnectSuccess(
+    const std::string& service_path) {
+  ConnectRequest* request = pending_request(service_path);
+  DCHECK(request);
+  request->connect_state = ConnectRequest::CONNECT_STARTED;
+  NET_LOG_EVENT("Connect Request Sent", service_path);
+  // Do not call success_callback here, wait for one of the following
+  // conditions:
+  // * State transitions to a non connecting state indicating succes or failure
+  // * Network is no longer in the visible list, indicating failure
+  CheckPendingRequest(service_path);
+}
+
+void NetworkConnectionHandler::HandleShillConnectFailure(
     const std::string& service_path,
-    const base::Closure& success_callback) {
-  // TODO(stevenjb): Currently, this only indicates that the connect request
-  // succeeded. It might be preferable to wait for the actually connect
-  // attempt to succeed or fail here and only call |success_callback| at that
-  // point (or maybe call it twice, once indicating in-progress, then success
-  // or failure).
+    const std::string& error_name,
+    const std::string& error_message) {
+  ConnectRequest* request = pending_request(service_path);
+  DCHECK(request);
+  network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
-  NET_LOG_EVENT("Connect Request Sent", service_path);
+  std::string error = "Connect Failure: " + error_name + ": " + error_message;
+  network_handler::ShillErrorCallbackFunction(
+      service_path, error_callback, error_name, error_message);
+}
+
+void NetworkConnectionHandler::CheckPendingRequest(
+    const std::string service_path) {
+  ConnectRequest* request = pending_request(service_path);
+  DCHECK(request);
+  if (request->connect_state == ConnectRequest::CONNECT_REQUESTED)
+    return;  // Request has not started, ignore update
+  const NetworkState* network =
+      network_state_handler_->GetNetworkState(service_path);
+  if (!network) {
+    network_handler::ErrorCallback error_callback = request->error_callback;
+    pending_requests_.erase(service_path);
+    InvokeErrorCallback(service_path, error_callback, kErrorNotFound);
+    return;
+  }
+  if (network->IsConnectingState()) {
+    request->connect_state = ConnectRequest::CONNECT_CONNECTING;
+    return;
+  }
+  if (network->IsConnectedState()) {
+    NET_LOG_EVENT("Connect Request Succeeded", service_path);
+    if (!request->success_callback.is_null())
+      request->success_callback.Run();
+    pending_requests_.erase(service_path);
+    return;
+  }
+  if (network->connection_state() == flimflam::kStateIdle &&
+      request->connect_state != ConnectRequest::CONNECT_CONNECTING) {
+    // Connection hasn't started yet, keep waiting.
+    return;
+  }
+
+  // Network is neither connecting or connected; an error occurred.
+  std::string error_name = kErrorConnectFailed;
+  std::string error_detail = network->error();
+  if (error_detail.empty()) {
+    if (network->connection_state() == flimflam::kStateFailure)
+      error_detail = flimflam::kUnknownString;
+    else
+      error_detail = "Unexpected State: " + network->connection_state();
+  }
+  std::string error_msg = error_name + ": " + error_detail;
+  NET_LOG_ERROR(error_msg, service_path);
+
+  network_handler::ErrorCallback error_callback = request->error_callback;
+  pending_requests_.erase(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());
+}
+
+void NetworkConnectionHandler::CheckAllPendingRequests() {
+  for (std::map<std::string, ConnectRequest>::iterator iter =
+           pending_requests_.begin(); iter != pending_requests_.end(); ++iter) {
+    CheckPendingRequest(iter->first);
+  }
+}
+
+// Disconnect
+
+void NetworkConnectionHandler::CallShillDisconnect(
+    const std::string& service_path,
+    const base::Closure& success_callback,
+    const network_handler::ErrorCallback& error_callback) {
+  NET_LOG_USER("Disconnect Request", service_path);
+  DBusThreadManager::Get()->GetShillServiceClient()->Disconnect(
+      dbus::ObjectPath(service_path),
+      base::Bind(&NetworkConnectionHandler::HandleShillDisconnectSuccess,
+                 AsWeakPtr(), service_path, success_callback),
+      base::Bind(&NetworkConnectionHandler::HandleShillDisconnectFailure,
+                 AsWeakPtr(), service_path, error_callback));
+}
+
+void NetworkConnectionHandler::HandleShillDisconnectSuccess(
+    const std::string& service_path,
+    const base::Closure& success_callback) {
+  NET_LOG_EVENT("Disconnect Request Sent", service_path);
   success_callback.Run();
 }
 
-void NetworkConnectionHandler::HandleShillFailure(
+void NetworkConnectionHandler::HandleShillDisconnectFailure(
     const std::string& service_path,
     const network_handler::ErrorCallback& error_callback,
     const std::string& error_name,
     const std::string& error_message) {
-  pending_requests_.erase(service_path);
-  std::string error = "Connect Failure: " + error_name + ": " + error_message;
+  std::string error =
+      "Disconnect Failure: " + error_name + ": " + error_message;
   network_handler::ShillErrorCallbackFunction(
       service_path, error_callback, error_name, error_message);
 }
diff --git a/chromeos/network/network_connection_handler.h b/chromeos/network/network_connection_handler.h
index a4d233de..42c2bdf 100644
--- a/chromeos/network/network_connection_handler.h
+++ b/chromeos/network/network_connection_handler.h
@@ -16,6 +16,7 @@
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_handler_callbacks.h"
+#include "chromeos/network/network_state_handler_observer.h"
 
 namespace chromeos {
 
@@ -29,28 +30,33 @@ class NetworkState;
 // 2. Request additional information (e.g. user data which contains certificate
 //    information) and determine whether sufficient information is available.
 // 3. Send the connect request.
-// 4. Invoke the appropriate callback (always) on success or failure.
+// 4. Wait for the network state to change to a non connecting state.
+// 5. Invoke the appropriate callback (always) on success or failure.
 //
 // NetworkConnectionHandler depends on NetworkStateHandler for immediately
 // available State information, and NetworkConfigurationHandler for any
 // configuration calls.
 
 class CHROMEOS_EXPORT NetworkConnectionHandler
-    : public base::SupportsWeakPtr<NetworkConnectionHandler> {
+    : public NetworkStateHandlerObserver,
+      public base::SupportsWeakPtr<NetworkConnectionHandler> {
  public:
-  // Constants for |error_name| from |error_callback| for Connect/Disconnect.
+  // Constants for |error_name| from |error_callback| for Connect.
   static const char kErrorNotFound[];
   static const char kErrorConnected[];
   static const char kErrorConnecting[];
-  static const char kErrorNotConnected[];
   static const char kErrorPassphraseRequired[];
   static const char kErrorActivationRequired[];
   static const char kErrorCertificateRequired[];
   static const char kErrorConfigurationRequired[];
   static const char kErrorShillError[];
-  static const char kErrorPreviousConnectFailed[];
+  static const char kErrorConnectFailed[];
+  static const char kErrorUnknown[];
+
+  // Constants for |error_name| from |error_callback| for Disconnect.
+  static const char kErrorNotConnected[];
 
-  ~NetworkConnectionHandler();
+  virtual ~NetworkConnectionHandler();
 
   // ConnectToNetwork() will start an asynchronous connection attempt.
   // On success, |success_callback| will be called.
@@ -82,63 +88,75 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
                          const base::Closure& success_callback,
                          const network_handler::ErrorCallback& error_callback);
 
+  // Returns true if ConnectToNetwork has been called with |service_path| and
+  // has not completed (i.e. success or error callback has been called).
+  bool HasConnectingNetwork(const std::string& service_path);
+
+  // NetworkStateHandlerObserver
+  virtual void NetworkListChanged() OVERRIDE;
+  virtual void NetworkPropertiesUpdated(const NetworkState* network) OVERRIDE;
+
  private:
   friend class NetworkHandler;
   friend class NetworkConnectionHandlerTest;
+
+  struct ConnectRequest;
+
   NetworkConnectionHandler();
 
   void Init(NetworkStateHandler* network_state_handler,
             NetworkConfigurationHandler* network_configuration_handler);
 
-  // Calls Shill.Manager.Connect asynchronously.
-  void CallShillConnect(
-      const std::string& service_path,
-      const base::Closure& success_callback,
-      const network_handler::ErrorCallback& error_callback);
-
-  // Calls Shill.Manager.Disconnect asynchronously.
-  void CallShillDisconnect(
-      const std::string& service_path,
-      const base::Closure& success_callback,
-      const network_handler::ErrorCallback& error_callback);
+  ConnectRequest* pending_request(const std::string& service_path);
 
   // Callback from Shill.Service.GetProperties. Parses |properties| to verify
   // whether or not the network appears to be configured. If configured,
-  // attempts a connection, otherwise invokes |error_callback|.
-  void VerifyConfiguredAndConnect(
-      const base::Closure& success_callback,
-      const network_handler::ErrorCallback& error_callback,
-      const std::string& service_path,
-      const base::DictionaryValue& properties);
+  // attempts a connection, otherwise invokes error_callback from
+  // pending_requests_[service_path].
+  void VerifyConfiguredAndConnect(const std::string& service_path,
+                                  const base::DictionaryValue& properties);
 
-  // Sets the property for the service with an empty callback (logs errors).
-  void SetServiceProperty(const std::string& service_path,
-                          const std::string& property,
-                          const std::string& value) const;
+  // Calls Shill.Manager.Connect asynchronously.
+  void CallShillConnect(const std::string& service_path);
 
   // Handle failure from ConfigurationHandler calls.
   void HandleConfigurationFailure(
       const std::string& service_path,
-      const network_handler::ErrorCallback& error_callback,
       const std::string& error_name,
       scoped_ptr<base::DictionaryValue> error_data);
 
   // Handle success or failure from Shill.Service.Connect.
-  void HandleShillSuccess(const std::string& service_path,
-                          const base::Closure& success_callback);
-  void HandleShillFailure(const std::string& service_path,
-                          const network_handler::ErrorCallback& error_callback,
-                          const std::string& error_name,
-                          const std::string& error_message);
+  void HandleShillConnectSuccess(const std::string& service_path);
+  void HandleShillConnectFailure(const std::string& service_path,
+                                 const std::string& error_name,
+                                 const std::string& error_message);
+
+  void CheckPendingRequest(const std::string service_path);
+  void CheckAllPendingRequests();
+
+  // Calls Shill.Manager.Disconnect asynchronously.
+  void CallShillDisconnect(
+      const std::string& service_path,
+      const base::Closure& success_callback,
+      const network_handler::ErrorCallback& error_callback);
+
+  // Handle success or failure from Shill.Service.Disconnect.
+  void HandleShillDisconnectSuccess(const std::string& service_path,
+                                    const base::Closure& success_callback);
+  void HandleShillDisconnectFailure(
+      const std::string& service_path,
+      const network_handler::ErrorCallback& error_callback,
+      const std::string& error_name,
+      const std::string& error_message);
 
   // Local references to the associated handler instances.
   NetworkStateHandler* network_state_handler_;
   NetworkProfileHandler* network_profile_handler_;
   NetworkConfigurationHandler* network_configuration_handler_;
 
-  // Set of pending connect requests, used to prevent repeat attempts while
-  // waiting for Shill.
-  std::set<std::string> pending_requests_;
+  // Map of pending connect requests, used to prevent repeat attempts while
+  // waiting for Shill and to trigger callbacks on eventual success or failure.
+  std::map<std::string, ConnectRequest> pending_requests_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkConnectionHandler);
 };