6 files changed, 244 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();