Clean up pref change notification handling.

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 37c116780a83677f2c0506b79f1aeb6f27c38e47..cce000ea739b8ee08915069498a67934aec64b15 100644
--- a/chrome/browser/prefs/pref_value_store.h
+++ b/chrome/browser/prefs/pref_value_store.h
@@ -17,10 +17,12 @@
 #include "base/scoped_ptr.h"
 #include "base/values.h"
 #include "chrome/browser/browser_thread.h"
-#include "chrome/browser/prefs/pref_notifier.h"
+#include "chrome/common/notification_observer.h"
+#include "chrome/common/notification_registrar.h"
 #include "chrome/common/pref_store.h"
 
 class FilePath;
+class PrefNotifier;
 class PrefStore;
 class Profile;
 
@@ -28,26 +30,47 @@ class Profile;
 // (e.g., configuration policies, extensions, and user settings). It returns
 // the value of a Preference from the source with the highest priority, and
 // allows setting user-defined values for preferences that are not managed.
-// See PrefNotifier for a list of the available preference sources (PrefStores)
-// and their descriptions.
 //
 // Unless otherwise explicitly noted, all of the methods of this class must
 // be called on the UI thread.
-class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
+class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore>,
+                       public NotificationObserver {
  public:
-  // Returns a new PrefValueStore with all applicable PrefStores. The
-  // |pref_filename| points to the user preference file. The |profile| is the
-  // one to which these preferences apply; it may be NULL if we're dealing
-  // with the local state. If |pref_filename| is empty, the user PrefStore will
-  // not be created. If |user_only| is true, no PrefStores will be created
-  // other than the user and default PrefStores. This should not normally be
-  // called directly: the usual way to create a PrefValueStore is by creating a
-  // PrefService.
-  static PrefValueStore* CreatePrefValueStore(const FilePath& pref_filename,
-                                              Profile* profile,
-                                              bool user_only);
-
-  ~PrefValueStore();
+  // 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.
+  //
+  // |pref_notifier| facilitates broadcasting preference change notifications
+  // to the world.
+  //
+  // The |profile| parameter is used to construct a replacement device
+  // management pref store. This is done after policy refresh when we swap out
+  // the policy pref stores for new ones, so the |profile| pointer needs to be
+  // kept around for then. It is safe to pass a NULL pointer for local state
+  // preferences.
+  //
+  // TODO(mnissler, danno): Refactor the pref store interface and refresh logic
+  // so refreshes can be handled by the pref store itself without swapping
+  // stores. This way we can get rid of the profile pointer here.
+  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,
+                 PrefNotifier* pref_notifier,
+                 Profile* profile);
+  virtual ~PrefValueStore();
 
   // Gets the value for the given preference name that has a valid value type;
   // that is, the same type the preference was registered with, or NULL for
@@ -56,7 +79,7 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
   // Preference::GetValue() instead of calling this method directly.
   bool GetValue(const std::string& name, Value** out_value) const;
 
-  // Same as GetValue but only searches USER_STORE.
+  // Same as GetValue but only searches the user store.
   bool GetUserValue(const std::string& name, Value** out_value) const;
 
   // Adds a preference to the mapping of names to types.
@@ -87,14 +110,6 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
   // store setting a value that happens to be equal to the default does.
   bool HasPrefPath(const char* name) const;
 
-  // Called by the PrefNotifier when the value of the preference at |path| has
-  // changed, been added, or been removed in one of the PrefStores. The
-  // |new_store| is the PrefStoreType of the caller. Returns true if the
-  // effective value of the preference has changed, or if the store controlling
-  // the pref has changed. Virtual so it can be mocked for a unit test.
-  virtual bool PrefHasChanged(const char* path,
-                              PrefNotifier::PrefStoreType new_store);
-
   // Returns true if the PrefValueStore is read-only.  Because the managed
   // platform, device management and recommended PrefStores are always
   // read-only, the PrefValueStore as a whole is read-only if the PrefStore
@@ -103,21 +118,21 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
 
   // Alters the user-defined value of a preference. Even if the preference is
   // managed this method allows the user-defined value of the preference to be
