Move DeviceManagementPolicyProvider into the profile.

chrome/browser/prefs/pref_value_store.h

Index: chrome/browser/prefs/pref_value_store.h
diff --git a/chrome/browser/prefs/pref_value_store.h b/chrome/browser/prefs/pref_value_store.h
index 87ed7b26b635cbfb6517d0b6d316a3d22c395e49..0e741c7701c6de28bf0e4bc71e12b4d5697135c9 100644
--- a/chrome/browser/prefs/pref_value_store.h
+++ b/chrome/browser/prefs/pref_value_store.h
@@ -172,7 +172,7 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
   // that conflict with proxy settings specified by proxy policy.
   bool HasPolicyConflictingUserProxySettings();
 
-      protected:
+ protected:
   // In decreasing order of precedence:
   //   |managed_platform_prefs| contains all managed platform (non-cloud policy)
   //        preference values.
@@ -186,6 +186,14 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
   //   |default_prefs| contains application-default preference values. It must
   //        be non-null if any preferences are to be registered.
   //
+  // The |profile| parameter is used to construct a replacement device
+  // management pref store in case of policy refresh, may be NULL for the local
+  // state preferences.

[markusheintz_, 2010/11/18 15:42:33]

I'm so sorry to bother you with comments, but I don't understand this comment
:(. What is a replacement device management pref store? and why do we need it. 

Maybe something like this:

The |profile| parameter is needed to construct a device management pref store.
Everytime a policy refresh happens a new management pref store is created and
used to replace the old one. The is necessary to prevent concurrency issues.

[Mattias Nissler (ping if slow), 2010/11/18 16:06:38]

Na, the comment is really about this: The device management prefstore is passed
in here, but since we construct a new one when policy refreshes, we need to also
keep the profile parameter so we can use it then.

[markusheintz_, 2010/11/19 09:53:08]

That's how I understood it too. But I had to read the code in other places
first. So why not make this comment more expressive, so that it becomes clear
that the passed managed_pref_store is replaced after a policy refresh. I think
that this is really important to mention.

How about something like this:
"After each cloud policy refresh a new pref-store instance is created that holds
the latest cloud policies. The new instance replaces the old
(device_management_pref) one. The |profile| parameter is needed to construct the
new replacement device management pref store."

If you want to keep this comment as it is then at least add something like the
following to line 180:

"The passed PrefStore is replaced be a new instance after cloud policies are
refreshed".

+  //
+  // TODO(mnissler, danno): Refactor the pref store interface and refresh logic
+  // so refreshes can be handled by the pref store itself and the profile
+  // parameter be removed here.
+  //
   // This constructor should only be used internally, or by subclasses in
   // testing. The usual way to create a PrefValueStore is by creating a
   // PrefService.
@@ -195,19 +203,16 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
                  PrefStore* command_line_prefs,
                  PrefStore* user_prefs,
                  PrefStore* recommended_prefs,
-                 PrefStore* default_prefs);
+                 PrefStore* default_prefs,
+                 Profile* profile);
 
  private:
+  typedef std::map<std::string, Value::ValueType> PrefTypeMap;
+
   friend class PrefValueStoreTest;
   FRIEND_TEST_ALL_PREFIXES(PrefValueStoreTest,
                            TestRefreshPolicyPrefsCompletion);
 
-  scoped_ptr<PrefStore> pref_stores_[PrefNotifier::PREF_STORE_TYPE_MAX + 1];
-
-  // A mapping of preference names to their registered types.
-  typedef std::map<std::string, Value::ValueType> PrefTypeMap;
-  PrefTypeMap pref_types_;
-
   // Returns true if the preference with the given name has a value in the
   // given PrefStoreType, of the same value type as the preference was
   // registered with.
@@ -237,6 +242,14 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
       PrefStore* new_recommended_pref_store,
       AfterRefreshCallback* callback);
 
+  scoped_ptr<PrefStore> pref_stores_[PrefNotifier::PREF_STORE_TYPE_MAX + 1];
+
+  // A mapping of preference names to their registered types.
+  PrefTypeMap pref_types_;
+
+  // The associated profile, if applicable.

[markusheintz_, 2010/11/18 15:42:33]

When is it not applicable? Maybe it's worth to add that. Or remove applicable if
not needed.

[Mattias Nissler (ping if slow), 2010/11/18 16:06:38]

It's only applicable if this pref_value_store is associated with a profile, i.e.
used for a profile PrefService and not local state. I hope you now understood
the comment above, so you probably also get this one? Or is there a need to
change?

[markusheintz_, 2010/11/19 09:53:08]

I understood this comment, but I think if someone reads it the first question
that pops up: What does that mean "if applicable" ? So either we clarify that or
we remove it.

s/if applicable/. No profile is needed if no cloud policies are read./

or something similar. Just that people now why when their read it.

+  Profile* profile_;
+
   DISALLOW_COPY_AND_ASSIGN(PrefValueStore);
 };