Clean up pref change notification handling.

chrome/browser/prefs/pref_service_unittest.cc

Index: chrome/browser/prefs/pref_service_unittest.cc
diff --git a/chrome/browser/prefs/pref_service_unittest.cc b/chrome/browser/prefs/pref_service_unittest.cc
index e7c737c593a3900d032ff08359ae431cbf0e10c3..7ad5d2e993ab986ae1f41980b9bb5b2a87f94f23 100644
--- a/chrome/browser/prefs/pref_service_unittest.cc
+++ b/chrome/browser/prefs/pref_service_unittest.cc
@@ -13,13 +13,14 @@
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/prefs/command_line_pref_store.h"
 #include "chrome/browser/prefs/default_pref_store.h"
-#include "chrome/browser/prefs/dummy_pref_store.h"
+#include "chrome/browser/prefs/testing_pref_store.h"
 #include "chrome/browser/prefs/pref_change_registrar.h"
 #include "chrome/browser/prefs/pref_value_store.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
-#include "chrome/common/notification_observer_mock.h"
-#include "chrome/common/notification_service.h"
+#include "chrome/common/notification_details.h"
+#include "chrome/common/notification_observer.h"
+#include "chrome/common/notification_source.h"
 #include "chrome/common/notification_type.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/testing_pref_service.h"
@@ -27,47 +28,46 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
+using testing::AllOf;
 using testing::Mock;
 using testing::Pointee;
 using testing::Property;
