Move DeviceManagementPolicyProvider into the profile.

chrome/browser/prefs/pref_value_store.h

 // Copyright (c) 2010 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 CHROME_BROWSER_PREFS_PREF_VALUE_STORE_H_
 #define CHROME_BROWSER_PREFS_PREF_VALUE_STORE_H_
 #pragma once
 
 #include <map>
 #include <string>
@@ skipping 154 matching lines @@
   // |callback|. |callback| is called with the set of preferences changed by the
   // policy refresh. |callback| is called on the caller's thread as a Task
   // after RefreshPolicyPrefs has returned.
   void RefreshPolicyPrefs(AfterRefreshCallback* callback);
 
   // Returns true if there are proxy preferences in user-modifiable
   // preference stores (e.g. CommandLinePrefStore, ExtensionPrefStore)
   // 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.
   //   |device_management_prefs| contains all device management (cloud policy)
   //        preference values.
   //   |extension_prefs| contains preference values set by extensions.
   //   |command_line_prefs| contains preference values set by command-line
   //        switches.
   //   |user_prefs| contains all user-set preference values.
   //   |recommended_prefs| contains all recommended (policy) preference values.
   //   |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.
   PrefValueStore(PrefStore* managed_platform_prefs,
                  PrefStore* device_management_prefs,
                  PrefStore* extension_prefs,
                  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.
   bool PrefValueInStore(const char* name,
                         PrefNotifier::PrefStoreType store) const;
 
   // Get a value from the specified store type.
   bool GetValueFromStore(const char* name,
                          PrefNotifier::PrefStoreType store,
                          Value** out_value) const;
 
   // Called during policy refresh after ReadPrefs completes on the thread
   // that initiated the policy refresh. RefreshPolicyPrefsCompletion takes
   // ownership of the |callback| object.
   void RefreshPolicyPrefsCompletion(
       PrefStore* new_managed_platform_pref_store,
       PrefStore* new_device_management_pref_store,
       PrefStore* new_recommended_pref_store,
       AfterRefreshCallback* callback);
 
   // Called during policy refresh to do the ReadPrefs on the FILE thread.
   // RefreshPolicyPrefsOnFileThread takes ownership of the |callback| object.
   void RefreshPolicyPrefsOnFileThread(
       BrowserThread::ID calling_thread_id,
       PrefStore* new_managed_platform_pref_store,
       PrefStore* new_device_management_pref_store,
       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);
 };
 
 #endif  // CHROME_BROWSER_PREFS_PREF_VALUE_STORE_H_