Let PrefStore::GetValue return a const Value* instead of Value* and add PersistentPrefStore::GetMutableValue

BUG=none
TEST=none

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

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

31 files changed, 170 insertions, 90 deletions

diff --git a/chrome/browser/extensions/extension_pref_value_map.cc b/chrome/browser/extensions/extension_pref_value_map.cc
index 7e68da1..02ebcdf 100644
--- a/chrome/browser/extensions/extension_pref_value_map.cc
+++ b/chrome/browser/extensions/extension_pref_value_map.cc
@@ -151,7 +151,7 @@ const Value* ExtensionPrefValueMap::GetEffectivePrefValue(
   if (winner == entries_.end())
     return NULL;
 
-  Value* value = NULL;
+  const Value* value = NULL;
   const std::string& ext_id = winner->first;
   if (incognito)
     GetExtensionPrefValueMap(ext_id, true)->GetValue(key, &value);
@@ -179,7 +179,7 @@ ExtensionPrefValueMap::GetEffectivePrefValueController(
     if (install_time < winners_install_time)
       continue;
 
-    Value* value = NULL;
+    const Value* value = NULL;
     const PrefValueMap* prefs = GetExtensionPrefValueMap(ext_id, false);
     if (prefs->GetValue(key, &value)) {
       winner = i;
diff --git a/chrome/browser/policy/configuration_policy_pref_store.cc b/chrome/browser/policy/configuration_policy_pref_store.cc
index 6933116..cae0144 100644
--- a/chrome/browser/policy/configuration_policy_pref_store.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store.cc
@@ -42,7 +42,8 @@ class ConfigurationPolicyPrefKeeper
   virtual ~ConfigurationPolicyPrefKeeper();
 
   // Get a preference value.
-  PrefStore::ReadResult GetValue(const std::string& key, Value** result) const;
+  PrefStore::ReadResult GetValue(const std::string& key,
+                                 const Value** result) const;
 
   // Compute the set of preference names that are different in |keeper|. This
   // includes preferences that are missing in either one.
@@ -311,8 +312,8 @@ ConfigurationPolicyPrefKeeper::~ConfigurationPolicyPrefKeeper() {
 
 PrefStore::ReadResult
 ConfigurationPolicyPrefKeeper::GetValue(const std::string& key,
-                                        Value** result) const {
-  Value* stored_value = NULL;
+                                        const Value** result) const {
+  const Value* stored_value = NULL;
   if (!prefs_.GetValue(key, &stored_value))
     return PrefStore::READ_NO_VALUE;
 
@@ -777,7 +778,7 @@ bool ConfigurationPolicyPrefStore::IsInitializationComplete() const {
 
 PrefStore::ReadResult
 ConfigurationPolicyPrefStore::GetValue(const std::string& key,
-                                       Value** value) const {
+                                       const Value** value) const {
   if (policy_keeper_.get())
     return policy_keeper_->GetValue(key, value);
 
diff --git a/chrome/browser/policy/configuration_policy_pref_store.h b/chrome/browser/policy/configuration_policy_pref_store.h
index bd1a990..a66b72c 100644
--- a/chrome/browser/policy/configuration_policy_pref_store.h
+++ b/chrome/browser/policy/configuration_policy_pref_store.h
@@ -38,7 +38,8 @@ class ConfigurationPolicyPrefStore
   virtual void AddObserver(PrefStore::Observer* observer);
   virtual void RemoveObserver(PrefStore::Observer* observer);
   virtual bool IsInitializationComplete() const;
-  virtual ReadResult GetValue(const std::string& key, Value** result) const;
+  virtual ReadResult GetValue(const std::string& key,
+                              const Value** result) const;
 
   // ConfigurationPolicyProvider::Observer methods:
   virtual void OnUpdatePolicy();
diff --git a/chrome/browser/policy/configuration_policy_pref_store_unittest.cc b/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
index edd2e43..a8c46a3 100644
--- a/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
@@ -61,7 +61,7 @@ TEST_P(ConfigurationPolicyPrefStoreListTest, SetValue) {
   in_value->Append(Value::CreateStringValue("test2,"));
   provider_.AddPolicy(GetParam().type(), in_value);
   store_->OnUpdatePolicy();
-  Value* value;
+  const Value* value;
   EXPECT_EQ(PrefStore::READ_OK,
             store_->GetValue(GetParam().pref_name(), &value));
   EXPECT_TRUE(in_value->Equals(value));
@@ -99,7 +99,7 @@ TEST_P(ConfigurationPolicyPrefStoreStringTest, SetValue) {
   provider_.AddPolicy(GetParam().type(),
                       Value::CreateStringValue("http://chromium.org"));
   store_->OnUpdatePolicy();
-  Value* value;
+  const Value* value;
   EXPECT_EQ(PrefStore::READ_OK,
             store_->GetValue(GetParam().pref_name(), &value));
   EXPECT_TRUE(StringValue("http://chromium.org").Equals(value));
@@ -140,7 +140,7 @@ TEST_P(ConfigurationPolicyPrefStoreBooleanTest, GetDefault) {
 TEST_P(ConfigurationPolicyPrefStoreBooleanTest, SetValue) {
   provider_.AddPolicy(GetParam().type(), Value::CreateBooleanValue(false));
   store_->OnUpdatePolicy();
-  Value* value;
+  const Value* value;
   bool result = true;
   EXPECT_EQ(PrefStore::READ_OK,
             store_->GetValue(GetParam().pref_name(), &value));
@@ -230,7 +230,7 @@ TEST_P(ConfigurationPolicyPrefStoreIntegerTest, GetDefault) {
 TEST_P(ConfigurationPolicyPrefStoreIntegerTest, SetValue) {
   provider_.AddPolicy(GetParam().type(), Value::CreateIntegerValue(2));
   store_->OnUpdatePolicy();
-  Value* value = NULL;
+  const Value* value = NULL;
   EXPECT_EQ(PrefStore::READ_OK,
             store_->GetValue(GetParam().pref_name(), &value));
   EXPECT_TRUE(FundamentalValue(2).Equals(value));
@@ -255,11 +255,11 @@ class ConfigurationPolicyPrefStoreProxyTest : public testing::Test {
       const std::string& expected_proxy_pac_url,
       const std::string& expected_proxy_bypass_list,
       const ProxyPrefs::ProxyMode& expected_proxy_mode) {
-    Value* value = NULL;
+    const Value* value = NULL;
     ASSERT_EQ(PrefStore::READ_OK,
               store.GetValue(prefs::kProxy, &value));
     ASSERT_EQ(Value::TYPE_DICTIONARY, value->GetType());
-    ProxyConfigDictionary dict(static_cast<DictionaryValue*>(value));
+    ProxyConfigDictionary dict(static_cast<const DictionaryValue*>(value));
     std::string s;
     if (expected_proxy_server.empty()) {
       EXPECT_FALSE(dict.GetProxyServer(&s));
@@ -426,7 +426,7 @@ TEST_F(ConfigurationPolicyPrefStoreProxyTest, ProxyInvalid) {
 
     scoped_refptr<ConfigurationPolicyPrefStore> store(
         new ConfigurationPolicyPrefStore(&provider));
-    Value* value = NULL;
+    const Value* value = NULL;
     EXPECT_EQ(PrefStore::READ_NO_VALUE,
               store->GetValue(prefs::kProxy, &value));
   }
@@ -448,7 +448,7 @@ TEST_F(ConfigurationPolicyPrefStoreDefaultSearchTest, MinimallyDefined) {
   scoped_refptr<ConfigurationPolicyPrefStore> store(
       new ConfigurationPolicyPrefStore(&provider));
 
-  Value* value = NULL;
+  const Value* value = NULL;
   EXPECT_EQ(PrefStore::READ_OK,
             store->GetValue(prefs::kDefaultSearchProviderSearchURL, &value));
   EXPECT_TRUE(StringValue(search_url).Equals(value));
@@ -506,7 +506,7 @@ TEST_F(ConfigurationPolicyPrefStoreDefaultSearchTest, FullyDefined) {
   scoped_refptr<ConfigurationPolicyPrefStore> store(
       new ConfigurationPolicyPrefStore(&provider));
 
-  Value* value = NULL;
+  const Value* value = NULL;
   EXPECT_EQ(PrefStore::READ_OK,
             store->GetValue(prefs::kDefaultSearchProviderSearchURL, &value));
   EXPECT_TRUE(StringValue(search_url).Equals(value));
@@ -635,7 +635,7 @@ TEST_F(ConfigurationPolicyPrefStoreSyncTest, Disabled) {
   provider_.AddPolicy(kPolicySyncDisabled, Value::CreateBooleanValue(true));
   store_->OnUpdatePolicy();
   // Sync should be flagged as managed.
-  Value* value = NULL;
+  const Value* value = NULL;
   EXPECT_EQ(PrefStore::READ_OK, store_->GetValue(prefs::kSyncManaged, &value));
   ASSERT_TRUE(value != NULL);
   EXPECT_TRUE(FundamentalValue(true).Equals(value));
@@ -663,7 +663,7 @@ TEST_F(ConfigurationPolicyPrefStoreAutofillTest, Disabled) {
   provider_.AddPolicy(kPolicyAutoFillEnabled, Value::CreateBooleanValue(false));
   store_->OnUpdatePolicy();
   // Disabling Autofill should switch the pref to managed.
-  Value* value = NULL;
+  const Value* value = NULL;
   EXPECT_EQ(PrefStore::READ_OK,
             store_->GetValue(prefs::kAutofillEnabled, &value));
   EXPECT_TRUE(FundamentalValue(false).Equals(value));
@@ -685,7 +685,7 @@ class ConfigurationPolicyPrefStoreRefreshTest
 };
 
 TEST_F(ConfigurationPolicyPrefStoreRefreshTest, Refresh) {
-  Value* value = NULL;
+  const Value* value = NULL;
   EXPECT_EQ(PrefStore::READ_NO_VALUE,
             store_->GetValue(prefs::kHomePage, NULL));
 
diff --git a/chrome/browser/prefs/command_line_pref_store_unittest.cc b/chrome/browser/prefs/command_line_pref_store_unittest.cc
index 28c35d19..866b9c8 100644
--- a/chrome/browser/prefs/command_line_pref_store_unittest.cc
+++ b/chrome/browser/prefs/command_line_pref_store_unittest.cc
@@ -26,10 +26,10 @@ class TestCommandLinePrefStore : public CommandLinePrefStore {
   }
 
   void VerifyProxyMode(ProxyPrefs::ProxyMode expected_mode) {
-    Value* value = NULL;
+    const Value* value = NULL;
     ASSERT_EQ(PrefStore::READ_OK, GetValue(prefs::kProxy, &value));
     ASSERT_EQ(Value::TYPE_DICTIONARY, value->GetType());
-    ProxyConfigDictionary dict(static_cast<DictionaryValue*>(value));
+    ProxyConfigDictionary dict(static_cast<const DictionaryValue*>(value));
     ProxyPrefs::ProxyMode actual_mode;
     ASSERT_TRUE(dict.GetMode(&actual_mode));
     EXPECT_EQ(expected_mode, actual_mode);
@@ -47,7 +47,7 @@ TEST(CommandLinePrefStoreTest, SimpleStringPref) {
   cl.AppendSwitchASCII(switches::kLang, "hi-MOM");
   scoped_refptr<CommandLinePrefStore> store = new CommandLinePrefStore(&cl);
 
-  Value* actual = NULL;
+  const Value* actual = NULL;
   EXPECT_EQ(PrefStore::READ_OK,
             store->GetValue(prefs::kApplicationLocale, &actual));
   std::string result;
@@ -72,7 +72,7 @@ TEST(CommandLinePrefStoreTest, NoPrefs) {
   cl.AppendSwitchASCII(unknown_bool, "a value");
   scoped_refptr<CommandLinePrefStore> store = new CommandLinePrefStore(&cl);
 
-  Value* actual = NULL;
+  const Value* actual = NULL;
   EXPECT_EQ(PrefStore::READ_NO_VALUE, store->GetValue(unknown_bool, &actual));
   EXPECT_EQ(PrefStore::READ_NO_VALUE, store->GetValue(unknown_string, &actual));
 }
@@ -87,16 +87,16 @@ TEST(CommandLinePrefStoreTest, MultipleSwitches) {
   scoped_refptr<TestCommandLinePrefStore> store =
       new TestCommandLinePrefStore(&cl);
 
-  Value* actual = NULL;
+  const Value* actual = NULL;
   EXPECT_EQ(PrefStore::READ_NO_VALUE, store->GetValue(unknown_bool, &actual));
   EXPECT_EQ(PrefStore::READ_NO_VALUE, store->GetValue(unknown_string, &actual));
 
   store->VerifyProxyMode(ProxyPrefs::MODE_FIXED_SERVERS);
 
-  Value* value = NULL;
+  const Value* value = NULL;
   ASSERT_EQ(PrefStore::READ_OK, store->GetValue(prefs::kProxy, &value));
   ASSERT_EQ(Value::TYPE_DICTIONARY, value->GetType());
-  ProxyConfigDictionary dict(static_cast<DictionaryValue*>(value));
+  ProxyConfigDictionary dict(static_cast<const DictionaryValue*>(value));
 
   std::string string_result = "";
 
diff --git a/chrome/browser/prefs/default_pref_store.cc b/chrome/browser/prefs/default_pref_store.cc
index cacc833..8e43438 100644
--- a/chrome/browser/prefs/default_pref_store.cc
+++ b/chrome/browser/prefs/default_pref_store.cc
@@ -14,7 +14,7 @@ void DefaultPrefStore::SetDefaultValue(const std::string& key, Value* value) {
 }
 
 Value::ValueType DefaultPrefStore::GetType(const std::string& key) const {
-  Value* value;
+  const Value* value;
   return GetValue(key, &value) == READ_OK ? value->GetType()
                                           : Value::TYPE_NULL;
 }
diff --git a/chrome/browser/prefs/overlay_persistent_pref_store.cc b/chrome/browser/prefs/overlay_persistent_pref_store.cc
index 8f940c1..e75fbc6 100644
--- a/chrome/browser/prefs/overlay_persistent_pref_store.cc
+++ b/chrome/browser/prefs/overlay_persistent_pref_store.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/prefs/overlay_persistent_pref_store.h"
 
+#include "base/values.h"
+
 OverlayPersistentPrefStore::OverlayPersistentPrefStore(
     PersistentPrefStore* underlay)
     : underlay_(underlay) {
@@ -30,12 +32,32 @@ bool OverlayPersistentPrefStore::IsInitializationComplete() const {
 }
 
 PrefStore::ReadResult OverlayPersistentPrefStore::GetValue(
-    const std::string& key, Value** result) const {
+    const std::string& key,
+    const Value** result) const {
   if (overlay_.GetValue(key, result))
     return READ_OK;
   return underlay_->GetValue(key, result);
 }
 
+PrefStore::ReadResult OverlayPersistentPrefStore::GetMutableValue(
+    const std::string& key,
+    Value** result) {
+  if (overlay_.GetValue(key, result))
+    return READ_OK;
+
+  // Try to create copy of underlay if the overlay does not contain a value.
+  Value* underlay_value = NULL;
+  PrefStore::ReadResult read_result =
+      underlay_->GetMutableValue(key, &underlay_value);
+  if (read_result == READ_OK) {
+    *result = underlay_value->DeepCopy();
+    overlay_.SetValue(key, *result);
+    return READ_OK;
+  }
+  // Return read failure if underlay stores no value for |key|.
+  return read_result;
+}
+
 void OverlayPersistentPrefStore::SetValue(const std::string& key,
                                           Value* value) {
   if (overlay_.SetValue(key, value))
diff --git a/chrome/browser/prefs/overlay_persistent_pref_store.h b/chrome/browser/prefs/overlay_persistent_pref_store.h
index cb2b53f..92d207c 100644
--- a/chrome/browser/prefs/overlay_persistent_pref_store.h
+++ b/chrome/browser/prefs/overlay_persistent_pref_store.h
@@ -33,9 +33,11 @@ class OverlayPersistentPrefStore : public PersistentPrefStore,
   virtual void AddObserver(PrefStore::Observer* observer);
   virtual void RemoveObserver(PrefStore::Observer* observer);
   virtual bool IsInitializationComplete() const;
-  virtual ReadResult GetValue(const std::string& key, Value** result) const;
+  virtual ReadResult GetValue(const std::string& key,
+                              const Value** result) const;
 
   // Methods of PersistentPrefStore.
+  virtual ReadResult GetMutableValue(const std::string& key, Value** result);
   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);
@@ -44,7 +46,6 @@ class OverlayPersistentPrefStore : public PersistentPrefStore,
   virtual bool WritePrefs();
   virtual void ScheduleWritePrefs();
   virtual void CommitPendingWrite();
-  // TODO(battre) remove this function
   virtual void ReportValueChanged(const std::string& key);
 
  private:
diff --git a/chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc b/chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc
index 7e2e0a2..11dcc05 100644
--- a/chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc
+++ b/chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc
@@ -74,7 +74,7 @@ TEST_F(OverlayPersistentPrefStoreTest, Observer) {
 }
 
 TEST_F(OverlayPersistentPrefStoreTest, GetAndSet) {
-  Value* value = NULL;
+  const Value* value = NULL;
   int i = -1;
   EXPECT_EQ(PrefStore::READ_NO_VALUE, overlay_->GetValue(key, &value));
   EXPECT_EQ(PrefStore::READ_NO_VALUE, underlay_->GetValue(key, &value));
@@ -117,3 +117,27 @@ TEST_F(OverlayPersistentPrefStoreTest, GetAndSet) {
   EXPECT_TRUE(value->GetAsInteger(&i));
   EXPECT_EQ(42, i);
 }
+
+// Check that GetMutableValue does not return the dictionary of the underlay.
+TEST_F(OverlayPersistentPrefStoreTest, ModifyDictionaries) {
+  underlay_->SetValue(key, new DictionaryValue);
+
+  Value* modify = NULL;
+  EXPECT_EQ(PrefStore::READ_OK, overlay_->GetMutableValue(key, &modify));
+  ASSERT_TRUE(modify);
+  ASSERT_TRUE(modify->GetType() == Value::TYPE_DICTIONARY);
+  static_cast<DictionaryValue*>(modify)->SetInteger(key, 42);
+
+  Value* original_in_underlay = NULL;
+  EXPECT_EQ(PrefStore::READ_OK,
+            underlay_->GetMutableValue(key, &original_in_underlay));
+  ASSERT_TRUE(original_in_underlay);
+  ASSERT_TRUE(original_in_underlay->GetType() == Value::TYPE_DICTIONARY);
+  EXPECT_TRUE(static_cast<DictionaryValue*>(original_in_underlay)->empty());
+
+  Value* modified = NULL;
+  EXPECT_EQ(PrefStore::READ_OK, overlay_->GetMutableValue(key, &modified));
+  ASSERT_TRUE(modified);
+  ASSERT_TRUE(modified->GetType() == Value::TYPE_DICTIONARY);
+  EXPECT_TRUE(Value::Equals(modify, static_cast<DictionaryValue*>(modified)));
+}
diff --git a/chrome/browser/prefs/pref_service.cc b/chrome/browser/prefs/pref_service.cc
index d9588b5..8e7563f 100644
--- a/chrome/browser/prefs/pref_service.cc
+++ b/chrome/browser/prefs/pref_service.cc
@@ -407,10 +407,6 @@ bool PrefService::ReadOnly() const {
   return user_pref_store_->ReadOnly();
 }
 
-PrefNotifier* PrefService::pref_notifier() const {
-  return pref_notifier_.get();
-}
-
 bool PrefService::IsManagedPreference(const char* pref_name) const {
   const Preference* pref = FindPreference(pref_name);
   return pref && pref->IsManaged();
@@ -571,7 +567,7 @@ DictionaryValue* PrefService::GetMutableDictionary(const char* path) {
   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_->GetValue(path, &tmp_value)
+  if (user_pref_store_->GetMutableValue(path, &tmp_value)
           != PersistentPrefStore::READ_OK ||
       !tmp_value->IsType(Value::TYPE_DICTIONARY)) {
     dict = new DictionaryValue;
@@ -601,7 +597,7 @@ ListValue* PrefService::GetMutableList(const char* path) {
   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_->GetValue(path, &tmp_value)
+  if (user_pref_store_->GetMutableValue(path, &tmp_value)
           != PersistentPrefStore::READ_OK ||
       !tmp_value->IsType(Value::TYPE_LIST)) {
     list = new ListValue;
@@ -612,7 +608,7 @@ ListValue* PrefService::GetMutableList(const char* path) {
   return list;
 }
 
-void PrefService::ReportValueChanged(const std::string& key) {
+void PrefService::ReportUserPrefChanged(const std::string& key) {
   user_pref_store_->ReportValueChanged(key);
 }
 
@@ -657,7 +653,7 @@ const Value* PrefService::Preference::GetValue() const {
   DCHECK(pref_service_->FindPreference(name_.c_str())) <<
       "Must register pref before getting its value";
 
-  Value* found_value = NULL;
+  const Value* found_value = NULL;
   if (pref_value_store()->GetValue(name_, type_, &found_value)) {
     DCHECK(found_value->IsType(type_));
     return found_value;
diff --git a/chrome/browser/prefs/pref_service.h b/chrome/browser/prefs/pref_service.h
index 5ca24fd..64b8227 100644
--- a/chrome/browser/prefs/pref_service.h
+++ b/chrome/browser/prefs/pref_service.h
@@ -220,8 +220,6 @@ class PrefService : public base::NonThreadSafe {
   // a mutable from the user preferences store.
   DictionaryValue* GetMutableDictionary(const char* path);
   ListValue* GetMutableList(const char* path);
-  // TODO(battre) remove this function (hack).
-  void ReportValueChanged(const std::string& key);
 
   // Returns true if a value has been set for the specified path.
   // NOTE: this is NOT the same as FindPreference. In particular
@@ -277,7 +275,7 @@ class PrefService : public base::NonThreadSafe {
   friend class PrefChangeRegistrar;
   friend class subtle::PrefMemberBase;
 
-  // Give access to pref_notifier();
+  // Give access to ReportUserPrefChanged();
   friend class ScopedUserPrefUpdate;
 
   // Construct an incognito version of the pref service. Use
@@ -285,9 +283,10 @@ class PrefService : public base::NonThreadSafe {
   PrefService(const PrefService& original,
               PrefStore* incognito_extension_prefs);
 
-  // Returns a PrefNotifier. If you desire access to this, you will probably
-  // want to use a ScopedUserPrefUpdate.
-  PrefNotifier* pref_notifier() const;
+  // Sends notification of a changed preference. This needs to be called by
+  // a ScopedUserPrefUpdate if a DictionaryValue or ListValue retrieved by the
+  // GetMutable... methods is changed.
+  void ReportUserPrefChanged(const std::string& key);
 
   // If the pref at the given path changes, we call the observer's Observe
   // method with PREF_CHANGED. Note that observers should not call these methods
diff --git a/chrome/browser/prefs/pref_value_map.cc b/chrome/browser/prefs/pref_value_map.cc
index 0857f6d..5eace84 100644
--- a/chrome/browser/prefs/pref_value_map.cc
+++ b/chrome/browser/prefs/pref_value_map.cc
@@ -15,7 +15,18 @@ PrefValueMap::~PrefValueMap() {
   Clear();
 }
 
-bool PrefValueMap::GetValue(const std::string& key, Value** value) const {
+bool PrefValueMap::GetValue(const std::string& key, const Value** value) const {
+  const Map::const_iterator entry = prefs_.find(key);
+  if (entry != prefs_.end()) {
+    if (value)
+      *value = entry->second;
+    return true;
+  }
+
+  return false;
+}
+
+bool PrefValueMap::GetValue(const std::string& key, Value** value) {
   const Map::const_iterator entry = prefs_.find(key);
   if (entry != prefs_.end()) {
     if (value)
@@ -76,13 +87,13 @@ PrefValueMap::const_iterator PrefValueMap::end() const {
 
 bool PrefValueMap::GetBoolean(const std::string& key,
                               bool* value) const {
-  Value* stored_value = NULL;
+  const Value* stored_value = NULL;
   return GetValue(key, &stored_value) && stored_value->GetAsBoolean(value);
 }
 
 bool PrefValueMap::GetString(const std::string& key,
                              std::string* value) const {
-  Value* stored_value = NULL;
+  const Value* stored_value = NULL;
   return GetValue(key, &stored_value) && stored_value->GetAsString(value);
 }
 
diff --git a/chrome/browser/prefs/pref_value_map.h b/chrome/browser/prefs/pref_value_map.h
index 19aeb05..d4b9f4e 100644
--- a/chrome/browser/prefs/pref_value_map.h
+++ b/chrome/browser/prefs/pref_value_map.h
@@ -26,7 +26,8 @@ class PrefValueMap {
   // Gets the value for |key| and stores it in |value|. Ownership remains with
   // the map. Returns true if a value is present. If not, |value| is not
   // touched.
-  bool GetValue(const std::string& key, Value** value) const;
+  bool GetValue(const std::string& key, const Value** value) const;
+  bool GetValue(const std::string& key, Value** value);
 
   // Sets a new |value| for |key|. Takes ownership of |value|, which must be
   // non-NULL. Returns true if the value changed.
diff --git a/chrome/browser/prefs/pref_value_map_unittest.cc b/chrome/browser/prefs/pref_value_map_unittest.cc
index 5e248b0..cc47a85 100644
--- a/chrome/browser/prefs/pref_value_map_unittest.cc
+++ b/chrome/browser/prefs/pref_value_map_unittest.cc
@@ -1,4 +1,4 @@
-// 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.
 
@@ -11,7 +11,7 @@ class PrefValueMapTest : public testing::Test {
 
 TEST_F(PrefValueMapTest, SetValue) {
   PrefValueMap map;
-  Value* result = NULL;
+  const Value* result = NULL;
   EXPECT_FALSE(map.GetValue("key", &result));
   EXPECT_FALSE(result);
 
diff --git a/chrome/browser/prefs/pref_value_store.cc b/chrome/browser/prefs/pref_value_store.cc
index b97a4fb..3318a1d 100644
--- a/chrome/browser/prefs/pref_value_store.cc
+++ b/chrome/browser/prefs/pref_value_store.cc
@@ -102,7 +102,7 @@ PrefValueStore* PrefValueStore::CloneAndSpecialize(
 
 bool PrefValueStore::GetValue(const std::string& name,
                               Value::ValueType type,
-                              Value** out_value) const {
+                              const Value** out_value) const {
   *out_value = NULL;
   // Check the |PrefStore|s in order of their priority from highest to lowest
   // to find the value of the preference described by the given preference name.
@@ -175,7 +175,7 @@ bool PrefValueStore::PrefValueInStore(
     PrefValueStore::PrefStoreType store) const {
   // Declare a temp Value* and call GetValueFromStore,
   // ignoring the output value.
-  Value* tmp_value = NULL;
+  const Value* tmp_value = NULL;
   return GetValueFromStore(name, store, &tmp_value);
 }
 
@@ -207,7 +207,7 @@ PrefValueStore::PrefStoreType PrefValueStore::ControllingPrefStoreForPref(
 
 bool PrefValueStore::GetValueFromStore(const char* name,
                                        PrefValueStore::PrefStoreType store_type,
-                                       Value** out_value) const {
+                                       const Value** out_value) const {
   // Only return true if we find a value and it is the correct type, so stale
   // values with the incorrect type will be ignored.
   const PrefStore* store = GetPrefStore(static_cast<PrefStoreType>(store_type));
diff --git a/chrome/browser/prefs/pref_value_store.h b/chrome/browser/prefs/pref_value_store.h
index bddbdaa..e477d1d 100644
--- a/chrome/browser/prefs/pref_value_store.h
+++ b/chrome/browser/prefs/pref_value_store.h
@@ -78,7 +78,7 @@ class PrefValueStore {
   // Preference::GetValue() instead of calling this method directly.
   bool GetValue(const std::string& name,
                 Value::ValueType type,
-                Value** out_value) const;
+                const Value** out_value) const;
 
   // 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
@@ -195,7 +195,7 @@ class PrefValueStore {
   // Get a value from the specified store type.
   bool GetValueFromStore(const char* name,
                          PrefStoreType store,
-                         Value** out_value) const;
+                         const 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
diff --git a/chrome/browser/prefs/pref_value_store_unittest.cc b/chrome/browser/prefs/pref_value_store_unittest.cc
index 81c068c..f711247 100644
--- a/chrome/browser/prefs/pref_value_store_unittest.cc
+++ b/chrome/browser/prefs/pref_value_store_unittest.cc
@@ -289,7 +289,7 @@ class PrefValueStoreTest : public testing::Test {
 };
 
 TEST_F(PrefValueStoreTest, GetValue) {
-  Value* value;
+  const Value* value;
 
   // The following tests read a value from the PrefService. The preferences are
   // set in a way such that all lower-priority stores have a value and we can
diff --git a/chrome/browser/prefs/scoped_user_pref_update.cc b/chrome/browser/prefs/scoped_user_pref_update.cc
index 2c3c995..3ef0029 100644
--- a/chrome/browser/prefs/scoped_user_pref_update.cc
+++ b/chrome/browser/prefs/scoped_user_pref_update.cc
@@ -1,4 +1,4 @@
-// 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.
 
@@ -13,8 +13,5 @@ ScopedUserPrefUpdate::ScopedUserPrefUpdate(
       path_(path) {}
 
 ScopedUserPrefUpdate::~ScopedUserPrefUpdate() {
-  // TODO(mnissler, danno): This sends a notification unconditionally, which is
-  // wrong. We should rather tell the PrefService that the user pref got
-  // updated.
-  service_->ReportValueChanged(path_);
+  service_->ReportUserPrefChanged(path_);
 }
diff --git a/chrome/browser/prefs/testing_pref_store.cc b/chrome/browser/prefs/testing_pref_store.cc
index a3d33d8..e85a836 100644
--- a/chrome/browser/prefs/testing_pref_store.cc
+++ b/chrome/browser/prefs/testing_pref_store.cc
@@ -14,7 +14,12 @@ TestingPrefStore::TestingPrefStore()
 TestingPrefStore::~TestingPrefStore() {}
 
 PrefStore::ReadResult TestingPrefStore::GetValue(const std::string& key,
-                                                 Value** value) const {
+                                                 const Value** value) const {
+  return prefs_.GetValue(key, value) ? READ_OK : READ_NO_VALUE;
+}
+
+PrefStore::ReadResult TestingPrefStore::GetMutableValue(const std::string& key,
+                                                        Value** value) {
   return prefs_.GetValue(key, value) ? READ_OK : READ_NO_VALUE;
 }
 
@@ -90,7 +95,7 @@ void TestingPrefStore::SetBoolean(const std::string& key, bool value) {
 
 bool TestingPrefStore::GetString(const std::string& key,
                                  std::string* value) const {
-  Value* stored_value;
+  const Value* stored_value;
   if (!prefs_.GetValue(key, &stored_value) || !stored_value)
     return false;
 
@@ -98,7 +103,7 @@ bool TestingPrefStore::GetString(const std::string& key,
 }
 
 bool TestingPrefStore::GetInteger(const std::string& key, int* value) const {
-  Value* stored_value;
+  const Value* stored_value;
   if (!prefs_.GetValue(key, &stored_value) || !stored_value)
     return false;
 
@@ -106,7 +111,7 @@ bool TestingPrefStore::GetInteger(const std::string& key, int* value) const {
 }
 
 bool TestingPrefStore::GetBoolean(const std::string& key, bool* value) const {
-  Value* stored_value;
+  const Value* stored_value;
   if (!prefs_.GetValue(key, &stored_value) || !stored_value)
     return false;
 
diff --git a/chrome/browser/prefs/testing_pref_store.h b/chrome/browser/prefs/testing_pref_store.h
index ee2f185..bd68e25 100644
--- a/chrome/browser/prefs/testing_pref_store.h
+++ b/chrome/browser/prefs/testing_pref_store.h
@@ -23,12 +23,14 @@ class TestingPrefStore : public PersistentPrefStore {
   virtual ~TestingPrefStore();
 
   // Overriden from PrefStore.
-  virtual ReadResult GetValue(const std::string& key, Value** result) const;
+  virtual ReadResult GetValue(const std::string& key,
+                              const Value** result) const;
   virtual void AddObserver(PrefStore::Observer* observer);
   virtual void RemoveObserver(PrefStore::Observer* observer);
   virtual bool IsInitializationComplete() const;
 
   // PersistentPrefStore overrides:
+  virtual ReadResult GetMutableValue(const std::string& key, Value** result);
   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);
diff --git a/chrome/browser/prefs/value_map_pref_store.cc b/chrome/browser/prefs/value_map_pref_store.cc
index 58c4016..4d2c8ca 100644
--- a/chrome/browser/prefs/value_map_pref_store.cc
+++ b/chrome/browser/prefs/value_map_pref_store.cc
@@ -1,4 +1,4 @@
-// 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.
 
@@ -14,7 +14,7 @@ ValueMapPrefStore::ValueMapPrefStore() {}
 ValueMapPrefStore::~ValueMapPrefStore() {}
 
 PrefStore::ReadResult ValueMapPrefStore::GetValue(const std::string& key,
-                                                  Value** value) const {
+                                                  const Value** value) const {
   return prefs_.GetValue(key, value) ? READ_OK : READ_NO_VALUE;
 }
 
diff --git a/chrome/browser/prefs/value_map_pref_store.h b/chrome/browser/prefs/value_map_pref_store.h
index a8b66c4..c713156 100644
--- a/chrome/browser/prefs/value_map_pref_store.h
+++ b/chrome/browser/prefs/value_map_pref_store.h
@@ -22,7 +22,8 @@ class ValueMapPrefStore : public PrefStore {
   virtual ~ValueMapPrefStore();
 
   // PrefStore overrides:
-  virtual ReadResult GetValue(const std::string& key, Value** value) const;
+  virtual ReadResult GetValue(const std::string& key,
+                              const Value** value) const;
   virtual void AddObserver(PrefStore::Observer* observer);
   virtual void RemoveObserver(PrefStore::Observer* observer);
 
diff --git a/chrome/common/json_pref_store.cc b/chrome/common/json_pref_store.cc
index d20d05b..b8d323b 100644
--- a/chrome/common/json_pref_store.cc
+++ b/chrome/common/json_pref_store.cc
@@ -30,8 +30,13 @@ JsonPrefStore::~JsonPrefStore() {
 }
 
 PrefStore::ReadResult JsonPrefStore::GetValue(const std::string& key,
-                                              Value** result) const {
-  return prefs_->Get(key, result) ? READ_OK : READ_NO_VALUE;
+                                              const Value** result) const {
+  Value* tmp = NULL;
+  if (prefs_->Get(key, &tmp)) {
+    *result = tmp;
+    return READ_OK;
+  }
+  return READ_NO_VALUE;
 }
 
 void JsonPrefStore::AddObserver(PrefStore::Observer* observer) {
@@ -42,6 +47,11 @@ void JsonPrefStore::RemoveObserver(PrefStore::Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+PrefStore::ReadResult JsonPrefStore::GetMutableValue(const std::string& key,
+                                                     Value** result) {
+  return prefs_->Get(key, result) ? READ_OK : READ_NO_VALUE;
+}
+
 void JsonPrefStore::SetValue(const std::string& key, Value* value) {
   DCHECK(value);
   scoped_ptr<Value> new_value(value);
diff --git a/chrome/common/json_pref_store.h b/chrome/common/json_pref_store.h
index 8f1cbd7..0b02363 100644
--- a/chrome/common/json_pref_store.h
+++ b/chrome/common/json_pref_store.h
@@ -34,11 +34,13 @@ class JsonPrefStore : public PersistentPrefStore,
   virtual ~JsonPrefStore();
 
   // PrefStore overrides:
-  virtual ReadResult GetValue(const std::string& key, Value** result) const;
+  virtual ReadResult GetValue(const std::string& key,
+                              const Value** result) const;
   virtual void AddObserver(PrefStore::Observer* observer);
   virtual void RemoveObserver(PrefStore::Observer* observer);
 
   // PersistentPrefStore overrides:
+  virtual ReadResult GetMutableValue(const std::string& key, Value** result);
   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);
@@ -47,7 +49,6 @@ class JsonPrefStore : public PersistentPrefStore,
   virtual bool WritePrefs();
   virtual void ScheduleWritePrefs();
   virtual void CommitPendingWrite();
-  // TODO(battre) remove this function
   virtual void ReportValueChanged(const std::string& key);
 
  private:
diff --git a/chrome/common/json_pref_store_unittest.cc b/chrome/common/json_pref_store_unittest.cc
index 1b98aa9..adf7d8d 100644
--- a/chrome/common/json_pref_store_unittest.cc
+++ b/chrome/common/json_pref_store_unittest.cc
@@ -108,7 +108,7 @@ TEST_F(JsonPrefStoreTest, Basic) {
 
   std::string cnn("http://www.cnn.com");
 
-  Value* actual;
+  const Value* actual;
   EXPECT_EQ(PrefStore::READ_OK,
             pref_store->GetValue(prefs::kHomePage, &actual));
   std::string string_value;
diff --git a/chrome/common/persistent_pref_store.h b/chrome/common/persistent_pref_store.h
index 300e649..8bd677a 100644
--- a/chrome/common/persistent_pref_store.h
+++ b/chrome/common/persistent_pref_store.h
@@ -34,6 +34,16 @@ class PersistentPrefStore : public PrefStore {
     PREF_READ_ERROR_FILE_NOT_SPECIFIED
   };
 
+  // Equivalent to PrefStore::GetValue but returns a mutable value.
+  virtual ReadResult GetMutableValue(const std::string& key,
+                                     Value** result) = 0;
+
+  // Triggers a value changed notification. This function needs to be called
+  // if one retrieves a list or dictionary with GetMutableValue and change its
+  // value. SetValue takes care of notifications itself. Note that
+  // ReportValueChanged will trigger notifications even if nothing has changed.
+  virtual void ReportValueChanged(const std::string& key) = 0;
+
   // Sets a |value| for |key| in the store. Assumes ownership of |value|, which
   // must be non-NULL.
   virtual void SetValue(const std::string& key, Value* value) = 0;
@@ -51,9 +61,6 @@ class PersistentPrefStore : public PrefStore {
   // Removes the value for |key|.
   virtual void RemoveValue(const std::string& key) = 0;
 
-  // TODO(battre) Remove this function.
-  virtual void ReportValueChanged(const std::string& key) = 0;
-
   // Whether the store is in a pseudo-read-only mode where changes are not
   // actually persisted to disk.  This happens in some cases when there are
   // read errors during startup.
diff --git a/chrome/common/pref_store.h b/chrome/common/pref_store.h
index 819a764..a9185fe 100644
--- a/chrome/common/pref_store.h
+++ b/chrome/common/pref_store.h
@@ -55,7 +55,8 @@ class PrefStore : public base::RefCounted<PrefStore> {
   // Get the value for a given preference |key| and stores it in |result|.
   // |result| is only modified if the return value is READ_OK. Ownership of the
   // |result| value remains with the PrefStore.
-  virtual ReadResult GetValue(const std::string& key, Value** result) const = 0;
+  virtual ReadResult GetValue(const std::string& key,
+                              const Value** result) const = 0;
 
  protected:
   friend class base::RefCounted<PrefStore>;
diff --git a/chrome/service/cloud_print/cloud_print_proxy.cc b/chrome/service/cloud_print/cloud_print_proxy.cc
index 10f147d..a6d070b 100644
--- a/chrome/service/cloud_print/cloud_print_proxy.cc
+++ b/chrome/service/cloud_print/cloud_print_proxy.cc
@@ -1,4 +1,4 @@
-// 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.
 
@@ -70,7 +70,7 @@ void CloudPrintProxy::EnableForUser(const std::string& lsid) {
   }
 
   // Getting print system specific settings from the preferences.
-  DictionaryValue* print_system_settings = NULL;
+  const DictionaryValue* print_system_settings = NULL;
   service_prefs_->GetDictionary(prefs::kCloudPrintPrintSystemSettings,
                                 &print_system_settings);
 
diff --git a/chrome/service/service_process_prefs.cc b/chrome/service/service_process_prefs.cc
index 9164712..7aa94b05 100644
--- a/chrome/service/service_process_prefs.cc
+++ b/chrome/service/service_process_prefs.cc
@@ -1,4 +1,4 @@
-// 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.
 
@@ -24,7 +24,7 @@ void ServiceProcessPrefs::WritePrefs() {
 
 void ServiceProcessPrefs::GetString(const std::string& key,
                                     std::string* result) {
-  Value* value;
+  const Value* value;
   if (prefs_->GetValue(key, &value) == PersistentPrefStore::READ_OK)
     value->GetAsString(result);
 }
@@ -35,7 +35,7 @@ void ServiceProcessPrefs::SetString(const std::string& key,
 }
 
 void ServiceProcessPrefs::GetBoolean(const std::string& key, bool* result) {
-  Value* value;
+  const Value* value;
   if (prefs_->GetValue(key, &value) == PersistentPrefStore::READ_OK)
     value->GetAsBoolean(result);
 }
@@ -45,12 +45,12 @@ void ServiceProcessPrefs::SetBoolean(const std::string& key, bool value) {
 }
 
 void ServiceProcessPrefs::GetDictionary(const std::string& key,
-                                        DictionaryValue** result) {
-  Value* value;
+                                        const DictionaryValue** result) {
+  const Value* value;
   if (prefs_->GetValue(key, &value) != PersistentPrefStore::READ_OK ||
       !value->IsType(Value::TYPE_DICTIONARY)) {
     return;
   }
 
-  *result = static_cast<DictionaryValue*>(value);
+  *result = static_cast<const DictionaryValue*>(value);
 }
diff --git a/chrome/service/service_process_prefs.h b/chrome/service/service_process_prefs.h
index e936515..a16f620 100644
--- a/chrome/service/service_process_prefs.h
+++ b/chrome/service/service_process_prefs.h
@@ -1,4 +1,4 @@
-// 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.
 
@@ -39,7 +39,7 @@ class ServiceProcessPrefs {
   void SetBoolean(const std::string& key, bool value);
 
   // Get a dictionary preference for |key| and store it in |result|.
-  void GetDictionary(const std::string& key, DictionaryValue** result);
+  void GetDictionary(const std::string& key, const DictionaryValue** result);
 
  private:
   scoped_refptr<JsonPrefStore> prefs_;
diff --git a/chrome/test/testing_pref_service.cc b/chrome/test/testing_pref_service.cc
index 043dae4..815ae1e 100644
--- a/chrome/test/testing_pref_service.cc
+++ b/chrome/test/testing_pref_service.cc
@@ -55,7 +55,7 @@ void TestingPrefServiceBase::RemoveUserPref(const char* path) {
 
 const Value* TestingPrefServiceBase::GetPref(TestingPrefStore* pref_store,
                                              const char* path) const {
-  Value* res;
+  const Value* res;
   return pref_store->GetValue(path, &res) == PrefStore::READ_OK ? res : NULL;
 }