Add NetworkConnectionHandler class

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
new file mode 100644
index 0000000000000000000000000000000000000000..3f4192b2640cee95f549830085d371ceee3dc19a
--- /dev/null
+++ b/chromeos/network/network_connection_handler.h
@@ -0,0 +1,153 @@
+// Copyright (c) 2012 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_H_
+#define CHROMEOS_NETWORK_NETWORK_CONNECTION_HANDLER_H_
+
+#include <set>
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "base/values.h"
+#include "chromeos/chromeos_export.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
+#include "chromeos/network/network_handler_callbacks.h"
+
+namespace chromeos {
+
+class NetworkState;
+
+// The NetworkConnectionHandler class is used to connect to networks. It

[pneubeck (no reviews), 2013/05/07 08:46:39]

The main responsibility (not contradicting to the listed steps) of this class
should be:
 Keeping track of the state transitions during the process of connecting. (I.e.
a state machine)

[stevenjb, 2013/05/08 01:57:22]

This is also the only class that should make Connect calls to Shill. I'll expand
the comment.

+// handles the following steps:
+// 1. Determine whether or not sufficient information (e.g. passphrase) is
+//    known to be available to connect to the network.
+// 2. Request additional information (e.g. user data which contains certificate

[pneubeck (no reviews), 2013/05/07 08:46:39]

I'm not happy about that checking of sufficient information (see also later
comments).
It's fine with respect to additional information from Shill (like Connectable,
PassphraseRequired) but not wrt. user configuration.
For configuration, we need such tests/verification also in other places (mostly
UI and import of config).

[stevenjb, 2013/05/08 01:57:22]

I agree with centralizing any shared logic, however this class should still make
those calls, at least to the extent of establishing whether or not sufficient
information is available.

[pneubeck (no reviews), 2013/05/08 08:07:46]

sg

+//    information) and determine whether sufficient information is available.
+// 3. Set certificate and related properties to allow a successful connection

[pneubeck (no reviews), 2013/05/07 08:46:39]

See my later comments: CertificateLoader/Handler is responsible for that.

[stevenjb, 2013/05/08 01:57:22]

I have thought about this and related comments, and I think I agree that we can
remove any certificate configuration from this class and handle it exclusively
elsewhere.

+//    (if necessary).
+// 4. Send the connect request.
+// 5. Call back (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:
+  // Sets the global instance. Must be called before any calls to Get().
+  static void Initialize();
+
+  // Destroys the global instance.
+  static void Shutdown();
+
+  // Gets the global instance. Initialize() must be called first.
+  static NetworkConnectionHandler* Get();
+
+  // 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:
+  //  kNetworkNotFound if no network matching |service_path| is found
+  //    (hidden networks must be configured before connecting).
+  //  kConnected if already connected to the network.
+  //  kConnecting if already connecting to the network.
+  //  kCertificateRequired if the network requires a cert and none exists.
+  //  kPassphraseRequired if passphrase only is required.
+  //  kConfigurationRequired if additional configuration is required.
+  //  kShillError if a DBus or Shill error occurred.
+  // |error_message| will contain and additional error string for debugging.

[pneubeck (no reviews), 2013/05/07 08:46:39]

and -> an

[stevenjb, 2013/05/08 01:57:22]

Done.

+  void ConnectToNetwork(const std::string& service_path,
+                        const base::Closure& success_callback,
+                        const network_handler::ErrorCallback& error_callback);
+
+  // DisconnectToNetwork() will send a Disconnect request to Shill.
+  // On success, |success_callback| will be called.
+  // On failure, |error_callback| will be called with |error_name| one of:
+  //  kNetworkNotFound if no network matching |service_path| is found.
+  //  kNotConnected if not connected to the network.
+  //  kShillError 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);
+
+  // Constants for |error_name| from |error_callback| for Connect/Disconnect.
+  static const char kNetworkNotFound[];
+  static const char kConnected[];
+  static const char kConnecting[];
+  static const char kNotConnected[];
+  static const char kPassphraseRequired[];
+  static const char kActivationRequired[];
+  static const char kCertificateRequired[];
+  static const char kConfigurationRequired[];
+  static const char kShillError[];
+
+ private:

[pneubeck (no reviews), 2013/05/07 08:46:39]

there are several functions that could be .cc-local or at least static (I marked
them in the .cc).
If they are not tested then I'd put them into the cc.

[pneubeck (no reviews), 2013/05/08 08:07:46]

Done.

+  NetworkConnectionHandler();
+  ~NetworkConnectionHandler();
+
+  // 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);
+
+  // Invokes |callback| with |error_name| for 'result'.

[pneubeck (no reviews), 2013/05/07 08:46:39]

'result' ?

[stevenjb, 2013/05/08 01:57:22]

Done.

+  void InvokeErrorCallback(const std::string& service_path,
+                           const network_handler::ErrorCallback& error_callback,
+                           const std::string& error_name);
+
+  // Returns true if we need to request properties for |network| to determine
+  // whether or not we can connect to it.
+  bool NetworkMayNeedCredentials(const NetworkState* network) const;
+
+  // Returns true if |network| is cellular and not activated or out of credits.
+  bool NetworkRequiresActivation(const NetworkState* network) const;
+
+  // Callback to Shill.Service.GetProperties. Parses properties to determine
+  // whether to invoke |error_callback|, or attemt a connection.
+  void GetPropertiesCallback(
+      const base::Closure& success_callback,
+      const network_handler::ErrorCallback& error_callback,
+      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;
+
+  // 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);
+
+  // Set of pending connect requests, used to prevent repeat attempts while
+  // waiting for Shill.
+  std::set<std::string> pending_requests_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkConnectionHandler);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_NETWORK_NETWORK_CONNECTION_HANDLER_H_