From d04323764ae7c11ca9818f473bb076f09e81d0d4 Mon Sep 17 00:00:00 2001 From: Nan Lin Date: Fri, 16 Dec 2022 21:58:24 +0000 Subject: [PATCH] [Code Health] Some replacements of base::DictionaryValue for prefs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: 1187023 Change-Id: I1d6a76c691b62f7c8b97c42213e47f60b11c0aac Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4095228 Reviewed-by: proberge Reviewed-by: Dominic Battré Reviewed-by: Gabriel Charette Commit-Queue: Nan Lin Cr-Commit-Position: refs/heads/main@{#1084540} --- components/prefs/json_pref_store.cc | 42 +++++---- components/prefs/json_pref_store.h | 4 +- components/prefs/json_pref_store_unittest.cc | 18 ++-- components/prefs/pref_filter.h | 13 +-- .../tracked/dictionary_hash_store_contents.cc | 2 +- .../tracked/dictionary_hash_store_contents.h | 6 +- .../tracked/interceptable_pref_filter.cc | 2 +- .../tracked/interceptable_pref_filter.h | 14 ++- .../interceptable_pref_filter_unittest.cc | 10 +-- .../preferences/tracked/pref_hash_filter.cc | 34 ++++--- .../preferences/tracked/pref_hash_filter.h | 6 +- .../tracked/pref_hash_filter_unittest.cc | 89 ++++++++++--------- .../tracked/pref_hash_store_impl.cc | 16 ++-- .../tracked/pref_hash_store_impl_unittest.cc | 42 ++++----- .../tracked/pref_hash_store_transaction.h | 10 +-- .../tracked/tracked_atomic_preference.cc | 12 +-- .../tracked/tracked_atomic_preference.h | 2 +- .../preferences/tracked/tracked_preference.h | 9 +- .../tracked/tracked_preferences_migration.cc | 53 +++++------ .../tracked_preferences_migration_unittest.cc | 58 ++++++------ .../tracked/tracked_split_preference.cc | 28 +++--- .../tracked/tracked_split_preference.h | 2 +- 22 files changed, 229 insertions(+), 243 deletions(-) diff --git a/components/prefs/json_pref_store.cc b/components/prefs/json_pref_store.cc index d9711847973fe..b348d49be23fa 100644 --- a/components/prefs/json_pref_store.cc +++ b/components/prefs/json_pref_store.cc @@ -152,7 +152,7 @@ const char* GetHistogramSuffix(const base::FilePath& path) { return it != kAllowList.end() ? *it : ""; } -bool DoSerialize(const base::Value& value, +bool DoSerialize(base::ValueView value, const base::FilePath& path, std::string* output) { JSONStringValueSerializer serializer(output); @@ -178,7 +178,6 @@ JsonPrefStore::JsonPrefStore( bool read_only) : path_(pref_filename), file_task_runner_(std::move(file_task_runner)), - prefs_(new base::DictionaryValue()), read_only_(read_only), writer_(pref_filename, file_task_runner_, @@ -196,7 +195,7 @@ bool JsonPrefStore::GetValue(base::StringPiece key, const base::Value** result) const { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - base::Value* tmp = prefs_->FindPath(key); + const base::Value* tmp = prefs_.FindByDottedPath(key); if (!tmp) return false; @@ -206,7 +205,7 @@ bool JsonPrefStore::GetValue(base::StringPiece key, } base::Value::Dict JsonPrefStore::GetValues() const { - return prefs_->GetDict().Clone(); + return prefs_.Clone(); } void JsonPrefStore::AddObserver(PrefStore::Observer* observer) { @@ -236,7 +235,7 @@ bool JsonPrefStore::GetMutableValue(const std::string& key, base::Value** result) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - base::Value* tmp = prefs_->FindPath(key); + base::Value* tmp = prefs_.FindByDottedPath(key); if (!tmp) return false; @@ -250,9 +249,9 @@ void JsonPrefStore::SetValue(const std::string& key, uint32_t flags) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - base::Value* old_value = prefs_->FindPath(key); + base::Value* old_value = prefs_.FindByDottedPath(key); if (!old_value || value != *old_value) { - prefs_->SetPath(key, std::move(value)); + prefs_.SetByDottedPath(key, std::move(value)); ReportValueChanged(key, flags); } } @@ -262,9 +261,9 @@ void JsonPrefStore::SetValueSilently(const std::string& key, uint32_t flags) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - base::Value* old_value = prefs_->FindPath(key); + base::Value* old_value = prefs_.FindByDottedPath(key); if (!old_value || value != *old_value) { - prefs_->SetPath(key, std::move(value)); + prefs_.SetByDottedPath(key, std::move(value)); ScheduleWrite(flags); } } @@ -272,15 +271,16 @@ void JsonPrefStore::SetValueSilently(const std::string& key, void JsonPrefStore::RemoveValue(const std::string& key, uint32_t flags) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - if (prefs_->RemovePath(key)) + if (prefs_.RemoveByDottedPath(key)) { ReportValueChanged(key, flags); + } } void JsonPrefStore::RemoveValueSilently(const std::string& key, uint32_t flags) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - prefs_->RemovePath(key); + prefs_.RemoveByDottedPath(key); ScheduleWrite(flags); } @@ -370,8 +370,7 @@ void JsonPrefStore::PerformPreserializationTasks() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); pending_lossy_write_ = false; if (pref_filter_) { - OnWriteCallbackPair callbacks = - pref_filter_->FilterSerializeData(prefs_.get()); + OnWriteCallbackPair callbacks = pref_filter_->FilterSerializeData(prefs_); if (!callbacks.first.is_null() || !callbacks.second.is_null()) RegisterOnNextWriteSynchronousCallbacks(std::move(callbacks)); } @@ -457,7 +456,7 @@ void JsonPrefStore::OnFileRead(std::unique_ptr read_result) { DCHECK(read_result); - auto unfiltered_prefs = std::make_unique(); + base::Value::Dict unfiltered_prefs; read_error_ = read_result->error; @@ -474,9 +473,9 @@ void JsonPrefStore::OnFileRead(std::unique_ptr read_result) { break; case PREF_READ_ERROR_NONE: DCHECK(read_result->value); + DCHECK(read_result->value->is_dict()); writer_.set_previous_data_size(read_result->num_bytes_read); - unfiltered_prefs.reset( - static_cast(read_result->value.release())); + unfiltered_prefs = std::move(*read_result->value).TakeDict(); break; case PREF_READ_ERROR_NO_FILE: // If the file just doesn't exist, maybe this is first run. In any case @@ -514,19 +513,18 @@ JsonPrefStore::~JsonPrefStore() { bool JsonPrefStore::SerializeData(std::string* output) { PerformPreserializationTasks(); - return DoSerialize(*prefs_, path_, output); + return DoSerialize(prefs_, path_, output); } base::ImportantFileWriter::BackgroundDataProducerCallback JsonPrefStore::GetSerializedDataProducerForBackgroundSequence() { PerformPreserializationTasks(); - return base::BindOnce(&DoSerialize, prefs_->Clone(), path_); + return base::BindOnce(&DoSerialize, prefs_.Clone(), path_); } -void JsonPrefStore::FinalizeFileRead( - bool initialization_successful, - std::unique_ptr prefs, - bool schedule_write) { +void JsonPrefStore::FinalizeFileRead(bool initialization_successful, + base::Value::Dict prefs, + bool schedule_write) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); filtering_in_progress_ = false; diff --git a/components/prefs/json_pref_store.h b/components/prefs/json_pref_store.h index d09dbbbae763d..b14ccca1dde56 100644 --- a/components/prefs/json_pref_store.h +++ b/components/prefs/json_pref_store.h @@ -178,7 +178,7 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore // (typically because the |pref_filter_| has already altered the |prefs|) -- // this will be ignored if this store is read-only. void FinalizeFileRead(bool initialization_successful, - std::unique_ptr prefs, + base::Value::Dict prefs, bool schedule_write); // Schedule a write with the file writer as long as |flags| doesn't contain @@ -188,7 +188,7 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore const base::FilePath path_; const scoped_refptr file_task_runner_; - std::unique_ptr prefs_; + base::Value::Dict prefs_; bool read_only_; diff --git a/components/prefs/json_pref_store_unittest.cc b/components/prefs/json_pref_store_unittest.cc index 4dd090a4323f1..b3984f5096c0d 100644 --- a/components/prefs/json_pref_store_unittest.cc +++ b/components/prefs/json_pref_store_unittest.cc @@ -73,12 +73,11 @@ class InterceptingPrefFilter : public PrefFilter { ~InterceptingPrefFilter() override; // PrefFilter implementation: - void FilterOnLoad( - PostFilterOnLoadCallback post_filter_on_load_callback, - std::unique_ptr pref_store_contents) override; + void FilterOnLoad(PostFilterOnLoadCallback post_filter_on_load_callback, + base::Value::Dict pref_store_contents) override; void FilterUpdate(const std::string& path) override {} OnWriteCallbackPair FilterSerializeData( - base::DictionaryValue* pref_store_contents) override { + base::Value::Dict& pref_store_contents) override { return std::move(on_write_callback_pair_); } void OnStoreDeletionFromDisk() override {} @@ -91,7 +90,7 @@ class InterceptingPrefFilter : public PrefFilter { private: PostFilterOnLoadCallback post_filter_on_load_callback_; - std::unique_ptr intercepted_prefs_; + std::unique_ptr intercepted_prefs_; OnWriteCallbackPair on_write_callback_pair_; }; @@ -106,15 +105,16 @@ InterceptingPrefFilter::~InterceptingPrefFilter() {} void InterceptingPrefFilter::FilterOnLoad( PostFilterOnLoadCallback post_filter_on_load_callback, - std::unique_ptr pref_store_contents) { + base::Value::Dict pref_store_contents) { post_filter_on_load_callback_ = std::move(post_filter_on_load_callback); - intercepted_prefs_ = std::move(pref_store_contents); + intercepted_prefs_ = + std::make_unique(std::move(pref_store_contents)); } void InterceptingPrefFilter::ReleasePrefs() { EXPECT_FALSE(post_filter_on_load_callback_.is_null()); - std::move(post_filter_on_load_callback_) - .Run(std::move(intercepted_prefs_), false); + std::unique_ptr prefs = std::move(intercepted_prefs_); + std::move(post_filter_on_load_callback_).Run(std::move(*prefs), false); } class MockPrefStoreObserver : public PrefStore::Observer { diff --git a/components/prefs/pref_filter.h b/components/prefs/pref_filter.h index 91003cd3226a0..a6c48b5ffe6ae 100644 --- a/components/prefs/pref_filter.h +++ b/components/prefs/pref_filter.h @@ -5,17 +5,13 @@ #ifndef COMPONENTS_PREFS_PREF_FILTER_H_ #define COMPONENTS_PREFS_PREF_FILTER_H_ -#include #include #include #include "base/callback_forward.h" +#include "base/values.h" #include "components/prefs/prefs_export.h" -namespace base { -class DictionaryValue; -} // namespace base - // Filters preferences as they are loaded from disk or updated at runtime. // Currently supported only by JsonPrefStore. class COMPONENTS_PREFS_EXPORT PrefFilter { @@ -29,8 +25,7 @@ class COMPONENTS_PREFS_EXPORT PrefFilter { // builder. |schedule_write| indicates whether a write should be immediately // scheduled (typically because the |prefs| were pre-modified). using PostFilterOnLoadCallback = - base::OnceCallback prefs, - bool schedule_write)>; + base::OnceCallback; virtual ~PrefFilter() {} @@ -43,7 +38,7 @@ class COMPONENTS_PREFS_EXPORT PrefFilter { // to external users (see SegregatedPrefStore::ReadPrefs() for an example). virtual void FilterOnLoad( PostFilterOnLoadCallback post_filter_on_load_callback, - std::unique_ptr pref_store_contents) = 0; + base::Value::Dict pref_store_contents) = 0; // Receives notification when a pref store value is changed, before Observers // are notified. @@ -57,7 +52,7 @@ class COMPONENTS_PREFS_EXPORT PrefFilter { // invoked synchronously after the next write (from the I/O TaskRunner so they // must not be bound to thread-unsafe member state). virtual OnWriteCallbackPair FilterSerializeData( - base::DictionaryValue* pref_store_contents) = 0; + base::Value::Dict& pref_store_contents) = 0; // Cleans preference data that may have been saved outside of the store. virtual void OnStoreDeletionFromDisk() = 0; diff --git a/services/preferences/tracked/dictionary_hash_store_contents.cc b/services/preferences/tracked/dictionary_hash_store_contents.cc index c2c5781f9242f..2016f3b921bde 100644 --- a/services/preferences/tracked/dictionary_hash_store_contents.cc +++ b/services/preferences/tracked/dictionary_hash_store_contents.cc @@ -19,7 +19,7 @@ const char kSuperMACPref[] = "protection.super_mac"; } DictionaryHashStoreContents::DictionaryHashStoreContents( - base::Value::Dict* storage) + base::Value::Dict& storage) : storage_(storage) {} // static diff --git a/services/preferences/tracked/dictionary_hash_store_contents.h b/services/preferences/tracked/dictionary_hash_store_contents.h index fe9165202bf45..27811e89efdc4 100644 --- a/services/preferences/tracked/dictionary_hash_store_contents.h +++ b/services/preferences/tracked/dictionary_hash_store_contents.h @@ -5,7 +5,7 @@ #ifndef SERVICES_PREFERENCES_TRACKED_DICTIONARY_HASH_STORE_CONTENTS_H_ #define SERVICES_PREFERENCES_TRACKED_DICTIONARY_HASH_STORE_CONTENTS_H_ -#include "base/memory/raw_ptr.h" +#include "base/memory/raw_ref.h" #include "base/values.h" #include "services/preferences/tracked/hash_store_contents.h" @@ -21,7 +21,7 @@ class DictionaryHashStoreContents : public HashStoreContents { public: // Constructs a DictionaryHashStoreContents that reads from and writes to // |storage|. - explicit DictionaryHashStoreContents(base::Value::Dict* storage); + explicit DictionaryHashStoreContents(base::Value::Dict& storage); DictionaryHashStoreContents(const DictionaryHashStoreContents&) = delete; DictionaryHashStoreContents& operator=(const DictionaryHashStoreContents&) = @@ -50,7 +50,7 @@ class DictionaryHashStoreContents : public HashStoreContents { void SetSuperMac(const std::string& super_mac) override; private: - raw_ptr storage_; + const raw_ref storage_; // Helper function to get a mutable version of the macs from |storage_|, // creating it if needed and |create_if_null| is true. diff --git a/services/preferences/tracked/interceptable_pref_filter.cc b/services/preferences/tracked/interceptable_pref_filter.cc index 67c94de58a16b..c1d3d6ea7b135 100644 --- a/services/preferences/tracked/interceptable_pref_filter.cc +++ b/services/preferences/tracked/interceptable_pref_filter.cc @@ -13,7 +13,7 @@ InterceptablePrefFilter::~InterceptablePrefFilter() {} void InterceptablePrefFilter::FilterOnLoad( PostFilterOnLoadCallback post_filter_on_load_callback, - std::unique_ptr pref_store_contents) { + base::Value::Dict pref_store_contents) { if (filter_on_load_interceptor_.is_null()) { FinalizeFilterOnLoad(std::move(post_filter_on_load_callback), std::move(pref_store_contents), false); diff --git a/services/preferences/tracked/interceptable_pref_filter.h b/services/preferences/tracked/interceptable_pref_filter.h index 36d05e12db03e..d274e710892cc 100644 --- a/services/preferences/tracked/interceptable_pref_filter.h +++ b/services/preferences/tracked/interceptable_pref_filter.h @@ -5,8 +5,6 @@ #ifndef SERVICES_PREFERENCES_TRACKED_INTERCEPTABLE_PREF_FILTER_H_ #define SERVICES_PREFERENCES_TRACKED_INTERCEPTABLE_PREF_FILTER_H_ -#include - #include "base/callback.h" #include "base/memory/weak_ptr.h" #include "base/values.h" @@ -25,23 +23,21 @@ class InterceptablePrefFilter // indicates whether the |prefs| were actually altered by the // FilterOnLoadInterceptor before being handed back. using FinalizeFilterOnLoadCallback = - base::OnceCallback prefs, - bool prefs_altered)>; + base::OnceCallback; // A callback to be invoked from FilterOnLoad. It takes ownership of prefs // and may modify them before handing them back to this // InterceptablePrefFilter via |finalize_filter_on_load|. using FilterOnLoadInterceptor = base::OnceCallback prefs)>; + base::Value::Dict prefs)>; InterceptablePrefFilter(); ~InterceptablePrefFilter() override; // PrefFilter partial implementation. - void FilterOnLoad( - PostFilterOnLoadCallback post_filter_on_load_callback, - std::unique_ptr pref_store_contents) override; + void FilterOnLoad(PostFilterOnLoadCallback post_filter_on_load_callback, + base::Value::Dict pref_store_contents) override; // Registers |filter_on_load_interceptor| to intercept the next FilterOnLoad // event. At most one FilterOnLoadInterceptor should be registered per @@ -57,7 +53,7 @@ class InterceptablePrefFilter // initial caller of FilterOnLoad. virtual void FinalizeFilterOnLoad( PostFilterOnLoadCallback post_filter_on_load_callback, - std::unique_ptr pref_store_contents, + base::Value::Dict pref_store_contents, bool prefs_altered) = 0; // Callback to be invoked only once (and subsequently reset) on the next diff --git a/services/preferences/tracked/interceptable_pref_filter_unittest.cc b/services/preferences/tracked/interceptable_pref_filter_unittest.cc index 30c46e6bf8f42..beb710c9b6d9d 100644 --- a/services/preferences/tracked/interceptable_pref_filter_unittest.cc +++ b/services/preferences/tracked/interceptable_pref_filter_unittest.cc @@ -17,7 +17,7 @@ class TestInterceptablePrefFilter : public InterceptablePrefFilter { public: void FinalizeFilterOnLoad( PostFilterOnLoadCallback post_filter_on_load_callback, - std::unique_ptr pref_store_contents, + base::Value::Dict pref_store_contents, bool prefs_altered) override { std::move(post_filter_on_load_callback) .Run(std::move(pref_store_contents), prefs_altered); @@ -26,19 +26,19 @@ class TestInterceptablePrefFilter : public InterceptablePrefFilter { void FilterUpdate(const std::string& path) override {} OnWriteCallbackPair FilterSerializeData( - base::DictionaryValue* pref_store_contents) override { + base::Value::Dict& pref_store_contents) override { return {}; } }; void NoOpIntercept(InterceptablePrefFilter::FinalizeFilterOnLoadCallback finalize_filter_on_load, - std::unique_ptr prefs) { + base::Value::Dict prefs) { std::move(finalize_filter_on_load).Run(std::move(prefs), false); } void DeleteFilter(std::unique_ptr* filter, - std::unique_ptr prefs, + base::Value::Dict prefs, bool schedule_write) { filter->reset(); } @@ -47,7 +47,7 @@ TEST(InterceptablePrefFilterTest, CallbackDeletes) { auto filter = std::make_unique(); filter->InterceptNextFilterOnLoad(base::BindOnce(&NoOpIntercept)); filter->FilterOnLoad(base::BindOnce(&DeleteFilter, &filter), - std::make_unique()); + base::Value::Dict()); EXPECT_FALSE(filter); } diff --git a/services/preferences/tracked/pref_hash_filter.cc b/services/preferences/tracked/pref_hash_filter.cc index bcecae4da2e49..ab40b8ca1ce0d 100644 --- a/services/preferences/tracked/pref_hash_filter.cc +++ b/services/preferences/tracked/pref_hash_filter.cc @@ -29,7 +29,7 @@ namespace { void CleanupDeprecatedTrackedPreferences( - base::DictionaryValue* pref_store_contents, + base::Value::Dict& pref_store_contents, PrefHashStoreTransaction* hash_store_transaction) { // Add deprecated previously tracked preferences below for them to be cleaned // up from both the pref files and the hash store. @@ -39,7 +39,7 @@ void CleanupDeprecatedTrackedPreferences( for (size_t i = 0; i < std::size(kDeprecatedTrackedPreferences); ++i) { const char* key = kDeprecatedTrackedPreferences[i]; - pref_store_contents->RemovePath(key); + pref_store_contents.RemoveByDottedPath(key); hash_store_transaction->ClearHash(key); } } @@ -143,7 +143,7 @@ void PrefHashFilter::ClearResetTime(PrefService* user_prefs) { } void PrefHashFilter::Initialize(base::Value::Dict& pref_store_contents) { - DictionaryHashStoreContents dictionary_contents(&pref_store_contents); + DictionaryHashStoreContents dictionary_contents(pref_store_contents); std::unique_ptr hash_store_transaction( pref_hash_store_->BeginTransaction(&dictionary_contents)); for (auto it = tracked_paths_.begin(); it != tracked_paths_.end(); ++it) { @@ -167,17 +167,15 @@ void PrefHashFilter::FilterUpdate(const std::string& path) { // disk. This is required as storing the hash everytime a pref's value changes // is too expensive (see perf regression @ http://crbug.com/331273). PrefFilter::OnWriteCallbackPair PrefHashFilter::FilterSerializeData( - base::DictionaryValue* pref_store_contents) { + base::Value::Dict& pref_store_contents) { // Generate the callback pair before clearing |changed_paths_|. - base::Value::Dict* pref_store_contents_dict = - pref_store_contents ? &pref_store_contents->GetDict() : nullptr; PrefFilter::OnWriteCallbackPair callback_pair = - GetOnWriteSynchronousCallbacks(pref_store_contents_dict); + GetOnWriteSynchronousCallbacks(pref_store_contents); if (!changed_paths_.empty()) { base::TimeTicks checkpoint = base::TimeTicks::Now(); { - DictionaryHashStoreContents dictionary_contents(pref_store_contents_dict); + DictionaryHashStoreContents dictionary_contents(pref_store_contents); std::unique_ptr hash_store_transaction( pref_hash_store_->BeginTransaction(&dictionary_contents)); @@ -194,7 +192,7 @@ PrefFilter::OnWriteCallbackPair PrefHashFilter::FilterSerializeData( const std::string& changed_path = it->first; const TrackedPreference* changed_preference = it->second; const base::Value* value = - pref_store_contents_dict->FindByDottedPath(changed_path); + pref_store_contents.FindByDottedPath(changed_path); changed_preference->OnNewValue(value, hash_store_transaction.get()); } changed_paths_.clear(); @@ -219,15 +217,13 @@ void PrefHashFilter::OnStoreDeletionFromDisk() { void PrefHashFilter::FinalizeFilterOnLoad( PostFilterOnLoadCallback post_filter_on_load_callback, - std::unique_ptr pref_store_contents, + base::Value::Dict pref_store_contents, bool prefs_altered) { - DCHECK(pref_store_contents); base::TimeTicks checkpoint = base::TimeTicks::Now(); bool did_reset = false; { - DictionaryHashStoreContents dictionary_contents( - pref_store_contents ? &pref_store_contents->GetDict() : nullptr); + DictionaryHashStoreContents dictionary_contents(pref_store_contents); std::unique_ptr hash_store_transaction( pref_hash_store_->BeginTransaction(&dictionary_contents)); @@ -239,12 +235,12 @@ void PrefHashFilter::FinalizeFilterOnLoad( external_validation_hash_store_pair_->second.get()); } - CleanupDeprecatedTrackedPreferences(pref_store_contents.get(), + CleanupDeprecatedTrackedPreferences(pref_store_contents, hash_store_transaction.get()); for (auto it = tracked_paths_.begin(); it != tracked_paths_.end(); ++it) { if (it->second->EnforceAndReport( - pref_store_contents.get(), hash_store_transaction.get(), + pref_store_contents, hash_store_transaction.get(), external_validation_hash_store_transaction.get())) { did_reset = true; prefs_altered = true; @@ -255,7 +251,7 @@ void PrefHashFilter::FinalizeFilterOnLoad( } if (did_reset) { - pref_store_contents->SetString( + pref_store_contents.SetByDottedPath( user_prefs::kPreferenceResetTime, base::NumberToString(base::Time::Now().ToInternalValue())); FilterUpdate(user_prefs::kPreferenceResetTime); @@ -317,7 +313,7 @@ void PrefHashFilter::FlushToExternalStore( } PrefFilter::OnWriteCallbackPair PrefHashFilter::GetOnWriteSynchronousCallbacks( - base::Value::Dict* pref_store_contents) { + base::Value::Dict& pref_store_contents) { if (changed_paths_.empty() || !external_validation_hash_store_pair_) { return std::make_pair(base::OnceClosure(), base::OnceCallback()); @@ -333,7 +329,7 @@ PrefFilter::OnWriteCallbackPair PrefHashFilter::GetOnWriteSynchronousCallbacks( switch (changed_preference->GetType()) { case TrackedPreferenceType::ATOMIC: { const base::Value* new_value = - pref_store_contents->FindByDottedPath(changed_path); + pref_store_contents.FindByDottedPath(changed_path); changed_paths_macs->Set( changed_path, external_validation_hash_store_pair_->first->ComputeMac( @@ -342,7 +338,7 @@ PrefFilter::OnWriteCallbackPair PrefHashFilter::GetOnWriteSynchronousCallbacks( } case TrackedPreferenceType::SPLIT: { const base::Value::Dict* dict = - pref_store_contents->FindDictByDottedPath(changed_path); + pref_store_contents.FindDictByDottedPath(changed_path); changed_paths_macs->Set( changed_path, external_validation_hash_store_pair_->first->ComputeSplitMacs( diff --git a/services/preferences/tracked/pref_hash_filter.h b/services/preferences/tracked/pref_hash_filter.h index 6e846bdd9cc0c..1abc7a99bd400 100644 --- a/services/preferences/tracked/pref_hash_filter.h +++ b/services/preferences/tracked/pref_hash_filter.h @@ -95,7 +95,7 @@ class PrefHashFilter : public InterceptablePrefFilter { // PrefFilter remaining implementation. void FilterUpdate(const std::string& path) override; OnWriteCallbackPair FilterSerializeData( - base::DictionaryValue* pref_store_contents) override; + base::Value::Dict& pref_store_contents) override; void OnStoreDeletionFromDisk() override; @@ -103,13 +103,13 @@ class PrefHashFilter : public InterceptablePrefFilter { // InterceptablePrefFilter implementation. void FinalizeFilterOnLoad( PostFilterOnLoadCallback post_filter_on_load_callback, - std::unique_ptr pref_store_contents, + base::Value::Dict pref_store_contents, bool prefs_altered) override; // Helper function to generate FilterSerializeData()'s pre-write and // post-write callbacks. The returned callbacks are thread-safe. OnWriteCallbackPair GetOnWriteSynchronousCallbacks( - base::Value::Dict* pref_store_contents); + base::Value::Dict& pref_store_contents); // Clears the MACs contained in |external_validation_hash_store_contents| // which are present in |paths_to_clear|. diff --git a/services/preferences/tracked/pref_hash_filter_unittest.cc b/services/preferences/tracked/pref_hash_filter_unittest.cc index 26eba4022f105..937f32773042a 100644 --- a/services/preferences/tracked/pref_hash_filter_unittest.cc +++ b/services/preferences/tracked/pref_hash_filter_unittest.cc @@ -179,10 +179,10 @@ class MockPrefHashStore : public PrefHashStore { const base::Value* new_value) override; ValueState CheckSplitValue( const std::string& path, - const base::DictionaryValue* initial_split_value, + const base::Value::Dict* initial_split_value, std::vector* invalid_keys) const override; void StoreSplitHash(const std::string& path, - const base::DictionaryValue* split_value) override; + const base::Value::Dict* split_value) override; bool HasHash(const std::string& path) const override; void ImportHash(const std::string& path, const base::Value* hash) override; void ClearHash(const std::string& path) override; @@ -195,12 +195,12 @@ class MockPrefHashStore : public PrefHashStore { // Records a call to this mock's CheckValue/CheckSplitValue methods. ValueState RecordCheckValue(const std::string& path, - const base::Value* value, + const void* value, PrefTrackingStrategy strategy); // Records a call to this mock's StoreHash/StoreSplitHash methods. void RecordStoreHash(const std::string& path, - const base::Value* new_value, + const void* new_value, PrefTrackingStrategy strategy); std::map check_results_; @@ -263,7 +263,7 @@ base::Value::Dict MockPrefHashStore::ComputeSplitMacs( } ValueState MockPrefHashStore::RecordCheckValue(const std::string& path, - const base::Value* value, + const void* value, PrefTrackingStrategy strategy) { // Record that |path| was checked and validate that it wasn't previously // checked. @@ -278,7 +278,7 @@ ValueState MockPrefHashStore::RecordCheckValue(const std::string& path, } void MockPrefHashStore::RecordStoreHash(const std::string& path, - const base::Value* new_value, + const void* new_value, PrefTrackingStrategy strategy) { EXPECT_TRUE( stored_values_ @@ -305,7 +305,7 @@ void MockPrefHashStore::MockPrefHashStoreTransaction::StoreHash( ValueState MockPrefHashStore::MockPrefHashStoreTransaction::CheckSplitValue( const std::string& path, - const base::DictionaryValue* initial_split_value, + const base::Value::Dict* initial_split_value, std::vector* invalid_keys) const { EXPECT_TRUE(invalid_keys && invalid_keys->empty()); @@ -323,7 +323,7 @@ ValueState MockPrefHashStore::MockPrefHashStoreTransaction::CheckSplitValue( void MockPrefHashStore::MockPrefHashStoreTransaction::StoreSplitHash( const std::string& path, - const base::DictionaryValue* new_value) { + const base::Value::Dict* new_value) { outer_->RecordStoreHash(path, new_value, PrefTrackingStrategy::SPLIT); } @@ -616,10 +616,12 @@ class PrefHashFilterTest : public testing::TestWithParam, // GetPrefsBack() as there is no FilterOnLoadInterceptor installed on // |pref_hash_filter_|. void DoFilterOnLoad(bool expect_prefs_modifications) { + std::unique_ptr prefs = + std::move(pref_store_contents_); pref_hash_filter_->FilterOnLoad( base::BindOnce(&PrefHashFilterTest::GetPrefsBack, base::Unretained(this), expect_prefs_modifications), - std::move(pref_store_contents_)); + std::move(*prefs).TakeDict()); } raw_ptr mock_pref_hash_store_; @@ -633,9 +635,10 @@ class PrefHashFilterTest : public testing::TestWithParam, // Stores |prefs| back in |pref_store_contents| and ensure // |expected_schedule_write| matches the reported |schedule_write|. void GetPrefsBack(bool expected_schedule_write, - std::unique_ptr prefs, + base::Value::Dict prefs, bool schedule_write) { - pref_store_contents_ = std::move(prefs); + pref_store_contents_ = base::DictionaryValue::From( + base::Value::ToUniquePtrValue(base::Value(std::move(prefs)))); EXPECT_TRUE(pref_store_contents_); EXPECT_EQ(expected_schedule_write, schedule_write); } @@ -690,15 +693,15 @@ TEST_P(PrefHashFilterTest, StampSuperMACAltersStore) { } TEST_P(PrefHashFilterTest, FilterTrackedPrefUpdate) { - base::DictionaryValue root_dict; - base::Value* string_value = root_dict.SetString(kAtomicPref, "string value"); + base::Value::Dict root_dict; + base::Value* string_value = root_dict.Set(kAtomicPref, "string value"); // No path should be stored on FilterUpdate. pref_hash_filter_->FilterUpdate(kAtomicPref); ASSERT_EQ(0u, mock_pref_hash_store_->stored_paths_count()); // One path should be stored on FilterSerializeData. - pref_hash_filter_->FilterSerializeData(&root_dict); + pref_hash_filter_->FilterSerializeData(root_dict); ASSERT_EQ(1u, mock_pref_hash_store_->stored_paths_count()); MockPrefHashStore::ValuePtrStrategyPair stored_value = mock_pref_hash_store_->stored_value(kAtomicPref); @@ -710,7 +713,7 @@ TEST_P(PrefHashFilterTest, FilterTrackedPrefUpdate) { } TEST_P(PrefHashFilterTest, FilterTrackedPrefClearing) { - base::DictionaryValue root_dict; + base::Value::Dict root_dict; // We don't actually add the pref's value to root_dict to simulate that // it was just cleared in the PrefStore. @@ -719,7 +722,7 @@ TEST_P(PrefHashFilterTest, FilterTrackedPrefClearing) { ASSERT_EQ(0u, mock_pref_hash_store_->stored_paths_count()); // One path should be stored on FilterSerializeData, with no value. - pref_hash_filter_->FilterSerializeData(&root_dict); + pref_hash_filter_->FilterSerializeData(root_dict); ASSERT_EQ(1u, mock_pref_hash_store_->stored_paths_count()); MockPrefHashStore::ValuePtrStrategyPair stored_value = mock_pref_hash_store_->stored_value(kAtomicPref); @@ -731,9 +734,8 @@ TEST_P(PrefHashFilterTest, FilterTrackedPrefClearing) { } TEST_P(PrefHashFilterTest, FilterSplitPrefUpdate) { - base::DictionaryValue root_dict; - base::Value* dict_value = - root_dict.SetKey(kSplitPref, base::Value(base::Value::Type::DICTIONARY)); + base::Value::Dict root_dict; + base::Value* dict_value = root_dict.Set(kSplitPref, base::Value::Dict()); dict_value->SetStringKey("a", "foo"); dict_value->SetIntKey("b", 1234); @@ -742,7 +744,7 @@ TEST_P(PrefHashFilterTest, FilterSplitPrefUpdate) { ASSERT_EQ(0u, mock_pref_hash_store_->stored_paths_count()); // One path should be stored on FilterSerializeData. - pref_hash_filter_->FilterSerializeData(&root_dict); + pref_hash_filter_->FilterSerializeData(root_dict); ASSERT_EQ(1u, mock_pref_hash_store_->stored_paths_count()); MockPrefHashStore::ValuePtrStrategyPair stored_value = mock_pref_hash_store_->stored_value(kSplitPref); @@ -754,7 +756,7 @@ TEST_P(PrefHashFilterTest, FilterSplitPrefUpdate) { } TEST_P(PrefHashFilterTest, FilterTrackedSplitPrefClearing) { - base::DictionaryValue root_dict; + base::Value::Dict root_dict; // We don't actually add the pref's value to root_dict to simulate that // it was just cleared in the PrefStore. @@ -763,7 +765,7 @@ TEST_P(PrefHashFilterTest, FilterTrackedSplitPrefClearing) { ASSERT_EQ(0u, mock_pref_hash_store_->stored_paths_count()); // One path should be stored on FilterSerializeData, with no value. - pref_hash_filter_->FilterSerializeData(&root_dict); + pref_hash_filter_->FilterSerializeData(root_dict); ASSERT_EQ(1u, mock_pref_hash_store_->stored_paths_count()); MockPrefHashStore::ValuePtrStrategyPair stored_value = mock_pref_hash_store_->stored_value(kSplitPref); @@ -775,15 +777,15 @@ TEST_P(PrefHashFilterTest, FilterTrackedSplitPrefClearing) { } TEST_P(PrefHashFilterTest, FilterUntrackedPrefUpdate) { - base::DictionaryValue root_dict; - root_dict.SetString("untracked", "some value"); + base::Value::Dict root_dict; + root_dict.Set("untracked", "some value"); pref_hash_filter_->FilterUpdate("untracked"); // No paths should be stored on FilterUpdate. ASSERT_EQ(0u, mock_pref_hash_store_->stored_paths_count()); // Nor on FilterSerializeData. - pref_hash_filter_->FilterSerializeData(&root_dict); + pref_hash_filter_->FilterSerializeData(root_dict); ASSERT_EQ(0u, mock_pref_hash_store_->stored_paths_count()); // No transaction should even be started on FilterSerializeData() if there are @@ -792,13 +794,12 @@ TEST_P(PrefHashFilterTest, FilterUntrackedPrefUpdate) { } TEST_P(PrefHashFilterTest, MultiplePrefsFilterSerializeData) { - base::DictionaryValue root_dict; - base::Value* int_value1 = root_dict.GetDict().Set(kAtomicPref, 1); - root_dict.GetDict().Set(kAtomicPref2, 2); - root_dict.GetDict().Set(kAtomicPref3, 3); - root_dict.GetDict().Set("untracked", 4); - base::Value* dict_value = - root_dict.SetKey(kSplitPref, base::Value(base::Value::Type::DICTIONARY)); + base::Value::Dict root_dict; + base::Value* int_value1 = root_dict.Set(kAtomicPref, 1); + root_dict.Set(kAtomicPref2, 2); + root_dict.Set(kAtomicPref3, 3); + root_dict.Set("untracked", 4); + base::Value* dict_value = root_dict.Set(kSplitPref, base::Value::Dict()); dict_value->SetBoolKey("a", true); // Only update kAtomicPref, kAtomicPref3, and kSplitPref. @@ -808,12 +809,12 @@ TEST_P(PrefHashFilterTest, MultiplePrefsFilterSerializeData) { ASSERT_EQ(0u, mock_pref_hash_store_->stored_paths_count()); // Update kAtomicPref3 again, nothing should be stored still. - base::Value* int_value5 = root_dict.GetDict().Set(kAtomicPref3, 5); + base::Value* int_value5 = root_dict.Set(kAtomicPref3, 5); ASSERT_EQ(0u, mock_pref_hash_store_->stored_paths_count()); // On FilterSerializeData, only kAtomicPref, kAtomicPref3, and kSplitPref // should get a new hash. - pref_hash_filter_->FilterSerializeData(&root_dict); + pref_hash_filter_->FilterSerializeData(root_dict); ASSERT_EQ(3u, mock_pref_hash_store_->stored_paths_count()); MockPrefHashStore::ValuePtrStrategyPair stored_value_atomic1 = mock_pref_hash_store_->stored_value(kAtomicPref); @@ -1223,19 +1224,19 @@ TEST_P(PrefHashFilterTest, DontResetReportOnly) { } TEST_P(PrefHashFilterTest, CallFilterSerializeDataCallbacks) { - base::DictionaryValue root_dict; - base::DictionaryValue dict_value; - dict_value.SetBoolean("a", true); - root_dict.GetDict().Set(kAtomicPref, 1); - root_dict.GetDict().Set(kAtomicPref2, 2); - root_dict.SetKey(kSplitPref, std::move(dict_value)); + base::Value::Dict root_dict; + base::Value::Dict dict_value; + dict_value.Set("a", true); + root_dict.Set(kAtomicPref, 1); + root_dict.Set(kAtomicPref2, 2); + root_dict.Set(kSplitPref, std::move(dict_value)); // Skip updating kAtomicPref2. pref_hash_filter_->FilterUpdate(kAtomicPref); pref_hash_filter_->FilterUpdate(kSplitPref); PrefHashFilter::OnWriteCallbackPair callbacks = - pref_hash_filter_->FilterSerializeData(&root_dict); + pref_hash_filter_->FilterSerializeData(root_dict); ASSERT_FALSE(callbacks.first.is_null()); @@ -1269,14 +1270,14 @@ TEST_P(PrefHashFilterTest, CallFilterSerializeDataCallbacks) { } TEST_P(PrefHashFilterTest, CallFilterSerializeDataCallbacksWithFailure) { - base::DictionaryValue root_dict; - root_dict.GetDict().Set(kAtomicPref, 1); + base::Value::Dict root_dict; + root_dict.Set(kAtomicPref, 1); // Only update kAtomicPref. pref_hash_filter_->FilterUpdate(kAtomicPref); PrefHashFilter::OnWriteCallbackPair callbacks = - pref_hash_filter_->FilterSerializeData(&root_dict); + pref_hash_filter_->FilterSerializeData(root_dict); ASSERT_FALSE(callbacks.first.is_null()); diff --git a/services/preferences/tracked/pref_hash_store_impl.cc b/services/preferences/tracked/pref_hash_store_impl.cc index 5d1bb8cdb5975..90289c869b201 100644 --- a/services/preferences/tracked/pref_hash_store_impl.cc +++ b/services/preferences/tracked/pref_hash_store_impl.cc @@ -62,10 +62,10 @@ class PrefHashStoreImpl::PrefHashStoreTransactionImpl void StoreHash(const std::string& path, const base::Value* value) override; ValueState CheckSplitValue( const std::string& path, - const base::DictionaryValue* initial_split_value, + const base::Value::Dict* initial_split_value, std::vector* invalid_keys) const override; void StoreSplitHash(const std::string& path, - const base::DictionaryValue* split_value) override; + const base::Value::Dict* split_value) override; bool HasHash(const std::string& path) const override; void ImportHash(const std::string& path, const base::Value* hash) override; void ClearHash(const std::string& path) override; @@ -203,7 +203,7 @@ void PrefHashStoreImpl::PrefHashStoreTransactionImpl::StoreHash( ValueState PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckSplitValue( const std::string& path, - const base::DictionaryValue* initial_split_value, + const base::Value::Dict* initial_split_value, std::vector* invalid_keys) const { DCHECK(invalid_keys && invalid_keys->empty()); @@ -213,8 +213,9 @@ ValueState PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckSplitValue( // Treat NULL and empty the same; otherwise we would need to store a hash for // the entire dictionary (or some other special beacon) to differentiate these // two cases which are really the same for dictionaries. - if (!initial_split_value || initial_split_value->DictEmpty()) + if (!initial_split_value || initial_split_value->empty()) { return has_hashes ? ValueState::CLEARED : ValueState::UNCHANGED; + } if (!has_hashes) return super_mac_valid_ ? ValueState::TRUSTED_UNKNOWN_VALUE @@ -224,7 +225,7 @@ ValueState PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckSplitValue( std::string keyed_path(path); keyed_path.push_back('.'); const size_t common_part_length = keyed_path.length(); - for (const auto item : initial_split_value->GetDict()) { + for (const auto item : *initial_split_value) { std::map::iterator entry = split_macs.find(item.first); if (entry == split_macs.end()) { @@ -268,12 +269,11 @@ ValueState PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckSplitValue( void PrefHashStoreImpl::PrefHashStoreTransactionImpl::StoreSplitHash( const std::string& path, - const base::DictionaryValue* split_value) { + const base::Value::Dict* split_value) { contents_->RemoveEntry(path); if (split_value) { - base::Value::Dict split_macs = - outer_->ComputeSplitMacs(path, &split_value->GetDict()); + base::Value::Dict split_macs = outer_->ComputeSplitMacs(path, split_value); for (const auto item : split_macs) { DCHECK(item.second.is_string()); diff --git a/services/preferences/tracked/pref_hash_store_impl_unittest.cc b/services/preferences/tracked/pref_hash_store_impl_unittest.cc index b7b79c6b9ca1d..9c4aeb774a082 100644 --- a/services/preferences/tracked/pref_hash_store_impl_unittest.cc +++ b/services/preferences/tracked/pref_hash_store_impl_unittest.cc @@ -17,7 +17,7 @@ using ValueState = class PrefHashStoreImplTest : public testing::Test { public: - PrefHashStoreImplTest() : contents_(&pref_store_contents_.GetDict()) {} + PrefHashStoreImplTest() : contents_(pref_store_contents_.GetDict()) {} PrefHashStoreImplTest(const PrefHashStoreImplTest&) = delete; PrefHashStoreImplTest& operator=(const PrefHashStoreImplTest&) = delete; @@ -325,17 +325,17 @@ TEST_F(PrefHashStoreImplTest, SuperMACDisabled) { } TEST_F(PrefHashStoreImplTest, SplitHashStoreAndCheck) { - base::DictionaryValue dict; - dict.SetKey("a", base::Value("to be replaced")); - dict.SetKey("unchanged.path.with.dots", base::Value("same")); - dict.SetKey("o", base::Value("old")); + base::Value::Dict dict; + dict.Set("a", base::Value("to be replaced")); + dict.Set("unchanged.path.with.dots", base::Value("same")); + dict.Set("o", base::Value("old")); - base::DictionaryValue modified_dict; - modified_dict.SetKey("a", base::Value("replaced")); - modified_dict.SetKey("unchanged.path.with.dots", base::Value("same")); - modified_dict.SetKey("c", base::Value("new")); + base::Value::Dict modified_dict; + modified_dict.Set("a", base::Value("replaced")); + modified_dict.Set("unchanged.path.with.dots", base::Value("same")); + modified_dict.Set("c", base::Value("new")); - base::DictionaryValue empty_dict; + base::Value::Dict empty_dict; std::vector invalid_keys; @@ -452,7 +452,7 @@ TEST_F(PrefHashStoreImplTest, SplitHashStoreAndCheck) { } TEST_F(PrefHashStoreImplTest, EmptyAndNULLSplitDict) { - base::DictionaryValue empty_dict; + base::Value::Dict empty_dict; std::vector invalid_keys; @@ -462,8 +462,8 @@ TEST_F(PrefHashStoreImplTest, EmptyAndNULLSplitDict) { pref_hash_store.BeginTransaction(GetHashStoreContents())); // Store hashes for a random dict to be overwritten below. - base::DictionaryValue initial_dict; - initial_dict.SetString("a", "foo"); + base::Value::Dict initial_dict; + initial_dict.Set("a", "foo"); transaction->StoreSplitHash("path1", &initial_dict); // Verify stored empty dictionary matches NULL and empty dictionary back. @@ -495,9 +495,9 @@ TEST_F(PrefHashStoreImplTest, EmptyAndNULLSplitDict) { std::unique_ptr transaction( pref_hash_store.BeginTransaction(GetHashStoreContents())); - base::DictionaryValue tested_dict; - tested_dict.SetString("a", "foo"); - tested_dict.SetString("b", "bar"); + base::Value::Dict tested_dict; + tested_dict.Set("a", "foo"); + tested_dict.Set("b", "bar"); EXPECT_EQ( ValueState::TRUSTED_UNKNOWN_VALUE, transaction->CheckSplitValue("new_path", &tested_dict, &invalid_keys)); @@ -514,11 +514,11 @@ TEST_F(PrefHashStoreImplTest, EmptyAndNULLSplitDict) { TEST_F(PrefHashStoreImplTest, TrustedUnknownSplitValueFromExistingAtomic) { base::Value string("string1"); - base::DictionaryValue dict; - dict.SetString("a", "foo"); - dict.SetString("d", "bad"); - dict.SetString("b", "bar"); - dict.SetString("c", "baz"); + base::Value::Dict dict; + dict.Set("a", "foo"); + dict.Set("d", "bad"); + dict.Set("b", "bar"); + dict.Set("c", "baz"); { PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true); diff --git a/services/preferences/tracked/pref_hash_store_transaction.h b/services/preferences/tracked/pref_hash_store_transaction.h index a73d1ef7aba4d..b0bc0b5aac324 100644 --- a/services/preferences/tracked/pref_hash_store_transaction.h +++ b/services/preferences/tracked/pref_hash_store_transaction.h @@ -9,13 +9,9 @@ #include #include "base/strings/string_piece.h" +#include "base/values.h" #include "services/preferences/public/mojom/tracked_preference_validation_delegate.mojom.h" -namespace base { -class DictionaryValue; -class Value; -} // namespace base - // Used to perform a series of checks/transformations on a PrefHashStore. class PrefHashStoreTransaction { public: @@ -42,13 +38,13 @@ class PrefHashStoreTransaction { // changed). virtual prefs::mojom::TrackedPreferenceValidationDelegate::ValueState CheckSplitValue(const std::string& path, - const base::DictionaryValue* initial_split_value, + const base::Value::Dict* initial_split_value, std::vector* invalid_keys) const = 0; // Stores hashes for the |value| of the split preference at |path|. // |split_value| being an empty dictionary or NULL is equivalent. virtual void StoreSplitHash(const std::string& path, - const base::DictionaryValue* split_value) = 0; + const base::Value::Dict* split_value) = 0; // Indicates whether the store contains a hash for the preference at |path|. virtual bool HasHash(const std::string& path) const = 0; diff --git a/services/preferences/tracked/tracked_atomic_preference.cc b/services/preferences/tracked/tracked_atomic_preference.cc index eae015cddde02..d7aa31ca7aec7 100644 --- a/services/preferences/tracked/tracked_atomic_preference.cc +++ b/services/preferences/tracked/tracked_atomic_preference.cc @@ -37,10 +37,10 @@ void TrackedAtomicPreference::OnNewValue( } bool TrackedAtomicPreference::EnforceAndReport( - base::DictionaryValue* pref_store_contents, + base::Value::Dict& pref_store_contents, PrefHashStoreTransaction* transaction, PrefHashStoreTransaction* external_validation_transaction) const { - const base::Value* value = pref_store_contents->FindPath(pref_path_); + const base::Value* value = pref_store_contents.FindByDottedPath(pref_path_); ValueState value_state = transaction->CheckValue(pref_path_, value); helper_.ReportValidationResult(value_state, transaction->GetStoreUMASuffix()); @@ -64,13 +64,14 @@ bool TrackedAtomicPreference::EnforceAndReport( bool was_reset = false; if (reset_action == TrackedPreferenceHelper::DO_RESET) { - pref_store_contents->RemovePath(pref_path_); + pref_store_contents.RemoveByDottedPath(pref_path_); was_reset = true; } if (value_state != ValueState::UNCHANGED) { // Store the hash for the new value (whether it was reset or not). - const base::Value* new_value = pref_store_contents->FindPath(pref_path_); + const base::Value* new_value = + pref_store_contents.FindByDottedPath(pref_path_); transaction->StoreHash(pref_path_, new_value); } @@ -78,7 +79,8 @@ bool TrackedAtomicPreference::EnforceAndReport( // reset or external validation failed. if (external_validation_transaction && (was_reset || external_validation_value_state != ValueState::UNCHANGED)) { - const base::Value* new_value = pref_store_contents->FindPath(pref_path_); + const base::Value* new_value = + pref_store_contents.FindByDottedPath(pref_path_); external_validation_transaction->StoreHash(pref_path_, new_value); } diff --git a/services/preferences/tracked/tracked_atomic_preference.h b/services/preferences/tracked/tracked_atomic_preference.h index 8abb42d622683..ae8b63408824d 100644 --- a/services/preferences/tracked/tracked_atomic_preference.h +++ b/services/preferences/tracked/tracked_atomic_preference.h @@ -43,7 +43,7 @@ class TrackedAtomicPreference : public TrackedPreference { void OnNewValue(const base::Value* value, PrefHashStoreTransaction* transaction) const override; bool EnforceAndReport( - base::DictionaryValue* pref_store_contents, + base::Value::Dict& pref_store_contents, PrefHashStoreTransaction* transaction, PrefHashStoreTransaction* external_validation_transaction) const override; diff --git a/services/preferences/tracked/tracked_preference.h b/services/preferences/tracked/tracked_preference.h index 072c6b38e137b..19b0454470636 100644 --- a/services/preferences/tracked/tracked_preference.h +++ b/services/preferences/tracked/tracked_preference.h @@ -5,12 +5,9 @@ #ifndef SERVICES_PREFERENCES_TRACKED_TRACKED_PREFERENCE_H_ #define SERVICES_PREFERENCES_TRACKED_TRACKED_PREFERENCE_H_ -class PrefHashStoreTransaction; +#include "base/values.h" -namespace base { -class DictionaryValue; -class Value; -} +class PrefHashStoreTransaction; enum class TrackedPreferenceType { ATOMIC, SPLIT }; @@ -36,7 +33,7 @@ class TrackedPreference { // |external_validation_transaction| and its associated state and as such // should only be called before any other subsystem is made aware of it. virtual bool EnforceAndReport( - base::DictionaryValue* pref_store_contents, + base::Value::Dict& pref_store_contents, PrefHashStoreTransaction* transaction, PrefHashStoreTransaction* external_validation_transaction) const = 0; }; diff --git a/services/preferences/tracked/tracked_preferences_migration.cc b/services/preferences/tracked/tracked_preferences_migration.cc index 9f2d55f74ed07..b82c3655a11ad 100644 --- a/services/preferences/tracked/tracked_preferences_migration.cc +++ b/services/preferences/tracked/tracked_preferences_migration.cc @@ -52,7 +52,7 @@ class TrackedPreferencesMigrator PrefFilterID id, InterceptablePrefFilter::FinalizeFilterOnLoadCallback finalize_filter_on_load, - std::unique_ptr prefs); + base::Value::Dict prefs); private: friend class base::RefCounted; @@ -83,8 +83,8 @@ class TrackedPreferencesMigrator std::unique_ptr unprotected_pref_hash_store_; std::unique_ptr protected_pref_hash_store_; - std::unique_ptr unprotected_prefs_; - std::unique_ptr protected_prefs_; + std::unique_ptr unprotected_prefs_; + std::unique_ptr protected_prefs_; }; // Invokes |store_cleaner| for every |keys_to_clean|. @@ -121,9 +121,8 @@ void ScheduleSourcePrefStoreCleanup( // the configuration/implementation in |origin_pref_hash_store|. void CleanupMigratedHashes(const PrefNameSet& migrated_pref_names, PrefHashStore* origin_pref_hash_store, - base::DictionaryValue* origin_pref_store) { - DictionaryHashStoreContents dictionary_contents( - origin_pref_store ? &origin_pref_store->GetDict() : nullptr); + base::Value::Dict& origin_pref_store) { + DictionaryHashStoreContents dictionary_contents(origin_pref_store); std::unique_ptr transaction( origin_pref_hash_store->BeginTransaction(&dictionary_contents)); for (std::set::const_iterator it = migrated_pref_names.begin(); @@ -137,16 +136,14 @@ void CleanupMigratedHashes(const PrefNameSet& migrated_pref_names, // true if any old duplicates remain in |old_store| and sets |new_store_altered| // to true if any value was copied to |new_store|. void MigratePrefsFromOldToNewStore(const PrefNameSet& pref_names, - base::DictionaryValue* old_store, - base::DictionaryValue* new_store, + base::Value::Dict& old_store, + base::Value::Dict& new_store, PrefHashStore* new_hash_store, bool* old_store_needs_cleanup, bool* new_store_altered) { const base::Value::Dict* old_hash_store_contents = - DictionaryHashStoreContents(old_store ? &old_store->GetDict() : nullptr) - .GetContents(); - DictionaryHashStoreContents dictionary_contents( - new_store ? &new_store->GetDict() : nullptr); + DictionaryHashStoreContents(old_store).GetContents(); + DictionaryHashStoreContents dictionary_contents(new_store); std::unique_ptr new_hash_store_transaction( new_hash_store->BeginTransaction(&dictionary_contents)); @@ -161,18 +158,18 @@ void MigratePrefsFromOldToNewStore(const PrefNameSet& pref_names, // If we migrate the value we will also attempt to migrate the hash. bool migrated_value = false; if (const base::Value* value_in_old_store = - old_store->FindPath(pref_name)) { + old_store.FindByDottedPath(pref_name)) { // Whether this value ends up being copied below or was left behind by a // previous incomplete migration, it should be cleaned up. *old_store_needs_cleanup = true; - if (!new_store->FindPath(pref_name)) { + if (!new_store.FindByDottedPath(pref_name)) { // Copy the value from |old_store| to |new_store| rather than moving it // to avoid data loss should |old_store| be flushed to disk without // |new_store| having equivalently been successfully flushed to disk // (e.g., on crash or in cases where |new_store| is read-only following // a read error on startup). - new_store->SetPath(pref_name, value_in_old_store->Clone()); + new_store.SetByDottedPath(pref_name, value_in_old_store->Clone()); migrated_value = true; *new_store_altered = true; } @@ -191,7 +188,7 @@ void MigratePrefsFromOldToNewStore(const PrefNameSet& pref_names, // value in order to provide the same no-op behaviour as if the pref was // added to the wrong file when there was already a value for // |pref_name| in |new_store|. - new_store->RemovePath(pref_name); + new_store.RemoveByDottedPath(pref_name); *new_store_altered = true; } } @@ -230,15 +227,16 @@ void TrackedPreferencesMigrator::InterceptFilterOnLoad( PrefFilterID id, InterceptablePrefFilter::FinalizeFilterOnLoadCallback finalize_filter_on_load, - std::unique_ptr prefs) { + base::Value::Dict prefs) { + auto prefs_migration = std::make_unique(std::move(prefs)); switch (id) { case UNPROTECTED_PREF_FILTER: finalize_unprotected_filter_on_load_ = std::move(finalize_filter_on_load); - unprotected_prefs_ = std::move(prefs); + unprotected_prefs_ = std::move(prefs_migration); break; case PROTECTED_PREF_FILTER: finalize_protected_filter_on_load_ = std::move(finalize_filter_on_load); - protected_prefs_ = std::move(prefs); + protected_prefs_ = std::move(prefs_migration); break; } @@ -253,13 +251,13 @@ void TrackedPreferencesMigrator::MigrateIfReady() { bool protected_prefs_need_cleanup = false; bool unprotected_prefs_altered = false; MigratePrefsFromOldToNewStore( - unprotected_pref_names_, protected_prefs_.get(), unprotected_prefs_.get(), + unprotected_pref_names_, *protected_prefs_, *unprotected_prefs_, unprotected_pref_hash_store_.get(), &protected_prefs_need_cleanup, &unprotected_prefs_altered); bool unprotected_prefs_need_cleanup = false; bool protected_prefs_altered = false; MigratePrefsFromOldToNewStore( - protected_pref_names_, unprotected_prefs_.get(), protected_prefs_.get(), + protected_pref_names_, *unprotected_prefs_, *protected_prefs_, protected_pref_hash_store_.get(), &unprotected_prefs_need_cleanup, &protected_prefs_altered); @@ -271,18 +269,21 @@ void TrackedPreferencesMigrator::MigrateIfReady() { // stores, and doing so in a subsequent launch is easier than within the // same process. CleanupMigratedHashes(unprotected_pref_names_, - protected_pref_hash_store_.get(), - protected_prefs_.get()); + protected_pref_hash_store_.get(), *protected_prefs_); CleanupMigratedHashes(protected_pref_names_, unprotected_pref_hash_store_.get(), - unprotected_prefs_.get()); + *unprotected_prefs_); } // Hand the processed prefs back to their respective filters. + std::unique_ptr unprotected_prefs = + std::move(unprotected_prefs_); std::move(finalize_unprotected_filter_on_load_) - .Run(std::move(unprotected_prefs_), unprotected_prefs_altered); + .Run(std::move(*unprotected_prefs), unprotected_prefs_altered); + std::unique_ptr protected_prefs = + std::move(protected_prefs_); std::move(finalize_protected_filter_on_load_) - .Run(std::move(protected_prefs_), protected_prefs_altered); + .Run(std::move(*protected_prefs), protected_prefs_altered); if (unprotected_prefs_need_cleanup) { // Schedule a cleanup of the |protected_pref_names_| from the unprotected diff --git a/services/preferences/tracked/tracked_preferences_migration_unittest.cc b/services/preferences/tracked/tracked_preferences_migration_unittest.cc index 2c2296a2f20e7..0ba141d60e0df 100644 --- a/services/preferences/tracked/tracked_preferences_migration_unittest.cc +++ b/services/preferences/tracked/tracked_preferences_migration_unittest.cc @@ -47,7 +47,7 @@ class SimpleInterceptablePrefFilter : public InterceptablePrefFilter { // PrefFilter remaining implementation. void FilterUpdate(const std::string& path) override { ADD_FAILURE(); } OnWriteCallbackPair FilterSerializeData( - base::DictionaryValue* pref_store_contents) override { + base::Value::Dict& pref_store_contents) override { ADD_FAILURE(); return std::make_pair(base::OnceClosure(), base::OnceCallback()); @@ -57,7 +57,7 @@ class SimpleInterceptablePrefFilter : public InterceptablePrefFilter { // InterceptablePrefFilter implementation. void FinalizeFilterOnLoad( PostFilterOnLoadCallback post_filter_on_load_callback, - std::unique_ptr pref_store_contents, + base::Value::Dict pref_store_contents, bool prefs_altered) override { std::move(post_filter_on_load_callback) .Run(std::move(pref_store_contents), prefs_altered); @@ -79,8 +79,8 @@ class TrackedPreferencesMigrationTest : public testing::Test { }; TrackedPreferencesMigrationTest() - : unprotected_prefs_(new base::DictionaryValue), - protected_prefs_(new base::DictionaryValue), + : unprotected_prefs_(new base::Value::Dict), + protected_prefs_(new base::Value::Dict), migration_modified_unprotected_store_(false), migration_modified_protected_store_(false), unprotected_store_migration_complete_(false), @@ -153,7 +153,7 @@ class TrackedPreferencesMigrationTest : public testing::Test { void PresetStoreValueHash(MockPrefStoreID store_id, const std::string& key, const std::string value) { - base::DictionaryValue* store = NULL; + base::Value::Dict* store = nullptr; std::unique_ptr pref_hash_store; switch (store_id) { case MOCK_UNPROTECTED_PREF_STORE: @@ -170,7 +170,7 @@ class TrackedPreferencesMigrationTest : public testing::Test { DCHECK(store); base::Value string_value(value); - DictionaryHashStoreContents contents(store ? &store->GetDict() : nullptr); + DictionaryHashStoreContents contents(*store); pref_hash_store->BeginTransaction(&contents)->StoreHash(key, &string_value); } @@ -191,7 +191,7 @@ class TrackedPreferencesMigrationTest : public testing::Test { // in the store identified by |store_id|. void VerifyValuesStored(MockPrefStoreID store_id, const base::StringPairs& expected_prefs_in_store) { - base::DictionaryValue* store = NULL; + base::Value::Dict* store = nullptr; switch (store_id) { case MOCK_UNPROTECTED_PREF_STORE: store = unprotected_prefs_.get(); @@ -204,9 +204,9 @@ class TrackedPreferencesMigrationTest : public testing::Test { for (base::StringPairs::const_iterator it = expected_prefs_in_store.begin(); it != expected_prefs_in_store.end(); ++it) { - std::string val; - EXPECT_TRUE(store->GetString(it->first, &val)); - EXPECT_EQ(it->second, val); + const std::string* val = store->FindStringByDottedPath(it->first); + ASSERT_TRUE(val); + EXPECT_EQ(it->second, *val); } } @@ -214,7 +214,7 @@ class TrackedPreferencesMigrationTest : public testing::Test { // store identified by |store_id|. bool ContainsHash(MockPrefStoreID store_id, std::string expected_pref_in_hash_store) { - base::DictionaryValue* store = NULL; + base::Value::Dict* store = nullptr; switch (store_id) { case MOCK_UNPROTECTED_PREF_STORE: store = unprotected_prefs_.get(); @@ -225,8 +225,7 @@ class TrackedPreferencesMigrationTest : public testing::Test { } DCHECK(store); const base::Value::Dict* hash_store_contents = - DictionaryHashStoreContents(store ? &store->GetDict() : nullptr) - .GetContents(); + DictionaryHashStoreContents(*store).GetContents(); return hash_store_contents && hash_store_contents->FindStringByDottedPath( expected_pref_in_hash_store); } @@ -235,18 +234,24 @@ class TrackedPreferencesMigrationTest : public testing::Test { // in. void HandPrefsToMigrator(MockPrefStoreID store_id) { switch (store_id) { - case MOCK_UNPROTECTED_PREF_STORE: + case MOCK_UNPROTECTED_PREF_STORE: { + std::unique_ptr unprotected_prefs = + std::move(unprotected_prefs_); mock_unprotected_pref_filter_.FilterOnLoad( base::BindOnce(&TrackedPreferencesMigrationTest::GetPrefsBack, base::Unretained(this), MOCK_UNPROTECTED_PREF_STORE), - std::move(unprotected_prefs_)); + std::move(*unprotected_prefs)); break; - case MOCK_PROTECTED_PREF_STORE: + } + case MOCK_PROTECTED_PREF_STORE: { + std::unique_ptr protected_prefs = + std::move(protected_prefs_); mock_protected_pref_filter_.FilterOnLoad( base::BindOnce(&TrackedPreferencesMigrationTest::GetPrefsBack, base::Unretained(this), MOCK_PROTECTED_PREF_STORE), - std::move(protected_prefs_)); + std::move(*protected_prefs)); break; + } } } @@ -311,18 +316,19 @@ class TrackedPreferencesMigrationTest : public testing::Test { // Helper given as an InterceptablePrefFilter::FinalizeFilterOnLoadCallback // to the migrator to be invoked when it's done. void GetPrefsBack(MockPrefStoreID store_id, - std::unique_ptr prefs, + base::Value::Dict prefs, bool prefs_altered) { + auto prefs_ptr = std::make_unique(std::move(prefs)); switch (store_id) { case MOCK_UNPROTECTED_PREF_STORE: EXPECT_FALSE(unprotected_prefs_); - unprotected_prefs_ = std::move(prefs); + unprotected_prefs_ = std::move(prefs_ptr); migration_modified_unprotected_store_ = prefs_altered; unprotected_store_migration_complete_ = true; break; case MOCK_PROTECTED_PREF_STORE: EXPECT_FALSE(protected_prefs_); - protected_prefs_ = std::move(prefs); + protected_prefs_ = std::move(prefs_ptr); migration_modified_protected_store_ = prefs_altered; protected_store_migration_complete_ = true; break; @@ -334,11 +340,11 @@ class TrackedPreferencesMigrationTest : public testing::Test { switch (store_id) { case MOCK_UNPROTECTED_PREF_STORE: ASSERT_TRUE(unprotected_prefs_); - unprotected_prefs_->RemovePath(key); + unprotected_prefs_->RemoveByDottedPath(key); break; case MOCK_PROTECTED_PREF_STORE: ASSERT_TRUE(protected_prefs_); - protected_prefs_->RemovePath(key); + protected_prefs_->RemoveByDottedPath(key); break; } } @@ -348,7 +354,7 @@ class TrackedPreferencesMigrationTest : public testing::Test { void PresetStoreValueOnly(MockPrefStoreID store_id, const std::string& key, const std::string value) { - base::DictionaryValue* store = NULL; + base::Value::Dict* store = nullptr; switch (store_id) { case MOCK_UNPROTECTED_PREF_STORE: store = unprotected_prefs_.get(); @@ -359,14 +365,14 @@ class TrackedPreferencesMigrationTest : public testing::Test { } DCHECK(store); - store->SetString(key, value); + store->SetByDottedPath(key, value); } static const char kSeed[]; static const char kDeviceId[]; - std::unique_ptr unprotected_prefs_; - std::unique_ptr protected_prefs_; + std::unique_ptr unprotected_prefs_; + std::unique_ptr protected_prefs_; SimpleInterceptablePrefFilter mock_unprotected_pref_filter_; SimpleInterceptablePrefFilter mock_protected_pref_filter_; diff --git a/services/preferences/tracked/tracked_split_preference.cc b/services/preferences/tracked/tracked_split_preference.cc index 3340b53494a38..e987cecbd9576 100644 --- a/services/preferences/tracked/tracked_split_preference.cc +++ b/services/preferences/tracked/tracked_split_preference.cc @@ -41,23 +41,23 @@ void TrackedSplitPreference::OnNewValue( NOTREACHED(); return; } - const base::DictionaryValue* dict_value = - value ? &base::Value::AsDictionaryValue(*value) : nullptr; - transaction->StoreSplitHash(pref_path_, dict_value); + + transaction->StoreSplitHash(pref_path_, value ? &value->GetDict() : nullptr); } bool TrackedSplitPreference::EnforceAndReport( - base::DictionaryValue* pref_store_contents, + base::Value::Dict& pref_store_contents, PrefHashStoreTransaction* transaction, PrefHashStoreTransaction* external_validation_transaction) const { - base::DictionaryValue* dict_value = nullptr; - if (!pref_store_contents->GetDictionary(pref_path_, &dict_value) && - pref_store_contents->FindPath(pref_path_)) { + base::Value* value = pref_store_contents.FindByDottedPath(pref_path_); + if (value && !value->is_dict()) { // There should be a dictionary or nothing at |pref_path_|. NOTREACHED(); return false; } + base::Value::Dict* dict_value = value ? &value->GetDict() : nullptr; + std::vector invalid_keys; ValueState value_state = transaction->CheckSplitValue(pref_path_, dict_value, &invalid_keys); @@ -91,28 +91,26 @@ bool TrackedSplitPreference::EnforceAndReport( for (std::vector::const_iterator it = invalid_keys.begin(); it != invalid_keys.end(); ++it) { - dict_value->RemoveKey(*it); + dict_value->Remove(*it); } } else { - pref_store_contents->RemovePath(pref_path_); + pref_store_contents.RemoveByDottedPath(pref_path_); } was_reset = true; } if (value_state != ValueState::UNCHANGED) { // Store the hash for the new value (whether it was reset or not). - const base::DictionaryValue* new_dict_value = NULL; - pref_store_contents->GetDictionary(pref_path_, &new_dict_value); - transaction->StoreSplitHash(pref_path_, new_dict_value); + transaction->StoreSplitHash( + pref_path_, pref_store_contents.FindDictByDottedPath(pref_path_)); } // Update MACs in the external store if there is one and there either was a // reset or external validation failed. if (external_validation_transaction && (was_reset || external_validation_value_state != ValueState::UNCHANGED)) { - const base::DictionaryValue* new_dict_value = nullptr; - pref_store_contents->GetDictionary(pref_path_, &new_dict_value); - external_validation_transaction->StoreSplitHash(pref_path_, new_dict_value); + external_validation_transaction->StoreSplitHash( + pref_path_, pref_store_contents.FindDictByDottedPath(pref_path_)); } return was_reset; diff --git a/services/preferences/tracked/tracked_split_preference.h b/services/preferences/tracked/tracked_split_preference.h index bc6adc8c44ee7..131296a59f93d 100644 --- a/services/preferences/tracked/tracked_split_preference.h +++ b/services/preferences/tracked/tracked_split_preference.h @@ -44,7 +44,7 @@ class TrackedSplitPreference : public TrackedPreference { void OnNewValue(const base::Value* value, PrefHashStoreTransaction* transaction) const override; bool EnforceAndReport( - base::DictionaryValue* pref_store_contents, + base::Value::Dict& pref_store_contents, PrefHashStoreTransaction* transaction, PrefHashStoreTransaction* external_validation_transaction) const override;