Add NetworkHandler to own network handlers in src/chromeos/network

chromeos/network/network_handler.h

+// Copyright (c) 2013 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_HANDLER_H_
+#define CHROMEOS_NETWORK_NETWORK_HANDLER_H_
+
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+#include "chromeos/chromeos_export.h"
+
+namespace chromeos {
+
+class CertLoader;
+class GeolocationHandler;
+class ManagedNetworkConfigurationHandler;
+class NetworkConfigurationHandler;
+class NetworkProfileHandler;
+class NetworkStateHandler;
+
+// Class for handling initialization and access to chromeos network handlers.
+

[pneubeck (no reviews), 2013/05/13 09:17:06]

nit: remove empty line.

[stevenjb, 2013/05/13 23:49:35]

Done.

+class CHROMEOS_EXPORT NetworkHandler {
+ 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 NetworkHandler* Get();
+
+  // Returns true if the global instance has been initialized.
+  static bool IsInitialized();
+
+  CertLoader* cert_loader();
+  NetworkStateHandler* network_state_handler();

[pneubeck (no reviews), 2013/05/13 09:17:06]

I thought you proposed state()?

I'd at least drop the prefix "network_".

[stevenjb, 2013/05/13 23:49:35]

I decided I prefer the name matching the class (clarity over brevity). I don't
like dropping 'network_' (geolocation at and cert would be inconsistent) but
could be persuaded to drop _handler if we really think brevity is important
here.

[pneubeck (no reviews), 2013/05/22 14:02:53]

I don't feel strong about that, so NIT:

That might indicate, that CertLoader/Geolocation do not belong here or have to
be renamed.

[stevenjb, 2013/05/22 17:09:27]

I don't think we should need to rename the other classes.

+  NetworkProfileHandler* network_profile_handler();
+  NetworkConfigurationHandler* network_configuration_handler();
+  ManagedNetworkConfigurationHandler* managed_network_configuration_handler();
+  GeolocationHandler* geolocation_handler();
+
+  void SetCertLoaderForTest(CertLoader* cert_loader);

[pneubeck (no reviews), 2013/05/13 09:17:06]

Since none of these are called yet, we could leave them out for now.

[stevenjb, 2013/05/13 23:49:35]

I added them specifically to make setting future mock implementations easier,
but yeah, should leave them out until we need them, especially since the test
strategy going forward is clearly up for discussion.
Done.

+  void SetNetworkStateHandlerForTest(
+      NetworkStateHandler* network_state_handler);
+  void SetNetworkProfileHandlerForTest(
+      NetworkProfileHandler* network_profile_handler);
+  void SetNetworkConfigurationHandlerForTest(
+      NetworkConfigurationHandler* network_configuration_handler);
+  void SetManagedNetworkConfigurationHandlerForTest(
+      ManagedNetworkConfigurationHandler*
+      managed_network_configuration_handler);
+  void SetGeolocationHandlerForTest(
+      GeolocationHandler* geolocation_handler);
+
+ private:
+  class LoginStateInitializer;
+
+  NetworkHandler();
+  virtual ~NetworkHandler();
+
+  void Init();
+
+  // Initialize LoginState for tests so they do not have to do so explicitly.

[pneubeck (no reviews), 2013/05/13 09:17:06]

I'm not convinced that this belongs into this class.

[stevenjb, 2013/05/13 23:49:35]

Requiring any test that initializes NetworkHandler to also initialize LoginState
is rather tedious; this seemed like a reasonable compromise.

[pneubeck (no reviews), 2013/05/22 14:02:53]

Tedious or not. It don't see an argument why it belongs here.

Which tests are we talking about? Browser tests shouldn't be affected. If a unit
tests uses this Handler instead of a Stub/Mock, then it's also correct to setup
the Handler's environment in the test (i.e. also the DBus clients).

[stevenjb, 2013/05/22 17:09:27]

I went ahead and eliminated this and made CertLoader check
LoginState::IsInitialized(). Hopefully this will not cause any future dev pain.

+  scoped_ptr<LoginStateInitializer> login_state_initializer_;
+
+  // The order of these determines the (inverse) destruction order.
+  scoped_ptr<CertLoader> cert_loader_;
+  scoped_ptr<NetworkStateHandler> network_state_handler_;
+  scoped_ptr<NetworkProfileHandler> network_profile_handler_;
+  scoped_ptr<NetworkConfigurationHandler> network_configuration_handler_;
+  scoped_ptr<ManagedNetworkConfigurationHandler>
+      managed_network_configuration_handler_;
+  scoped_ptr<GeolocationHandler> geolocation_handler_;
+
+  // Unowned test implementations. If non-NULL these will be used instead of
+  // the owned implementations. The test is responsible for destroying these.
+  CertLoader* test_cert_loader_;
+  NetworkStateHandler* test_network_state_handler_;
+  NetworkProfileHandler* test_network_profile_handler_;
+  NetworkConfigurationHandler* test_network_configuration_handler_;
+  ManagedNetworkConfigurationHandler*
+      test_managed_network_configuration_handler_;
+  GeolocationHandler* test_geolocation_handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkHandler);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_NETWORK_NETWORK_HANDLER_H_