Adding policy support to the new network configuration stack.

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 76fd02cbc7f20e175dbcb5ccfe653fcd4a7cbf8d..739be04a2a41c00f67b07f52ad7650c5865d4116 100644
--- a/chromeos/network/managed_network_configuration_handler.h
+++ b/chromeos/network/managed_network_configuration_handler.h
@@ -5,14 +5,16 @@
 #ifndef CHROMEOS_NETWORK_MANAGED_NETWORK_CONFIGURATION_HANDLER_H_
 #define CHROMEOS_NETWORK_MANAGED_NETWORK_CONFIGURATION_HANDLER_H_
 
+#include <map>
 #include <string>
-#include <vector>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
+#include "base/memory/weak_ptr.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/network/network_handler_callbacks.h"
+#include "chromeos/network/onc/onc_constants.h"
 
 namespace base {
 class DictionaryValue;
@@ -67,6 +69,13 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandler {
       const network_handler::DictionaryResultCallback& callback,
       const network_handler::ErrorCallback& error_callback) const;
 
+  // Provides the managed properties of the network with |service_path| to
+  // |callback|.
+  void GetManagedProperties(
+      const std::string& service_path,
+      const network_handler::DictionaryResultCallback& callback,
+      const network_handler::ErrorCallback& error_callback);
+
   // Sets the user's settings of an already configured network with
   // |service_path|. A network can be initially configured by calling
   // CreateConfiguration or if it is managed by a policy. The given properties
@@ -74,7 +83,7 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandler {
   // properties.
   void SetProperties(
       const std::string& service_path,
-      const base::DictionaryValue& properties,
+      const base::DictionaryValue& user_settings,
       const base::Closure& callback,
       const network_handler::ErrorCallback& error_callback) const;
 
@@ -109,10 +118,32 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandler {
       const base::Closure& callback,
       const network_handler::ErrorCallback& error_callback) const;
 
+  // Only to be called by NetworkConfigurationUpdater.

[stevenjb, 2013/04/11 18:20:08]

Also describe what the method does.

[pneubeck (no reviews), 2013/04/15 12:16:24]

Done.

+  void SetPolicy(onc::ONCSource onc_source,
+                 const base::DictionaryValue& onc_dict);
+
  private:
+  typedef std::map<std::string, const base::DictionaryValue*> PolicyMap;
+
+  class PolicyApplicator;
+
   ManagedNetworkConfigurationHandler();
   ~ManagedNetworkConfigurationHandler();
 
+  void GetManagedPropertiesCallback(
+      const network_handler::DictionaryResultCallback& callback,
+      const network_handler::ErrorCallback& error_callback,
+      const std::string& service_path,
+      const base::DictionaryValue& shill_properties);
+
+  const PolicyMap* GetPoliciesForProfile(const std::string& profile) const;
+
+  scoped_ptr<PolicyMap> user_policies_by_guid_;
+  scoped_ptr<PolicyMap> device_policies_by_guid_;

[stevenjb, 2013/04/11 18:20:08]

This is pretty confusing; it's a pointer to a map of pointers.

Do we actually need to pass ownership of these maps elsewhere, or could we just
make these maps, and convert:

user_policies_by_guid_ = GetFoo(..)
to
SetFoo(..., &user_policies_by_guid_);

And

!user_policies_by_guid_ to user_policies_by_guid_.empty()?

Or do we need to differentiate between an empty map and an unset one?

There is also the question of who owns the Dictionary*'s in the maps. A scoped
pointer implies that destroying the member will clean up any contents, but it
appears that is not the case. If we need to differentiate between empty and
NULL, then I would use raw *'s here, not scoped_ptr.

[pneubeck (no reviews), 2013/04/15 12:16:24]

Yes, that must be differentiated.
In the previous patch I used a separate boolean "...is_initialized". Which
Julian thought not to be ideal.
What I like about the current usage of "pointer to map" is that every user of
the map is reminded by the type that the pointer might be NULL.
What I don't like is the bloated syntax in some of the functions.
As in other cases of map<*> usages of the code base, I added a clarifying
comment about the ownership.
Alternatively, I could change the Map to DictionaryValue, which however drops
the type information about it's content (the entries will be of type Value
instead of DictionaryValue).


I think, we'll have to come up with a compromise here.
Reverting to the booleans "...initialized_" as in the previous patch? At least
the semantics are more obvious then and don't infer with ownership.

[stevenjb, 2013/04/15 17:38:54]

How about raw *'s here then, instead of scoped_ptr? scoped_ptr is great when a
member fully cleans itself up, but that is not the case here.

Alternatively, make PolicyMap a class with a destructor that does the cleanup
instead of a typedef.

+
+  // For Shill client callbacks
+  base::WeakPtrFactory<ManagedNetworkConfigurationHandler> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ManagedNetworkConfigurationHandler);
 };