Notify pref observers when the PrefStore controlling a preference changes, even if its

effective value doesn't change. BUG=50772 TEST=unit tests (PrefValueStoreTest.PrefHasChanged) Review URL: http://codereview.chromium.org/3115016 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@56660 0039d316-1c4b-4281-b951-d872f2087c98
author: pam@chromium.org <pam@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2010-08-19 09:08:14 +0000
committer: pam@chromium.org <pam@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2010-08-19 09:08:14 +0000
commit: b3247a8028d460f7317cadb63bee50744a2e93ee (patch)
tree: 169c34d52546760c5614c067bb4f7cf28cb5ef02 /chrome/browser
parent: ba4f8a3a345aa860b99e04fcee04d167582d9979 (diff)
download: chromium_src-b3247a8028d460f7317cadb63bee50744a2e93ee.zip
chromium_src-b3247a8028d460f7317cadb63bee50744a2e93ee.tar.gz
chromium_src-b3247a8028d460f7317cadb63bee50744a2e93ee.tar.bz2
9 files changed, 173 insertions, 54 deletions
diff --git a/chrome/browser/extensions/extension_pref_store.cc b/chrome/browser/extensions/extension_pref_store.cc
index c8f4b91..e77fb0c 100644
--- a/chrome/browser/extensions/extension_pref_store.cc
+++ b/chrome/browser/extensions/extension_pref_store.cc
@@ -12,9 +12,11 @@
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/notification_service.h"
 
-ExtensionPrefStore::ExtensionPrefStore(Profile* profile)
+ExtensionPrefStore::ExtensionPrefStore(Profile* profile,
+                                       PrefNotifier::PrefStoreType type)
     : prefs_(new DictionaryValue()),
-      profile_(profile) {
+      profile_(profile),
+      type_(type) {
   RegisterObservers();
 }
 
@@ -114,8 +116,10 @@ void ExtensionPrefStore::UpdateOnePref(const char* path) {
     }
   }
 
-  if (pref_service)
-    pref_service->pref_notifier()->OnPreferenceSet(path, old_value.get());
+  if (pref_service) {
+    pref_service->pref_notifier()->OnPreferenceSet(
+        path, type_, old_value.get());
+  }
 }
 
 void ExtensionPrefStore::UpdatePrefs(const PrefValueMap* pref_values) {
diff --git a/chrome/browser/extensions/extension_pref_store.h b/chrome/browser/extensions/extension_pref_store.h
index 47eac8d..29db238 100644
--- a/chrome/browser/extensions/extension_pref_store.h
+++ b/chrome/browser/extensions/extension_pref_store.h
@@ -15,6 +15,7 @@
 #include "base/basictypes.h"
 #include "base/scoped_ptr.h"
 #include "base/stl_util-inl.h"
+#include "chrome/browser/pref_notifier.h"
 #include "chrome/common/notification_registrar.h"
 #include "chrome/common/pref_store.h"
 
@@ -31,7 +32,7 @@ class Value;
 class ExtensionPrefStore : public PrefStore,
                            public NotificationObserver {
  public:
-  explicit ExtensionPrefStore(Profile* profile);
+  ExtensionPrefStore(Profile* profile, PrefNotifier::PrefStoreType type);
   virtual ~ExtensionPrefStore();
 
   // Begins tracking the preference and value an extension wishes to set. This
@@ -112,6 +113,9 @@ class ExtensionPrefStore : public PrefStore,
   // extensions.
   Profile* profile_;
 
+  // My PrefStore type, assigned by the PrefValueStore.
+  PrefNotifier::PrefStoreType type_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionPrefStore);
 };
 
