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/common/json_pref_store.cc b/chrome/common/json_pref_store.cc
index f8b054f..f391225 100644
--- a/chrome/common/json_pref_store.cc
+++ b/chrome/common/json_pref_store.cc
@@ -10,8 +10,8 @@
#include "base/callback.h"
#include "base/file_util.h"
#include "base/memory/ref_counted.h"
+#include "base/message_loop_proxy.h"
#include "base/values.h"
-#include "content/browser/browser_thread.h"
#include "content/common/json_value_serializer.h"
namespace {
@@ -19,47 +19,54 @@
// Some extensions we'll tack on to copies of the Preferences files.
const FilePath::CharType* kBadExtension = FILE_PATH_LITERAL("bad");
-// Differentiates file loading between UI and FILE threads.
+// Differentiates file loading between origin thread and passed
+// (aka file) thread.
class FileThreadDeserializer
: public base::RefCountedThreadSafe<FileThreadDeserializer> {
public:
- explicit FileThreadDeserializer(JsonPrefStore* delegate)
- : delegate_(delegate) {
+ explicit FileThreadDeserializer(JsonPrefStore* delegate,
+ base::MessageLoopProxy* file_loop_proxy)
+ : delegate_(delegate),
+ file_loop_proxy_(file_loop_proxy),
+ origin_loop_proxy_(base::MessageLoopProxy::CreateForCurrentThread()) {
}
void Start(const FilePath& path) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
- BrowserThread::PostTask(
- BrowserThread::FILE,
+ DCHECK(origin_loop_proxy_->BelongsToCurrentThread());
+ file_loop_proxy_->PostTask(
FROM_HERE,
NewRunnableMethod(this,
&FileThreadDeserializer::ReadFileAndReport,
path));
}
- // Deserializes JSON on the FILE thread.
+ // Deserializes JSON on the file thread.
void ReadFileAndReport(const FilePath& path) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+ DCHECK(file_loop_proxy_->BelongsToCurrentThread());
+ value_.reset(DoReading(path, &error_, &no_dir_));
+
+ origin_loop_proxy_->PostTask(
+ FROM_HERE,
+ NewRunnableMethod(this, &FileThreadDeserializer::ReportOnOriginThread));
+ }
+
+ // Reports deserialization result on the origin thread.
+ void ReportOnOriginThread() {
+ DCHECK(origin_loop_proxy_->BelongsToCurrentThread());
+ 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_.reset(serializer.Deserialize(&error_code, &error_msg));
-
- HandleErrors(value_.get(), path, error_code, error_msg, &error_);
-
- no_dir_ = !file_util::PathExists(path.DirName());
-
- 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_);
+ 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,
@@ -75,6 +82,8 @@
PersistentPrefStore::PrefReadError error_;
scoped_ptr<Value> value_;
scoped_refptr<JsonPrefStore> delegate_;
+ scoped_refptr<base::MessageLoopProxy> file_loop_proxy_;
+ scoped_refptr<base::MessageLoopProxy> origin_loop_proxy_;
};
// static
@@ -128,9 +137,12 @@
JsonPrefStore::JsonPrefStore(const FilePath& filename,
base::MessageLoopProxy* file_message_loop_proxy)
: path_(filename),
+ file_message_loop_proxy_(file_message_loop_proxy),
prefs_(new DictionaryValue()),
read_only_(false),
- writer_(filename, file_message_loop_proxy) {
+ writer_(filename, file_message_loop_proxy),
+ error_delegate_(NULL),
+ initialized_(false) {
}
JsonPrefStore::~JsonPrefStore() {
@@ -155,6 +167,10 @@
observers_.RemoveObserver(observer);
}
+bool JsonPrefStore::IsInitializationComplete() const {
+ return initialized_;
+}
+
PrefStore::ReadResult JsonPrefStore::GetMutableValue(const std::string& key,
Value** result) {
return prefs_->Get(key, result) ? READ_OK : READ_NO_VALUE;
@@ -194,11 +210,21 @@
PersistentPrefStore::PrefReadError error,
bool no_dir) {
scoped_ptr<Value> value(value_owned);
+ initialized_ = true;
+
+ if (no_dir) {
+ FOR_EACH_OBSERVER(PrefStore::Observer,
+ observers_,
+ OnInitializationCompleted(false));
+ return;
+ }
+
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,53 +242,39 @@
NOTREACHED() << "Unknown error: " << error;
}
- if (delegate_)
- delegate_->OnPrefsRead(error, no_dir);
+ if (error_delegate_.get() && error != PREF_READ_ERROR_NONE)
+ error_delegate_->OnError(error);
+
+ FOR_EACH_OBSERVER(PrefStore::Observer,
+ observers_,
+ OnInitializationCompleted(true));
}
-void JsonPrefStore::ReadPrefs(Delegate* delegate) {
- DCHECK(delegate);
- delegate_ = delegate;
-
+void JsonPrefStore::ReadPrefsAsync(ReadErrorDelegate *error_delegate) {
+ initialized_ = false;
+ error_delegate_.reset(error_delegate);
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));
+ new FileThreadDeserializer(this, file_message_loop_proxy_.get()));
deserializer->Start(path_);
}
PersistentPrefStore::PrefReadError JsonPrefStore::ReadPrefs() {
- delegate_ = NULL;
-
if (path_.empty()) {
- read_only_ = true;
+ OnFileRead(NULL, PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
return PREF_READ_ERROR_FILE_NOT_SPECIFIED;
}
- 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);
-
+ PrefReadError error;
+ bool no_dir;
+ Value* value = FileThreadDeserializer::DoReading(path_, &error, &no_dir);
+ OnFileRead(value, error, no_dir);
return error;
}
diff --git a/chrome/common/json_pref_store.h b/chrome/common/json_pref_store.h
index d8da37d0..8291a02 100644
--- a/chrome/common/json_pref_store.h
+++ b/chrome/common/json_pref_store.h
@@ -27,11 +27,6 @@
class JsonPrefStore : public PersistentPrefStore,
public ImportantFileWriter::DataSerializer {
public:
- class Delegate {
- public:
- virtual void OnPrefsRead(PrefReadError error, bool no_dir) = 0;
- };
-
// |file_message_loop_proxy| is the MessageLoopProxy for a thread on which
// file I/O can be done.
JsonPrefStore(const FilePath& pref_filename,
@@ -43,6 +38,7 @@
const Value** result) const;
virtual void AddObserver(PrefStore::Observer* observer);
virtual void RemoveObserver(PrefStore::Observer* observer);
+ virtual bool IsInitializationComplete() const;
// PersistentPrefStore overrides:
virtual ReadResult GetMutableValue(const std::string& key, Value** result);
@@ -51,15 +47,16 @@
virtual void RemoveValue(const std::string& key);
virtual bool ReadOnly() const;
virtual PrefReadError ReadPrefs();
- // todo(altimofeev): move it to the PersistentPrefStore inteface.
- void ReadPrefs(Delegate* delegate);
+ virtual void ReadPrefsAsync(ReadErrorDelegate* error_delegate);
virtual bool WritePrefs();
virtual void ScheduleWritePrefs();
virtual void CommitPendingWrite();
virtual void ReportValueChanged(const std::string& key);
// This method is called after JSON file has been read. Method takes
- // ownership of the |value| pointer.
+ // ownership of the |value| pointer. Note, this method is used with
+ // asynchronous file reading, so class exposes it only for the internal needs.
+ // (read: do not call it manually).
void OnFileRead(Value* value_owned, PrefReadError error, bool no_dir);
private:
@@ -67,6 +64,7 @@
virtual bool SerializeData(std::string* output);
FilePath path_;
+ scoped_refptr<base::MessageLoopProxy> file_message_loop_proxy_;
scoped_ptr<DictionaryValue> prefs_;
@@ -77,7 +75,9 @@
ObserverList<PrefStore::Observer, true> observers_;
- Delegate* delegate_;
+ scoped_ptr<ReadErrorDelegate> error_delegate_;
+
+ bool initialized_;
DISALLOW_COPY_AND_ASSIGN(JsonPrefStore);
};
diff --git a/chrome/common/json_pref_store_unittest.cc b/chrome/common/json_pref_store_unittest.cc
index e7f34f52..d46f4328 100644
--- a/chrome/common/json_pref_store_unittest.cc
+++ b/chrome/common/json_pref_store_unittest.cc
@@ -5,6 +5,7 @@
#include "base/file_util.h"
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_temp_dir.h"
#include "base/message_loop.h"
#include "base/message_loop_proxy.h"
#include "base/path_service.h"
@@ -13,12 +14,27 @@
#include "base/threading/thread.h"
#include "base/utf_string_conversions.h"
#include "base/values.h"
-#include "base/memory/scoped_temp_dir.h"
-#include "chrome/common/json_pref_store.h"
#include "chrome/common/chrome_paths.h"
+#include "chrome/common/json_pref_store.h"
#include "chrome/common/pref_names.h"
+#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
+namespace {
+
+class MockPrefStoreObserver : public PrefStore::Observer {
+ public:
+ MOCK_METHOD1(OnPrefValueChanged, void (const std::string&));
+ MOCK_METHOD1(OnInitializationCompleted, void (bool));
+};
+
+class MockReadErrorDelegate : public PersistentPrefStore::ReadErrorDelegate {
+ public:
+ MOCK_METHOD1(OnError, void(PersistentPrefStore::PrefReadError));
+};
+
+} // namespace
+
class JsonPrefStoreTest : public testing::Test {
protected:
virtual void SetUp() {
@@ -70,28 +86,11 @@
moved_aside));
}
-TEST_F(JsonPrefStoreTest, Basic) {
- ASSERT_TRUE(file_util::CopyFile(data_dir_.AppendASCII("read.json"),
- temp_dir_.path().AppendASCII("write.json")));
-
- // Test that the persistent value can be loaded.
- FilePath input_file = temp_dir_.path().AppendASCII("write.json");
- ASSERT_TRUE(file_util::PathExists(input_file));
- scoped_refptr<JsonPrefStore> pref_store =
- new JsonPrefStore(input_file, message_loop_proxy_.get());
- ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE, pref_store->ReadPrefs());
- ASSERT_FALSE(pref_store->ReadOnly());
-
- // The JSON file looks like this:
- // {
- // "homepage": "http://www.cnn.com",
- // "some_directory": "/usr/local/",
- // "tabs": {
- // "new_windows_in_tabs": true,
- // "max_tabs": 20
- // }
- // }
-
+// This function is used to avoid code duplication while testing synchronous and
+// asynchronous version of the JsonPrefStore loading.
+void RunBasicJsonPrefStoreTest(JsonPrefStore *pref_store,
+ const FilePath& output_file,
+ const FilePath& golden_output_file) {
const char kNewWindowsInTabs[] = "tabs.new_windows_in_tabs";
const char kMaxTabs[] = "tabs.max_tabs";
const char kLongIntPref[] = "long_int.pref";
@@ -152,11 +151,96 @@
EXPECT_EQ(214748364842LL, value);
// Serialize and compare to expected output.
- FilePath output_file = input_file;
- FilePath golden_output_file = data_dir_.AppendASCII("write.golden.json");
ASSERT_TRUE(file_util::PathExists(golden_output_file));
ASSERT_TRUE(pref_store->WritePrefs());
MessageLoop::current()->RunAllPending();
EXPECT_TRUE(file_util::TextContentsEqual(golden_output_file, output_file));
ASSERT_TRUE(file_util::Delete(output_file, false));
}
+
+TEST_F(JsonPrefStoreTest, Basic) {
+ ASSERT_TRUE(file_util::CopyFile(data_dir_.AppendASCII("read.json"),
+ temp_dir_.path().AppendASCII("write.json")));
+
+ // Test that the persistent value can be loaded.
+ FilePath input_file = temp_dir_.path().AppendASCII("write.json");
+ ASSERT_TRUE(file_util::PathExists(input_file));
+ scoped_refptr<JsonPrefStore> pref_store =
+ new JsonPrefStore(input_file, message_loop_proxy_.get());
+ ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE, pref_store->ReadPrefs());
+ ASSERT_FALSE(pref_store->ReadOnly());
+
+ // The JSON file looks like this:
+ // {
+ // "homepage": "http://www.cnn.com",
+ // "some_directory": "/usr/local/",
+ // "tabs": {
+ // "new_windows_in_tabs": true,
+ // "max_tabs": 20
+ // }
+ // }
+
+ RunBasicJsonPrefStoreTest(pref_store,
+ input_file,
+ data_dir_.AppendASCII("write.golden.json"));
+}
+
+TEST_F(JsonPrefStoreTest, BasicAsync) {
+ ASSERT_TRUE(file_util::CopyFile(data_dir_.AppendASCII("read.json"),
+ temp_dir_.path().AppendASCII("write.json")));
+
+ // Test that the persistent value can be loaded.
+ FilePath input_file = temp_dir_.path().AppendASCII("write.json");
+ ASSERT_TRUE(file_util::PathExists(input_file));
+ scoped_refptr<JsonPrefStore> pref_store =
+ new JsonPrefStore(input_file, message_loop_proxy_.get());
+
+ MockPrefStoreObserver mock_observer;
+ pref_store->AddObserver(&mock_observer);
+
+ MockReadErrorDelegate *mock_error_delegate = new MockReadErrorDelegate;
+ pref_store->ReadPrefsAsync(mock_error_delegate);
+
+ EXPECT_CALL(mock_observer, OnInitializationCompleted(true)).Times(1);
+ EXPECT_CALL(*mock_error_delegate,
+ OnError(PersistentPrefStore::PREF_READ_ERROR_NONE)).Times(0);
+ message_loop_.RunAllPending();
+ pref_store->RemoveObserver(&mock_observer);
+
+ ASSERT_FALSE(pref_store->ReadOnly());
+
+ // The JSON file looks like this:
+ // {
+ // "homepage": "http://www.cnn.com",
+ // "some_directory": "/usr/local/",
+ // "tabs": {
+ // "new_windows_in_tabs": true,
+ // "max_tabs": 20
+ // }
+ // }
+
+ RunBasicJsonPrefStoreTest(pref_store,
+ input_file,
+ data_dir_.AppendASCII("write.golden.json"));
+}
+
+// Tests asynchronous reading of the file when there is no file.
+TEST_F(JsonPrefStoreTest, AsyncNonExistingFile) {
+ FilePath bogus_input_file = data_dir_.AppendASCII("read.txt");
+ ASSERT_FALSE(file_util::PathExists(bogus_input_file));
+ scoped_refptr<JsonPrefStore> pref_store =
+ new JsonPrefStore(bogus_input_file, message_loop_proxy_.get());
+ MockPrefStoreObserver mock_observer;
+ pref_store->AddObserver(&mock_observer);
+
+ MockReadErrorDelegate *mock_error_delegate = new MockReadErrorDelegate;
+ pref_store->ReadPrefsAsync(mock_error_delegate);
+
+ EXPECT_CALL(mock_observer, OnInitializationCompleted(true)).Times(1);
+ EXPECT_CALL(*mock_error_delegate,
+ OnError(PersistentPrefStore::PREF_READ_ERROR_NO_FILE)).Times(1);
+ message_loop_.RunAllPending();
+ pref_store->RemoveObserver(&mock_observer);
+
+ EXPECT_FALSE(pref_store->ReadOnly());
+}
diff --git a/chrome/common/persistent_pref_store.h b/chrome/common/persistent_pref_store.h
index 620fee8..0f007c5a 100644
--- a/chrome/common/persistent_pref_store.h
+++ b/chrome/common/persistent_pref_store.h
@@ -34,6 +34,13 @@
PREF_READ_ERROR_FILE_NOT_SPECIFIED
};
+ class ReadErrorDelegate {
+ public:
+ virtual ~ReadErrorDelegate() {}
+
+ virtual void OnError(PrefReadError error) = 0;
+ };
+
// Equivalent to PrefStore::GetValue but returns a mutable value.
virtual ReadResult GetMutableValue(const std::string& key,
Value** result) = 0;
@@ -62,9 +69,16 @@
// read errors during startup.
virtual bool ReadOnly() const = 0;
- // Reads the preferences from disk.
+ // Reads the preferences from disk. Notifies observers via
+ // "PrefStore::OnInitializationCompleted" when done.
virtual PrefReadError ReadPrefs() = 0;
+ // Reads the preferences from disk asynchronously. Notifies observers via
+ // "PrefStore::OnInitializationCompleted" when done. Also it fires
+ // |error_delegate| if it is not NULL and reading error has occurred.
+ // Owns |error_delegate|.
+ virtual void ReadPrefsAsync(ReadErrorDelegate* error_delegate) = 0;
+
// Writes the preferences to disk immediately.
virtual bool WritePrefs() = 0;
diff --git a/chrome/common/pref_store.h b/chrome/common/pref_store.h
index 48b51a3..eec74e9 100644
--- a/chrome/common/pref_store.h
+++ b/chrome/common/pref_store.h
@@ -30,7 +30,7 @@
// Called when the value for the given |key| in the store changes.
virtual void OnPrefValueChanged(const std::string& key) = 0;
// Notification about the PrefStore being fully initialized.
- virtual void OnInitializationCompleted() = 0;
+ virtual void OnInitializationCompleted(bool succeeded) = 0;
};
// Return values for GetValue().
diff --git a/chrome/common/pref_store_observer_mock.h b/chrome/common/pref_store_observer_mock.h
index 3aadf342..0218d339 100644
--- a/chrome/common/pref_store_observer_mock.h
+++ b/chrome/common/pref_store_observer_mock.h
@@ -17,7 +17,7 @@
virtual ~PrefStoreObserverMock();
MOCK_METHOD1(OnPrefValueChanged, void(const std::string&));
- MOCK_METHOD0(OnInitializationCompleted, void());
+ MOCK_METHOD1(OnInitializationCompleted, void(bool));
private:
DISALLOW_COPY_AND_ASSIGN(PrefStoreObserverMock);
