Introduce a new way of using ScopedUserPrefUpdate that ensures notifications are sent.

This new ScopedUserPrefUpdate class should subsitute access to GetMutableList() and GetMutableDictionary() in PrefService. It ensures that update notifications are sent.

BUG=none
TEST=none

Review URL: http://codereview.chromium.org/6757011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@79821 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 245 insertions, 49 deletions

diff --git a/chrome/browser/prefs/pref_service.cc b/chrome/browser/prefs/pref_service.cc
index 8e7563f..06b45c3 100644
--- a/chrome/browser/prefs/pref_service.cc
+++ b/chrome/browser/prefs/pref_service.cc
@@ -548,7 +548,9 @@ void PrefService::RegisterInt64Pref(const char* path, int64 default_value) {
       path, Value::CreateStringValue(base::Int64ToString(default_value)));
 }
 
-DictionaryValue* PrefService::GetMutableDictionary(const char* path) {
+Value* PrefService::GetMutableUserPref(const char* path,
+                                       Value::ValueType type) {
+  CHECK(type == Value::TYPE_DICTIONARY || type == Value::TYPE_LIST);
   DCHECK(CalledOnValidThread());
   DLOG_IF(WARNING, IsManagedPreference(path)) <<
       "Attempt to change managed preference " << path;
@@ -558,54 +560,36 @@ DictionaryValue* PrefService::GetMutableDictionary(const char* path) {
     NOTREACHED() << "Trying to get an unregistered pref: " << path;
     return NULL;
   }
-  if (pref->GetType() != Value::TYPE_DICTIONARY) {
-    NOTREACHED() << "Wrong type for GetMutableDictionary: " << path;
+  if (pref->GetType() != type) {
+    NOTREACHED() << "Wrong type for GetMutableValue: " << path;
     return NULL;
   }
 
-  DictionaryValue* dict = NULL;
-  Value* tmp_value = NULL;
   // Look for an existing preference in the user store. If it doesn't
   // exist or isn't the correct type, create a new user preference.
-  if (user_pref_store_->GetMutableValue(path, &tmp_value)
+  Value* value = NULL;
+  if (user_pref_store_->GetMutableValue(path, &value)
           != PersistentPrefStore::READ_OK ||
-      !tmp_value->IsType(Value::TYPE_DICTIONARY)) {
-    dict = new DictionaryValue;
-    user_pref_store_->SetValueSilently(path, dict);
-  } else {
-    dict = static_cast<DictionaryValue*>(tmp_value);
+      !value->IsType(type)) {
+    if (type == Value::TYPE_DICTIONARY) {
+      value = new DictionaryValue;
+    } else if (type == Value::TYPE_LIST) {
+      value = new ListValue;
+    } else {
+      NOTREACHED();
+    }
+    user_pref_store_->SetValueSilently(path, value);
   }
-  return dict;
+  return value;
 }
 
-ListValue* PrefService::GetMutableList(const char* path) {
-  DCHECK(CalledOnValidThread());
-  DLOG_IF(WARNING, IsManagedPreference(path)) <<
-      "Attempt to change managed preference " << path;
-
-  const Preference* pref = FindPreference(path);
-  if (!pref) {
-    NOTREACHED() << "Trying to get an unregistered pref: " << path;
-    return NULL;
-  }
-  if (pref->GetType() != Value::TYPE_LIST) {
-    NOTREACHED() << "Wrong type for GetMutableList: " << path;
-    return NULL;
-  }
+DictionaryValue* PrefService::GetMutableDictionary(const char* path) {
+  return static_cast<DictionaryValue*>(
+      GetMutableUserPref(path, Value::TYPE_DICTIONARY));
+}
 
-  ListValue* list = NULL;
-  Value* tmp_value = NULL;
-  // Look for an existing preference in the user store. If it doesn't
-  // exist or isn't the correct type, create a new user preference.
-  if (user_pref_store_->GetMutableValue(path, &tmp_value)
-          != PersistentPrefStore::READ_OK ||
-      !tmp_value->IsType(Value::TYPE_LIST)) {
-    list = new ListValue;
-    user_pref_store_->SetValueSilently(path, list);
-  } else {
-    list = static_cast<ListValue*>(tmp_value);
-  }
-  return list;
+ListValue* PrefService::GetMutableList(const char* path) {
+  return static_cast<ListValue*>(GetMutableUserPref(path, Value::TYPE_LIST));
 }
 
 void PrefService::ReportUserPrefChanged(const std::string& key) {
diff --git a/chrome/browser/prefs/pref_service.h b/chrome/browser/prefs/pref_service.h
index db1f126..7ff1970 100644
--- a/chrome/browser/prefs/pref_service.h
+++ b/chrome/browser/prefs/pref_service.h
@@ -29,6 +29,7 @@ class Profile;
 
 namespace subtle {
 class PrefMemberBase;
+class ScopedUserPrefUpdateBase;
 };
 
 class PrefService : public base::NonThreadSafe {
@@ -192,9 +193,10 @@ class PrefService : public base::NonThreadSafe {
   void ClearPref(const char* path);
 
   // If the path is valid (i.e., registered), update the pref value in the user
-  // prefs. Seting a null value on a preference of List or Dictionary type is
-  // equivalent to removing the user value for that preference, allowing the
-  // default value to take effect unless another value takes precedence.
+  // prefs.
+  // To set the value of dictionary or list values in the pref tree use
+  // Set(), but to modify the value of a dictionary or list use
+  // ScopedUserPrefUpdate.
   void Set(const char* path, const Value& value);
   void SetBoolean(const char* path, bool value);
   void SetInteger(const char* path, int value);
@@ -209,6 +211,10 @@ class PrefService : public base::NonThreadSafe {
   int64 GetInt64(const char* path) const;
   void RegisterInt64Pref(const char* path, int64 default_value);
 
+  // TODO(battre): Remove GetMutableDictionary and GetMutableList,
+  // once this has been substituted by the new ScopedUserPrefUpdate.
+  // http://crbug.com/58489
+  //
   // Used to set the value of dictionary or list values in the pref tree.  This
   // will create a dictionary or list if one does not exist in the pref tree.
   // This method returns NULL only if you're requesting an unregistered pref or
@@ -275,8 +281,11 @@ class PrefService : public base::NonThreadSafe {
   friend class PrefChangeRegistrar;
   friend class subtle::PrefMemberBase;
 
-  // Give access to ReportUserPrefChanged();
+  // Give access to ReportUserPrefChanged() and GetMutableUserPref().
+  // TODO(battre) Remove following line once everything uses the new
+  // ScopedUserPrefUpdate. http://crbug.com/58489
   friend class ScopedUserPrefUpdate;
+  friend class subtle::ScopedUserPrefUpdateBase;
 
   // Construct an incognito version of the pref service. Use
   // CreateIncognitoPrefService() instead of calling this constructor directly.
@@ -309,6 +318,15 @@ class PrefService : public base::NonThreadSafe {
   // This should only be called from the constructor.
   void InitFromStorage();
 
+  // Used to set the value of dictionary or list values in the user pref store.
+  // This will create a dictionary or list if one does not exist in the user
+  // pref store. This method returns NULL only if you're requesting an
+  // unregistered pref or a non-dict/non-list pref.
+  // |type| may only be Values::TYPE_DICTIONARY or Values::TYPE_LIST and
+  // |path| must point to a registered preference of type |type|.
+  // Ownership of the returned value remains at the user pref store.
+  Value* GetMutableUserPref(const char* path, Value::ValueType type);
+
   // The PrefValueStore provides prioritized preference values. It is created
   // and owned by this PrefService. Subclasses may access it for unit testing.
   scoped_ptr<PrefValueStore> pref_value_store_;
diff --git a/chrome/browser/prefs/scoped_user_pref_update.cc b/chrome/browser/prefs/scoped_user_pref_update.cc
index 3ef0029..bc033f1 100644
--- a/chrome/browser/prefs/scoped_user_pref_update.cc
+++ b/chrome/browser/prefs/scoped_user_pref_update.cc
@@ -4,11 +4,39 @@
 
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 
+#include "base/logging.h"
 #include "chrome/browser/prefs/pref_notifier.h"
 #include "chrome/browser/prefs/pref_service.h"
 
-ScopedUserPrefUpdate::ScopedUserPrefUpdate(
-    PrefService* service, const char* path)
+namespace subtle {
+
+ScopedUserPrefUpdateBase::ScopedUserPrefUpdateBase(PrefService* service,
+                                                   const char* path)
+    : service_(service),
+      path_(path),
+      value_(NULL) {}
+
+ScopedUserPrefUpdateBase::~ScopedUserPrefUpdateBase() {
+  Notify();
+}
+
+Value* ScopedUserPrefUpdateBase::Get(Value::ValueType type) {
+  if (!value_)
+    value_ = service_->GetMutableUserPref(path_.c_str(), type);
+  return value_;
+}
+
+void ScopedUserPrefUpdateBase::Notify() {
+  if (value_) {
+    service_->ReportUserPrefChanged(path_);
+    value_ = NULL;
+  }
+}
+
+}  // namespace subtle
+
+ScopedUserPrefUpdate::ScopedUserPrefUpdate(PrefService* service,
+                                           const char* path)
     : service_(service),
       path_(path) {}
 
diff --git a/chrome/browser/prefs/scoped_user_pref_update.h b/chrome/browser/prefs/scoped_user_pref_update.h
index 8992d09..69572fa 100644
--- a/chrome/browser/prefs/scoped_user_pref_update.h
+++ b/chrome/browser/prefs/scoped_user_pref_update.h
@@ -1,16 +1,63 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 //
 // A helper class that assists preferences in firing notifications when lists
-// are changed.
+// or dictionaries are changed.
 
 #ifndef CHROME_BROWSER_PREFS_SCOPED_USER_PREF_UPDATE_H_
 #define CHROME_BROWSER_PREFS_SCOPED_USER_PREF_UPDATE_H_
 #pragma once
 
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/scoped_ptr.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/values.h"
 #include "chrome/browser/prefs/pref_service.h"
 
+class DictionaryValue;
+class ListValue;
+class PrefService;
+
+namespace subtle {
+
+// Base class for ScopedUserPrefUpdateTemplate that contains the parts
+// that do not depend on ScopedUserPrefUpdateTemplate's template parameter.
+//
+// We need this base class mostly for making it a friend of PrefService
+// and getting access to PrefService::GetMutableUserPref and
+// PrefService::ReportUserPrefChanged.
+class ScopedUserPrefUpdateBase : public base::NonThreadSafe {
+ protected:
+  ScopedUserPrefUpdateBase(PrefService* service, const char* path);
+
+  // Calls Notify().
+  virtual ~ScopedUserPrefUpdateBase();
+
+  // Sets |value_| to |service_|->GetMutableUserPref and returns it.
+  Value* Get(Value::ValueType type);
+
+ private:
+  // If |value_| is not null, triggers a notification of PrefObservers and
+  // resets |value_|.
+  virtual void Notify();
+
+  // Weak pointer.
+  PrefService* service_;
+  // Path of the preference being updated.
+  std::string path_;
+  // Cache of value from user pref store (set between Get() and Notify() calls).
+  Value* value_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedUserPrefUpdateBase);
+};
+
+}  // namespace subtle
+
+// TODO(battre) This class is legacy and will be substituted by
+// ScopedUserPrefUpdateTemplate soon. http://crbug.com/58489
 class ScopedUserPrefUpdate {
  public:
   ScopedUserPrefUpdate(PrefService* service, const char* path);
@@ -21,4 +68,44 @@ class ScopedUserPrefUpdate {
   std::string path_;
 };
 
+// TODO(battre) Rename ScopedUserPrefUpdateTemplate to ScopedUserPrefUpdate
+// once the legacy class above is gone. http://crbug.com/58489
+
+// Class to support modifications to DictionaryValues and ListValues while
+// guaranteeing that PrefObservers are notified of changed values.
+//
+// This class may only be used on the UI thread as it requires access to the
+// PrefService.
+template <typename T, Value::ValueType type_enum_value>
+class ScopedUserPrefUpdateTemplate : public subtle::ScopedUserPrefUpdateBase {
+ public:
+  ScopedUserPrefUpdateTemplate(PrefService* service, const char* path)
+      : ScopedUserPrefUpdateBase(service, path) {}
+
+  // Triggers an update notification if Get() was called.
+  virtual ~ScopedUserPrefUpdateTemplate() {}
+
+  // Returns a mutable |T| instance that
+  // - is already in the user pref store, or
+  // - is (silently) created and written to the user pref store if none existed
+  //   before.
+  //
+  // Calling Get() implies that an update notification is necessary at
+  // destruction time.
+  //
+  // The ownership of the return value remains with the user pref store.
+  virtual T* Get() {
+    return static_cast<T*>(
+        subtle::ScopedUserPrefUpdateBase::Get(type_enum_value));
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ScopedUserPrefUpdateTemplate);
+};
+
+typedef ScopedUserPrefUpdateTemplate<DictionaryValue, Value::TYPE_DICTIONARY>
+    DictionaryPrefUpdate;
+typedef ScopedUserPrefUpdateTemplate<ListValue, Value::TYPE_LIST>
+    ListPrefUpdate;
+
 #endif  // CHROME_BROWSER_PREFS_SCOPED_USER_PREF_UPDATE_H_
diff --git a/chrome/browser/prefs/scoped_user_pref_update_unittest.cc b/chrome/browser/prefs/scoped_user_pref_update_unittest.cc
new file mode 100644
index 0000000..88faebe
--- /dev/null
+++ b/chrome/browser/prefs/scoped_user_pref_update_unittest.cc
@@ -0,0 +1,79 @@
+// Copyright (c) 2011 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.
+
+#include "chrome/browser/prefs/scoped_user_pref_update.h"
+#include "chrome/browser/prefs/pref_change_registrar.h"
+#include "chrome/browser/prefs/pref_observer_mock.h"
+#include "chrome/test/testing_pref_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using testing::Mock;
+
+class ScopedUserPrefUpdateTest : public testing::Test {
+ public:
+  ~ScopedUserPrefUpdateTest() {}
+
+ protected:
+  virtual void SetUp() {
+    prefs_.RegisterDictionaryPref(kPref);
+    registrar_.Init(&prefs_);
+    registrar_.Add(kPref, &observer_);
+  }
+
+  static const char kPref[];
+  static const char kKey[];
+  static const char kValue[];
+
+  TestingPrefService prefs_;
+  PrefObserverMock observer_;
+  PrefChangeRegistrar registrar_;
+};
+
+const char ScopedUserPrefUpdateTest::kPref[] = "name";
+const char ScopedUserPrefUpdateTest::kKey[] = "key";
+const char ScopedUserPrefUpdateTest::kValue[] = "value";
+
+TEST_F(ScopedUserPrefUpdateTest, RegularUse) {
+  // Dictionary that will be expected to be set at the end.
+  DictionaryValue expected_dictionary;
+  expected_dictionary.SetString(kKey, kValue);
+
+  {
+    EXPECT_CALL(observer_, Observe(_, _, _)).Times(0);
+    DictionaryPrefUpdate update(&prefs_, kPref);
+    DictionaryValue* value = update.Get();
+    ASSERT_TRUE(value);
+    value->SetString(kKey, kValue);
+
+    // The dictionary was created for us but the creation should have happened
+    // silently without notifications.
+    Mock::VerifyAndClearExpectations(&observer_);
+
+    // Modifications happen online and are instantly visible, though.
+    const DictionaryValue* current_value = prefs_.GetDictionary(kPref);
+    ASSERT_TRUE(current_value);
+    EXPECT_TRUE(expected_dictionary.Equals(current_value));
+
+    // Now we are leaving the scope of the update so we should be notified.
+    observer_.Expect(&prefs_, kPref, &expected_dictionary);
+  }
+  Mock::VerifyAndClearExpectations(&observer_);
+
+  const DictionaryValue* current_value = prefs_.GetDictionary(kPref);
+  ASSERT_TRUE(current_value);
+  EXPECT_TRUE(expected_dictionary.Equals(current_value));
+}
+
+TEST_F(ScopedUserPrefUpdateTest, NeverTouchAnything) {
+  const DictionaryValue* old_value = prefs_.GetDictionary(kPref);
+  EXPECT_CALL(observer_, Observe(_, _, _)).Times(0);
+  {
+    DictionaryPrefUpdate update(&prefs_, kPref);
+  }
+  const DictionaryValue* new_value = prefs_.GetDictionary(kPref);
+  EXPECT_EQ(old_value, new_value);
+  Mock::VerifyAndClearExpectations(&observer_);
+}
diff --git a/chrome/browser/prefs/testing_pref_store.h b/chrome/browser/prefs/testing_pref_store.h
index 2ec7696..048dcc7 100644
--- a/chrome/browser/prefs/testing_pref_store.h
+++ b/chrome/browser/prefs/testing_pref_store.h
@@ -31,6 +31,7 @@ class TestingPrefStore : public PersistentPrefStore {
 
   // PersistentPrefStore overrides:
   virtual ReadResult GetMutableValue(const std::string& key, Value** result);
+  virtual void ReportValueChanged(const std::string& key);
   virtual void SetValue(const std::string& key, Value* value);
   virtual void SetValueSilently(const std::string& key, Value* value);
   virtual void RemoveValue(const std::string& key);
@@ -39,8 +40,6 @@ class TestingPrefStore : public PersistentPrefStore {
   virtual bool WritePrefs();
   virtual void ScheduleWritePrefs() {}
   virtual void CommitPendingWrite() {}
-  // TODO(battre) remove this function
-  virtual void ReportValueChanged(const std::string& key);
 
   // Marks the store as having completed initialization.
   void SetInitializationCompleted();
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index dfba2fd..6625b70 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1426,6 +1426,7 @@
         'browser/prefs/pref_value_store_unittest.cc',
         'browser/prefs/proxy_config_dictionary_unittest.cc',
         'browser/prefs/proxy_prefs_unittest.cc',
+        'browser/prefs/scoped_user_pref_update_unittest.cc',
         'browser/prefs/session_startup_pref_unittest.cc',
         'browser/prerender/prerender_manager_unittest.cc',
         'browser/prerender/prerender_resource_handler_unittest.cc',