-  // set. But GetValue calls will not return this value as long as the
-  // preference is managed. Instead GetValue will return the managed value
-  // of the preference. Note that the PrefValueStore takes the ownership of
-  // the value referenced by |in_value|. It is an error to call this when no
-  // user PrefStore has been set. Returns true if the user-set value of the
-  // preference was newly added or changed.
-  bool SetUserPrefValue(const char* name, Value* in_value);
-
-  // Removes a value from the user PrefStore. If a preference is managed
-  // this function should have no visible effect. Returns true if there was a
-  // user-set value to be removed.
-  bool RemoveUserPrefValue(const char* name);
-
-  // Sets a value in the DefaultPrefStore, which takes ownership of the Value.
-  void SetDefaultPrefValue(const char* name, Value* in_value);
+  // set. However, GetValue calls will not return this value as long as the
+  // preference is overriden by a store of higher precedence. Note that the
+  // PrefValueStore takes the ownership of the value referenced by |in_value|.
+  // It is an error to call this when no user PrefStore has been set. Triggers
+  // notifications if the user-visible value changes.
+  void SetUserPrefValue(const char* name, Value* in_value);
+
+  // Like SetUserPrefValue, but silently puts the value without triggering
+  // notifications.
+  void PutUserPrefValue(const char* name, Value* in_value);

[battre (please use the other), 2010/12/02 10:41:19]

How about SetUserPrefValueSilently? Put and Set sound very much alike to me.

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Done.

+
+  // Removes a value from the user PrefStore. If a preference is overriden by a
+  // store of higher precedence, this function will have no immediately visible
+  // effect. Triggers notifications if the user-visible value changes.
+  void RemoveUserPrefValue(const char* name);
 
   // These methods return true if a preference with the given name is in the
   // indicated pref store, even if that value is currently being overridden by
@@ -127,14 +142,6 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
   bool PrefValueInExtensionStore(const char* name) const;
   bool PrefValueInUserStore(const char* name) const;
 
-  // Returns true if a preference has an explicit value in any of the
-  // stores in the range specified by |first_checked_store| and
-  // |last_checked_store|, even if that value is currently being
-  // overridden by a higher-priority store.
-  bool PrefValueInStoreRange(const char* name,
-                             PrefNotifier::PrefStoreType first_checked_store,
-                             PrefNotifier::PrefStoreType last_checked_store);
-
   // These methods return true if a preference with the given name is actually
   // being controlled by the indicated pref store and not being overridden by
   // a higher-priority source.
@@ -146,94 +153,130 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
   // there is no higher-priority source controlling it.
   bool PrefValueUserModifiable(const char* name) const;

[battre (please use the other), 2010/12/02 11:51:34]

Can we make this const?

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Done.

 
-  // Returns the pref store type identifying the source that controls the
-  // Preference identified by |name|. If none of the sources has a value,
-  // PrefNotifier::INVALID_STORE is returned. In practice, the default PrefStore
-  // should always have a value for any registered preferencem, so INVALID_STORE
-  // indicates an error.
-  PrefNotifier::PrefStoreType ControllingPrefStoreForPref(
-      const char* name) const;
-
-  // Signature of callback triggered after policy refresh. Parameter is not
-  // passed as reference to prevent passing along a pointer to a set whose
-  // lifecycle is managed in another thread.
-  typedef Callback1<std::vector<std::string> >::Type AfterRefreshCallback;
-
-  // Called as a result of a notification of policy change. Triggers a reload of
-  // managed platform, device management and recommended preferences from policy
-  // from a Task on the FILE thread. The Task will take ownership of the
-  // |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();

[battre (please use the other), 2010/12/02 11:51:34]

Can we make this const?

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Done.

 
  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. This is done after policy refresh when we swap out
-  // the policy pref stores for new ones, so the |profile| pointer needs to be
-  // kept around for then. It is safe to pass a NULL pointer for local state
-  // preferences.
-  //
-  // TODO(mnissler, danno): Refactor the pref store interface and refresh logic
-  // so refreshes can be handled by the pref store itself without swapping
-  // stores. This way we can get rid of the profile pointer 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,
-                 Profile* profile);
+  // PrefStores must be listed here in order from highest to lowest priority.

[danno, 2010/12/02 10:31:52]

