Fixed regression: various preferences were not persisted when changed from incognito window

This CL renames the OverlayPrefStore to an IncognitoPrefStore. This IncognitoPrefStore stores write operations in memory and prevents persisting them to the user prefs file. The CL also blacklists two preferences from being stored in the in-memory IncognitoPrefStore to fix the regressions mentioned in the bugs.

BUG=87191,84472
TEST=see bug descriptions

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

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

13 files changed, 403 insertions, 289 deletions

diff --git a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
index e54d721..181afac 100644
--- a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
+++ b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
@@ -10,7 +10,7 @@
 #include "chrome/browser/content_settings/mock_settings_observer.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/prefs/default_pref_store.h"
-#include "chrome/browser/prefs/overlay_persistent_pref_store.h"
+#include "chrome/browser/prefs/incognito_user_pref_store.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/pref_service_mock_builder.h"
 #include "chrome/browser/prefs/testing_pref_store.h"
@@ -187,8 +187,8 @@ TEST_F(PrefProviderTest, Observer) {
 // of the OTR unintentionally: http://crbug.com/74466.
 TEST_F(PrefProviderTest, Incognito) {
   PersistentPrefStore* user_prefs = new TestingPrefStore();
-  OverlayPersistentPrefStore* otr_user_prefs =
-      new OverlayPersistentPrefStore(user_prefs);
+  IncognitoUserPrefStore* otr_user_prefs =
+      new IncognitoUserPrefStore(user_prefs);
 
   PrefServiceMockBuilder builder;
   PrefService* regular_prefs = builder.WithUserPrefs(user_prefs).Create();
diff --git a/chrome/browser/policy/configuration_policy_pref_store.cc b/chrome/browser/policy/configuration_policy_pref_store.cc
index 1915234..7291544 100644
--- a/chrome/browser/policy/configuration_policy_pref_store.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store.cc
@@ -327,7 +327,8 @@ ConfigurationPolicyPrefKeeper::GetValue(const std::string& key,
   if (stored_value->IsType(Value::TYPE_NULL))
     return PrefStore::READ_USE_DEFAULT;
 
-  *result = stored_value;
+  if (result)
+    *result = stored_value;
   return PrefStore::READ_OK;
 }
 
diff --git a/chrome/browser/prefs/incognito_user_pref_store.cc b/chrome/browser/prefs/incognito_user_pref_store.cc
new file mode 100644
index 0000000..87bcb2c
--- /dev/null
+++ b/chrome/browser/prefs/incognito_user_pref_store.cc
@@ -0,0 +1,152 @@
+// 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/incognito_user_pref_store.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "base/values.h"
+#include "chrome/common/pref_names.h"
+
+IncognitoUserPrefStore::IncognitoUserPrefStore(
+    PersistentPrefStore* underlay)
+    : underlay_(underlay) {
+  underlay_->AddObserver(this);
+}
+
+IncognitoUserPrefStore::~IncognitoUserPrefStore() {
+  underlay_->RemoveObserver(this);
+}
+
+bool IncognitoUserPrefStore::IsSetInOverlay(const std::string& key) const {
+  return overlay_.GetValue(key, NULL);
+}
+
+void IncognitoUserPrefStore::AddObserver(PrefStore::Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void IncognitoUserPrefStore::RemoveObserver(PrefStore::Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+bool IncognitoUserPrefStore::IsInitializationComplete() const {
+  return underlay_->IsInitializationComplete();
+}
+
+PrefStore::ReadResult IncognitoUserPrefStore::GetValue(
+    const std::string& key,
+    const Value** result) const {
+  // If the |key| shall NOT be stored in the overlay store, there must not
+  // be an entry.
+  DCHECK(ShallBeStoredInOverlay(key) || !overlay_.GetValue(key, NULL));
+
+  if (overlay_.GetValue(key, result))
+    return READ_OK;
+  return underlay_->GetValue(key, result);
+}
+
+PrefStore::ReadResult IncognitoUserPrefStore::GetMutableValue(
+    const std::string& key,
+    Value** result) {
+  if (!ShallBeStoredInOverlay(key))
+    return underlay_->GetMutableValue(key, 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)
+    return read_result;
+
+  *result = underlay_value->DeepCopy();
+  overlay_.SetValue(key, *result);
+  return READ_OK;
+}
+
+void IncognitoUserPrefStore::SetValue(const std::string& key,
+                                      Value* value) {
+  if (!ShallBeStoredInOverlay(key)) {
+    underlay_->SetValue(key, value);
+    return;
+  }
+
+  if (overlay_.SetValue(key, value))
+    ReportValueChanged(key);
+}
+
+void IncognitoUserPrefStore::SetValueSilently(const std::string& key,
+                                              Value* value) {
+  if (!ShallBeStoredInOverlay(key)) {
+    underlay_->SetValueSilently(key, value);
+    return;
+  }
+
+  overlay_.SetValue(key, value);
+}
+
+void IncognitoUserPrefStore::RemoveValue(const std::string& key) {
+  if (!ShallBeStoredInOverlay(key)) {
+    underlay_->RemoveValue(key);
+    return;
+  }
+
+  if (overlay_.RemoveValue(key))
+    ReportValueChanged(key);
+}
+
+bool IncognitoUserPrefStore::ReadOnly() const {
+  return false;
+}
+
+PersistentPrefStore::PrefReadError IncognitoUserPrefStore::ReadPrefs() {
+  // We do not read intentionally.
+  OnInitializationCompleted(true);
+  return PersistentPrefStore::PREF_READ_ERROR_NONE;
+}
+
+void IncognitoUserPrefStore::ReadPrefsAsync(
+    ReadErrorDelegate* error_delegate_raw) {
+  scoped_ptr<ReadErrorDelegate> error_delegate(error_delegate_raw);
+  // We do not read intentionally.
+  OnInitializationCompleted(true);
+}
+
+bool IncognitoUserPrefStore::WritePrefs() {
+  // We do not write our content intentionally.
+  return true;
+}
+
+void IncognitoUserPrefStore::ScheduleWritePrefs() {
+  underlay_->ScheduleWritePrefs();
+  // We do not write our content intentionally.
+}
+
+void IncognitoUserPrefStore::CommitPendingWrite() {
+  underlay_->CommitPendingWrite();
+  // We do not write our content intentionally.
+}
+
+void IncognitoUserPrefStore::ReportValueChanged(const std::string& key) {
+  FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
+}
+
+void IncognitoUserPrefStore::OnPrefValueChanged(const std::string& key) {
+  if (!overlay_.GetValue(key, NULL))
+    ReportValueChanged(key);
+}
+
+void IncognitoUserPrefStore::OnInitializationCompleted(bool succeeded) {
+  FOR_EACH_OBSERVER(PrefStore::Observer, observers_,
+                    OnInitializationCompleted(succeeded));
+}
+
+bool IncognitoUserPrefStore::ShallBeStoredInOverlay(
+    const std::string& key) const {
+  // List of keys that cannot be changed in the user prefs file by the incognito
+  // profile:
+  return key == prefs::kBrowserWindowPlacement;
+}
diff --git a/chrome/browser/prefs/overlay_persistent_pref_store.h b/chrome/browser/prefs/incognito_user_pref_store.h
index 47ce6afb..b5be17e 100644
--- a/chrome/browser/prefs/overlay_persistent_pref_store.h
+++ b/chrome/browser/prefs/incognito_user_pref_store.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_PREFS_OVERLAY_PERSISTENT_PREF_STORE_H_
-#define CHROME_BROWSER_PREFS_OVERLAY_PERSISTENT_PREF_STORE_H_
+#ifndef CHROME_BROWSER_PREFS_INCOGNITO_USER_PREF_STORE_H_
+#define CHROME_BROWSER_PREFS_INCOGNITO_USER_PREF_STORE_H_
 #pragma once
 
 #include <string>
@@ -14,18 +14,18 @@
 #include "chrome/browser/prefs/pref_value_map.h"
 #include "chrome/common/persistent_pref_store.h"
 
-// PersistentPrefStore that directs all write operations into a in-memory
+// PersistentPrefStore that directs all write operations into an in-memory
 // PrefValueMap. Read operations are first answered by the PrefValueMap.
 // If the PrefValueMap does not contain a value for the requested key,
 // the look-up is passed on to an underlying PersistentPrefStore |underlay_|.
-class OverlayPersistentPrefStore : public PersistentPrefStore,
-                                   public PrefStore::Observer {
+class IncognitoUserPrefStore : public PersistentPrefStore,
+                               public PrefStore::Observer {
  public:
-  explicit OverlayPersistentPrefStore(PersistentPrefStore* underlay);
-  virtual ~OverlayPersistentPrefStore();
+  explicit IncognitoUserPrefStore(PersistentPrefStore* underlay);
+  virtual ~IncognitoUserPrefStore();
 
   // Returns true if a value has been set for the |key| in this
-  // OverlayPersistentPrefStore, i.e. if it potentially overrides a value
+  // IncognitoUserPrefStore, i.e. if it potentially overrides a value
   // from the |underlay_|.
   virtual bool IsSetInOverlay(const std::string& key) const;
 
@@ -55,11 +55,17 @@ class OverlayPersistentPrefStore : public PersistentPrefStore,
   virtual void OnPrefValueChanged(const std::string& key);
   virtual void OnInitializationCompleted(bool succeeded);
 
+  // Returns true if |key| corresponds to a preference that shall be stored in
+  // an in-memory PrefStore that is not persisted to disk. All preferences
+  // that store information about the browsing history or behavior of the user
+  // should have this property.
+  virtual bool ShallBeStoredInOverlay(const std::string& key) const;
+
   ObserverList<PrefStore::Observer, true> observers_;
   PrefValueMap overlay_;
   scoped_refptr<PersistentPrefStore> underlay_;
 
-  DISALLOW_COPY_AND_ASSIGN(OverlayPersistentPrefStore);
+  DISALLOW_COPY_AND_ASSIGN(IncognitoUserPrefStore);
 };
 
-#endif  // CHROME_BROWSER_PREFS_OVERLAY_PERSISTENT_PREF_STORE_H_
+#endif  // CHROME_BROWSER_PREFS_INCOGNITO_USER_PREF_STORE_H_
diff --git a/chrome/browser/prefs/incognito_user_pref_store_unittest.cc b/chrome/browser/prefs/incognito_user_pref_store_unittest.cc
new file mode 100644
index 0000000..283ad17
--- /dev/null
+++ b/chrome/browser/prefs/incognito_user_pref_store_unittest.cc
@@ -0,0 +1,217 @@
+// 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 "base/values.h"
+#include "chrome/browser/prefs/incognito_user_pref_store.h"
+#include "chrome/browser/prefs/testing_pref_store.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/common/pref_store_observer_mock.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::Mock;
+using ::testing::StrEq;
+
+namespace {
+
+const char* incognito_key = prefs::kBrowserWindowPlacement;
+const char* regular_key = prefs::kShowBookmarkBar;
+
+}  // namespace
+
+class IncognitoUserPrefStoreTest : public testing::Test {
+ public:
+  IncognitoUserPrefStoreTest()
+      : underlay_(new TestingPrefStore()),
+        overlay_(new IncognitoUserPrefStore(underlay_.get())) {
+  }
+
+  virtual ~IncognitoUserPrefStoreTest() {}
+
+  scoped_refptr<TestingPrefStore> underlay_;
+  scoped_refptr<IncognitoUserPrefStore> overlay_;
+};
+
+TEST_F(IncognitoUserPrefStoreTest, Observer) {
+  PrefStoreObserverMock obs;
+  overlay_->AddObserver(&obs);
+
+  // Check that underlay first value is reported.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(incognito_key))).Times(1);
+  underlay_->SetValue(incognito_key, Value::CreateIntegerValue(42));
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Check that underlay overwriting is reported.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(incognito_key))).Times(1);
+  underlay_->SetValue(incognito_key, Value::CreateIntegerValue(43));
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Check that overwriting change in overlay is reported.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(incognito_key))).Times(1);
+  overlay_->SetValue(incognito_key, Value::CreateIntegerValue(44));
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Check that hidden underlay change is not reported.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(incognito_key))).Times(0);
+  underlay_->SetValue(incognito_key, Value::CreateIntegerValue(45));
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Check that overlay remove is reported.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(incognito_key))).Times(1);
+  overlay_->RemoveValue(incognito_key);
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Check that underlay remove is reported.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(incognito_key))).Times(1);
+  underlay_->RemoveValue(incognito_key);
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Check respecting of silence.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(incognito_key))).Times(0);
+  overlay_->SetValueSilently(incognito_key, Value::CreateIntegerValue(46));
+  Mock::VerifyAndClearExpectations(&obs);
+
+  overlay_->RemoveObserver(&obs);
+
+  // Check successful unsubscription.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(incognito_key))).Times(0);
+  underlay_->SetValue(incognito_key, Value::CreateIntegerValue(47));
+  overlay_->SetValue(incognito_key, Value::CreateIntegerValue(48));
+  Mock::VerifyAndClearExpectations(&obs);
+}
+
+TEST_F(IncognitoUserPrefStoreTest, GetAndSet) {
+  const Value* value = NULL;
+  int i = -1;
+  EXPECT_EQ(PrefStore::READ_NO_VALUE,
+            overlay_->GetValue(incognito_key, &value));
+  EXPECT_EQ(PrefStore::READ_NO_VALUE,
+            underlay_->GetValue(incognito_key, &value));
+
+  underlay_->SetValue(incognito_key, Value::CreateIntegerValue(42));
+
+  // Value shines through:
+  EXPECT_EQ(PrefStore::READ_OK, overlay_->GetValue(incognito_key, &value));
+  ASSERT_TRUE(value);
+  EXPECT_TRUE(value->GetAsInteger(&i));
+  EXPECT_EQ(42, i);
+
+  EXPECT_EQ(PrefStore::READ_OK, underlay_->GetValue(incognito_key, &value));
+  ASSERT_TRUE(value);
+  EXPECT_TRUE(value->GetAsInteger(&i));
+  EXPECT_EQ(42, i);
+
+  overlay_->SetValue(incognito_key, Value::CreateIntegerValue(43));
+
+  EXPECT_EQ(PrefStore::READ_OK, overlay_->GetValue(incognito_key, &value));
+  ASSERT_TRUE(value);
+  EXPECT_TRUE(value->GetAsInteger(&i));
+  EXPECT_EQ(43, i);
+
+  EXPECT_EQ(PrefStore::READ_OK, underlay_->GetValue(incognito_key, &value));
+  ASSERT_TRUE(value);
+  EXPECT_TRUE(value->GetAsInteger(&i));
+  EXPECT_EQ(42, i);
+
+  overlay_->RemoveValue(incognito_key);
+
+  // Value shines through:
+  EXPECT_EQ(PrefStore::READ_OK, overlay_->GetValue(incognito_key, &value));
+  ASSERT_TRUE(value);
+  EXPECT_TRUE(value->GetAsInteger(&i));
+  EXPECT_EQ(42, i);
+
+  EXPECT_EQ(PrefStore::READ_OK, underlay_->GetValue(incognito_key, &value));
+  ASSERT_TRUE(value);
+  EXPECT_TRUE(value->GetAsInteger(&i));
+  EXPECT_EQ(42, i);
+}
+
+// Check that GetMutableValue does not return the dictionary of the underlay.
+TEST_F(IncognitoUserPrefStoreTest, ModifyDictionaries) {
+  underlay_->SetValue(incognito_key, new DictionaryValue);
+
+  Value* modify = NULL;
+  EXPECT_EQ(PrefStore::READ_OK,
+            overlay_->GetMutableValue(incognito_key, &modify));
+  ASSERT_TRUE(modify);
+  ASSERT_TRUE(modify->GetType() == Value::TYPE_DICTIONARY);
+  static_cast<DictionaryValue*>(modify)->SetInteger(incognito_key, 42);
+
+  Value* original_in_underlay = NULL;
+  EXPECT_EQ(PrefStore::READ_OK,
+            underlay_->GetMutableValue(incognito_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(incognito_key, &modified));
+  ASSERT_TRUE(modified);
+  ASSERT_TRUE(modified->GetType() == Value::TYPE_DICTIONARY);
+  EXPECT_TRUE(Value::Equals(modify, static_cast<DictionaryValue*>(modified)));
+}
+
+// Here we consider a global preference that is not incognito specific.
+TEST_F(IncognitoUserPrefStoreTest, GlobalPref) {
+  PrefStoreObserverMock obs;
+  overlay_->AddObserver(&obs);
+
+  const Value* value = NULL;
+  int i = -1;
+
+  // Check that underlay first value is reported.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(regular_key))).Times(1);
+  underlay_->SetValue(regular_key, Value::CreateIntegerValue(42));
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Check that underlay overwriting is reported.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(regular_key))).Times(1);
+  underlay_->SetValue(regular_key, Value::CreateIntegerValue(43));
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Check that we get this value from the overlay
+  EXPECT_EQ(PrefStore::READ_OK, overlay_->GetValue(regular_key, &value));
+  ASSERT_TRUE(value);
+  EXPECT_TRUE(value->GetAsInteger(&i));
+  EXPECT_EQ(43, i);
+
+  // Check that overwriting change in overlay is reported.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(regular_key))).Times(1);
+  overlay_->SetValue(regular_key, Value::CreateIntegerValue(44));
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Check that we get this value from the overlay and the underlay.
+  EXPECT_EQ(PrefStore::READ_OK, overlay_->GetValue(regular_key, &value));
+  ASSERT_TRUE(value);
+  EXPECT_TRUE(value->GetAsInteger(&i));
+  EXPECT_EQ(44, i);
+  EXPECT_EQ(PrefStore::READ_OK, underlay_->GetValue(regular_key, &value));
+  ASSERT_TRUE(value);
+  EXPECT_TRUE(value->GetAsInteger(&i));
+  EXPECT_EQ(44, i);
+
+  // Check that overlay remove is reported.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(regular_key))).Times(1);
+  overlay_->RemoveValue(regular_key);
+  Mock::VerifyAndClearExpectations(&obs);
+
+  // Check that value was removed from overlay and underlay
+  EXPECT_EQ(PrefStore::READ_NO_VALUE, overlay_->GetValue(regular_key, &value));
+  EXPECT_EQ(PrefStore::READ_NO_VALUE, underlay_->GetValue(regular_key, &value));
+
+  // Check respecting of silence.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(regular_key))).Times(0);
+  overlay_->SetValueSilently(regular_key, Value::CreateIntegerValue(46));
+  Mock::VerifyAndClearExpectations(&obs);
+
+  overlay_->RemoveObserver(&obs);
+
+  // Check successful unsubscription.
+  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(regular_key))).Times(0);
+  underlay_->SetValue(regular_key, Value::CreateIntegerValue(47));
+  overlay_->SetValue(regular_key, Value::CreateIntegerValue(48));
+  Mock::VerifyAndClearExpectations(&obs);
+}
diff --git a/chrome/browser/prefs/overlay_persistent_pref_store.cc b/chrome/browser/prefs/overlay_persistent_pref_store.cc
deleted file mode 100644
index 13c51bb3..0000000
--- a/chrome/browser/prefs/overlay_persistent_pref_store.cc
+++ /dev/null
@@ -1,120 +0,0 @@
-// 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/overlay_persistent_pref_store.h"
-
-#include "base/memory/scoped_ptr.h"
-#include "base/values.h"
-
-OverlayPersistentPrefStore::OverlayPersistentPrefStore(
-    PersistentPrefStore* underlay)
-    : underlay_(underlay) {
-  underlay_->AddObserver(this);
-}
-OverlayPersistentPrefStore::~OverlayPersistentPrefStore() {
-  underlay_->RemoveObserver(this);
-}
-
-bool OverlayPersistentPrefStore::IsSetInOverlay(const std::string& key) const {
-  return overlay_.GetValue(key, NULL);
-}
-
-void OverlayPersistentPrefStore::AddObserver(PrefStore::Observer* observer) {
-  observers_.AddObserver(observer);
-}
-
-void OverlayPersistentPrefStore::RemoveObserver(PrefStore::Observer* observer) {
-  observers_.RemoveObserver(observer);
-}
-
-bool OverlayPersistentPrefStore::IsInitializationComplete() const {
-  return underlay_->IsInitializationComplete();
-}
-
-PrefStore::ReadResult OverlayPersistentPrefStore::GetValue(
-    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))
-    FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
-}
-
-void OverlayPersistentPrefStore::SetValueSilently(const std::string& key,
-                                                  Value* value) {
-  overlay_.SetValue(key, value);
-}
-
-void OverlayPersistentPrefStore::RemoveValue(const std::string& key) {
-  if (overlay_.RemoveValue(key))
-    FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
-}
-
-bool OverlayPersistentPrefStore::ReadOnly() const {
-  return false;
-}
-
-PersistentPrefStore::PrefReadError OverlayPersistentPrefStore::ReadPrefs() {
-  // We do not read intentionally.
-  OnInitializationCompleted(true);
-  return PersistentPrefStore::PREF_READ_ERROR_NONE;
-}
-
-void OverlayPersistentPrefStore::ReadPrefsAsync(
-    ReadErrorDelegate* error_delegate_raw) {
-  scoped_ptr<ReadErrorDelegate> error_delegate(error_delegate_raw);
-  // We do not read intentionally.
-  OnInitializationCompleted(true);
-}
-
-bool OverlayPersistentPrefStore::WritePrefs() {
-  // We do not write intentionally.
-  return true;
-}
-
-void OverlayPersistentPrefStore::ScheduleWritePrefs() {
-  // We do not write intentionally.
-}
-
-void OverlayPersistentPrefStore::CommitPendingWrite() {
-  // We do not write intentionally.
-}
-
-void OverlayPersistentPrefStore::ReportValueChanged(const std::string& key) {
-  FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
-}
-
-void OverlayPersistentPrefStore::OnPrefValueChanged(const std::string& key) {
-  if (!overlay_.GetValue(key, NULL))
-    FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
-}
-
-void OverlayPersistentPrefStore::OnInitializationCompleted(bool succeeded) {
-  FOR_EACH_OBSERVER(PrefStore::Observer, observers_,
-                    OnInitializationCompleted(succeeded));
-}
diff --git a/chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc b/chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc
deleted file mode 100644
index 11dcc05..0000000
--- a/chrome/browser/prefs/overlay_persistent_pref_store_unittest.cc
+++ /dev/null
@@ -1,143 +0,0 @@
-// 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 "base/values.h"
-#include "chrome/browser/prefs/overlay_persistent_pref_store.h"
-#include "chrome/browser/prefs/testing_pref_store.h"
-#include "chrome/common/pref_store_observer_mock.h"
-#include "testing/gmock/include/gmock/gmock.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-using ::testing::Mock;
-using ::testing::StrEq;
-
-const char key[] = "test.key";
-
-class OverlayPersistentPrefStoreTest : public testing::Test {
- public:
-  OverlayPersistentPrefStoreTest()
-      : underlay_(new TestingPrefStore()),
-        overlay_(new OverlayPersistentPrefStore(underlay_.get())) {
-  }
-
-  scoped_refptr<TestingPrefStore> underlay_;
-  scoped_refptr<OverlayPersistentPrefStore> overlay_;
-};
-
-TEST_F(OverlayPersistentPrefStoreTest, Observer) {
-  PrefStoreObserverMock obs;
-  overlay_->AddObserver(&obs);
-
-  // Check that underlay first value is reported.
-  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(key))).Times(1);
-  underlay_->SetValue(key, Value::CreateIntegerValue(42));
-  Mock::VerifyAndClearExpectations(&obs);
-
-  // Check that underlay overwriting is reported.
-  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(key))).Times(1);
-  underlay_->SetValue(key, Value::CreateIntegerValue(43));
-  Mock::VerifyAndClearExpectations(&obs);
-
-  // Check that overwriting change in overlay is reported.
-  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(key))).Times(1);
-  overlay_->SetValue(key, Value::CreateIntegerValue(44));
-  Mock::VerifyAndClearExpectations(&obs);
-
-  // Check that hidden underlay change is not reported.
-  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(key))).Times(0);
-  underlay_->SetValue(key, Value::CreateIntegerValue(45));
-  Mock::VerifyAndClearExpectations(&obs);
-
-  // Check that overlay remove is reported.
-  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(key))).Times(1);
-  overlay_->RemoveValue(key);
-  Mock::VerifyAndClearExpectations(&obs);
-
-  // Check that underlay remove is reported.
-  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(key))).Times(1);
-  underlay_->RemoveValue(key);
-  Mock::VerifyAndClearExpectations(&obs);
-
-  // Check respecting of silence.
-  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(key))).Times(0);
-  overlay_->SetValueSilently(key, Value::CreateIntegerValue(46));
-  Mock::VerifyAndClearExpectations(&obs);
-
-  overlay_->RemoveObserver(&obs);
-
-  // Check successful unsubscription.
-  EXPECT_CALL(obs, OnPrefValueChanged(StrEq(key))).Times(0);
-  underlay_->SetValue(key, Value::CreateIntegerValue(47));
-  overlay_->SetValue(key, Value::CreateIntegerValue(48));
-  Mock::VerifyAndClearExpectations(&obs);
-}
-
-TEST_F(OverlayPersistentPrefStoreTest, GetAndSet) {
-  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));
-
-  underlay_->SetValue(key, Value::CreateIntegerValue(42));
-
-  // Value shines through:
-  EXPECT_EQ(PrefStore::READ_OK, overlay_->GetValue(key, &value));
-  ASSERT_TRUE(value);
-  EXPECT_TRUE(value->GetAsInteger(&i));
-  EXPECT_EQ(42, i);
-
-  EXPECT_EQ(PrefStore::READ_OK, underlay_->GetValue(key, &value));
-  ASSERT_TRUE(value);
-  EXPECT_TRUE(value->GetAsInteger(&i));
-  EXPECT_EQ(42, i);
-
-  overlay_->SetValue(key, Value::CreateIntegerValue(43));
-
-  EXPECT_EQ(PrefStore::READ_OK, overlay_->GetValue(key, &value));
-  ASSERT_TRUE(value);
-  EXPECT_TRUE(value->GetAsInteger(&i));
-  EXPECT_EQ(43, i);
-
-  EXPECT_EQ(PrefStore::READ_OK, underlay_->GetValue(key, &value));
-  ASSERT_TRUE(value);
-  EXPECT_TRUE(value->GetAsInteger(&i));
-  EXPECT_EQ(42, i);
-
-  overlay_->RemoveValue(key);
-
-  // Value shines through:
-  EXPECT_EQ(PrefStore::READ_OK, overlay_->GetValue(key, &value));
-  ASSERT_TRUE(value);
-  EXPECT_TRUE(value->GetAsInteger(&i));
-  EXPECT_EQ(42, i);
-
-  EXPECT_EQ(PrefStore::READ_OK, underlay_->GetValue(key, &value));
-  ASSERT_TRUE(value);
-  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 341822e..5c46f2e 100644
--- a/chrome/browser/prefs/pref_service.cc
+++ b/chrome/browser/prefs/pref_service.cc
@@ -23,7 +23,7 @@
 #include "chrome/browser/policy/configuration_policy_pref_store.h"
 #include "chrome/browser/prefs/command_line_pref_store.h"
 #include "chrome/browser/prefs/default_pref_store.h"
