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

chrome/browser/prefs/incognito_user_pref_store.cc

+// 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 (!StoreInOverlay(key))

[Mattias Nissler (ping if slow), 2011/07/13 12:15:32]

This shouldn't be necessary, since we should not place values in the overlay in
this case anyway.

[battre, 2011/07/14 12:31:56]

Done.

+    return underlay_->GetValue(key, result);
+
+  if (overlay_.GetValue(key, result))
+    return READ_OK;
+  return underlay_->GetValue(key, result);
+}
+
+PrefStore::ReadResult IncognitoUserPrefStore::GetMutableValue(
+    const std::string& key,
+    Value** result) {
+  if (!StoreInOverlay(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) {

[Peter Kasting, 2011/07/13 22:02:06]

Nit: Shorter:

if (read_result != READ_OK)
  return read_result;
... deep copy ...

[battre, 2011/07/14 12:31:56]

Done.

+    *result = underlay_value->DeepCopy();
+    overlay_.SetValue(key, *result);
+    return READ_OK;
+  }
+  // Return read failure if underlay stores no value for |key|.

[Peter Kasting, 2011/07/13 22:02:06]

Nit: This comment doesn't really add much to the code.

[battre, 2011/07/14 12:31:56]

Done.

+  return read_result;
+}
+
+void IncognitoUserPrefStore::SetValue(const std::string& key,
+                                      Value* value) {
+  if (!StoreInOverlay(key)) {
+    underlay_->SetValue(key, value);
+    return;
+  }
+
+  if (overlay_.SetValue(key, value))
+    ReportValueChanged(key);
+}
+
+void IncognitoUserPrefStore::SetValueSilently(const std::string& key,
+                                              Value* value) {
+  if (!StoreInOverlay(key)) {

[Peter Kasting, 2011/07/13 22:02:06]

Nit: Shorter:

if (StoreInOverlay(key))
  overlay_.SetValue(key, value);
else
  underlay_->SetValueSilently(key, value);

[battre, 2011/07/13 22:28:32]

Hm, do you prefer brevity over common patterns? All functions followed the "if
(!StoreInOverlay(...))" pattern. I prefer my proposed style, but I will change
it to your proposed style, if I don't hear anything else.

+    underlay_->SetValueSilently(key, value);
+    return;
+  }
+
+  overlay_.SetValue(key, value);
+}
+
+void IncognitoUserPrefStore::RemoveValue(const std::string& key) {
+  if (!StoreInOverlay(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::StoreInOverlay(const std::string& key) const {
+  // We exclude some preferences from being stored in a non-persisted
+  // incognito user pref store. These settings are stored in the regular
+  // user pref store on disk even if they are modified from within an
+  // incognito profile.
+  if (key == prefs::kCheckDefaultBrowser)

[Peter Kasting, 2011/07/13 22:02:06]

Nit: Seems just as good, and shorter:

return (key != prefs::kCheckDefaultBrowser) &&
       (key != prefs::kShowBookmarkBar);

[battre, 2011/07/14 12:31:56]

Done.

+    return false;
+  if (key == prefs::kShowBookmarkBar)
+    return false;
+
+  return true;
+}