Can these be private?

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Done.

+  //   MANAGED_PLATFORM contains all managed preference values that are
+  //       provided by a platform-specific policy mechanism (e.g. Windows
+  //       Group Policy).
+  //   DEVICE_MANAGEMENT contains all managed preference values supplied
+  //       by the device management server (cloud policy).
+  //   EXTENSION contains preference values set by extensions.
+  //   COMMAND_LINE contains preference values set by command-line switches.
+  //   USER contains all user-set preference values.
+  //   RECOMMENDED contains all recommended (policy) preference values.
+  //   DEFAULT contains all application default preference values.
+  enum PrefStoreType {
+    // INVALID_STORE is not associated with an actual PrefStore but used as
+    // an invalid marker, e.g. as a return value.
+    INVALID_STORE = -1,
+    MANAGED_PLATFORM_STORE = 0,
+    DEVICE_MANAGEMENT_STORE,
+    EXTENSION_STORE,
+    COMMAND_LINE_STORE,
+    USER_STORE,
+    RECOMMENDED_STORE,
+    DEFAULT_STORE,
+    PREF_STORE_TYPE_MAX = DEFAULT_STORE
+  };
 
  private:
+  // Keeps a PrefStore reference on behalf of the PrefValueStore and monitors
+  // the PrefStore for changes, forwarding notifications to PrefValueStore. This
+  // indirection is here for the sake of disambiguating notifications from the
+  // individual PrefStores.
+  class PrefStoreKeeper : public PrefStore::ObserverInterface {
+   public:
+    PrefStoreKeeper();
+    virtual ~PrefStoreKeeper();
+
+    // Takes ownership of |pref_store|.
+    void Initialize(PrefValueStore* store,
+                    PrefStore* pref_store,
+                    PrefStoreType type);
+
+    PrefStore* store() { return pref_store_.get(); }
+    const PrefStore* store() const { return pref_store_.get(); }
+
+   private:
+    // PrefStore::ObserverInterface implementation.
+    virtual void OnPrefValueChanged(const std::string& key);
+    virtual void OnInitializationCompleted();
+
+    // PrefValueStore this keeper is part of.
+    PrefValueStore* pref_value_store_;
+
+    // The PrefStore managed by this keeper.
+    scoped_ptr<PrefStore> pref_store_;
+
+    // Type of the pref store.
+    PrefStoreType type_;
+
+    DISALLOW_COPY_AND_ASSIGN(PrefStoreKeeper);
+  };
+
   typedef std::map<std::string, Value::ValueType> PrefTypeMap;
 
   friend class PrefValueStoreTest;
+  FRIEND_TEST_ALL_PREFIXES(PrefValueStoreTest, TestPolicyRefresh);
   FRIEND_TEST_ALL_PREFIXES(PrefValueStoreTest,
                            TestRefreshPolicyPrefsCompletion);
+  FRIEND_TEST_ALL_PREFIXES(PrefValueStoreTest,
+                           TestConcurrentPolicyRefresh);
+
+  // Returns true if the actual value is a valid type for the expected type when

[battre (please use the other), 2010/12/02 11:51:34]

actual value? This description is not quite clear to me.

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Changed to "actual type".

+  // found in the given store.
+  static bool IsValidType(Value::ValueType expected,
+                          Value::ValueType actual,
+                          PrefStoreType store);
 
   // 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;
+  bool PrefValueInStore(const char* name, PrefStoreType store) const;
+
+  // Returns true if a preference has an explicit value in any of the
+  // stores in the range specified by |first_checked_store| and
+  // |last_checked_store|, even if that value is currently being
+  // overridden by a higher-priority store.
+  bool PrefValueInStoreRange(const char* name,
+                             PrefStoreType first_checked_store,
+                             PrefStoreType last_checked_store);

[battre (please use the other), 2010/12/02 11:51:34]

can we make this const?

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Done.

+
+  // Returns the pref store type identifying the source that controls the
+  // Preference identified by |name|. If none of the sources has a value,
+  // INVALID_STORE is returned. In practice, the default PrefStore
+  // should always have a value for any registered preferencem, so INVALID_STORE
+  // indicates an error.
+  PrefStoreType ControllingPrefStoreForPref(const char* name) const;
 
   // Get a value from the specified store type.
   bool GetValueFromStore(const char* name,
-                         PrefNotifier::PrefStoreType store,
+                         PrefStoreType store,
                          Value** out_value) const;
 
