Adds async interface method to PersistentPrefStore.

chrome/common/json_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/common/json_pref_store.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 23 matching lines @@
         FROM_HERE,
         NewRunnableMethod(this,
                           &FileThreadDeserializer::ReadFileAndReport,
                           path));
   }
 
   // Deserializes JSON on the FILE thread.
   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,
         FROM_HERE,
         NewRunnableMethod(this, &FileThreadDeserializer::ReportOnUIThread));
   }
 
   // Reports deserialization result on the UI thread.
   void ReportOnUIThread() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     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,
                            const std::string& error_msg,
                            PersistentPrefStore::PrefReadError* error);
 
  private:
   friend class base::RefCountedThreadSafe<FileThreadDeserializer>;
 
   bool no_dir_;
@@ skipping 48 matching lines @@
   }
 }
 
 }  // namespace
 
 JsonPrefStore::JsonPrefStore(const FilePath& filename,
                              base::MessageLoopProxy* file_message_loop_proxy)
     : 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() {
   CommitPendingWrite();
 }
 
 PrefStore::ReadResult JsonPrefStore::GetValue(const std::string& key,
                                               const Value** result) const {
   Value* tmp = NULL;
   if (prefs_->Get(key, &tmp)) {
@@ skipping 42 matching lines @@
   }
 }
 
 bool JsonPrefStore::ReadOnly() const {
   return read_only_;
 }
 
 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:
       DCHECK(value.get());
       prefs_.reset(static_cast<DictionaryValue*>(value.release()));
       break;
     case PREF_READ_ERROR_NO_FILE:
       // If the file just doesn't exist, maybe this is first run.  In any case
       // there's no harm in writing out default prefs in this case.
       break;
     case PREF_READ_ERROR_JSON_PARSE:
     case PREF_READ_ERROR_JSON_REPEAT:
       break;
     default:
       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;
   }
 
   // This guarantees that class will not be deleted while JSON is readed.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Start async reading of the preferences file. It will delete itself
   // in the end.
   scoped_refptr<FileThreadDeserializer> deserializer(
       new FileThreadDeserializer(this));
   deserializer->Start(path_);
 }
 
 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;
+  Value* value = FileThreadDeserializer::DoReading(path_,
+                                                   &error_,
+                                                   &is_fatal_);
+  OnFileRead(value, error_, is_fatal_);
+  return error_;
+}
 
-  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);
-
-  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() {
   std::string data;
   if (!SerializeData(&data))
     return false;
 
   // Lie about our ability to save.
   if (read_only_)
     return true;
@@ skipping 19 matching lines @@
 }
 
 bool JsonPrefStore::SerializeData(std::string* output) {
   // TODO(tc): Do we want to prune webkit preferences that match the default
   // value?
   JSONStringValueSerializer serializer(output);
   serializer.set_pretty_print(true);
   scoped_ptr<DictionaryValue> copy(prefs_->DeepCopyWithoutEmptyChildren());
   return serializer.Serialize(*(copy.get()));
 }