NetworkConnectionHandler::ConnectToNetwork: support immediate succeed

This adds a parameter to ConnectToNetwork 'succeed_on_send_connect'.
If set, ConnectToNetwork will call the 'success' callback as soon as
a connect request is sent to Shill (if there were no configuration
errors) instead of after the connect succeeds.

This is used by the networkingPrivate implementation to more correctly
implement the API.

This CL causes the Settings based network config UI to close its dialog
when the connection starts.

Bug: 795715
For minor changes to comonents/arc/net:
TBR=hidehiko@chromium.org

Change-Id: I5de65a32a46d57dc628c65542a275505d4d7c944
Reviewed-on: https://chromium-review.googlesource.com/875002Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530661}

chrome/browser/chromeos/login/helper.cc

@@ -169,7 +169,8 @@ void NetworkStateHelper::OnCreateConfiguration(
     const std::string& guid) const {
   // Connect to the network.
   NetworkHandler::Get()->network_connection_handler()->ConnectToNetwork(
-      service_path, success_callback, error_callback, false);
+      service_path, success_callback, error_callback,
+      false /* check_error_state */, ConnectCallbackMode::ON_COMPLETED);
 }
 
 content::StoragePartition* GetSigninPartition() {

chrome/browser/chromeos/mobile/mobile_activator.cc

@@ -560,10 +560,9 @@ void MobileActivator::HandleOTASPTimeout() {
 
 void MobileActivator::ConnectNetwork(const NetworkState* network) {
   NetworkHandler::Get()->network_connection_handler()->ConnectToNetwork(
-      network->path(),
-      base::Bind(&base::DoNothing),
-      network_handler::ErrorCallback(),
-      false /* check_error_state */);
+      network->path(), base::Bind(&base::DoNothing),
+      network_handler::ErrorCallback(), false /* check_error_state */,
+      ConnectCallbackMode::ON_STARTED);
 }
 
 void MobileActivator::ForceReconnect(const NetworkState* network,

chrome/browser/ui/ash/auto_connect_notifier_unittest.cc

@@ -61,7 +61,8 @@ class TestNetworkConnectionHandler : public chromeos::NetworkConnectionHandler {
       const std::string& service_path,
       const base::Closure& success_callback,
       const chromeos::network_handler::ErrorCallback& error_callback,
-      bool check_error_state) override {}
+      bool check_error_state,
+      chromeos::ConnectCallbackMode mode) override {}
 
   bool HasConnectingNetwork(const std::string& service_path) override {
     return false;

chromeos/components/tether/network_connection_handler_tether_delegate_unittest.cc

@@ -69,7 +69,8 @@ class TestNetworkConnectionHandler : public NetworkConnectionHandler {
   void ConnectToNetwork(const std::string& service_path,
                         const base::Closure& success_callback,
                         const network_handler::ErrorCallback& error_callback,
-                        bool check_error_state) override {}
+                        bool check_error_state,
+                        ConnectCallbackMode mode) override {}
 
   void DisconnectNetwork(
       const std::string& service_path,

chromeos/components/tether/wifi_hotspot_disconnector_impl_unittest.cc

@@ -73,7 +73,8 @@ class TestNetworkConnectionHandler : public NetworkConnectionHandler {
   void ConnectToNetwork(const std::string& service_path,
                         const base::Closure& success_callback,
                         const network_handler::ErrorCallback& error_callback,
-                        bool check_error_state) override {}
+                        bool check_error_state,
+                        ConnectCallbackMode mode) override {}
   bool HasConnectingNetwork(const std::string& service_path) override {
     return false;
   }

chromeos/network/auto_connect_handler_unittest.cc

@@ -95,7 +95,8 @@ class TestNetworkConnectionHandler : public NetworkConnectionHandler {
   void ConnectToNetwork(const std::string& service_path,
                         const base::Closure& success_callback,
                         const network_handler::ErrorCallback& error_callback,
-                        bool check_error_state) override {}
+                        bool check_error_state,
+                        ConnectCallbackMode mode) override {}
 
   bool HasConnectingNetwork(const std::string& service_path) override {
     return false;

chromeos/network/network_connect.cc

@@ -254,7 +254,7 @@ void NetworkConnectImpl::CallConnectToNetwork(const std::string& network_id,
                  weak_factory_.GetWeakPtr(), network_id),
       base::Bind(&NetworkConnectImpl::OnConnectFailed,
                  weak_factory_.GetWeakPtr(), network_id),
-      check_error_state);
+      check_error_state, ConnectCallbackMode::ON_COMPLETED);
 }
 
 void NetworkConnectImpl::OnActivateFailed(

chromeos/network/network_connection_handler.h

@@ -37,6 +37,8 @@ namespace chromeos {
 // available State information, and NetworkConfigurationHandler for any
 // configuration calls.
 
+enum class ConnectCallbackMode { ON_STARTED, ON_COMPLETED };
+
 class CHROMEOS_EXPORT NetworkConnectionHandler {
  public:
   // Constants for |error_name| from |error_callback| for Connect.
@@ -86,7 +88,7 @@ class CHROMEOS_EXPORT NetworkConnectionHandler {
   // Certificate load timed out.
   static const char kErrorCertLoadTimeout[];
 
-  // Trying to configure an unmanged network but policy prohibits that
+  // Trying to configure an unmanaged network but policy prohibits that
   static const char kErrorUnmanagedNetwork[];
 
   // Network activation failed.
@@ -131,18 +133,25 @@ class CHROMEOS_EXPORT NetworkConnectionHandler {
   void SetTetherDelegate(TetherDelegate* tether_delegate);
 
   // 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.
+  // |success_callback| will be called if the connection request succeeds
+  //   or if a request is sent if |mode| is ON_STARTED (see below).
+  // |error_callback| will be called with |error_name| set to one of the
+  //   constants defined above if the connection request fails.
   // |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
-  //  configured networks or repeat attempts).
+  //   checked for errors, otherwise current state is ignored (e.g. for recently
+  //   configured networks or repeat attempts).
+  // If |mode| is ON_STARTED, |success_callback| will be invoked when the
+  //   connect request is successfully made and not when the connection
+  //   completes. Note: This also prevents |error_callback| from being called
+  //   if the connection request is successfully sent but the network does not
+  //   connect.
   virtual void ConnectToNetwork(
       const std::string& service_path,
       const base::Closure& success_callback,
       const network_handler::ErrorCallback& error_callback,
-      bool check_error_state) = 0;
+      bool check_error_state,
+      ConnectCallbackMode mode) = 0;
 
   // DisconnectNetwork() will send a Disconnect request to Shill.
   // On success, |success_callback| will be called.

chromeos/network/network_connection_handler_impl.cc

@@ -95,11 +95,13 @@ std::string GetDefaultUserProfilePath(const NetworkState* network) {
 }  // namespace
 
 NetworkConnectionHandlerImpl::ConnectRequest::ConnectRequest(
+    ConnectCallbackMode mode,
     const std::string& service_path,
     const std::string& profile_path,
     const base::Closure& success,
     const network_handler::ErrorCallback& error)
-    : service_path(service_path),
+    : mode(mode),
+      service_path(service_path),
       profile_path(profile_path),
       connect_state(CONNECT_REQUESTED),
       success_callback(success),
@@ -181,7 +183,8 @@ void NetworkConnectionHandlerImpl::ConnectToNetwork(
     const std::string& service_path,
     const base::Closure& success_callback,
     const network_handler::ErrorCallback& error_callback,
-    bool check_error_state) {
+    bool check_error_state,
+    ConnectCallbackMode mode) {
   NET_LOG_USER("ConnectToNetwork", service_path);
   for (auto& observer : observers_)
     observer.ConnectToNetworkRequested(service_path);
@@ -248,20 +251,26 @@ void NetworkConnectionHandlerImpl::ConnectToNetwork(
   if (!network || network->profile_path().empty())
     profile_path = GetDefaultUserProfilePath(network);
 
-  // All synchronous checks passed, add |service_path| to connecting list.
-  pending_requests_.emplace(
-      service_path, ConnectRequest(service_path, profile_path, success_callback,
-                                   error_callback));
+  bool call_connect = false;
 
   // Connect immediately to 'connectable' networks.
   // TODO(stevenjb): Shill needs to properly set Connectable for VPN.
   if (network && network->connectable() && network->type() != shill::kTypeVPN) {
     if (IsNetworkProhibitedByPolicy(network->type(), network->guid(),
                                     network->profile_path())) {
-      ErrorCallbackForPendingRequest(service_path, kErrorUnmanagedNetwork);
+      InvokeConnectErrorCallback(service_path, error_callback,
+                                 kErrorUnmanagedNetwork);
       return;
     }
+    call_connect = true;
+  }
 
+  // All synchronous checks passed, add |service_path| to connecting list.
+  pending_requests_.emplace(service_path,
+                            ConnectRequest(mode, service_path, profile_path,
+                                           success_callback, error_callback));
+
+  if (call_connect) {
     CallShillConnect(service_path);
     return;
   }
@@ -565,9 +574,9 @@ void NetworkConnectionHandlerImpl::QueueConnectRequest(
   }
 
   NET_LOG_EVENT("Connect Request Queued", service_path);
-  queued_connect_.reset(new ConnectRequest(service_path, request->profile_path,
-                                           request->success_callback,
-                                           request->error_callback));
+  queued_connect_.reset(
+      new ConnectRequest(request->mode, service_path, request->profile_path,
+                         request->success_callback, request->error_callback));
   pending_requests_.erase(service_path);
 
   // Post a delayed task to check to see if certificates have loaded. If they
@@ -607,7 +616,7 @@ void NetworkConnectionHandlerImpl::ConnectToQueuedNetwork() {
 
   NET_LOG_EVENT("Connecting to Queued Network", service_path);
   ConnectToNetwork(service_path, success_callback, error_callback,
-                   false /* check_error_state */);
+                   false /* check_error_state */, queued_connect_->mode);
 }
 
 void NetworkConnectionHandlerImpl::CallShillConnect(
@@ -626,6 +635,7 @@ void NetworkConnectionHandlerImpl::HandleConfigurationFailure(
     const std::string& service_path,
     const std::string& error_name,
     std::unique_ptr<base::DictionaryValue> error_data) {
+  NET_LOG(ERROR) << "Connect configuration failure: " << error_name;
   ConnectRequest* request = GetPendingRequest(service_path);
   if (!request) {
     NET_LOG_ERROR("HandleConfigurationFailure called with no pending request.",
@@ -646,6 +656,14 @@ void NetworkConnectionHandlerImpl::HandleShillConnectSuccess(
                   service_path);
     return;
   }
+  if (request->mode == ConnectCallbackMode::ON_STARTED) {
+    if (!request->success_callback.is_null())
+      request->success_callback.Run();
+    // Request started; do not invoke success or error callbacks on
+    // completion.
+    request->success_callback = base::Closure();
+    request->error_callback = network_handler::ErrorCallback();
+  }
   request->connect_state = ConnectRequest::CONNECT_STARTED;
   NET_LOG_EVENT("Connect Request Acknowledged", service_path);
   // Do not call success_callback here, wait for one of the following

chromeos/network/network_connection_handler_impl.h

@@ -29,7 +29,8 @@ class CHROMEOS_EXPORT NetworkConnectionHandlerImpl
   void ConnectToNetwork(const std::string& service_path,
                         const base::Closure& success_callback,
                         const network_handler::ErrorCallback& error_callback,
-                        bool check_error_state) override;
+                        bool check_error_state,
+                        ConnectCallbackMode mode) override;
   void DisconnectNetwork(
       const std::string& service_path,
       const base::Closure& success_callback,
@@ -56,7 +57,8 @@ class CHROMEOS_EXPORT NetworkConnectionHandlerImpl
 
  private:
   struct ConnectRequest {
-    ConnectRequest(const std::string& service_path,
+    ConnectRequest(ConnectCallbackMode mode,
+                   const std::string& service_path,
                    const std::string& profile_path,
                    const base::Closure& success,
                    const network_handler::ErrorCallback& error);
@@ -69,6 +71,7 @@ class CHROMEOS_EXPORT NetworkConnectionHandlerImpl
       CONNECT_CONNECTING = 2
     };
 
+    ConnectCallbackMode mode;
     std::string service_path;
     std::string profile_path;
     ConnectState connect_state;

chromeos/network/network_connection_handler_impl_unittest.cc

@@ -206,14 +206,13 @@ class NetworkConnectionHandlerImplTest : public NetworkStateTest {
 
  protected:
   void Connect(const std::string& service_path) {
-    const bool check_error_state = true;
     network_connection_handler_->ConnectToNetwork(
         service_path,
         base::Bind(&NetworkConnectionHandlerImplTest::SuccessCallback,
                    base::Unretained(this)),
         base::Bind(&NetworkConnectionHandlerImplTest::ErrorCallback,
                    base::Unretained(this)),
-        check_error_state);
+        true /* check_error_state */, ConnectCallbackMode::ON_COMPLETED);
     scoped_task_environment_.RunUntilIdle();
   }
 

components/arc/net/arc_net_host_impl.cc

@@ -638,7 +638,8 @@ void ArcNetHostImpl::StartConnect(const std::string& guid,
       base::AdaptCallbackForRepeating(std::move(callback));
   GetNetworkConnectionHandler()->ConnectToNetwork(
       path, base::Bind(&StartConnectSuccessCallback, repeating_callback),
-      base::Bind(&StartConnectFailureCallback, repeating_callback), false);
+      base::Bind(&StartConnectFailureCallback, repeating_callback),
+      false /* check_error_state */, chromeos::ConnectCallbackMode::ON_STARTED);
 }
 
 void ArcNetHostImpl::StartDisconnect(const std::string& guid,
@@ -806,7 +807,8 @@ void ArcNetHostImpl::ConnectArcVpn(const std::string& service_path,
 
   GetNetworkConnectionHandler()->ConnectToNetwork(
       service_path, base::Bind(&ArcVpnSuccessCallback),
-      base::Bind(&ArcVpnErrorCallback), false /* check_error_state */);
+      base::Bind(&ArcVpnErrorCallback), false /* check_error_state */,
+      chromeos::ConnectCallbackMode::ON_COMPLETED);
 }
 
 std::unique_ptr<base::Value> ArcNetHostImpl::TranslateStringListToValue(

extensions/browser/api/networking_private/networking_private_chromeos.cc

@@ -550,7 +550,7 @@ void NetworkingPrivateChromeOS::StartConnect(
   NetworkHandler::Get()->network_connection_handler()->ConnectToNetwork(
       service_path, success_callback,
       base::Bind(&NetworkHandlerFailureCallback, failure_callback),
-      check_error_state);
+      check_error_state, chromeos::ConnectCallbackMode::ON_STARTED);
 }
 
 void NetworkingPrivateChromeOS::StartDisconnect(

extensions/common/api/networking_onc.idl

@@ -997,7 +997,9 @@ namespace networking.onc {
     // |networkGuid|: The GUID of the network to connect to.
     // |callback|: Called when the connect request has been sent. Note: the
     //     connection may not have completed. Observe $(ref:onNetworksChanged)
-    //     to be notified when a network state changes.
+    //     to be notified when a network state changes. If the connect request
+    //     immediately failed (e.g. the network is unconfigured),
+    //     $(ref:runtime.lastError) will be set with a failure reason.
     static void startConnect(DOMString networkGuid,
                              optional VoidCallback callback);
 

extensions/common/api/networking_private.idl

@@ -904,7 +904,7 @@ namespace networkingPrivate {
   callback GetGlobalPolicyCallback = void(GlobalPolicy result);
   callback GetCertificateListsCallback = void(CertificateLists result);
 
-  // These functions all report failures via chrome.runtime.lastError.
+  // These functions all report failures via $(ref:runtime.lastError).
   interface Functions {
     // Gets all the properties of the network with id networkGuid. Includes all
     // properties of the network (read-only and read/write values).
@@ -1012,7 +1012,9 @@ namespace networkingPrivate {
     // |networkGuid|: The GUID of the network to connect to.
     // |callback|: Called when the connect request has been sent. Note: the
     //     connection may not have completed. Observe $(ref:onNetworksChanged)
-    //     to be notified when a network state changes.
+    //     to be notified when a network state changes. If the connect request
+    //     immediately failed (e.g. the network is unconfigured),
+    //     $(ref:runtime.lastError) will be set with a failure reason.
     static void startConnect(DOMString networkGuid,
                              optional VoidCallback callback);
 
@@ -1120,7 +1122,7 @@ namespace networkingPrivate {
     // the operation will fail. This will not lock the SIM; that is handled
     // automatically by the device. NOTE: If the SIM is locked, it must first be
     // unlocked with unlockCellularSim() before this can be called (otherwise it
-    // will fail and chrome.runtime.lastError will be set to Error.SimLocked).
+    // will fail and $(ref:runtime.lastError) will be set to Error.SimLocked).
     // |networkGuid|: The GUID of the cellular network to set the SIM state of.
     //     If empty, the default cellular device will be used.
     // |simState|: The SIM state to set.

extensions/shell/browser/shell_network_controller_chromeos.cc

@@ -199,7 +199,8 @@ void ShellNetworkController::ConnectIfUnconnected() {
                  weak_ptr_factory_.GetWeakPtr()),
       base::Bind(&ShellNetworkController::HandleConnectionError,
                  weak_ptr_factory_.GetWeakPtr()),
-      false /* check_error_state */);
+      false /* check_error_state */,
+      chromeos::ConnectCallbackMode::ON_COMPLETED);
 }
 
 void ShellNetworkController::HandleConnectionSuccess() {