Adding a NetworkProfileHandler used by ManagedNetworkConfigurationHandler.

chromeos/network/managed_network_configuration_handler.h

Index: chromeos/network/managed_network_configuration_handler.h
diff --git a/chromeos/network/managed_network_configuration_handler.h b/chromeos/network/managed_network_configuration_handler.h
index e563fe48be88230bfda10a35a8284fd6a20555b4..9bffb0209720b850f198df9ea8dd25939150d21a 100644
--- a/chromeos/network/managed_network_configuration_handler.h
+++ b/chromeos/network/managed_network_configuration_handler.h
@@ -14,6 +14,7 @@
 #include "base/memory/weak_ptr.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/network/network_handler_callbacks.h"
+#include "chromeos/network/network_profile_observer.h"
 #include "chromeos/network/onc/onc_constants.h"
 
 namespace base {
@@ -23,6 +24,8 @@ class ListValue;
 
 namespace chromeos {
 
+class NetworkProfileHandler;
+
 // The ManagedNetworkConfigurationHandler class is used to create and configure
 // networks in ChromeOS using ONC and takes care of network policies.
 //
@@ -48,12 +51,14 @@ namespace chromeos {
 // that is suitable for logging. None of the error message text is meant for
 // user consumption.
 
-class CHROMEOS_EXPORT ManagedNetworkConfigurationHandler {
+class CHROMEOS_EXPORT ManagedNetworkConfigurationHandler
+    : public NetworkProfileObserver {
  public:
-  typedef std::map<std::string, const base::DictionaryValue*> PolicyMap;
+  struct Policies;
+  typedef std::map<std::string, Policies*> UserToPolicies;
 
   // Initializes the singleton.
-  static void Initialize();
+  static void Initialize(NetworkProfileHandler* profile_handler);

[stevenjb, 2013/04/26 21:00:49]

Again, I dislike passing singletons as construction arguments.

If we need to set a handler other than the singleton instance (and I don't think
we should ever need to), set profile_handler_ to the singleton in the
constructor, and add SetNetworkProfileHandlerForTest() to override it.

[pneubeck (no reviews), 2013/04/29 18:05:51]

My reasons/ideas were:

The unit test of ManangedNetworkConfigurationHandler provides a Stub as an
argument.

This way the compiler forces the caller to provide a profile_handler and thus to
construct the Handlers in the correct order. It also doesn't depend on the
ProfileHandler to be a singleton and isn't using a global access.

In order to reduce the number of singletons (global ownership) and global
accesses, one solution could be to let DBusServices own the bunch of Handlers
which are explicitly dependent on each other. DBusServices itself could be
reachable globally if necessary.
Using the explicit argument here, was meant to be a step in that direction.

[stevenjb, 2013/04/30 17:42:47]

I agree that we might want to create a single class to maintain the various
network handlers at some point, once they start to stabalize.

I do not feel that passing them in constructors is necessarily a step in that
direction.

For now I would much rather treat singletons as singletons, until / unless that
changes.

[pneubeck (no reviews), 2013/05/03 17:32:55]

It is a step to prevent cyclic dependencies and to prevent incorrect
initialization order.
How will we prevent that otherwise?

Chrome code ended up with cyclic dependencies in other cases already. One was
related to UserManager and DeviceSettingsService AFAIK.

 
   // Returns if the singleton is initialized.
   static bool IsInitialized();
@@ -71,8 +76,10 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandler {
       const network_handler::ErrorCallback& error_callback) const;
 
   // Provides the managed properties of the network with |service_path| to
-  // |callback|.
+  // |callback|. |userhash| is only used to ensure that the user's policy is
+  // already applied.
   void GetManagedProperties(
+      const std::string& userhash,
       const std::string& service_path,
       const network_handler::DictionaryResultCallback& callback,
       const network_handler::ErrorCallback& error_callback);
@@ -105,8 +112,10 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandler {
   // Initially configures an unconfigured network with the given user settings
   // and returns the new identifier to |callback| if successful. Fails if the
   // network was already configured by a call to this function or because of a
-  // policy.
+  // policy. The new configuration will be owned by user |userhash|. If
+  // |userhash| is empty, the new configuration will be shared.
   void CreateConfiguration(
+      const std::string& userhash,
       const base::DictionaryValue& properties,
       const network_handler::StringResultCallback& callback,
       const network_handler::ErrorCallback& error_callback) const;
@@ -123,16 +132,22 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandler {
   // |network_configs_onc| as the current policy of |onc_source|. The network
   // configurations of the policy will be applied (not necessarily immediately)
   // to Shill's profiles and enforced in future configurations until the policy
-  // associated with |onc_source| is changed again with this function.
-  // This function doesn't validate the policy. The caller must ensure validity.
+  // associated with |onc_source| is changed again with this function. For
+  // device policies, |userhash| must be empty.
   void SetPolicy(onc::ONCSource onc_source,
+                 const std::string& userhash,
                  const base::ListValue& network_configs_onc);
 
+  // NetworkProfileObserver overrides
+  virtual void OnProfileAdded(const NetworkProfile& profile) OVERRIDE;
+  virtual void OnProfileRemoved(const NetworkProfile& profile) OVERRIDE;
+
  private:
   class PolicyApplicator;
 
-  ManagedNetworkConfigurationHandler();
-  ~ManagedNetworkConfigurationHandler();
+  explicit ManagedNetworkConfigurationHandler(
+      NetworkProfileHandler* profile_handler);
+  virtual ~ManagedNetworkConfigurationHandler();
 
   void GetManagedPropertiesCallback(
       const network_handler::DictionaryResultCallback& callback,
@@ -140,14 +155,16 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandler {
       const std::string& service_path,
       const base::DictionaryValue& shill_properties);
 
-  const PolicyMap* GetPoliciesForProfile(const std::string& profile) const;
+  const Policies *GetPoliciesForUser(const std::string& userhash) const;
+  const Policies *GetPoliciesForProfile(const NetworkProfile& profile) const;
 
   // The entries of these maps are owned by this class and are explicitly
   // deleted where necessary.
-  PolicyMap user_policies_by_guid_;
-  PolicyMap device_policies_by_guid_;
-  bool user_policies_initialized_;
-  bool device_policies_initialized_;
+  // Empty string maps to device policy.
+  UserToPolicies policies_by_user_;
+
+  // A local reference to the policy handler singleton.
+  NetworkProfileHandler* profile_handler_;
 
   // For Shill client callbacks
   base::WeakPtrFactory<ManagedNetworkConfigurationHandler> weak_ptr_factory_;