From cfcf0e53fa4ee2f4b89b477390f1906db0cbf4d9 Mon Sep 17 00:00:00 2001 From: "gab@chromium.org" Date: Fri, 20 Jun 2014 18:29:47 +0000 Subject: Expand the JsonPrefStore's interface to allow for an alternate filename to be specified. The alternate filename is intended to be the old name of the pref file being read, JsonPrefStore handles the migration if required. JsonPrefStore's FileThreadDeserializer is the best place to do this as it's the only code in the prefs stack running on a thread allowed to do IO (another solution would be to let JsonPrefStore take a PreReadOnFileThreadCallback to abstract the required work away, but the direct approach seemed easier here without breaking encapsulation). BUG=372547 Review URL: https://codereview.chromium.org/347793002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278776 0039d316-1c4b-4281-b951-d872f2087c98 --- base/prefs/json_pref_store.cc | 37 +++++- base/prefs/json_pref_store.h | 18 ++- base/prefs/json_pref_store_unittest.cc | 204 ++++++++++++++++++++++++++++++++- 3 files changed, 249 insertions(+), 10 deletions(-) (limited to 'base/prefs') diff --git a/base/prefs/json_pref_store.cc b/base/prefs/json_pref_store.cc index a6c2362..9180984 100644 --- a/base/prefs/json_pref_store.cc +++ b/base/prefs/json_pref_store.cc @@ -37,21 +37,23 @@ class FileThreadDeserializer origin_loop_proxy_(base::MessageLoopProxy::current()) { } - void Start(const base::FilePath& path) { + void Start(const base::FilePath& path, + const base::FilePath& alternate_path) { DCHECK(origin_loop_proxy_->BelongsToCurrentThread()); // TODO(gab): This should use PostTaskAndReplyWithResult instead of using // the |error_| member to pass data across tasks. sequenced_task_runner_->PostTask( FROM_HERE, base::Bind(&FileThreadDeserializer::ReadFileAndReport, - this, path)); + this, path, alternate_path)); } // Deserializes JSON on the sequenced task runner. - void ReadFileAndReport(const base::FilePath& path) { + void ReadFileAndReport(const base::FilePath& path, + const base::FilePath& alternate_path) { DCHECK(sequenced_task_runner_->RunsTasksOnCurrentThread()); - value_.reset(DoReading(path, &error_, &no_dir_)); + value_.reset(DoReading(path, alternate_path, &error_, &no_dir_)); origin_loop_proxy_->PostTask( FROM_HERE, @@ -65,8 +67,14 @@ class FileThreadDeserializer } static base::Value* DoReading(const base::FilePath& path, + const base::FilePath& alternate_path, PersistentPrefStore::PrefReadError* error, bool* no_dir) { + if (!base::PathExists(path) && !alternate_path.empty() && + base::PathExists(alternate_path)) { + base::Move(alternate_path, path); + } + int error_code; std::string error_msg; JSONFileValueSerializer serializer(path); @@ -167,6 +175,22 @@ JsonPrefStore::JsonPrefStore(const base::FilePath& filename, read_error_(PREF_READ_ERROR_NONE) { } +JsonPrefStore::JsonPrefStore(const base::FilePath& filename, + const base::FilePath& alternate_filename, + base::SequencedTaskRunner* sequenced_task_runner, + scoped_ptr pref_filter) + : path_(filename), + alternate_path_(alternate_filename), + sequenced_task_runner_(sequenced_task_runner), + prefs_(new base::DictionaryValue()), + read_only_(false), + writer_(filename, sequenced_task_runner), + pref_filter_(pref_filter.Pass()), + initialized_(false), + filtering_in_progress_(false), + read_error_(PREF_READ_ERROR_NONE) { +} + bool JsonPrefStore::GetValue(const std::string& key, const base::Value** result) const { base::Value* tmp = NULL; @@ -252,7 +276,8 @@ PersistentPrefStore::PrefReadError JsonPrefStore::ReadPrefs() { PrefReadError error; bool no_dir; scoped_ptr value( - FileThreadDeserializer::DoReading(path_, &error, &no_dir)); + FileThreadDeserializer::DoReading(path_, alternate_path_, &error, + &no_dir)); OnFileRead(value.Pass(), error, no_dir); return filtering_in_progress_ ? PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE : error; @@ -271,7 +296,7 @@ void JsonPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) { // in the end. scoped_refptr deserializer( new FileThreadDeserializer(this, sequenced_task_runner_.get())); - deserializer->Start(path_); + deserializer->Start(path_, alternate_path_); } void JsonPrefStore::CommitPendingWrite() { diff --git a/base/prefs/json_pref_store.h b/base/prefs/json_pref_store.h index 49e74ee..6ceea68 100644 --- a/base/prefs/json_pref_store.h +++ b/base/prefs/json_pref_store.h @@ -43,12 +43,23 @@ class BASE_PREFS_EXPORT JsonPrefStore const base::FilePath& pref_filename, base::SequencedWorkerPool* worker_pool); - // |sequenced_task_runner| is must be a shutdown-blocking task runner, ideally - // created by GetTaskRunnerForFile() method above. + // Same as the constructor below with no alternate filename. JsonPrefStore(const base::FilePath& pref_filename, base::SequencedTaskRunner* sequenced_task_runner, scoped_ptr pref_filter); + // |sequenced_task_runner| must be a shutdown-blocking task runner, ideally + // created by the GetTaskRunnerForFile() method above. + // |pref_filename| is the path to the file to read prefs from. + // |pref_alternate_filename| is the path to an alternate file which the + // desired prefs may have previously been written to. If |pref_filename| + // doesn't exist and |pref_alternate_filename| does, |pref_alternate_filename| + // will be moved to |pref_filename| before the read occurs. + JsonPrefStore(const base::FilePath& pref_filename, + const base::FilePath& pref_alternate_filename, + base::SequencedTaskRunner* sequenced_task_runner, + scoped_ptr pref_filter); + // PrefStore overrides: virtual bool GetValue(const std::string& key, const base::Value** result) const OVERRIDE; @@ -115,7 +126,8 @@ class BASE_PREFS_EXPORT JsonPrefStore scoped_ptr prefs, bool schedule_write); - base::FilePath path_; + const base::FilePath path_; + const base::FilePath alternate_path_; const scoped_refptr sequenced_task_runner_; scoped_ptr prefs_; diff --git a/base/prefs/json_pref_store_unittest.cc b/base/prefs/json_pref_store_unittest.cc index 2b4a9e8..441c229 100644 --- a/base/prefs/json_pref_store_unittest.cc +++ b/base/prefs/json_pref_store_unittest.cc @@ -44,7 +44,7 @@ class InterceptingPrefFilter : public PrefFilter { bool has_intercepted_prefs() const { return intercepted_prefs_ != NULL; } - // Finalize an intercepted read, handing |intercept_prefs_| back to its + // Finalize an intercepted read, handing |intercepted_prefs_| back to its // JsonPrefStore. void ReleasePrefs(); @@ -122,6 +122,23 @@ TEST_F(JsonPrefStoreTest, NonExistentFile) { EXPECT_FALSE(pref_store->ReadOnly()); } +// Test fallback behavior for a nonexistent file and alternate file. +TEST_F(JsonPrefStoreTest, NonExistentFileAndAlternateFile) { + base::FilePath bogus_input_file = data_dir_.AppendASCII("read.txt"); + base::FilePath bogus_alternate_input_file = + data_dir_.AppendASCII("read_alternate.txt"); + ASSERT_FALSE(PathExists(bogus_input_file)); + ASSERT_FALSE(PathExists(bogus_alternate_input_file)); + scoped_refptr pref_store = new JsonPrefStore( + bogus_input_file, + bogus_alternate_input_file, + message_loop_.message_loop_proxy().get(), + scoped_ptr()); + EXPECT_EQ(PersistentPrefStore::PREF_READ_ERROR_NO_FILE, + pref_store->ReadPrefs()); + EXPECT_FALSE(pref_store->ReadOnly()); +} + // Test fallback behavior for an invalid file. TEST_F(JsonPrefStoreTest, InvalidFile) { base::FilePath invalid_file_original = data_dir_.AppendASCII("invalid.json"); @@ -469,4 +486,189 @@ TEST_F(JsonPrefStoreTest, ReadAsyncWithInterceptor) { pref_store.get(), input_file, data_dir_.AppendASCII("write.golden.json")); } +TEST_F(JsonPrefStoreTest, AlternateFile) { + ASSERT_TRUE( + base::CopyFile(data_dir_.AppendASCII("read.json"), + temp_dir_.path().AppendASCII("alternate.json"))); + + // Test that the alternate file is moved to the main file and read as-is from + // there. + base::FilePath input_file = temp_dir_.path().AppendASCII("write.json"); + base::FilePath alternate_input_file = + temp_dir_.path().AppendASCII("alternate.json"); + ASSERT_FALSE(PathExists(input_file)); + ASSERT_TRUE(PathExists(alternate_input_file)); + scoped_refptr pref_store = new JsonPrefStore( + input_file, + alternate_input_file, + message_loop_.message_loop_proxy().get(), + scoped_ptr()); + + ASSERT_FALSE(PathExists(input_file)); + ASSERT_TRUE(PathExists(alternate_input_file)); + ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE, pref_store->ReadPrefs()); + + ASSERT_TRUE(PathExists(input_file)); + ASSERT_FALSE(PathExists(alternate_input_file)); + + EXPECT_FALSE(pref_store->ReadOnly()); + EXPECT_TRUE(pref_store->IsInitializationComplete()); + + // 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.get(), input_file, data_dir_.AppendASCII("write.golden.json")); +} + +TEST_F(JsonPrefStoreTest, AlternateFileIgnoredWhenMainFileExists) { + ASSERT_TRUE( + base::CopyFile(data_dir_.AppendASCII("read.json"), + temp_dir_.path().AppendASCII("write.json"))); + ASSERT_TRUE( + base::CopyFile(data_dir_.AppendASCII("invalid.json"), + temp_dir_.path().AppendASCII("alternate.json"))); + + // Test that the alternate file is ignored and that the read occurs from the + // existing main file. There is no attempt at even deleting the alternate + // file as this scenario should never happen in normal user-data-dirs. + base::FilePath input_file = temp_dir_.path().AppendASCII("write.json"); + base::FilePath alternate_input_file = + temp_dir_.path().AppendASCII("alternate.json"); + ASSERT_TRUE(PathExists(input_file)); + ASSERT_TRUE(PathExists(alternate_input_file)); + scoped_refptr pref_store = new JsonPrefStore( + input_file, + alternate_input_file, + message_loop_.message_loop_proxy().get(), + scoped_ptr()); + + ASSERT_TRUE(PathExists(input_file)); + ASSERT_TRUE(PathExists(alternate_input_file)); + ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE, pref_store->ReadPrefs()); + + ASSERT_TRUE(PathExists(input_file)); + ASSERT_TRUE(PathExists(alternate_input_file)); + + EXPECT_FALSE(pref_store->ReadOnly()); + EXPECT_TRUE(pref_store->IsInitializationComplete()); + + // 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.get(), input_file, data_dir_.AppendASCII("write.golden.json")); +} + +TEST_F(JsonPrefStoreTest, AlternateFileDNE) { + ASSERT_TRUE( + base::CopyFile(data_dir_.AppendASCII("read.json"), + temp_dir_.path().AppendASCII("write.json"))); + + // Test that the basic read works fine when an alternate file is specified but + // does not exist. + base::FilePath input_file = temp_dir_.path().AppendASCII("write.json"); + base::FilePath alternate_input_file = + temp_dir_.path().AppendASCII("alternate.json"); + ASSERT_TRUE(PathExists(input_file)); + ASSERT_FALSE(PathExists(alternate_input_file)); + scoped_refptr pref_store = new JsonPrefStore( + input_file, + alternate_input_file, + message_loop_.message_loop_proxy().get(), + scoped_ptr()); + + ASSERT_TRUE(PathExists(input_file)); + ASSERT_FALSE(PathExists(alternate_input_file)); + ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE, pref_store->ReadPrefs()); + + ASSERT_TRUE(PathExists(input_file)); + ASSERT_FALSE(PathExists(alternate_input_file)); + + EXPECT_FALSE(pref_store->ReadOnly()); + EXPECT_TRUE(pref_store->IsInitializationComplete()); + + // 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.get(), input_file, data_dir_.AppendASCII("write.golden.json")); +} + +TEST_F(JsonPrefStoreTest, BasicAsyncWithAlternateFile) { + ASSERT_TRUE( + base::CopyFile(data_dir_.AppendASCII("read.json"), + temp_dir_.path().AppendASCII("alternate.json"))); + + // Test that the alternate file is moved to the main file and read as-is from + // there even when the read is made asynchronously. + base::FilePath input_file = temp_dir_.path().AppendASCII("write.json"); + base::FilePath alternate_input_file = + temp_dir_.path().AppendASCII("alternate.json"); + ASSERT_FALSE(PathExists(input_file)); + ASSERT_TRUE(PathExists(alternate_input_file)); + scoped_refptr pref_store = new JsonPrefStore( + input_file, + alternate_input_file, + message_loop_.message_loop_proxy().get(), + scoped_ptr()); + + ASSERT_FALSE(PathExists(input_file)); + ASSERT_TRUE(PathExists(alternate_input_file)); + + { + 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); + RunLoop().RunUntilIdle(); + pref_store->RemoveObserver(&mock_observer); + + EXPECT_FALSE(pref_store->ReadOnly()); + EXPECT_TRUE(pref_store->IsInitializationComplete()); + } + + ASSERT_TRUE(PathExists(input_file)); + ASSERT_FALSE(PathExists(alternate_input_file)); + + // 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.get(), input_file, data_dir_.AppendASCII("write.golden.json")); +} + } // namespace base -- cgit v1.1