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 127 matching lines @@
 }
 
 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)

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

Why do we still pass this PrefServiceDelegate to the PrefService? There already
is PrefNotifierImpl, which sends out a PREF_INITIALIZATION_COMPLETED
notification. Is it possible to just use the notification and remove
PrefServiceDelegate?

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

Done.

     : user_pref_store_(user_prefs),
       default_store_(default_store),
       delegate_(delegate) {
   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_,
@@ skipping 28 matching lines @@
   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;
 }
 
-void PrefService::OnPrefsRead(PersistentPrefStore::PrefReadError error,
-                              bool no_dir) {
-  if (no_dir) {
+void PrefService::OnInitializationCompleted() {

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

If we use the PREF_INITIALIZATION_COMPLETED notification instead, this can go
away and the error handling here can really be moved either down to
JsonPrefStore, or over to PrefNotifier, which is the designated home for all
signaling to the outer world.

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

I have leaved "PrefStore::Observer" here, because "PrefStore::Observer" and
"PrefNotifier" both have OnInitializationCompleted() method, so
"PrefNotiferImpl" can't simply inherit from both of this interfaces.

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

Yes, PrefNotifier already has OnInitializationCompleted(). Signaling the outer
world is a PrefNotifier responsibility, which is why I'd like to see the
signaling code moved there. You can change the interface between PrefValueStore
and PrefNotifier to what you need, it's internal to the pref system.

Here is more details on how I imagine this to work:

1. PrefService::CreatePrefService() gets called and PrefStores get constructed
2. ReadPrefs or ReadPrefsAsync() gets called
3. All the pref stores (including JsonPrefStore) notify PrefValueStore about
their initialization status by calling OnInitializationCompleted().
4. Once PrefValueStore has heard back from all of them (or decided there is an
error), it delegates to the PrefNotifier to tell the outside world.
5. PrefNotifierImpl sends the notification.

+  // This is the only message PrefService is waiting for, so remove us from the
+  // observers list.
+  user_pref_store_->RemoveObserver(this);
+
+  PersistentPrefStore::PrefReadError error;
+  bool is_fatal;
+  user_pref_store_->GetErrors(&error, &is_fatal);

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

Could you pass these values directly to OnInitializationCompleted? Then you
wouldn't need to store them in the user pref store, making reading the
preferences stateless.

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

The problem is that "OnInitializationCompleted" is part of the generic
"PrefStore::Observer" interface, which has other users as well.

[Bernhard Bauer, 2011/04/27 10:52:16]

I think passing (PREF_READ_ERROR_NONE, false) at the other call sites should be
fine. Mattias, WDYT?

Another idea would be to add an OnInitializationError method which gets these
values. The contract would then be that eventually one of these two methods is
called.

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

I think passing the parameters makes sense. At the moment, your only use for the
GetErrors() function is so you can call it when you get notified. As long as
there's no reason to call GetErrors() in other context, it doesn't seem useful
to have it separately from the callback.

+  if (is_fatal) {
     // 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
@@ skipping 11 matching lines @@
           NewRunnableFunction(&NotifyReadError, this, message_id));
     }
     UMA_HISTOGRAM_ENUMERATION("PrefService.ReadError", error, 20);
   }
 
   if (delegate_)
     delegate_->OnPrefsLoaded(this, true);
 }
 
 void PrefService::InitFromStorage() {
+  user_pref_store_->AddObserver(this);
   if (!delegate_) {
-    const PersistentPrefStore::PrefReadError error =
-        user_pref_store_->ReadPrefs();
-    OnPrefsRead(error, false);
+    user_pref_store_->ReadPrefs();
   } else {
-    // todo(altimofeev): move this method to PersistentPrefStore interface.
-    (static_cast<JsonPrefStore*>(user_pref_store_.get()))->ReadPrefs(this);
+    user_pref_store_.get()->ReadPrefsAsync();
   }
 }
 
 bool PrefService::ReloadPersistentPrefs() {
   return user_pref_store_->ReadPrefs() ==
              PersistentPrefStore::PREF_READ_ERROR_NONE;
 }
 
 bool PrefService::SavePersistentPrefs() {
   DCHECK(CalledOnValidThread());
@@ skipping 451 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());
 }