+using testing::Truly;
 
 namespace {
 
+MATCHER_P3(PrefValueMatches, prefs, pref_name, value, "") {

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

Add comment please.

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

Done.

+  const PrefService::Preference* pref =
+      prefs->FindPreference(pref_name.c_str());
+  if (!pref)
+    return false;
+
+  const Value* actual_value = pref->GetValue();
+  if (!actual_value)
+    return value == NULL;
+  if (!value)
+    return actual_value == NULL;
+  return value->Equals(actual_value);
+}
+
 class TestPrefObserver : public NotificationObserver {
  public:
-  TestPrefObserver(const PrefService* prefs,
-                   const std::string& pref_name,
-                   const std::string& new_pref_value)
-      : observer_fired_(false),
-        prefs_(prefs),
-        pref_name_(pref_name),
-        new_pref_value_(new_pref_value) {}
+  TestPrefObserver() {}
   virtual ~TestPrefObserver() {}
 
-  virtual void Observe(NotificationType type,
-                       const NotificationSource& source,
-                       const NotificationDetails& details) {
-    EXPECT_EQ(type.value, NotificationType::PREF_CHANGED);
-    const PrefService* prefs_in = Source<PrefService>(source).ptr();
-    EXPECT_EQ(prefs_in, prefs_);
-    const std::string* pref_name_in = Details<std::string>(details).ptr();
-    EXPECT_EQ(*pref_name_in, pref_name_);
-    EXPECT_EQ(new_pref_value_, prefs_in->GetString("homepage"));
-    observer_fired_ = true;
+  MOCK_METHOD3(Observe, void(NotificationType type,
+                             const NotificationSource& source,
+                             const NotificationDetails& details));
+
+  void Expect(const PrefService* prefs,
+              const std::string& pref_name,
+              const Value* value) {
+    EXPECT_CALL(*this, Observe(NotificationType(NotificationType::PREF_CHANGED),
+                               Source<PrefService>(prefs),
+                               Property(&Details<std::string>::ptr,
+                                        Pointee(pref_name))))
+        .With(PrefValueMatches(prefs, pref_name, value));
   }
-
-  bool observer_fired() { return observer_fired_; }
-
-  void Reset(const std::string& new_pref_value) {
-    observer_fired_ = false;
-    new_pref_value_ = new_pref_value;
-  }
-
- private:
-  bool observer_fired_;
-  const PrefService* prefs_;
-  const std::string pref_name_;
-  std::string new_pref_value_;
 };
 
 }  // namespace
@@ -103,33 +103,34 @@ TEST(PrefServiceTest, NoObserverFire) {
   const char pref_name[] = "homepage";
   prefs.RegisterStringPref(pref_name, std::string());
 
-  const std::string new_pref_value("http://www.google.com/");
-  TestPrefObserver obs(&prefs, pref_name, new_pref_value);
-
+  const char new_pref_value[] = "http://www.google.com/";
+  TestPrefObserver obs;
   PrefChangeRegistrar registrar;
   registrar.Init(&prefs);
   registrar.Add(pref_name, &obs);
+
   // This should fire the checks in TestPrefObserver::Observe.
+  StringValue expected_value(new_pref_value);
+  obs.Expect(&prefs, pref_name, &expected_value);
   prefs.SetString(pref_name, new_pref_value);
-
-  // Make sure the observer was actually fired.
-  EXPECT_TRUE(obs.observer_fired());
+  Mock::VerifyAndClearExpectations(&obs);
 
   // Setting the pref to the same value should not set the pref value a second
   // time.
-  obs.Reset(new_pref_value);
+  EXPECT_CALL(obs, Observe(_, _, _)).Times(0);
   prefs.SetString(pref_name, new_pref_value);
-  EXPECT_FALSE(obs.observer_fired());
+  Mock::VerifyAndClearExpectations(&obs);
 
   // Clearing the pref should cause the pref to fire.
-  obs.Reset(std::string());
+  StringValue expected_default_value("");
+  obs.Expect(&prefs, pref_name, &expected_default_value);
   prefs.ClearPref(pref_name);
-  EXPECT_TRUE(obs.observer_fired());
+  Mock::VerifyAndClearExpectations(&obs);
 
   // Clearing the pref again should not cause the pref to fire.
-  obs.Reset(std::string());
+  EXPECT_CALL(obs, Observe(_, _, _)).Times(0);
   prefs.ClearPref(pref_name);
-  EXPECT_FALSE(obs.observer_fired());
+  Mock::VerifyAndClearExpectations(&obs);
 }
 
 TEST(PrefServiceTest, HasPrefPath) {
@@ -157,35 +158,38 @@ TEST(PrefServiceTest, Observers) {
   prefs.SetUserPref(pref_name, Value::CreateStringValue("http://www.cnn.com"));
   prefs.RegisterStringPref(pref_name, std::string());
 
-  const std::string new_pref_value("http://www.google.com/");
-  TestPrefObserver obs(&prefs, pref_name, new_pref_value);
+  const char new_pref_value[] = "http://www.google.com/";
+  StringValue expected_new_pref_value(new_pref_value);
+  TestPrefObserver obs;
   PrefChangeRegistrar registrar;
   registrar.Init(&prefs);
   registrar.Add(pref_name, &obs);
+
   // This should fire the checks in TestPrefObserver::Observe.
+  obs.Expect(&prefs, pref_name, &expected_new_pref_value);
   prefs.SetString(pref_name, new_pref_value);
-
-  // Make sure the tests were actually run.
-  EXPECT_TRUE(obs.observer_fired());
+  Mock::VerifyAndClearExpectations(&obs);
 
   // Now try adding a second pref observer.
-  const std::string new_pref_value2("http://www.youtube.com/");
-  obs.Reset(new_pref_value2);
-  TestPrefObserver obs2(&prefs, pref_name, new_pref_value2);
+  const char new_pref_value2[] = "http://www.youtube.com/";
+  StringValue expected_new_pref_value2(new_pref_value2);
+  TestPrefObserver obs2;
+  obs.Expect(&prefs, pref_name, &expected_new_pref_value2);
+  obs2.Expect(&prefs, pref_name, &expected_new_pref_value2);
   registrar.Add(pref_name, &obs2);
   // This should fire the checks in obs and obs2.
   prefs.SetString(pref_name, new_pref_value2);
-  EXPECT_TRUE(obs.observer_fired());
-  EXPECT_TRUE(obs2.observer_fired());
+  Mock::VerifyAndClearExpectations(&obs);
+  Mock::VerifyAndClearExpectations(&obs2);
 
   // Make sure obs2 still works after removing obs.
   registrar.Remove(pref_name, &obs);
-  obs.Reset(std::string());
-  obs2.Reset(new_pref_value);
+  EXPECT_CALL(obs, Observe(_, _, _)).Times(0);
+  obs2.Expect(&prefs, pref_name, &expected_new_pref_value);
   // This should only fire the observer in obs2.
   prefs.SetString(pref_name, new_pref_value);
-  EXPECT_FALSE(obs.observer_fired());
-  EXPECT_TRUE(obs2.observer_fired());
+  Mock::VerifyAndClearExpectations(&obs);
+  Mock::VerifyAndClearExpectations(&obs2);
 }
 
 TEST(PrefServiceTest, ProxyFromCommandLineNotPolicy) {
@@ -341,24 +345,11 @@ class PrefServiceSetValueTest : public testing::Test {
   static const char kValue[];
 
   PrefServiceSetValueTest()
-      : name_string_(kName),
-        null_value_(Value::CreateNullValue()) {}
-
-  void SetExpectNoNotification() {
-    EXPECT_CALL(observer_, Observe(_, _, _)).Times(0);
-  }
-
-  void SetExpectPrefChanged() {
-    EXPECT_CALL(observer_,
-                Observe(NotificationType(NotificationType::PREF_CHANGED), _,
-                        Property(&Details<std::string>::ptr,
-                                 Pointee(name_string_))));
-  }
+      : null_value_(Value::CreateNullValue()) {}
 
   TestingPrefService prefs_;
-  std::string name_string_;
   scoped_ptr<Value> null_value_;
-  NotificationObserverMock observer_;
+  TestPrefObserver observer_;
 };
 
 const char PrefServiceSetValueTest::kName[] = "name";
@@ -366,8 +357,7 @@ const char PrefServiceSetValueTest::kValue[] = "value";
 
 TEST_F(PrefServiceSetValueTest, SetStringValue) {
   const char default_string[] = "default";
-  const scoped_ptr<Value> default_value(
-      Value::CreateStringValue(default_string));
+  StringValue default_value(default_string);
   prefs_.RegisterStringPref(kName, default_string);
 
   PrefChangeRegistrar registrar;
@@ -375,18 +365,18 @@ TEST_F(PrefServiceSetValueTest, SetStringValue) {
   registrar.Add(kName, &observer_);
 
   // Changing the controlling store from default to user triggers notification.
-  SetExpectPrefChanged();
-  prefs_.Set(kName, *default_value);
+  observer_.Expect(&prefs_, kName, &default_value);
+  prefs_.Set(kName, default_value);
   Mock::VerifyAndClearExpectations(&observer_);
 
-  SetExpectNoNotification();
-  prefs_.Set(kName, *default_value);
+  EXPECT_CALL(observer_, Observe(_, _, _)).Times(0);
+  prefs_.Set(kName, default_value);
   Mock::VerifyAndClearExpectations(&observer_);
 
-  const scoped_ptr<Value> new_value(Value::CreateStringValue(kValue));
-  SetExpectPrefChanged();
-  prefs_.Set(kName, *new_value);
-  EXPECT_EQ(kValue, prefs_.GetString(kName));
+  StringValue new_value(kValue);
+  observer_.Expect(&prefs_, kName, &new_value);
+  prefs_.Set(kName, new_value);
+  Mock::VerifyAndClearExpectations(&observer_);
 }
 
 TEST_F(PrefServiceSetValueTest, SetDictionaryValue) {
@@ -397,30 +387,23 @@ TEST_F(PrefServiceSetValueTest, SetDictionaryValue) {
 
   // Dictionary values are special: setting one to NULL is the same as clearing
   // the user value, allowing the NULL default to take (or keep) control.
-  SetExpectNoNotification();
+  EXPECT_CALL(observer_, Observe(_, _, _)).Times(0);
   prefs_.Set(kName, *null_value_);
   Mock::VerifyAndClearExpectations(&observer_);
 
   DictionaryValue new_value;
   new_value.SetString(kName, kValue);
-  SetExpectPrefChanged();
+  observer_.Expect(&prefs_, kName, &new_value);
   prefs_.Set(kName, new_value);
   Mock::VerifyAndClearExpectations(&observer_);
-  DictionaryValue* dict = prefs_.GetMutableDictionary(kName);
-  EXPECT_EQ(1U, dict->size());
-  std::string out_value;
-  dict->GetString(kName, &out_value);
-  EXPECT_EQ(kValue, out_value);
 
-  SetExpectNoNotification();
+  EXPECT_CALL(observer_, Observe(_, _, _)).Times(0);
   prefs_.Set(kName, new_value);
   Mock::VerifyAndClearExpectations(&observer_);
 
-  SetExpectPrefChanged();
+  observer_.Expect(&prefs_, kName, null_value_.get());
   prefs_.Set(kName, *null_value_);
   Mock::VerifyAndClearExpectations(&observer_);
-  dict = prefs_.GetMutableDictionary(kName);
-  EXPECT_EQ(0U, dict->size());
 }
 
 TEST_F(PrefServiceSetValueTest, SetListValue) {
@@ -431,28 +414,21 @@ TEST_F(PrefServiceSetValueTest, SetListValue) {
 
   // List values are special: setting one to NULL is the same as clearing the
   // user value, allowing the NULL default to take (or keep) control.
-  SetExpectNoNotification();
+  EXPECT_CALL(observer_, Observe(_, _, _)).Times(0);
   prefs_.Set(kName, *null_value_);
   Mock::VerifyAndClearExpectations(&observer_);
 
   ListValue new_value;
   new_value.Append(Value::CreateStringValue(kValue));
-  SetExpectPrefChanged();
+  observer_.Expect(&prefs_, kName, &new_value);
   prefs_.Set(kName, new_value);
   Mock::VerifyAndClearExpectations(&observer_);
-  const ListValue* list = prefs_.GetMutableList(kName);
-  ASSERT_EQ(1U, list->GetSize());
-  std::string out_value;
-  list->GetString(0, &out_value);
-  EXPECT_EQ(kValue, out_value);
 
-  SetExpectNoNotification();
+  EXPECT_CALL(observer_, Observe(_, _, _)).Times(0);
   prefs_.Set(kName, new_value);
   Mock::VerifyAndClearExpectations(&observer_);
 
-  SetExpectPrefChanged();
+  observer_.Expect(&prefs_, kName, null_value_.get());
   prefs_.Set(kName, *null_value_);
   Mock::VerifyAndClearExpectations(&observer_);
-  list = prefs_.GetMutableList(kName);
-  EXPECT_EQ(0U, list->GetSize());
 }