Adds async interface method to PersistentPrefStore.

chrome/common/json_pref_store.cc

Index: chrome/common/json_pref_store.cc
diff --git a/chrome/common/json_pref_store.cc b/chrome/common/json_pref_store.cc
index 038f6331b6a88289d459af13f0e21d07494f3c92..a0e2f9dc8f278ceeea572318daf48fbccf71b8b9 100644
--- a/chrome/common/json_pref_store.cc
+++ b/chrome/common/json_pref_store.cc
@@ -41,14 +41,7 @@ class FileThreadDeserializer
   void ReadFileAndReport(const FilePath& path) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-    int error_code;
-    std::string error_msg;
-    JSONFileValueSerializer serializer(path);
-    value_.reset(serializer.Deserialize(&error_code, &error_msg));
-
-    HandleErrors(value_.get(), path, error_code, error_msg, &error_);
-
-    no_dir_ = !file_util::PathExists(path.DirName());
+    value_.reset(DoReading(path, &error_, &no_dir_));
 
     BrowserThread::PostTask(
         BrowserThread::UI,
@@ -62,6 +55,18 @@ class FileThreadDeserializer
     delegate_->OnFileRead(value_.release(), error_, no_dir_);
   }
 
+  static Value* DoReading(const FilePath& path,
+                          PersistentPrefStore::PrefReadError* error,
+                          bool* no_dir) {
+    int error_code;
+    std::string error_msg;
+    JSONFileValueSerializer serializer(path);
+    Value* value = serializer.Deserialize(&error_code, &error_msg);
+    HandleErrors(value, path, error_code, error_msg, error);
+    (*no_dir) = !file_util::PathExists(path.DirName());
+    return value;
+  }
+
   static void HandleErrors(const Value* value,
                            const FilePath& path,
                            int error_code,
@@ -130,7 +135,9 @@ JsonPrefStore::JsonPrefStore(const FilePath& filename,
     : path_(filename),
       prefs_(new DictionaryValue()),
       read_only_(false),
-      writer_(filename, file_message_loop_proxy) {
+      writer_(filename, file_message_loop_proxy),
+      error_(PREF_READ_ERROR_NONE),
+      is_fatal_(false) {
 }
 
 JsonPrefStore::~JsonPrefStore() {
@@ -193,12 +200,16 @@ bool JsonPrefStore::ReadOnly() const {
 void JsonPrefStore::OnFileRead(Value* value_owned,
                                PersistentPrefStore::PrefReadError error,
                                bool no_dir) {
+  error_ = error;
+  is_fatal_ = no_dir;
+
   scoped_ptr<Value> value(value_owned);
   switch (error) {
     case PREF_READ_ERROR_ACCESS_DENIED:
     case PREF_READ_ERROR_FILE_OTHER:
     case PREF_READ_ERROR_FILE_LOCKED:
     case PREF_READ_ERROR_JSON_TYPE:
+    case PREF_READ_ERROR_FILE_NOT_SPECIFIED:
       read_only_ = true;
       break;
     case PREF_READ_ERROR_NONE:
@@ -216,17 +227,14 @@ void JsonPrefStore::OnFileRead(Value* value_owned,
       NOTREACHED() << "Unknown error: " << error;
   }
 
-  if (delegate_)
-    delegate_->OnPrefsRead(error, no_dir);
+  FOR_EACH_OBSERVER(PrefStore::Observer,
+                    observers_,
+                    OnInitializationCompleted());
 }
 
-void JsonPrefStore::ReadPrefs(Delegate* delegate) {
-  DCHECK(delegate);
-  delegate_ = delegate;
-
+void JsonPrefStore::ReadPrefsAsync() {

[Mattias Nissler (ping if slow), 2011/04/26 09:04:26]

I was wondering before if we could just merge ReadPrefs() and ReadPrefsAsync()
into one. After all, doing I/O on the main thread is a bad thing (TM), and
always trying to do our I/O on the FILE thread and only falling back if that's
impossible seems like a sane strategy to me.

Would you be willing to give that a try? We'd just execute the asynchronous path
till the PostTask() call in FileThreadDeserializer::Start(). If that  fails
(indicating the associated threads are not yet spinning), we could just read
synchronously.

[altimofeev, 2011/04/27 10:32:08]

I remember, and actually thought about it. And the main thing which I don't like
about this idea is that the caller of the ReadPrefs() will not know what version
(sync or async) will be used. At the same time, there are situations, when the
caller want this function to be synchronous (e.g. unittest, browser init). So it
is either overkill to change these parts to support asynchronous reading or
unclear code, which assumes that ReadPrefs() will read synchronously.

[Mattias Nissler (ping if slow), 2011/04/27 12:16:30]

If we do it, we should make sure we don't put additional requirements on client
code. You are right in that we'd probably need some unit test changes (probably
calling MessageLoop::RunAllPending() at some places). I think your reasoning
about unclear expectations is valid, so let's go with the async flag then.

   if (path_.empty()) {
-    read_only_ = true;
-    delegate_->OnPrefsRead(PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
+    OnFileRead(NULL, PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
     return;
   }
 
@@ -241,29 +249,21 @@ void JsonPrefStore::ReadPrefs(Delegate* delegate) {
 }
 
 PersistentPrefStore::PrefReadError JsonPrefStore::ReadPrefs() {
-  delegate_ = NULL;
-
   if (path_.empty()) {
-    read_only_ = true;
-    return PREF_READ_ERROR_FILE_NOT_SPECIFIED;
+    OnFileRead(NULL, PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
+    return error_;
   }
 
-  int error_code = 0;
-  std::string error_msg;
-
-  JSONFileValueSerializer serializer(path_);
-  scoped_ptr<Value> value(serializer.Deserialize(&error_code, &error_msg));
-
-  PersistentPrefStore::PrefReadError error;
-  FileThreadDeserializer::HandleErrors(value.get(),
-                                       path_,
-                                       error_code,
-                                       error_msg,
-                                       &error);
-
-  OnFileRead(value.release(), error, false);
+  Value* value = FileThreadDeserializer::DoReading(path_,
+                                                   &error_,
+                                                   &is_fatal_);
+  OnFileRead(value, error_, is_fatal_);
+  return error_;
+}
 
-  return error;
+void JsonPrefStore::GetErrors(PrefReadError* error, bool* is_fatal) {
+  (*error) = error_;

[Bernhard Bauer, 2011/04/25 15:54:10]

Nit: the parentheses are not actually necessary, but I don't mind them very much
either.

[altimofeev, 2011/04/27 10:32:08]

This is the matter of taste, I think. But grep says that your variant is more
common in Chromium code (1497 vs 9). So will not use unnecessary parentheses.

+  (*is_fatal) = is_fatal_;
 }
 
 bool JsonPrefStore::WritePrefs() {