[CrOS Tether] Transform NetworkConnectionHandler to an interface.

chromeos/network/network_connection_handler.h

Index: chromeos/network/network_connection_handler.h
diff --git a/chromeos/network/network_connection_handler.h b/chromeos/network/network_connection_handler.h
index edbda71f1824df8d1161de7941cfe8f2a38eaa57..b286e3c0747a340bab51f1152f4d6aba4c89b8f3 100644
--- a/chromeos/network/network_connection_handler.h
+++ b/chromeos/network/network_connection_handler.h
@@ -13,7 +13,6 @@
 #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"
@@ -26,8 +25,6 @@
 
 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:
@@ -47,8 +44,7 @@ class NetworkState;
 class CHROMEOS_EXPORT NetworkConnectionHandler
     : public LoginState::Observer,
       public CertLoader::Observer,
-      public NetworkStateHandlerObserver,
-      public base::SupportsWeakPtr<NetworkConnectionHandler> {
+      public NetworkStateHandlerObserver {

[stevenjb, 2017/05/03 22:05:38]

Put these in the Impl

[Kyle Horimoto, 2017/05/03 22:38:30]

Done.

  public:
   // Constants for |error_name| from |error_callback| for Connect.
 
@@ -128,6 +124,10 @@ class CHROMEOS_EXPORT 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 +136,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 +149,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;
-
-  // LoginState::Observer
-  void LoggedInStateChanged() override;
-
-  // CertLoader::Observer
-  void OnCertificatesLoaded(const net::CertificateList& cert_list,
-                            bool initial_load) override;
+  virtual bool HasPendingConnectRequest() = 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();
+  virtual void Init(NetworkStateHandler* network_state_handler,
+                    NetworkConfigurationHandler* network_configuration_handler,
+                    ManagedNetworkConfigurationHandler*
+                        managed_network_configuration_handler) = 0;
 
   // 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,36 +180,27 @@ 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);

[stevenjb, 2017/05/03 22:05:38]

These should also be in the Impl

[Kyle Horimoto, 2017/05/03 22:38:30]

I'd like to keep these three functions in the base class as non-virtual
functions. I use these functions in my TetherConnector test and will also be
using them in my upcoming TetherDisconnector test.

 
-  // 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_;
+  // Delegate used to start a connection to a tether network.
+  TetherDelegate* tether_delegate_;
 
-  // 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_;
+ private:
+  friend class NetworkHandler;

[stevenjb, 2017/05/03 22:05:38]

Don't do this, see below.

[Kyle Horimoto, 2017/05/03 22:38:30]

Done.

+  friend class NetworkConnectionHandlerTest;

[stevenjb, 2017/05/03 22:05:38]

Do we need this?

[Kyle Horimoto, 2017/05/03 22:38:30]

Done.

 
-  // Track certificate loading state.
-  bool logged_in_;
-  bool certificates_loaded_;
-  base::TimeTicks logged_in_time_;
+  // Should only be called once by NetworkHandler during initialization.
+  static std::unique_ptr<NetworkConnectionHandler> Create();

[stevenjb, 2017/05/03 22:05:38]

Just make this public, or better yet, just have NetworkHandler include the _impl
header and construct a NCHImpl (and remove ths).

[Kyle Horimoto, 2017/05/03 22:38:30]

Done.

 
-  // Delegate used to start a connection to a tether network.
-  TetherDelegate* tether_delegate_;
+  // Only to be used by NetworkConnectionHandler implementation (and not by
+  // derived classes).
+  base::WeakPtrFactory<NetworkConnectionHandler> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkConnectionHandler);
 };