Reload 104510: Automatically schedule saving preferences on change.

Fix previously crashing SignedSettingsTempStorageTest by using a TestingPrefService.

BUG=99306
TEST=none


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

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

2 files changed, 31 insertions, 49 deletions

diff --git a/chrome/browser/chromeos/login/signed_settings_temp_storage_unittest.cc b/chrome/browser/chromeos/login/signed_settings_temp_storage_unittest.cc
index 6bcefaa..c6e4131 100644
--- a/chrome/browser/chromeos/login/signed_settings_temp_storage_unittest.cc
+++ b/chrome/browser/chromeos/login/signed_settings_temp_storage_unittest.cc
@@ -12,8 +12,8 @@
 #include "base/file_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/scoped_temp_dir.h"
-#include "chrome/browser/prefs/pref_service.h"
 #include "chrome/common/logging_chrome.h"
+#include "chrome/test/base/testing_pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromeos {
@@ -26,52 +26,30 @@ class SignedSettingsTempStorageTest : public testing::Test {
     ref_map_["name"] = "value";
     ref_map_["2bc6aa16-e0ea-11df-b13d-18a90520e2e5"] = "512";
 
-    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-    FilePath temp_file;
-    ASSERT_TRUE(
-        file_util::CreateTemporaryFileInDir(temp_dir_.path(), &temp_file));
-    local_state_.reset(
-        PrefService::CreatePrefService(temp_file, NULL, false));
-    ASSERT_TRUE(NULL != local_state_.get());
-    SignedSettingsTempStorage::RegisterPrefs(local_state_.get());
+    SignedSettingsTempStorage::RegisterPrefs(&local_state_);
   }
 
   std::map<std::string, std::string> ref_map_;
-  ScopedTempDir temp_dir_;
-  scoped_ptr<PrefService> local_state_;
+  TestingPrefService local_state_;
 };
 
 TEST_F(SignedSettingsTempStorageTest, Basic) {
-  EXPECT_GT(ref_map_.size(), 3u);  // Number above 3 is many.
   typedef std::map<std::string, std::string>::iterator It;
-  std::vector<It> a_list;
   for (It it = ref_map_.begin(); it != ref_map_.end(); ++it) {
-    a_list.push_back(it);
+    EXPECT_TRUE(SignedSettingsTempStorage::Store(it->first,
+                                                 it->second,
+                                                 &local_state_));
   }
-  std::random_shuffle(a_list.begin(), a_list.end());
-  std::vector<It> b_list(a_list);
-  std::copy(b_list.begin(),
-            b_list.begin() + b_list.size() / 2,
-            std::back_inserter(a_list));
-  std::random_shuffle(a_list.begin(), a_list.end());
-  for (size_t i = 0; i < a_list.size(); ++i) {
-    EXPECT_TRUE(SignedSettingsTempStorage::Store(a_list[i]->first,
-                                                 a_list[i]->second,
-                                                 local_state_.get()));
-  }
-  for (int i = 0; i < 3; ++i) {
-    std::copy(a_list.begin(), a_list.end(), std::back_inserter(b_list));
+  for (It it = ref_map_.begin(); it != ref_map_.end(); ++it) {
+    std::string value;
+    EXPECT_TRUE(SignedSettingsTempStorage::Retrieve(it->first, &value,
+                                                    &local_state_));
+    EXPECT_EQ(it->second, value);
   }
-  std::random_shuffle(b_list.begin(), b_list.end());
   std::string value;
-  for (size_t i = 0; i < b_list.size(); ++i) {
-    EXPECT_TRUE(SignedSettingsTempStorage::Retrieve(b_list[i]->first, &value,
-                                                    local_state_.get()));
-    EXPECT_EQ(b_list[i]->second, value);
-    EXPECT_FALSE(SignedSettingsTempStorage::Retrieve("non-existent tv-series",
-                                                     &value,
-                                                     local_state_.get()));
-  }
+  EXPECT_FALSE(SignedSettingsTempStorage::Retrieve("non-existent tv-series",
+                                                   &value,
+                                                   &local_state_));
 }
 
 }  // namespace chromeos
diff --git a/chrome/common/json_pref_store.cc b/chrome/common/json_pref_store.cc
index 83c8187..ac4a780 100644
--- a/chrome/common/json_pref_store.cc
+++ b/chrome/common/json_pref_store.cc
@@ -186,7 +186,7 @@ void JsonPrefStore::SetValue(const std::string& key, Value* value) {
   prefs_->Get(key, &old_value);
   if (!old_value || !value->Equals(old_value)) {
     prefs_->Set(key, new_value.release());
-    FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
+    ReportValueChanged(key);
   }
 }
 
@@ -195,14 +195,16 @@ void JsonPrefStore::SetValueSilently(const std::string& key, Value* value) {
   scoped_ptr<Value> new_value(value);
   Value* old_value = NULL;
   prefs_->Get(key, &old_value);
-  if (!old_value || !value->Equals(old_value))
+  if (!old_value || !value->Equals(old_value)) {
     prefs_->Set(key, new_value.release());
+    if (!read_only_)
+      writer_.ScheduleWrite(this);
+  }
 }
 
 void JsonPrefStore::RemoveValue(const std::string& key) {
-  if (prefs_->Remove(key, NULL)) {
-    FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
-  }
+  if (prefs_->Remove(key, NULL))
+    ReportValueChanged(key);
 }
 
 bool JsonPrefStore::ReadOnly() const {
@@ -286,19 +288,19 @@ bool JsonPrefStore::WritePrefs() {
   if (!SerializeData(&data))
     return false;
 
-  // Lie about our ability to save.
-  if (read_only_)
-    return true;
+  // Don't actually write prefs if we're read-only or don't have any pending
+  // writes.
+  // TODO(bauerb): Make callers of this method call CommitPendingWrite directly.
+  if (writer_.HasPendingWrite() && !read_only_)
+    writer_.WriteNow(data);
 
-  writer_.WriteNow(data);
   return true;
 }
 
 void JsonPrefStore::ScheduleWritePrefs() {
-  if (read_only_)
-    return;
-
-  writer_.ScheduleWrite(this);
+  // Writing prefs should be scheduled automatically, so this is a no-op
+  // for now.
+  // TODO(bauerb): Remove calls to this method.
 }
 
 void JsonPrefStore::CommitPendingWrite() {
@@ -308,6 +310,8 @@ void JsonPrefStore::CommitPendingWrite() {
 
 void JsonPrefStore::ReportValueChanged(const std::string& key) {
   FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
+  if (!read_only_)
+    writer_.ScheduleWrite(this);
 }
 
 bool JsonPrefStore::SerializeData(std::string* output) {