+  // Called upon changes in individual pref stores in order to determine whether
+  // the user-visible pref value has changed. Triggers the change notification
+  // if the effective value of the preference has changed, or if the store
+  // controlling the pref has changed.
+  void NotifyPrefChanged(const char* path, PrefStoreType new_store);

[danno, 2010/12/02 10:31:52]

OnPrefValueChanged?

[battre (please use the other), 2010/12/02 11:51:34]

can we make this const?

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Sending a notification has side effects. So making this const would be lying.

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

As discussed, we want to keep this since it hosts the
check-if-user-visible-value-changed logic.

+
+  // Called as a result of a notification of policy change. Triggers a reload of
+  // managed platform, device management and recommended preferences from policy
+  // from a Task on the FILE thread.
+  void RefreshPolicyPrefs();
+
   // 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);
+      PrefStore* new_recommended_pref_store);
 
   // Called during policy refresh to do the ReadPrefs on the FILE thread.
   // RefreshPolicyPrefsOnFileThread takes ownership of the |callback| object.
@@ -241,10 +284,39 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
       BrowserThread::ID calling_thread_id,
       PrefStore* new_managed_platform_pref_store,
       PrefStore* new_device_management_pref_store,
-      PrefStore* new_recommended_pref_store,
-      AfterRefreshCallback* callback);
+      PrefStore* new_recommended_pref_store);
+
+  // NotificationObserver methods:
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details);
+
+  // Called from the PrefStoreKeeper implementation when a pref value for |key|
+  // changed in the pref store for |type|.
+  void PrefValueChangedInStore(const std::string& key, PrefStoreType type);

[danno, 2010/12/02 10:31:52]

Is this not the same as NotifyPrefChanged?

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

No, it's the handler for OnPrefValueChanged notifications received from the
PrefStores.

 
-  scoped_ptr<PrefStore> pref_stores_[PrefNotifier::PREF_STORE_TYPE_MAX + 1];
+  // Handle the event that the store for |type| has completed initialization.
+  void InitializationCompleteForStore(PrefStoreType type);

[danno, 2010/12/02 10:31:52]

OnInitializedCompleted?

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Done.

+
+  // Initializes a pref store keeper. Sets up a PrefStoreKeeper that will take
+  // ownership of the passed |pref_store|.
+  void InitPrefStore(PrefStoreType type, PrefStore* pref_store);
+
+  // Checks whether initialization is completed and tells the notifier if that
+  // is the case.
+  void CheckInitializationCompleted();
+
+  // Get the PrefStore pointer for the given type. May return NULL if there is
+  // no PrefStore for that type.
+  PrefStore* GetPrefStore(PrefStoreType type);

[danno, 2010/12/02 10:31:52]

inline?

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Done.

+  const PrefStore* GetPrefStore(PrefStoreType type) const;

[danno, 2010/12/02 10:31:52]

inline?

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Done.

+
+  // Keeps the PrefStore references in order of precedence.
+  PrefStoreKeeper pref_stores_[PREF_STORE_TYPE_MAX + 1];
+
+  // Used for generation PREF_CHANGED notifications. This is a weak reference,

[battre (please use the other), 2010/12/02 11:51:34]

nit: "for generating" or "for generation of"

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Done.

+  // since the notifier is owned by the corresponding PrefService.

[danno, 2010/12/02 10:31:52]

Add initialization completed comment.

[Mattias Nissler (ping if slow), 2010/12/02 16:38:24]

Done.

+  PrefNotifier* pref_notifier_;
 
   // A mapping of preference names to their registered types.
   PrefTypeMap pref_types_;
@@ -254,6 +326,9 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
   // upon policy refresh.
   Profile* profile_;
 
+  // TODO(mnissler): Remove this after cleaning up policy refresh handling.
+  NotificationRegistrar registrar_;
+
   DISALLOW_COPY_AND_ASSIGN(PrefValueStore);
 };