Adds async interface method to PersistentPrefStore.

chrome/browser/prefs/pref_service.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/pref_service.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/command_line.h"
@@ skipping 61 matching lines @@
       NOTREACHED() <<
           "list and dictionary types cannot have default locale values";
     }
   }
   NOTREACHED();
   return Value::CreateNullValue();
 }
 
 // Forwards a notification after a PostMessage so that we can wait for the
 // MessageLoop to run.
-void NotifyReadError(PrefService* pref, int message_id) {
+void NotifyReadError(int message_id) {
   ShowProfileErrorDialog(message_id);
 }
 
+// Shows notifications which correspond to PersistentPrefStore's reading errors.
+class ReadErrorHandler : public PersistentPrefStore::ReadErrorDelegate {
+ public:
+  virtual void OnError(PersistentPrefStore::PrefReadError error) {
+    if (error != PersistentPrefStore::PREF_READ_ERROR_NONE) {
+      // Failing to load prefs on startup is a bad thing(TM). See bug 38352 for
+      // an example problem that this can cause.
+      // Do some diagnosis and try to avoid losing data.
+      int message_id = 0;
+      if (error <= PersistentPrefStore::PREF_READ_ERROR_JSON_TYPE) {
+        message_id = IDS_PREFERENCES_CORRUPT_ERROR;
+      } else if (error != PersistentPrefStore::PREF_READ_ERROR_NO_FILE) {
+        message_id = IDS_PREFERENCES_UNREADABLE_ERROR;
+      }
+
+      if (message_id) {
+        BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+            NewRunnableFunction(&NotifyReadError, message_id));
+      }
+      UMA_HISTOGRAM_ENUMERATION("PrefService.ReadError", error, 20);
+    }
+  }
+};
+
 }  // namespace
 
 // static
 PrefService* PrefService::CreatePrefService(const FilePath& pref_filename,
                                             PrefStore* extension_prefs,
-                                            Profile* profile) {
-  return CreatePrefServiceAsync(pref_filename, extension_prefs, profile, NULL);
-}
-
-// static
-PrefService* PrefService::CreatePrefServiceAsync(
-    const FilePath& pref_filename,
-    PrefStore* extension_prefs,
-    Profile* profile,
-    PrefServiceDelegate* delegate) {
+                                            Profile* profile,
+                                            bool async) {
   using policy::ConfigurationPolicyPrefStore;
 
 #if defined(OS_LINUX)
   // We'd like to see what fraction of our users have the preferences
   // stored on a network file system, as we've had no end of troubles
   // with NFS/AFS.
   // TODO(evanm): remove this once we've collected state.
   file_util::FileSystemType fstype;
   if (file_util::GetFileSystemType(pref_filename.DirName(), &fstype)) {
     UMA_HISTOGRAM_ENUMERATION("PrefService.FileSystemType",
@@ skipping 11 matching lines @@
   JsonPrefStore* user = new JsonPrefStore(
       pref_filename,
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE));
   ConfigurationPolicyPrefStore* recommended_platform =
       ConfigurationPolicyPrefStore::CreateRecommendedPlatformPolicyPrefStore();
   ConfigurationPolicyPrefStore* recommended_cloud =
       ConfigurationPolicyPrefStore::CreateRecommendedCloudPolicyPrefStore(
           profile);
   DefaultPrefStore* default_pref_store = new DefaultPrefStore();
 
-  return new PrefService(managed_platform, managed_cloud, extension_prefs,
-                         command_line, user, recommended_platform,
-                         recommended_cloud, default_pref_store, delegate);
+  return new PrefService(
+      managed_platform, managed_cloud, extension_prefs,
+      command_line, user, recommended_platform,
+      recommended_cloud, default_pref_store, async);
 }
 
 PrefService* PrefService::CreateIncognitoPrefService(
     PrefStore* incognito_extension_prefs) {
   return new PrefService(*this, incognito_extension_prefs);
 }
 
 PrefService::PrefService(PrefStore* managed_platform_prefs,
                          PrefStore* managed_cloud_prefs,
                          PrefStore* extension_prefs,
                          PrefStore* command_line_prefs,
                          PersistentPrefStore* user_prefs,
                          PrefStore* recommended_platform_prefs,
                          PrefStore* recommended_cloud_prefs,
                          DefaultPrefStore* default_store,
-                         PrefServiceDelegate* delegate)
+                         bool async)
     : user_pref_store_(user_prefs),
