Adds async interface method, which is used by JsonPrefStore. Also see the bug description for more info.
BUG=chromium-os:14289
TEST=JsonPrefStoreTest.*
Review URL: http://codereview.chromium.org/6894020
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@84962 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/prefs/overlay_persistent_pref_store.cc b/chrome/browser/prefs/overlay_persistent_pref_store.cc
index e75fbc66..13c51bb3 100644
--- a/chrome/browser/prefs/overlay_persistent_pref_store.cc
+++ b/chrome/browser/prefs/overlay_persistent_pref_store.cc
@@ -4,6 +4,7 @@
#include "chrome/browser/prefs/overlay_persistent_pref_store.h"
+#include "base/memory/scoped_ptr.h"
#include "base/values.h"
OverlayPersistentPrefStore::OverlayPersistentPrefStore(
@@ -80,9 +81,17 @@
PersistentPrefStore::PrefReadError OverlayPersistentPrefStore::ReadPrefs() {
// We do not read intentionally.
+ OnInitializationCompleted(true);
return PersistentPrefStore::PREF_READ_ERROR_NONE;
}
+void OverlayPersistentPrefStore::ReadPrefsAsync(
+ ReadErrorDelegate* error_delegate_raw) {
+ scoped_ptr<ReadErrorDelegate> error_delegate(error_delegate_raw);
+ // We do not read intentionally.
+ OnInitializationCompleted(true);
+}
+
bool OverlayPersistentPrefStore::WritePrefs() {
// We do not write intentionally.
return true;
@@ -105,7 +114,7 @@
FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
}
-void OverlayPersistentPrefStore::OnInitializationCompleted() {
+void OverlayPersistentPrefStore::OnInitializationCompleted(bool succeeded) {
FOR_EACH_OBSERVER(PrefStore::Observer, observers_,
- OnInitializationCompleted());
+ OnInitializationCompleted(succeeded));
}
diff --git a/chrome/browser/prefs/overlay_persistent_pref_store.h b/chrome/browser/prefs/overlay_persistent_pref_store.h
index bbb85c3..57ffe8d7 100644
--- a/chrome/browser/prefs/overlay_persistent_pref_store.h
+++ b/chrome/browser/prefs/overlay_persistent_pref_store.h
@@ -43,6 +43,7 @@
virtual void RemoveValue(const std::string& key);
virtual bool ReadOnly() const;
virtual PrefReadError ReadPrefs();
+ virtual void ReadPrefsAsync(ReadErrorDelegate* delegate);
virtual bool WritePrefs();
virtual void ScheduleWritePrefs();
virtual void CommitPendingWrite();
@@ -51,7 +52,7 @@
private:
// Methods of PrefStore::Observer.
virtual void OnPrefValueChanged(const std::string& key);
- virtual void OnInitializationCompleted();
+ virtual void OnInitializationCompleted(bool succeeded);
ObserverList<PrefStore::Observer, true> observers_;
PrefValueMap overlay_;
diff --git a/chrome/browser/prefs/pref_notifier.h b/chrome/browser/prefs/pref_notifier.h
index f2d9c52..f4d3a5a 100644
--- a/chrome/browser/prefs/pref_notifier.h
+++ b/chrome/browser/prefs/pref_notifier.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
@@ -21,7 +21,7 @@
virtual void OnPreferenceChanged(const std::string& pref_name) = 0;
// Broadcasts the intialization completed notification.
- virtual void OnInitializationCompleted() = 0;
+ virtual void OnInitializationCompleted(bool succeeded) = 0;
};
#endif // CHROME_BROWSER_PREFS_PREF_NOTIFIER_H_
diff --git a/chrome/browser/prefs/pref_notifier_impl.cc b/chrome/browser/prefs/pref_notifier_impl.cc
index d5ad24c..6d1dd2a 100644
--- a/chrome/browser/prefs/pref_notifier_impl.cc
+++ b/chrome/browser/prefs/pref_notifier_impl.cc
@@ -74,13 +74,13 @@
FireObservers(path);
}
-void PrefNotifierImpl::OnInitializationCompleted() {
+void PrefNotifierImpl::OnInitializationCompleted(bool succeeded) {
DCHECK(CalledOnValidThread());
NotificationService::current()->Notify(
NotificationType::PREF_INITIALIZATION_COMPLETED,
Source<PrefService>(pref_service_),
- NotificationService::NoDetails());
+ Details<bool>(&succeeded));
}
void PrefNotifierImpl::FireObservers(const std::string& path) {
diff --git a/chrome/browser/prefs/pref_notifier_impl.h b/chrome/browser/prefs/pref_notifier_impl.h
index 085e547..8323fcbe 100644
--- a/chrome/browser/prefs/pref_notifier_impl.h
+++ b/chrome/browser/prefs/pref_notifier_impl.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
@@ -30,7 +30,7 @@
// PrefNotifier overrides.
virtual void OnPreferenceChanged(const std::string& pref_name);
- virtual void OnInitializationCompleted();
+ virtual void OnInitializationCompleted(bool succeeded);
protected:
// A map from pref names to a list of observers. Observers get fired in the
diff --git a/chrome/browser/prefs/pref_notifier_impl_unittest.cc b/chrome/browser/prefs/pref_notifier_impl_unittest.cc
index 096b625..c413e32 100644
--- a/chrome/browser/prefs/pref_notifier_impl_unittest.cc
+++ b/chrome/browser/prefs/pref_notifier_impl_unittest.cc
@@ -86,8 +86,8 @@
Field(&NotificationType::value,
NotificationType::PREF_INITIALIZATION_COMPLETED),
Source<PrefService>(&pref_service_),
- NotificationService::NoDetails()));
- notifier.OnInitializationCompleted();
+ Property(&Details<bool>::ptr, testing::Pointee(true))));
+ notifier.OnInitializationCompleted(true);
}
TEST_F(PrefNotifierTest, AddAndRemovePrefObservers) {
diff --git a/chrome/browser/prefs/pref_service.cc b/chrome/browser/prefs/pref_service.cc
index 5096d1d7..5614bbf 100644
--- a/chrome/browser/prefs/pref_service.cc
+++ b/chrome/browser/prefs/pref_service.cc
@@ -79,25 +79,41 @@
// 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)
@@ -129,9 +145,10 @@
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(
@@ -147,10 +164,9 @@
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(
@@ -164,15 +180,14 @@
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(
@@ -186,7 +201,6 @@
default_store_.get(),
NULL, // pref_sync_associator_
pref_notifier_.get()));
- InitFromStorage();
}
PrefService::~PrefService() {
@@ -203,47 +217,17 @@
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()));
}
}
diff --git a/chrome/browser/prefs/pref_service.h b/chrome/browser/prefs/pref_service.h
index 3990d5a..0a98c4c 100644
--- a/chrome/browser/prefs/pref_service.h
+++ b/chrome/browser/prefs/pref_service.h
@@ -36,13 +36,7 @@
class PrefService;
-class PrefServiceDelegate {
- public:
- virtual void OnPrefsLoaded(PrefService* prefs, bool success) = 0;
-};
-
-class PrefService : public base::NonThreadSafe,
- public JsonPrefStore::Delegate {
+class PrefService : public base::NonThreadSafe {
public:
// Enum used when registering preferences to determine if it should be synced
// or not. This is only used for profile prefs, not local state prefs.
@@ -129,26 +123,21 @@
DISALLOW_COPY_AND_ASSIGN(Preference);
};
- // JsonPrefStore::Delegate implementaion.
- virtual void OnPrefsRead(PersistentPrefStore::PrefReadError error,
- bool no_dir);
-
// Factory method that creates a new instance of a PrefService with the
// applicable PrefStores. The |pref_filename| points to the user preference
// file. The |profile| is the one to which these preferences apply; it may be
// NULL if we're dealing with the local state. This is the usual way to create
// a new PrefService. |extension_pref_store| is used as the source for
// extension-controlled preferences and may be NULL. The PrefService takes
- // ownership of |extension_pref_store|.
+ // ownership of |extension_pref_store|. If |async| is true, asynchronous
+ // version is used. Notifies using PREF_INITIALIZATION_COMPLETED in the end.
+ // Details is set to the created PrefService or NULL if creation has failed.
+ // Note, it is guaranteed that in asynchronous version initialization happens
+ // after this function returned.
static PrefService* CreatePrefService(const FilePath& pref_filename,
PrefStore* extension_pref_store,
- Profile* profile);
-
- // Same as above, but with async initialization.
- static PrefService* CreatePrefServiceAsync(const FilePath& pref_filename,
- PrefStore* extension_pref_store,
- Profile* profile,
- PrefServiceDelegate* delegate);
+ Profile* profile,
+ bool async);
// Creates an incognito copy of the pref service that shares most pref stores
// but uses a fresh non-persistent overlay for the user pref store and an
@@ -316,7 +305,7 @@
PrefStore* recommended_platform_prefs,
PrefStore* recommended_cloud_prefs,
DefaultPrefStore* default_store,
- PrefServiceDelegate* delegate);
+ bool async);
// The PrefNotifier handles registering and notifying preference observers.
// It is created and owned by this PrefService. Subclasses may access it for
@@ -376,7 +365,7 @@
// Load preferences from storage, attempting to diagnose and handle errors.
// This should only be called from the constructor.
- void InitFromStorage();
+ void InitFromStorage(bool async);
// Used to set the value of dictionary or list values in the user pref store.
// This will create a dictionary or list if one does not exist in the user
@@ -400,10 +389,6 @@
// of registered preferences are.
mutable PreferenceSet prefs_;
- // Holds delegator to be called after initialization, if async version
- // is used.
- PrefServiceDelegate* delegate_;
-
// The model associator that maintains the links with the sync db.
scoped_ptr<PrefModelAssociator> pref_sync_associator_;
diff --git a/chrome/browser/prefs/pref_service_mock_builder.cc b/chrome/browser/prefs/pref_service_mock_builder.cc
index 1de0403..66e3e99 100644
--- a/chrome/browser/prefs/pref_service_mock_builder.cc
+++ b/chrome/browser/prefs/pref_service_mock_builder.cc
@@ -114,7 +114,7 @@
recommended_platform_prefs_.get(),
recommended_cloud_prefs_.get(),
new DefaultPrefStore(),
- NULL);
+ false);
managed_platform_prefs_ = NULL;
managed_cloud_prefs_ = NULL;
extension_prefs_ = NULL;
diff --git a/chrome/browser/prefs/pref_value_store.cc b/chrome/browser/prefs/pref_value_store.cc
index dbb29aebd..44b526f 100644
--- a/chrome/browser/prefs/pref_value_store.cc
+++ b/chrome/browser/prefs/pref_value_store.cc
@@ -38,8 +38,9 @@
pref_value_store_->OnPrefValueChanged(type_, key);
}
-void PrefValueStore::PrefStoreKeeper::OnInitializationCompleted() {
- pref_value_store_->OnInitializationCompleted(type_);
+void PrefValueStore::PrefStoreKeeper::OnInitializationCompleted(
+ bool succeeded) {
+ pref_value_store_->OnInitializationCompleted(type_, succeeded);
}
PrefValueStore::PrefValueStore(PrefStore* managed_platform_prefs,
@@ -53,7 +54,8 @@
PrefModelAssociator* pref_sync_associator,
PrefNotifier* pref_notifier)
: pref_sync_associator_(pref_sync_associator),
- pref_notifier_(pref_notifier) {
+ pref_notifier_(pref_notifier),
+ initialization_failed_(false) {
InitPrefStore(MANAGED_PLATFORM_STORE, managed_platform_prefs);
InitPrefStore(MANAGED_CLOUD_STORE, managed_cloud_prefs);
InitPrefStore(EXTENSION_STORE, extension_prefs);
@@ -245,7 +247,14 @@
}
void PrefValueStore::OnInitializationCompleted(
- PrefValueStore::PrefStoreType type) {
+ PrefValueStore::PrefStoreType type, bool succeeded) {
+ if (initialization_failed_)
+ return;
+ if (!succeeded) {
+ initialization_failed_ = true;
+ pref_notifier_->OnInitializationCompleted(false);
+ return;
+ }
CheckInitializationCompleted();
}
@@ -255,11 +264,13 @@
}
void PrefValueStore::CheckInitializationCompleted() {
+ if (initialization_failed_)
+ return;
for (size_t i = 0; i <= PREF_STORE_TYPE_MAX; ++i) {
scoped_refptr<PrefStore> store =
GetPrefStore(static_cast<PrefStoreType>(i));
if (store && !store->IsInitializationComplete())
return;
}
- pref_notifier_->OnInitializationCompleted();
+ pref_notifier_->OnInitializationCompleted(true);
}
diff --git a/chrome/browser/prefs/pref_value_store.h b/chrome/browser/prefs/pref_value_store.h
index 07a5b61..5d3d5bb7 100644
--- a/chrome/browser/prefs/pref_value_store.h
+++ b/chrome/browser/prefs/pref_value_store.h
@@ -155,7 +155,7 @@
private:
// PrefStore::Observer implementation.
virtual void OnPrefValueChanged(const std::string& key);
- virtual void OnInitializationCompleted();
+ virtual void OnInitializationCompleted(bool succeeded);
// PrefValueStore this keeper is part of.
PrefValueStore* pref_value_store_;
@@ -214,7 +214,7 @@
void OnPrefValueChanged(PrefStoreType type, const std::string& key);
// Handle the event that the store for |type| has completed initialization.
- void OnInitializationCompleted(PrefStoreType type);
+ void OnInitializationCompleted(PrefStoreType type, bool succeeded);
// Initializes a pref store keeper. Sets up a PrefStoreKeeper that will take
// ownership of the passed |pref_store|.
@@ -247,6 +247,9 @@
// A mapping of preference names to their registered types.
PrefTypeMap pref_types_;
+ // True if not all of the PrefStores were initialized successfully.
+ bool initialization_failed_;
+
DISALLOW_COPY_AND_ASSIGN(PrefValueStore);
};
diff --git a/chrome/browser/prefs/pref_value_store_unittest.cc b/chrome/browser/prefs/pref_value_store_unittest.cc
index 17d939c..b5eaff0 100644
--- a/chrome/browser/prefs/pref_value_store_unittest.cc
+++ b/chrome/browser/prefs/pref_value_store_unittest.cc
@@ -29,7 +29,7 @@
class MockPrefNotifier : public PrefNotifier {
public:
MOCK_METHOD1(OnPreferenceChanged, void(const std::string&));
- MOCK_METHOD0(OnInitializationCompleted, void());
+ MOCK_METHOD1(OnInitializationCompleted, void(bool));
};
// Allows to capture sync model associator interaction.
@@ -494,7 +494,7 @@
}
TEST_F(PrefValueStoreTest, OnInitializationCompleted) {
- EXPECT_CALL(pref_notifier_, OnInitializationCompleted()).Times(0);
+ EXPECT_CALL(pref_notifier_, OnInitializationCompleted(true)).Times(0);
managed_platform_pref_store_->SetInitializationCompleted();
managed_cloud_pref_store_->SetInitializationCompleted();
extension_pref_store_->SetInitializationCompleted();
@@ -505,7 +505,7 @@
Mock::VerifyAndClearExpectations(&pref_notifier_);
// The notification should only be triggered after the last store is done.
- EXPECT_CALL(pref_notifier_, OnInitializationCompleted()).Times(1);
+ EXPECT_CALL(pref_notifier_, OnInitializationCompleted(true)).Times(1);
user_pref_store_->SetInitializationCompleted();
Mock::VerifyAndClearExpectations(&pref_notifier_);
}
diff --git a/chrome/browser/prefs/testing_pref_store.cc b/chrome/browser/prefs/testing_pref_store.cc
index e85a836..d080169 100644
--- a/chrome/browser/prefs/testing_pref_store.cc
+++ b/chrome/browser/prefs/testing_pref_store.cc
@@ -55,9 +55,16 @@
PersistentPrefStore::PrefReadError TestingPrefStore::ReadPrefs() {
prefs_.Clear();
+ NotifyInitializationCompleted();
return PersistentPrefStore::PREF_READ_ERROR_NONE;
}
+void TestingPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate_raw) {
+ scoped_ptr<ReadErrorDelegate> error_delegate(error_delegate_raw);
+ prefs_.Clear();
+ NotifyInitializationCompleted();
+}
+
bool TestingPrefStore::WritePrefs() {
prefs_written_ = true;
return prefs_written_;
@@ -73,7 +80,7 @@
}
void TestingPrefStore::NotifyInitializationCompleted() {
- FOR_EACH_OBSERVER(Observer, observers_, OnInitializationCompleted());
+ FOR_EACH_OBSERVER(Observer, observers_, OnInitializationCompleted(true));
}
void TestingPrefStore::ReportValueChanged(const std::string& key) {
diff --git a/chrome/browser/prefs/testing_pref_store.h b/chrome/browser/prefs/testing_pref_store.h
index 048dcc7..ff6dc00 100644
--- a/chrome/browser/prefs/testing_pref_store.h
+++ b/chrome/browser/prefs/testing_pref_store.h
@@ -37,6 +37,7 @@
virtual void RemoveValue(const std::string& key);
virtual bool ReadOnly() const;
virtual PersistentPrefStore::PrefReadError ReadPrefs();
+ virtual void ReadPrefsAsync(ReadErrorDelegate* error_delegate);
virtual bool WritePrefs();
virtual void ScheduleWritePrefs() {}
virtual void CommitPendingWrite() {}
diff --git a/chrome/browser/prefs/value_map_pref_store.cc b/chrome/browser/prefs/value_map_pref_store.cc
index 4d2c8ca5..39ee3a1 100644
--- a/chrome/browser/prefs/value_map_pref_store.cc
+++ b/chrome/browser/prefs/value_map_pref_store.cc
@@ -37,7 +37,7 @@
}
void ValueMapPrefStore::NotifyInitializationCompleted() {
- FOR_EACH_OBSERVER(Observer, observers_, OnInitializationCompleted());
+ FOR_EACH_OBSERVER(Observer, observers_, OnInitializationCompleted(true));
}
ValueMapPrefStore::iterator ValueMapPrefStore::begin() {
