[CrOS Tether] Transform NetworkConnectionHandler to an interface.

The implementation is now part of a class private to network_connection_handler.cc.

This is in preparation for using a test double for NetworkConnectionHandler.

BUG=672263

Review-Url: https://codereview.chromium.org/2861883002
Cr-Commit-Position: refs/heads/master@{#469229}

chromeos/BUILD.gn

@@ -339,6 +339,8 @@ component("chromeos") {
     "network/network_connect.h",
     "network/network_connection_handler.cc",
     "network/network_connection_handler.h",
+    "network/network_connection_handler_impl.cc",
+    "network/network_connection_handler_impl.h",
     "network/network_connection_observer.cc",
     "network/network_connection_observer.h",
     "network/network_device_handler.cc",
@@ -657,7 +659,7 @@ test("chromeos_unittests") {
     "network/network_change_notifier_chromeos_unittest.cc",
     "network/network_configuration_handler_unittest.cc",
     "network/network_connect_unittest.cc",
-    "network/network_connection_handler_unittest.cc",
+    "network/network_connection_handler_impl_unittest.cc",
     "network/network_device_handler_unittest.cc",
     "network/network_sms_handler_unittest.cc",
     "network/network_state_handler_unittest.cc",

chromeos/components/tether/initializer_unittest.cc

@@ -89,6 +89,28 @@ class TestNetworkConnectionHandler : public NetworkConnectionHandler {
  public:
   TestNetworkConnectionHandler() : NetworkConnectionHandler() {}
   ~TestNetworkConnectionHandler() override {}
+
+  // NetworkConnectionHandler:
+  void ConnectToNetwork(const std::string& service_path,
+                        const base::Closure& success_callback,
+                        const network_handler::ErrorCallback& error_callback,
+                        bool check_error_state) override {}
+
+  void DisconnectNetwork(
+      const std::string& service_path,
+      const base::Closure& success_callback,
+      const network_handler::ErrorCallback& error_callback) override {}
+
+  bool HasConnectingNetwork(const std::string& service_path) override {
+    return false;
+  }
+
+  bool HasPendingConnectRequest() override { return false; }
+
+  void Init(NetworkStateHandler* network_state_handler,
+            NetworkConfigurationHandler* network_configuration_handler,
+            ManagedNetworkConfigurationHandler*
+                managed_network_configuration_handler) override {}
 };
 
 }  // namespace

chromeos/components/tether/tether_connector_unittest.cc

@@ -60,6 +60,28 @@ class TestNetworkConnectionHandler : public NetworkConnectionHandler {
     InitiateTetherNetworkConnection(tether_network_guid, success_callback,
                                     error_callback);
   }
+
+  // NetworkConnectionHandler:
+  void ConnectToNetwork(const std::string& service_path,
+                        const base::Closure& success_callback,
+                        const network_handler::ErrorCallback& error_callback,
+                        bool check_error_state) override {}
+
+  void DisconnectNetwork(
+      const std::string& service_path,
+      const base::Closure& success_callback,
+      const network_handler::ErrorCallback& error_callback) override {}
+
+  bool HasConnectingNetwork(const std::string& service_path) override {
+    return false;
+  }
+
+  bool HasPendingConnectRequest() override { return false; }
+
+  void Init(NetworkStateHandler* network_state_handler,
+            NetworkConfigurationHandler* network_configuration_handler,
+            ManagedNetworkConfigurationHandler*
+                managed_network_configuration_handler) override {}
 };
 
 class FakeConnectTetheringOperation : public ConnectTetheringOperation {

chromeos/network/network_connection_handler.h

@@ -13,21 +13,14 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
-#include "base/time/time.h"
 #include "base/values.h"
-#include "chromeos/cert_loader.h"
 #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"
 
 namespace chromeos {
 
-class NetworkState;
-
 // The NetworkConnectionHandler class is used to manage network connection
 // requests. This is the only class that should make Shill Connect calls.
 // It handles the following steps:
@@ -44,11 +37,7 @@ class NetworkState;
 // available State information, and NetworkConfigurationHandler for any
 // configuration calls.
 
-class CHROMEOS_EXPORT NetworkConnectionHandler
-    : public LoginState::Observer,
-      public CertLoader::Observer,
-      public NetworkStateHandlerObserver,
-      public base::SupportsWeakPtr<NetworkConnectionHandler> {
+class CHROMEOS_EXPORT NetworkConnectionHandler {
  public:
   // Constants for |error_name| from |error_callback| for Connect.
 
@@ -123,11 +112,15 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
     virtual ~TetherDelegate() {}
   };
 
-  ~NetworkConnectionHandler() override;
+  virtual ~NetworkConnectionHandler();
 
   void AddObserver(NetworkConnectionObserver* observer);
   void RemoveObserver(NetworkConnectionObserver* observer);
 
+  // Sets the TetherDelegate to handle Tether actions. |tether_delegate| is
+  // owned by the caller.
+  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
@@ -136,10 +129,11 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   // 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).
-  void ConnectToNetwork(const std::string& service_path,
-                        const base::Closure& success_callback,
-                        const network_handler::ErrorCallback& error_callback,
-                        bool check_error_state);
+  virtual void ConnectToNetwork(
+      const std::string& service_path,
+      const base::Closure& success_callback,
+      const network_handler::ErrorCallback& error_callback,
+      bool check_error_state) = 0;
 
   // DisconnectNetwork() will send a Disconnect request to Shill.
   // On success, |success_callback| will be called.
@@ -148,108 +142,30 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   //  kErrorNotConnected if not connected to the network.
   //  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,
-                         const network_handler::ErrorCallback& error_callback);
+  virtual void DisconnectNetwork(
+      const std::string& service_path,
+      const base::Closure& success_callback,
+      const network_handler::ErrorCallback& error_callback) = 0;
 
   // 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);
+  virtual bool HasConnectingNetwork(const std::string& service_path) = 0;
 
   // Returns true if there are any pending connect requests.
-  bool HasPendingConnectRequest();
-
-  // Sets the TetherDelegate to handle Tether actions. |tether_delegate| is
-  // owned by the caller.
-  void SetTetherDelegate(TetherDelegate* tether_delegate);
-
-  // NetworkStateHandlerObserver
-  void NetworkListChanged() override;
-  void NetworkPropertiesUpdated(const NetworkState* network) override;
+  virtual bool HasPendingConnectRequest() = 0;
 
-  // LoginState::Observer
-  void LoggedInStateChanged() override;
-
-  // CertLoader::Observer
-  void OnCertificatesLoaded(const net::CertificateList& cert_list,
-                            bool initial_load) override;
+  virtual void Init(NetworkStateHandler* network_state_handler,
+                    NetworkConfigurationHandler* network_configuration_handler,
+                    ManagedNetworkConfigurationHandler*
+                        managed_network_configuration_handler) = 0;
 
  protected:
   NetworkConnectionHandler();
 
-  void InitiateTetherNetworkConnection(
-      const std::string& tether_network_guid,
-      const base::Closure& success_callback,
-      const network_handler::ErrorCallback& error_callback);
-
- private:
-  friend class NetworkHandler;
-  friend class NetworkConnectionHandlerTest;
-
-  struct ConnectRequest;
-
-  void Init(NetworkStateHandler* network_state_handler,
-            NetworkConfigurationHandler* network_configuration_handler,
-            ManagedNetworkConfigurationHandler*
-                managed_network_configuration_handler);
-
-  ConnectRequest* GetPendingRequest(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 from
-  // pending_requests_[service_path]. |check_error_state| is passed from
-  // ConnectToNetwork(), see comment for info.
-  void VerifyConfiguredAndConnect(bool check_error_state,
-                                  const std::string& service_path,
-                                  const base::DictionaryValue& properties);
-
-  bool IsNetworkProhibitedByPolicy(const std::string& type,
-                                   const std::string& guid,
-                                   const std::string& profile_path);
-
-  // Queues a connect request until certificates have loaded.
-  void QueueConnectRequest(const std::string& service_path);
-
-  // Checks to see if certificates have loaded and if not, cancels any queued
-  // connect request and notifies the user.
-  void CheckCertificatesLoaded();
-
-  // Handles connecting to a queued network after certificates are loaded or
-  // handle cert load timeout.
-  void ConnectToQueuedNetwork();
-
-  // Calls Shill.Manager.Connect asynchronously.
-  void CallShillConnect(const std::string& service_path);
-
-  // Handles failure from ConfigurationHandler calls.
-  void HandleConfigurationFailure(
-      const std::string& service_path,
-      const std::string& error_name,
-      std::unique_ptr<base::DictionaryValue> error_data);
-
-  // Handles success or failure from Shill.Service.Connect.
-  void HandleShillConnectSuccess(const std::string& service_path);
-  void HandleShillConnectFailure(const std::string& service_path,
-                                 const std::string& error_name,
-                                 const std::string& error_message);
-
-  // Note: |service_path| is passed by value here, because in some cases
-  // the value may be located in the map and then it can be deleted, producing
-  // a reference to invalid memory.
-  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(
@@ -257,37 +173,22 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
       const network_handler::ErrorCallback& error_callback,
       const std::string& error_name);
 
-  // Calls Shill.Manager.Disconnect asynchronously.
-  void CallShillDisconnect(
-      const std::string& service_path,
+  // Initiates a connection to a Tether network.
+  void InitiateTetherNetworkConnection(
+      const std::string& tether_network_guid,
       const base::Closure& success_callback,
       const network_handler::ErrorCallback& error_callback);
 
-  // Handle success from Shill.Service.Disconnect.
-  void HandleShillDisconnectSuccess(const std::string& service_path,
-                                    const base::Closure& success_callback);
-
   base::ObserverList<NetworkConnectionObserver, true> observers_;
 
-  // Local references to the associated handler instances.
-  CertLoader* cert_loader_;
-  NetworkStateHandler* network_state_handler_;
-  NetworkConfigurationHandler* configuration_handler_;
-  ManagedNetworkConfigurationHandler* managed_configuration_handler_;
-
-  // Map of pending connect requests, used to prevent repeated attempts while
-  // waiting for Shill and to trigger callbacks on eventual success or failure.
-  std::map<std::string, ConnectRequest> pending_requests_;
-  std::unique_ptr<ConnectRequest> queued_connect_;
-
-  // Track certificate loading state.
-  bool logged_in_;
-  bool certificates_loaded_;
-  base::TimeTicks logged_in_time_;
-
   // Delegate used to start a connection to a tether network.
   TetherDelegate* tether_delegate_;
 
+ private:
+  // Only to be used by NetworkConnectionHandler implementation (and not by
+  // derived classes).
+  base::WeakPtrFactory<NetworkConnectionHandler> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkConnectionHandler);
 };
 

chromeos/network/network_connection_handler_impl.h

+// Copyright (c) 2017 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_HANDLER_IMPL_H_
+#define CHROMEOS_NETWORK_NETWORK_CONNECTION_HANDLER_IMPL_H_
+
+#include "chromeos/cert_loader.h"
+#include "chromeos/chromeos_export.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
+#include "chromeos/login/login_state.h"
+#include "chromeos/network/network_connection_handler.h"
+#include "chromeos/network/network_state_handler_observer.h"
+
+namespace chromeos {
+
+// Implementation of NetworkConnectionHandler.
+class CHROMEOS_EXPORT NetworkConnectionHandlerImpl
+    : public NetworkConnectionHandler,
+      public LoginState::Observer,
+      public CertLoader::Observer,
+      public NetworkStateHandlerObserver,
+      public base::SupportsWeakPtr<NetworkConnectionHandlerImpl> {
+ public:
+  NetworkConnectionHandlerImpl();
+  ~NetworkConnectionHandlerImpl() override;
+
+  // NetworkConnectionHandler:
+  void ConnectToNetwork(const std::string& service_path,
+                        const base::Closure& success_callback,
+                        const network_handler::ErrorCallback& error_callback,
+                        bool check_error_state) override;
+  void DisconnectNetwork(
+      const std::string& service_path,
+      const base::Closure& success_callback,
+      const network_handler::ErrorCallback& error_callback) override;
+  bool HasConnectingNetwork(const std::string& service_path) override;
+  bool HasPendingConnectRequest() override;
+
+  // NetworkStateHandlerObserver
+  void NetworkListChanged() override;
+  void NetworkPropertiesUpdated(const NetworkState* network) override;
+
+  // LoginState::Observer
+  void LoggedInStateChanged() override;
+
+  // CertLoader::Observer
+  void OnCertificatesLoaded(const net::CertificateList& cert_list,
+                            bool initial_load) override;
+
+ protected:
+  void Init(NetworkStateHandler* network_state_handler,
+            NetworkConfigurationHandler* network_configuration_handler,
+            ManagedNetworkConfigurationHandler*
+                managed_network_configuration_handler) override;
+
+ private:
+  struct ConnectRequest {
+    ConnectRequest(const std::string& service_path,
+                   const std::string& profile_path,
+                   const base::Closure& success,
+                   const network_handler::ErrorCallback& error);
+    ~ConnectRequest();
+    explicit ConnectRequest(const ConnectRequest& other);
+
+    enum ConnectState {
+      CONNECT_REQUESTED = 0,
+      CONNECT_STARTED = 1,
+      CONNECT_CONNECTING = 2
+    };
+
+    std::string service_path;
+    std::string profile_path;
+    ConnectState connect_state;
+    base::Closure success_callback;
+    network_handler::ErrorCallback error_callback;
+  };
+
+  ConnectRequest* GetPendingRequest(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 from
+  // pending_requests_[service_path]. |check_error_state| is passed from
+  // ConnectToNetwork(), see comment for info.
+  void VerifyConfiguredAndConnect(bool check_error_state,
+                                  const std::string& service_path,
+                                  const base::DictionaryValue& properties);
+
+  bool IsNetworkProhibitedByPolicy(const std::string& type,
+                                   const std::string& guid,
+                                   const std::string& profile_path);
+
+  // Queues a connect request until certificates have loaded.
+  void QueueConnectRequest(const std::string& service_path);
+
+  // Checks to see if certificates have loaded and if not, cancels any queued
+  // connect request and notifies the user.
+  void CheckCertificatesLoaded();
+
+  // Handles connecting to a queued network after certificates are loaded or
+  // handle cert load timeout.
+  void ConnectToQueuedNetwork();
+
+  // Calls Shill.Manager.Connect asynchronously.
+  void CallShillConnect(const std::string& service_path);
+
+  // Handles failure from ConfigurationHandler calls.
+  void HandleConfigurationFailure(
+      const std::string& service_path,
+      const std::string& error_name,
+      std::unique_ptr<base::DictionaryValue> error_data);
+
+  // Handles success or failure from Shill.Service.Connect.
+  void HandleShillConnectSuccess(const std::string& service_path);
+  void HandleShillConnectFailure(const std::string& service_path,
+                                 const std::string& error_name,
+                                 const std::string& error_message);
+
+  // Note: |service_path| is passed by value here, because in some cases
+  // the value may be located in the map and then it can be deleted, producing
+  // a reference to invalid memory.
+  void CheckPendingRequest(const std::string service_path);
+
+  void CheckAllPendingRequests();
+
+  // Look up the ConnectRequest for |service_path| and call
+  // InvokeConnectErrorCallback.
+  void ErrorCallbackForPendingRequest(const std::string& service_path,
+                                      const std::string& error_name);
+
+  // 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 from Shill.Service.Disconnect.
+  void HandleShillDisconnectSuccess(const std::string& service_path,
+                                    const base::Closure& success_callback);
+
+  // Local references to the associated handler instances.
+  CertLoader* cert_loader_;
+  NetworkStateHandler* network_state_handler_;
+  NetworkConfigurationHandler* configuration_handler_;
+  ManagedNetworkConfigurationHandler* managed_configuration_handler_;
+
+  // Map of pending connect requests, used to prevent repeated attempts while
+  // waiting for Shill and to trigger callbacks on eventual success or failure.
+  std::map<std::string, ConnectRequest> pending_requests_;
+  std::unique_ptr<ConnectRequest> queued_connect_;
+
+  // Track certificate loading state.
+  bool logged_in_;
+  bool certificates_loaded_;
+  base::TimeTicks logged_in_time_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkConnectionHandlerImpl);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_NETWORK_NETWORK_CONNECTION_HANDLER_IMPL_H_

chromeos/network/network_connection_handler_unittest.cc → chromeos/network/network_connection_handler_impl_unittest.cc

@@ -2,7 +2,7 @@
 // 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_handler.h"
+#include "chromeos/network/network_connection_handler_impl.h"
 
 #include <map>
 #include <memory>
@@ -152,7 +152,7 @@ class NetworkConnectionHandlerTest : public NetworkStateTest {
         network_config_handler_.get(), nullptr /* network_device_handler */,
         nullptr /* prohibited_tecnologies_handler */);
 
-    network_connection_handler_.reset(new NetworkConnectionHandler);
+    network_connection_handler_.reset(new NetworkConnectionHandlerImpl());
     network_connection_handler_->Init(network_state_handler(),
                                       network_config_handler_.get(),
                                       managed_config_handler_.get());
@@ -209,9 +209,7 @@ class NetworkConnectionHandlerTest : public NetworkStateTest {
     base::RunLoop().RunUntilIdle();
   }
 
-  void SuccessCallback() {
-    result_ = kSuccessResult;
-  }
+  void SuccessCallback() { result_ = kSuccessResult; }
 
   void ErrorCallback(const std::string& error_name,
                      std::unique_ptr<base::DictionaryValue> error_data) {
@@ -279,8 +277,7 @@ class NetworkConnectionHandlerTest : public NetworkStateTest {
     } else {
       managed_config_handler_->SetPolicy(::onc::ONC_SOURCE_DEVICE_POLICY,
                                          std::string(),  // no username hash
-                                         *network_configs,
-                                         global_config);
+                                         *network_configs, global_config);
     }
     base::RunLoop().RunUntilIdle();
   }

chromeos/network/network_handler.cc

@@ -13,7 +13,7 @@
 #include "chromeos/network/network_activation_handler.h"
 #include "chromeos/network/network_cert_migrator.h"
 #include "chromeos/network/network_configuration_handler.h"
-#include "chromeos/network/network_connection_handler.h"
+#include "chromeos/network/network_connection_handler_impl.h"
 #include "chromeos/network/network_device_handler_impl.h"
 #include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/network_profile_observer.h"
@@ -44,7 +44,7 @@ NetworkHandler::NetworkHandler()
     client_cert_resolver_.reset(new ClientCertResolver());
   }
   network_activation_handler_.reset(new NetworkActivationHandler());
-  network_connection_handler_.reset(new NetworkConnectionHandler());
+  network_connection_handler_.reset(new NetworkConnectionHandlerImpl());
   network_sms_handler_.reset(new NetworkSmsHandler());
   geolocation_handler_.reset(new GeolocationHandler());
 }