diff --git a/chrome/browser/extensions/extension_pref_store_unittest.cc b/chrome/browser/extensions/extension_pref_store_unittest.cc
index a05730a..c69f597 100644
--- a/chrome/browser/extensions/extension_pref_store_unittest.cc
+++ b/chrome/browser/extensions/extension_pref_store_unittest.cc
@@ -20,11 +20,12 @@ namespace {
 
 class TestExtensionPrefStore : public ExtensionPrefStore {
  public:
-  TestExtensionPrefStore() : ExtensionPrefStore(NULL),
-                             ext1(NULL),
-                             ext2(NULL),
-                             ext3(NULL),
-                             pref_service_(NULL) {
+  TestExtensionPrefStore()
+      : ExtensionPrefStore(NULL, PrefNotifier::EXTENSION_STORE),
+        ext1(NULL),
+        ext2(NULL),
+        ext3(NULL),
+        pref_service_(NULL) {
     // Can't use ASSERT_TRUE here because a constructor can't return a value.
     if (!temp_dir_.CreateUniqueTempDir()) {
       ADD_FAILURE() << "Failed to create temp dir";
diff --git a/chrome/browser/pref_notifier.cc b/chrome/browser/pref_notifier.cc
index 8cf5f1d..d73dfa9d 100644
--- a/chrome/browser/pref_notifier.cc
+++ b/chrome/browser/pref_notifier.cc
@@ -38,15 +38,15 @@ PrefNotifier::~PrefNotifier() {
 }
 
 void PrefNotifier::OnPreferenceSet(const char* pref_name,
+                                   PrefNotifier::PrefStoreType new_store,
                                    const Value* old_value) {
-  if (pref_value_store_->PrefHasChanged(pref_name, old_value))
+  if (pref_value_store_->PrefHasChanged(pref_name, new_store, old_value))
     FireObservers(pref_name);
 }
 
 void PrefNotifier::OnUserPreferenceSet(const char* pref_name,
                                        const Value* old_value) {
-  // TODO(pamg): Adjust to account for source PrefStore.
-  OnPreferenceSet(pref_name, old_value);
+  OnPreferenceSet(pref_name, PrefNotifier::USER_STORE, old_value);
 }
 
 void PrefNotifier::FireObservers(const char* path) {
diff --git a/chrome/browser/pref_notifier.h b/chrome/browser/pref_notifier.h
index 3779818..5392c45 100644
--- a/chrome/browser/pref_notifier.h
+++ b/chrome/browser/pref_notifier.h
@@ -51,14 +51,17 @@ class PrefNotifier : public NonThreadSafe,
 
   virtual ~PrefNotifier();
 
-  // For the given pref_name, fire any observer of the pref only if |old_value|
-  // is different from the current value.
-  // TODO(pamg): Also send notifications if the controlling store has changed.
-  void OnPreferenceSet(const char* pref_name, const Value* old_value);
-
-  // For the given pref_name, fire any observer of the pref only if |old_value|
-  // is different from the current value. Convenience method to be called when
-  // a preference is set in the USER_STORE.
+  // For the given pref_name, fire any observer of the pref if |old_value| is
+  // different from the current value, or if the store controlling the value
+  // has changed.
+  // TODO(pamg): Currently notifies in some cases when it shouldn't. See
+  // comments for PrefHasChanged() in pref_value_store.h.
+  void OnPreferenceSet(const char* pref_name,
+                       PrefNotifier::PrefStoreType new_store,
+                       const Value* old_value);
+
+  // Convenience method to be called when a preference is set in the
+  // USER_STORE. See OnPreferenceSet().
   void OnUserPreferenceSet(const char* pref_name, const Value* old_value);
 
   // For the given pref_name, fire any observer of the pref. Virtual so it can
diff --git a/chrome/browser/pref_notifier_unittest.cc b/chrome/browser/pref_notifier_unittest.cc
index a6c2da9..11f678e 100644
--- a/chrome/browser/pref_notifier_unittest.cc
+++ b/chrome/browser/pref_notifier_unittest.cc
@@ -64,7 +64,9 @@ class MockPrefValueStore : public PrefValueStore {
   virtual ~MockPrefValueStore() {}
 
   // This mock version returns true if the pref path starts with "changed".
-  virtual bool PrefHasChanged(const char* path, const Value* old_value) {
+  virtual bool PrefHasChanged(const char* path,
+                              PrefNotifier::PrefStoreType new_store,
+                              const Value* old_value) {
     std::string path_str(path);
     if (path_str.compare(0, 7, "changed") == 0)
       return true;
@@ -114,10 +116,16 @@ class PrefNotifierTest : public testing::Test {
 
 
 TEST_F(PrefNotifierTest, OnPreferenceSet) {
-  EXPECT_CALL(*notifier_, FireObservers(kChangedPref));
+  EXPECT_CALL(*notifier_, FireObservers(kChangedPref))
+      .Times(PrefNotifier::PREF_STORE_TYPE_MAX + 1);
   EXPECT_CALL(*notifier_, FireObservers(kUnchangedPref)).Times(0);
-  notifier_->OnPreferenceSet(kChangedPref, NULL);
-  notifier_->OnPreferenceSet(kUnchangedPref, NULL);
+
+  for (size_t i = 0; i <= PrefNotifier::PREF_STORE_TYPE_MAX; ++i) {
+    notifier_->OnPreferenceSet(kChangedPref,
+        static_cast<PrefNotifier::PrefStoreType>(i), NULL);
+    notifier_->OnPreferenceSet(kUnchangedPref,
+        static_cast<PrefNotifier::PrefStoreType>(i), NULL);
+  }
 }
 
 TEST_F(PrefNotifierTest, OnUserPreferenceSet) {
@@ -210,13 +218,13 @@ TEST_F(PrefNotifierTest, FireObservers) {
       Source<PrefService>(pref_service_.get()),
       Truly(DetailsAreChangedPref)));
   EXPECT_CALL(obs2_, Observe(_, _, _)).Times(0);
-  notifier_->OnPreferenceSet(kChangedPref, NULL);
+  notifier_->OnUserPreferenceSet(kChangedPref, NULL);
   Mock::VerifyAndClearExpectations(&obs1_);
   Mock::VerifyAndClearExpectations(&obs2_);
 
   EXPECT_CALL(obs1_, Observe(_, _, _)).Times(0);
   EXPECT_CALL(obs2_, Observe(_, _, _)).Times(0);
-  notifier_->OnPreferenceSet(kUnchangedPref, NULL);
+  notifier_->OnUserPreferenceSet(kUnchangedPref, NULL);
   Mock::VerifyAndClearExpectations(&obs1_);
   Mock::VerifyAndClearExpectations(&obs2_);
 
@@ -231,13 +239,13 @@ TEST_F(PrefNotifierTest, FireObservers) {
       Field(&NotificationType::value, NotificationType::PREF_CHANGED),
       Source<PrefService>(pref_service_.get()),
       Truly(DetailsAreChangedPref)));
-  notifier_->OnPreferenceSet(kChangedPref, NULL);
+  notifier_->OnUserPreferenceSet(kChangedPref, NULL);
   Mock::VerifyAndClearExpectations(&obs1_);
   Mock::VerifyAndClearExpectations(&obs2_);
 
   EXPECT_CALL(obs1_, Observe(_, _, _)).Times(0);
   EXPECT_CALL(obs2_, Observe(_, _, _)).Times(0);
-  notifier_->OnPreferenceSet(kUnchangedPref, NULL);
+  notifier_->OnUserPreferenceSet(kUnchangedPref, NULL);
   Mock::VerifyAndClearExpectations(&obs1_);
   Mock::VerifyAndClearExpectations(&obs2_);
 
@@ -249,13 +257,13 @@ TEST_F(PrefNotifierTest, FireObservers) {
       Field(&NotificationType::value, NotificationType::PREF_CHANGED),
       Source<PrefService>(pref_service_.get()),
       Truly(DetailsAreChangedPref)));
-  notifier_->OnPreferenceSet(kChangedPref, NULL);
+  notifier_->OnUserPreferenceSet(kChangedPref, NULL);
   Mock::VerifyAndClearExpectations(&obs1_);
   Mock::VerifyAndClearExpectations(&obs2_);
 
   EXPECT_CALL(obs1_, Observe(_, _, _)).Times(0);
   EXPECT_CALL(obs2_, Observe(_, _, _)).Times(0);
-  notifier_->OnPreferenceSet(kUnchangedPref, NULL);
+  notifier_->OnUserPreferenceSet(kUnchangedPref, NULL);
   Mock::VerifyAndClearExpectations(&obs1_);
   Mock::VerifyAndClearExpectations(&obs2_);
 
@@ -267,13 +275,13 @@ TEST_F(PrefNotifierTest, FireObservers) {
       Field(&NotificationType::value, NotificationType::PREF_CHANGED),
       Source<PrefService>(pref_service_.get()),
       Truly(DetailsAreChangedPref)));
-  notifier_->OnPreferenceSet(kChangedPref, NULL);
+  notifier_->OnUserPreferenceSet(kChangedPref, NULL);
   Mock::VerifyAndClearExpectations(&obs1_);
   Mock::VerifyAndClearExpectations(&obs2_);
 
   EXPECT_CALL(obs1_, Observe(_, _, _)).Times(0);
   EXPECT_CALL(obs2_, Observe(_, _, _)).Times(0);
-  notifier_->OnPreferenceSet(kUnchangedPref, NULL);
+  notifier_->OnUserPreferenceSet(kUnchangedPref, NULL);
 
   notifier_->RemovePrefObserver(kChangedPref, &obs2_);
   notifier_->RemovePrefObserver(kUnchangedPref, &obs2_);
diff --git a/chrome/browser/pref_value_store.cc b/chrome/browser/pref_value_store.cc
index d5c7ab4..6b6385d 100644
--- a/chrome/browser/pref_value_store.cc
+++ b/chrome/browser/pref_value_store.cc
@@ -31,7 +31,7 @@ PrefValueStore* PrefValueStore::CreatePrefValueStore(
 
   if (!user_only) {
     managed = ConfigurationPolicyPrefStore::CreateManagedPolicyPrefStore();
-    extension = new ExtensionPrefStore(profile);
+    extension = new ExtensionPrefStore(profile, PrefNotifier::EXTENSION_STORE);
     command_line = new CommandLinePrefStore(CommandLine::ForCurrentProcess());
     recommended =
         ConfigurationPolicyPrefStore::CreateRecommendedPolicyPrefStore();
@@ -95,11 +95,30 @@ bool PrefValueStore::HasPrefPath(const char* path) const {
 }
 
 bool PrefValueStore::PrefHasChanged(const char* path,
+                                    PrefNotifier::PrefStoreType new_store,
                                     const Value* old_value) {
+  DCHECK(new_store != PrefNotifier::INVALID_STORE);
+  // Replying that the pref has changed may cause extra work, but it should
+  // not be actively harmful.
+  if (new_store == PrefNotifier::INVALID_STORE)
+    return true;
+
   Value* new_value = NULL;
   GetValue(path, &new_value);
   // Some unit tests have no values for certain prefs.
-  return (!new_value || !old_value->Equals(new_value));
+  if (!new_value || !old_value->Equals(new_value))
+    return true;
+
+  // If there's a value in a store with lower priority than the |new_store|,
+  // and no value in a store with higher priority, assume the |new_store| just
+  // took control of the pref. (This assumption is wrong if the new value
+  // and store are both the same as the old one, but that situation should be
+  // rare, and reporting a change when none happened should not be harmful.)
+  if (PrefValueInStoreRange(path, new_store, false) &&
+      !PrefValueInStoreRange(path, new_store, true))
+    return true;
+
+  return false;
 }
 
 // Note the |DictionaryValue| referenced by the |PrefStore| user_prefs_
@@ -156,6 +175,23 @@ bool PrefValueStore::PrefValueInStore(const char* name,
   return pref_stores_[type]->prefs()->Get(name, &tmp_value);
 }
 
+bool PrefValueStore::PrefValueInStoreRange(const char* name,
+                                           PrefNotifier::PrefStoreType boundary,
+                                           bool higher_priority) {
+  // Higher priorities are lower PrefStoreType values. The range is
+  // non-inclusive of the boundary.
+  int start = higher_priority ? 0 : boundary + 1;
+  int end = higher_priority ? boundary - 1 :
+      PrefNotifier::PREF_STORE_TYPE_MAX;
+
+  for (int i = start; i <= end ; ++i) {
+    if (PrefValueInStore(name, static_cast<PrefNotifier::PrefStoreType>(i)))
+      return true;
+  }
+  return false;
+}
+
+
 PrefNotifier::PrefStoreType PrefValueStore::ControllingPrefStoreForPref(
     const char* name) {
   for (int i = 0; i <= PrefNotifier::PREF_STORE_TYPE_MAX; ++i) {
diff --git a/chrome/browser/pref_value_store.h b/chrome/browser/pref_value_store.h
index e0ac64d..2950d54 100644
--- a/chrome/browser/pref_value_store.h
+++ b/chrome/browser/pref_value_store.h
@@ -71,9 +71,17 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
   bool HasPrefPath(const char* name) const;
 
   // Returns true if the effective value of the preference has changed from its
-  // |old_value|. Virtual so it can be mocked for a unit test.
-  // TODO(pamg): Also return true when the controlling PrefStore changes.
-  virtual bool PrefHasChanged(const char* path, const Value* old_value);
+  // |old_value| (which should be the effective value of the preference as
+  // reported by GetValue() or the PrefService before the PrefStore changed it),
+  // or if the store controlling the pref has changed. Virtual so it can be
+  // mocked for a unit test.
+  // TODO(pamg): If we're setting the same value as we already had, into the
+  // same store that was controlling it before, and there's also a value set in
+  // a lower-priority store, *and* we're not the highest-priority store, then
+  // this will return true when it shouldn't. Fix that if it causes problems.
+  virtual bool PrefHasChanged(const char* path,
+                              PrefNotifier::PrefStoreType new_store,
+                              const Value* old_value);
 
   // Returns true if the PrefValueStore is read-only.
   // Because the managed and recommended PrefStores are always read-only, the
@@ -156,6 +164,14 @@ class PrefValueStore : public base::RefCountedThreadSafe<PrefValueStore> {
 
   bool PrefValueInStore(const char* name, PrefNotifier::PrefStoreType type);
 
+  // Returns true if the preference |name| is found in any PrefStore starting
+  // just beyond the |boundary|, non-inclusive, and checking either
+  // higher-priority stores (if |higher_priority| is true) or lower-priority
+  // stores.
+  bool PrefValueInStoreRange(const char* name,
+                             PrefNotifier::PrefStoreType boundary,
+                             bool higher_priority);
+
   // Returns the pref store type identifying the source that controls the
   // Preference identified by |name|. If none of the sources has a value,
   // INVALID is returned.
diff --git a/chrome/browser/pref_value_store_unittest.cc b/chrome/browser/pref_value_store_unittest.cc
index c159945..b33d67a 100644
--- a/chrome/browser/pref_value_store_unittest.cc
+++ b/chrome/browser/pref_value_store_unittest.cc
@@ -292,22 +292,69 @@ TEST_F(PrefValueStoreTest, HasPrefPath) {
 }
 
 TEST_F(PrefValueStoreTest, PrefHasChanged) {
-  ASSERT_TRUE(pref_value_store_->PrefValueInManagedStore(prefs::kHomepage));
-
-  // Pretend we used to have a different enforced value set.
-  scoped_ptr<Value> value(Value::CreateStringValue("http://www.youtube.com"));
-  EXPECT_TRUE(pref_value_store_->PrefHasChanged(prefs::kHomepage, value.get()));
-
-  // Pretend we used to have the same enforced value set.
-  value.reset(Value::CreateStringValue(enforced_pref::kHomepageValue));
-  EXPECT_FALSE(pref_value_store_->PrefHasChanged(prefs::kHomepage,
-      value.get()));
-
-  // Really set a new value in a lower-priority store.
-  Value* new_value = Value::CreateStringValue("http://www.chromium.org");
-  pref_value_store_->SetUserPrefValue(prefs::kHomepage, new_value);
-  EXPECT_FALSE(pref_value_store_->PrefHasChanged(prefs::kHomepage,
-      value.get()));
+  // Pref controlled by highest-priority store, set to same value in same store.
+  const char managed_pref_path[] = "managed_pref";
+  const char same_str[] = "same value";
+  scoped_ptr<Value> same_value(Value::CreateStringValue(same_str));
+  enforced_pref_store_->prefs()->SetString(managed_pref_path, same_str);
+  EXPECT_FALSE(pref_value_store_->PrefHasChanged(managed_pref_path,
+      static_cast<PrefNotifier::PrefStoreType>(0), same_value.get()));
+
+  // Pref controlled by highest-priority store, set to different value in
+  // same store.
+  const char other_str[] = "other value";
+  scoped_ptr<Value> other_value(Value::CreateStringValue(other_str));
+  EXPECT_TRUE(pref_value_store_->PrefHasChanged(managed_pref_path,
+      static_cast<PrefNotifier::PrefStoreType>(0), other_value.get()));
+
+  // Pref controlled by user store, set to same value in user store, no lower
+  // store has a value.
+  const char user_pref_path[] = "user_pref";
+  user_pref_store_->prefs()->SetString(user_pref_path, same_str);
+  EXPECT_FALSE(pref_value_store_->PrefHasChanged(user_pref_path,
+      PrefNotifier::USER_STORE, same_value.get()));
+
+  // Pref controlled by user store, set to new value in user store, no lower
+  // store has a value.
+  EXPECT_TRUE(pref_value_store_->PrefHasChanged(user_pref_path,
+      PrefNotifier::USER_STORE, other_value.get()));
+
+  // Pref controlled by user store, set to same value in user store, some lower
+  // store has a value.
+  const char third_str[] = "third value";
+  recommended_pref_store_->prefs()->SetString(user_pref_path, third_str);
+  // This is not necessarily the correct behavior, but it is the current
+  // behavior. See comments in pref_value_store.h and .cc.
+  EXPECT_TRUE(pref_value_store_->PrefHasChanged(user_pref_path,
+      PrefNotifier::USER_STORE, same_value.get()));
+
+  // Pref controlled by user store, set to new value in user store, some lower
+  // store has a value.
+  EXPECT_TRUE(pref_value_store_->PrefHasChanged(user_pref_path,
+      PrefNotifier::USER_STORE, other_value.get()));
+
+  // Pref controlled by user store, set to same value in managed store.
+  EXPECT_TRUE(pref_value_store_->PrefHasChanged(user_pref_path,
+      PrefNotifier::MANAGED_STORE, same_value.get()));
+
+  // Pref controlled by user store, set to new value in managed store.
+  EXPECT_TRUE(pref_value_store_->PrefHasChanged(user_pref_path,
+      PrefNotifier::MANAGED_STORE, other_value.get()));
+
+  // Pref controlled by highest-priority store, set to any value in user store.
+  EXPECT_FALSE(pref_value_store_->PrefHasChanged(managed_pref_path,
+      PrefNotifier::USER_STORE, same_value.get()));
+
+  // Pref controlled by lowest-priority store, set to same value in same store.
+  const char recommended_pref_path[] = "recommended_pref";
+  recommended_pref_store_->prefs()->SetString(recommended_pref_path, same_str);
+  EXPECT_FALSE(pref_value_store_->PrefHasChanged(recommended_pref_path,
+      PrefNotifier::PREF_STORE_TYPE_MAX, same_value.get()));
+
+  // Pref controlled by lowest-priority store, set to different value in same
+  // store.
+  EXPECT_TRUE(pref_value_store_->PrefHasChanged(recommended_pref_path,
+      PrefNotifier::PREF_STORE_TYPE_MAX, other_value.get()));
 }
 
 TEST_F(PrefValueStoreTest, ReadPrefs) {
author	pam@chromium.org <pam@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2010-08-19 09:08:14 +0000
committer	pam@chromium.org <pam@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2010-08-19 09:08:14 +0000
commit	b3247a8028d460f7317cadb63bee50744a2e93ee (patch)
tree	169c34d52546760c5614c067bb4f7cf28cb5ef02 /chrome/browser
parent	ba4f8a3a345aa860b99e04fcee04d167582d9979 (diff)
download	chromium_src-b3247a8028d460f7317cadb63bee50744a2e93ee.zip chromium_src-b3247a8028d460f7317cadb63bee50744a2e93ee.tar.gz chromium_src-b3247a8028d460f7317cadb63bee50744a2e93ee.tar.bz2