-      default_store_(default_store),
-      delegate_(delegate) {
+      default_store_(default_store) {
   pref_sync_associator_.reset(new PrefModelAssociator(this));
   pref_notifier_.reset(new PrefNotifierImpl(this));
   pref_value_store_.reset(
       new PrefValueStore(managed_platform_prefs,
                          managed_cloud_prefs,
                          extension_prefs,
                          command_line_prefs,
                          user_pref_store_,
                          recommended_platform_prefs,
                          recommended_cloud_prefs,
                          default_store,
                          pref_sync_associator_.get(),
                          pref_notifier_.get()));
-  InitFromStorage();
+  InitFromStorage(async);
 }
 
 PrefService::PrefService(const PrefService& original,
                          PrefStore* incognito_extension_prefs)
       : user_pref_store_(
             new OverlayPersistentPrefStore(original.user_pref_store_.get())),
-        default_store_(original.default_store_.get()),
-        delegate_(NULL) {
+        default_store_(original.default_store_.get()) {
   // Incognito mode doesn't sync, so no need to create PrefModelAssociator.
   pref_notifier_.reset(new PrefNotifierImpl(this));
   pref_value_store_.reset(original.pref_value_store_->CloneAndSpecialize(
       NULL, // managed_platform_prefs
       NULL, // managed_cloud_prefs
       incognito_extension_prefs,
       NULL, // command_line_prefs
       user_pref_store_.get(),
       NULL, // recommended_platform_prefs
       NULL, // recommended_cloud_prefs
       default_store_.get(),
       NULL, // pref_sync_associator_
       pref_notifier_.get()));
-  InitFromStorage();
 }
 
 PrefService::~PrefService() {
   DCHECK(CalledOnValidThread());
   STLDeleteContainerPointers(prefs_.begin(), prefs_.end());
   prefs_.clear();
 
   // Reset pointers so accesses after destruction reliably crash.
   pref_value_store_.reset();
   user_pref_store_ = NULL;
   default_store_ = NULL;
   if (pref_sync_associator_.get())
     pref_sync_associator_->DisassociateModels();
   pref_sync_associator_.reset();
 }
 
-void PrefService::OnPrefsRead(PersistentPrefStore::PrefReadError error,
-                              bool no_dir) {
-  if (no_dir) {
-    // Bad news. When profile is created, the process that creates the directory
-    // is explicitly started. So if directory is missing it probably means that
-    // Chromium hasn't sufficient privileges.
-    CHECK(delegate_);
-    delegate_->OnPrefsLoaded(this, false);
-    return;
-  }
-
-  if (error != PersistentPrefStore::PREF_READ_ERROR_NONE) {
-    // Failing to load prefs on startup is a bad thing(TM). See bug 38352 for
-    // an example problem that this can cause.
-    // Do some diagnosis and try to avoid losing data.
-    int message_id = 0;
-    if (error <= PersistentPrefStore::PREF_READ_ERROR_JSON_TYPE) {
-      message_id = IDS_PREFERENCES_CORRUPT_ERROR;
-    } else if (error != PersistentPrefStore::PREF_READ_ERROR_NO_FILE) {
-      message_id = IDS_PREFERENCES_UNREADABLE_ERROR;
-    }
-
-    if (message_id) {
-      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-          NewRunnableFunction(&NotifyReadError, this, message_id));
-    }
-    UMA_HISTOGRAM_ENUMERATION("PrefService.ReadError", error, 20);
-  }
-
-  if (delegate_)
-    delegate_->OnPrefsLoaded(this, true);
-}
-
-void PrefService::InitFromStorage() {
-  if (!delegate_) {
-    const PersistentPrefStore::PrefReadError error =
-        user_pref_store_->ReadPrefs();
-    OnPrefsRead(error, false);
+void PrefService::InitFromStorage(bool async) {
+  if (!async) {
+    ReadErrorHandler error_handler;
+    error_handler.OnError(user_pref_store_->ReadPrefs());
   } else {
-    // todo(altimofeev): move this method to PersistentPrefStore interface.
-    (static_cast<JsonPrefStore*>(user_pref_store_.get()))->ReadPrefs(this);
+    // Guarantee that initialization happens after this function returned.
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        NewRunnableMethod(user_pref_store_.get(),
+                          &PersistentPrefStore::ReadPrefsAsync,
+                          new ReadErrorHandler()));

[Mattias Nissler (ping if slow), 2011/05/10 11:27:50]

It seems like this ReadErrorHandler instance gets leaked. 
You cloud solve this e.g. by making OnError delete the ReadErrorHandler and
changing the if branch above to

(new ReadErrorHandler).OnError(user_pref_store_->ReadPrefs());

[altimofeev, 2011/05/10 12:01:18]

Excuse me, but I didn't get you.
ReadPrefsAsync owns ReadErrorDelegate (delegate is saved as scoped_ptr<>). Also
note, it is mentioned in the corresponding comments.

[Mattias Nissler (ping if slow), 2011/05/10 12:16:41]

I'm not talking about the ReadErrorDelegate, but the ReadErrorHandler instance
you create in line 230. I couldn't find where that object gets deleted. Maybe I
miss something?

[altimofeev, 2011/05/10 14:24:19]

ReadErrorDelegate is an interface which is implemented by ReadErrorHandler. Also
this interface is provided to the ReadPrefsAsync to be called, if reading error
occurs.

So, as I have said, ReadErrorHandler is owned by |user_pref_store_| and will be
deleted by it in the end.

But you are right - I have forgotten to delete it in some PersistenPrefStore
realizations (non-JsonPrefStore ones) - fixed.

[Mattias Nissler (ping if slow), 2011/05/10 14:43:54]

Ah, sorry, I didn't realize ReadPrefAsync takes ownership. What you do is
correct.

On a side-note: In this particular case, you could also use the standard
Callback infrastructure (see base/callback.h and base/callback_old.h,
respectively), instead of coding up an ad-hoc interface class. Not a review
comment, feel free to decide yourself :)
Thanks!

   }
 }
 
 bool PrefService::ReloadPersistentPrefs() {
   return user_pref_store_->ReadPrefs() ==
              PersistentPrefStore::PREF_READ_ERROR_NONE;
 }
 
 bool PrefService::SavePersistentPrefs() {
   DCHECK(CalledOnValidThread());
@@ skipping 653 matching lines @@
   return pref_value_store()->PrefValueFromDefaultStore(name_.c_str());
 }
 
 bool PrefService::Preference::IsUserModifiable() const {
   return pref_value_store()->PrefValueUserModifiable(name_.c_str());
 }
 
 bool PrefService::Preference::IsExtensionModifiable() const {
   return pref_value_store()->PrefValueExtensionModifiable(name_.c_str());
 }