-#include "chrome/browser/prefs/overlay_persistent_pref_store.h"
+#include "chrome/browser/prefs/incognito_user_pref_store.h"
 #include "chrome/browser/prefs/pref_model_associator.h"
 #include "chrome/browser/prefs/pref_notifier_impl.h"
 #include "chrome/browser/prefs/pref_value_store.h"
@@ -192,7 +192,7 @@ PrefService::PrefService(PrefStore* managed_platform_prefs,
 PrefService::PrefService(const PrefService& original,
                          PrefStore* incognito_extension_prefs)
       : user_pref_store_(
-            new OverlayPersistentPrefStore(original.user_pref_store_.get())),
+            new IncognitoUserPrefStore(original.user_pref_store_.get())),
         default_store_(original.default_store_.get()) {
   // Incognito mode doesn't sync, so no need to create PrefModelAssociator.
   pref_notifier_.reset(new PrefNotifierImpl(this));
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index d5f9749..221fd61 100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -1675,8 +1675,8 @@
         'browser/prefs/command_line_pref_store.h',
         'browser/prefs/default_pref_store.cc',
         'browser/prefs/default_pref_store.h',
-        'browser/prefs/overlay_persistent_pref_store.cc',
-        'browser/prefs/overlay_persistent_pref_store.h',
+        'browser/prefs/incognito_user_pref_store.cc',
+        'browser/prefs/incognito_user_pref_store.h',
         'browser/prefs/pref_change_registrar.cc',
         'browser/prefs/pref_change_registrar.h',
         'browser/prefs/pref_member.cc',
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index ea6b9b8a..8b88913 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1536,7 +1536,7 @@
         'browser/preferences_mock_mac.cc',
         'browser/preferences_mock_mac.h',
         'browser/prefs/command_line_pref_store_unittest.cc',
-        'browser/prefs/overlay_persistent_pref_store_unittest.cc',
+        'browser/prefs/incognito_user_pref_store_unittest.cc',
         'browser/prefs/pref_change_registrar_unittest.cc',
         'browser/prefs/pref_member_unittest.cc',
         'browser/prefs/pref_model_associator_unittest.cc',
diff --git a/chrome/common/json_pref_store.cc b/chrome/common/json_pref_store.cc
index 5891633..5dd3cf2 100644
--- a/chrome/common/json_pref_store.cc
+++ b/chrome/common/json_pref_store.cc
@@ -155,7 +155,8 @@ PrefStore::ReadResult JsonPrefStore::GetValue(const std::string& key,
                                               const Value** result) const {
   Value* tmp = NULL;
   if (prefs_->Get(key, &tmp)) {
-    *result = tmp;
+    if (result)
+      *result = tmp;
     return READ_OK;
   }
   return READ_NO_VALUE;
diff --git a/chrome/common/pref_names.cc b/chrome/common/pref_names.cc
index a0a3378..a7be7a3 100644
--- a/chrome/common/pref_names.cc
+++ b/chrome/common/pref_names.cc
@@ -1327,11 +1327,11 @@ const char kCloudPrintEnableJobPoll[] = "cloud_print.enable_job_poll";
 const char kCloudPrintRobotRefreshToken[] = "cloud_print.robot_refresh_token";
 const char kCloudPrintRobotEmail[] = "cloud_print.robot_email";
 
-// Preference to story proxy settings.
+// Preference to store proxy settings.
 const char kProxy[] = "proxy";
 const char kMaxConnectionsPerProxy[] = "net.max_connections_per_proxy";
 
-// Preferences that are exclusivly used to store managed values for default
+// Preferences that are exclusively used to store managed values for default
 // content settings.
 const char kManagedDefaultCookiesSetting[] =
     "profile.managed_default_content_settings.cookies";
@@ -1344,7 +1344,7 @@ const char kManagedDefaultPluginsSetting[] =
 const char kManagedDefaultPopupsSetting[] =
     "profile.managed_default_content_settings.popups";
 
-// Preferences that are exclusivly used to store managed
+// Preferences that are exclusively used to store managed
 // content settings patterns.
 const char kManagedCookiesAllowedForUrls[] =
     "profile.managed_cookies_allowed_for_urls";
diff --git a/chrome/common/pref_store.h b/chrome/common/pref_store.h
index bc74af1..5e8b2c5 100644
--- a/chrome/common/pref_store.h
+++ b/chrome/common/pref_store.h
@@ -54,9 +54,9 @@ class PrefStore : public base::RefCounted<PrefStore> {
   // Whether the store has completed all asynchronous initialization.
   virtual bool IsInitializationComplete() const;
 
-  // 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.
+  // Get the value for a given preference |key| and stores it in |*result|.
+  // |*result| is only modified if the return value is READ_OK and if |result|
+  // is not NULL. Ownership of the |*result| value remains with the PrefStore.
   virtual ReadResult GetValue(const std::string& key,
                               const base::Value** result) const = 0;