From 4bf4ff56f7b100350e1d3821650f0064525a6c01 Mon Sep 17 00:00:00 2001 From: cobalt-github-releaser-bot <95661244+cobalt-github-releaser-bot@users.noreply.github.com> Date: Wed, 19 Jul 2023 15:20:23 -0700 Subject: [PATCH] Simplify PersistentSetting initialization. (#935) (#980) Simplify PersistentSetting initialization by waiting for pref store. b/280430510 b/283529011 Change-Id: Ib2c233a0fdad2415ee6c326da6aa198811905a1e (cherry picked from commit 390c8c23c972d8254a8c2ccaa4970adaddde2c65) Co-authored-by: Brian Ting --- .../persistent_storage/persistent_settings.cc | 69 ++++++++----------- .../persistent_storage/persistent_settings.h | 23 ++----- 2 files changed, 34 insertions(+), 58 deletions(-) diff --git a/cobalt/persistent_storage/persistent_settings.cc b/cobalt/persistent_storage/persistent_settings.cc index 881e5446352d..52bc86432414 100644 --- a/cobalt/persistent_storage/persistent_settings.cc +++ b/cobalt/persistent_storage/persistent_settings.cc @@ -19,7 +19,6 @@ #include "base/bind.h" #include "components/prefs/json_pref_store.h" -#include "components/prefs/json_read_only_pref_store.h" #include "starboard/common/file.h" #include "starboard/common/log.h" #include "starboard/configuration_constants.h" @@ -51,37 +50,23 @@ PersistentSettings::PersistentSettings(const std::string& file_name) std::string(storage_dir.data()) + kSbFileSepString + file_name; LOG(INFO) << "Persistent settings file path: " << persistent_settings_file_; - // Initialize pref_store_ with a JSONReadOnlyPrefStore, Used for - // synchronous PersistentSettings::Get calls made before the asynchronous - // InitializeWriteablePrefStore initializes pref_store_ with a writable - // instance. - { - base::AutoLock auto_lock(pref_store_lock_); - pref_store_ = base::MakeRefCounted( - base::FilePath(persistent_settings_file_)); - pref_store_->ReadPrefs(); - } - message_loop()->task_runner()->PostTask( - FROM_HERE, base::Bind(&PersistentSettings::InitializeWriteablePrefStore, + FROM_HERE, base::Bind(&PersistentSettings::InitializePrefStore, base::Unretained(this))); + pref_store_initialized_.Wait(); + destruction_observer_added_.Wait(); } PersistentSettings::~PersistentSettings() { DCHECK(message_loop()); DCHECK(thread_.IsRunning()); - // Ensure that the destruction observer got added and the pref store was - // initialized before stopping the thread. Stop the thread. This will cause - // the destruction observer to be notified. - writeable_pref_store_initialized_.Wait(); - destruction_observer_added_.Wait(); // Wait for all previously posted tasks to finish. thread_.message_loop()->task_runner()->WaitForFence(); thread_.Stop(); } -void PersistentSettings::InitializeWriteablePrefStore() { +void PersistentSettings::InitializePrefStore() { DCHECK_EQ(base::MessageLoop::current(), message_loop()); // Register as a destruction observer to shut down the thread once all // pending tasks have been executed and the message loop is about to be @@ -94,9 +79,7 @@ void PersistentSettings::InitializeWriteablePrefStore() { pref_store_ = base::MakeRefCounted( base::FilePath(persistent_settings_file_)); pref_store_->ReadPrefs(); - // PersistentSettings Set and Remove Helper methods will now be able to - // access the pref_store_ initialized from the dedicated thread_. - writeable_pref_store_initialized_.Signal(); + pref_store_initialized_.Signal(); } validated_initial_settings_ = GetPersistentSettingAsBool(kValidated, false); if (!validated_initial_settings_) { @@ -116,10 +99,9 @@ void PersistentSettings::ValidatePersistentSettingsHelper() { DCHECK_EQ(base::MessageLoop::current(), message_loop()); if (!validated_initial_settings_) { base::AutoLock auto_lock(pref_store_lock_); - writeable_pref_store()->SetValue( - kValidated, std::make_unique(true), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - writeable_pref_store()->CommitPendingWrite(); + pref_store_->SetValue(kValidated, std::make_unique(true), + WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); + CommitPendingWrite(false); validated_initial_settings_ = true; } } @@ -207,12 +189,11 @@ void PersistentSettings::SetPersistentSettingHelper( DCHECK_EQ(base::MessageLoop::current(), message_loop()); if (validated_initial_settings_) { base::AutoLock auto_lock(pref_store_lock_); - writeable_pref_store()->SetValue( - kValidated, std::make_unique(false), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - writeable_pref_store()->SetValue( - key, std::move(value), WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - commit_pending_write(blocking); + pref_store_->SetValue(kValidated, std::make_unique(false), + WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); + pref_store_->SetValue(key, std::move(value), + WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); + CommitPendingWrite(blocking); } else { LOG(ERROR) << "Cannot set persistent setting while unvalidated: " << key; } @@ -234,12 +215,10 @@ void PersistentSettings::RemovePersistentSettingHelper( DCHECK_EQ(base::MessageLoop::current(), message_loop()); if (validated_initial_settings_) { base::AutoLock auto_lock(pref_store_lock_); - writeable_pref_store()->SetValue( - kValidated, std::make_unique(false), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - writeable_pref_store()->RemoveValue( - key, WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - commit_pending_write(blocking); + pref_store_->SetValue(kValidated, std::make_unique(false), + WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); + pref_store_->RemoveValue(key, WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); + CommitPendingWrite(blocking); } else { LOG(ERROR) << "Cannot remove persistent setting while unvalidated: " << key; } @@ -259,12 +238,24 @@ void PersistentSettings::DeletePersistentSettingsHelper( if (validated_initial_settings_) { starboard::SbFileDeleteRecursive(persistent_settings_file_.c_str(), true); base::AutoLock auto_lock(pref_store_lock_); - writeable_pref_store()->ReadPrefs(); + pref_store_->ReadPrefs(); } else { LOG(ERROR) << "Cannot delete persistent setting while unvalidated."; } std::move(closure).Run(); } +void PersistentSettings::CommitPendingWrite(bool blocking) { + if (blocking) { + base::WaitableEvent written; + pref_store_->CommitPendingWrite( + base::OnceClosure(), + base::BindOnce(&base::WaitableEvent::Signal, Unretained(&written))); + written.Wait(); + } else { + pref_store_->CommitPendingWrite(); + } +} + } // namespace persistent_storage } // namespace cobalt diff --git a/cobalt/persistent_storage/persistent_settings.h b/cobalt/persistent_storage/persistent_settings.h index ff3ea58439bc..6c2c15703442 100644 --- a/cobalt/persistent_storage/persistent_settings.h +++ b/cobalt/persistent_storage/persistent_settings.h @@ -84,8 +84,8 @@ class PersistentSettings : public base::MessageLoop::DestructionObserver { private: // Called by the constructor to initialize pref_store_ from - // the dedicated thread_ as a writeable JSONPrefStore. - void InitializeWriteablePrefStore(); + // the dedicated thread_ as a JSONPrefStore. + void InitializePrefStore(); void ValidatePersistentSettingsHelper(); @@ -98,22 +98,7 @@ class PersistentSettings : public base::MessageLoop::DestructionObserver { void DeletePersistentSettingsHelper(base::OnceClosure closure); - scoped_refptr writeable_pref_store() { - writeable_pref_store_initialized_.Wait(); - return pref_store_; - } - - void commit_pending_write(bool blocking) { - if (blocking) { - base::WaitableEvent written; - writeable_pref_store()->CommitPendingWrite( - base::OnceClosure(), - base::BindOnce(&base::WaitableEvent::Signal, Unretained(&written))); - written.Wait(); - } else { - writeable_pref_store()->CommitPendingWrite(); - } - } + void CommitPendingWrite(bool blocking); // Persistent settings file path. std::string persistent_settings_file_; @@ -131,7 +116,7 @@ class PersistentSettings : public base::MessageLoop::DestructionObserver { // This event is used to signal when Initialize has been called and // pref_store_ mutations can now occur. - base::WaitableEvent writeable_pref_store_initialized_ = { + base::WaitableEvent pref_store_initialized_ = { base::WaitableEvent::ResetPolicy::MANUAL, base::WaitableEvent::InitialState::NOT_SIGNALED};