[Code Health] Some replacements of base::DictionaryValue for prefs

Bug: 1187023
Change-Id: I1d6a76c691b62f7c8b97c42213e47f60b11c0aac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4095228
Reviewed-by: proberge <proberge@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Nan Lin <linnan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1084540}

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,25 +261,26 @@ 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);
   }
 }
 
 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<ReadResult> read_result) {
 
   DCHECK(read_result);
 
-  auto unfiltered_prefs = std::make_unique<base::DictionaryValue>();
+  base::Value::Dict unfiltered_prefs;
 
   read_error_ = read_result->error;
 
@@ -474,9 +473,9 @@ void JsonPrefStore::OnFileRead(std::unique_ptr<ReadResult> 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<base::DictionaryValue*>(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<base::DictionaryValue> 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;

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<base::DictionaryValue> 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<base::SequencedTaskRunner> file_task_runner_;
 
-  std::unique_ptr<base::DictionaryValue> prefs_;
+  base::Value::Dict prefs_;
 
   bool read_only_;
 

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<base::DictionaryValue> 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<base::DictionaryValue> intercepted_prefs_;
+  std::unique_ptr<base::Value::Dict> 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<base::DictionaryValue> 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<base::Value::Dict>(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<base::Value::Dict> prefs = std::move(intercepted_prefs_);
+  std::move(post_filter_on_load_callback_).Run(std::move(*prefs), false);
 }
 
 class MockPrefStoreObserver : public PrefStore::Observer {

components/prefs/pref_filter.h

@@ -5,17 +5,13 @@
 #ifndef COMPONENTS_PREFS_PREF_FILTER_H_
 #define COMPONENTS_PREFS_PREF_FILTER_H_
 
-#include <memory>
 #include <string>
 #include <utility>
 
 #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<void(std::unique_ptr<base::DictionaryValue> prefs,
-                              bool schedule_write)>;
+      base::OnceCallback<void(base::Value::Dict prefs, bool schedule_write)>;
 
   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<base::DictionaryValue> 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;

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

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<base::Value::Dict> storage_;
+  const raw_ref<base::Value::Dict> storage_;
 
   // Helper function to get a mutable version of the macs from |storage_|,
   // creating it if needed and |create_if_null| is true.

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<base::DictionaryValue> 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);

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 <memory>
-
 #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<void(std::unique_ptr<base::DictionaryValue> prefs,
-                              bool prefs_altered)>;
+      base::OnceCallback<void(base::Value::Dict prefs, bool prefs_altered)>;
 
   // 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<void(
       FinalizeFilterOnLoadCallback finalize_filter_on_load,
-      std::unique_ptr<base::DictionaryValue> prefs)>;
+      base::Value::Dict prefs)>;
 
   InterceptablePrefFilter();
   ~InterceptablePrefFilter() override;
 
   // PrefFilter partial implementation.
-  void FilterOnLoad(
-      PostFilterOnLoadCallback post_filter_on_load_callback,
-      std::unique_ptr<base::DictionaryValue> 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<base::DictionaryValue> 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

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<base::DictionaryValue> 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<base::DictionaryValue> prefs) {
+                   base::Value::Dict prefs) {
   std::move(finalize_filter_on_load).Run(std::move(prefs), false);
 }
 
 void DeleteFilter(std::unique_ptr<TestInterceptablePrefFilter>* filter,
-                  std::unique_ptr<base::DictionaryValue> prefs,
+                  base::Value::Dict prefs,
                   bool schedule_write) {
   filter->reset();
 }
@@ -47,7 +47,7 @@ TEST(InterceptablePrefFilterTest, CallbackDeletes) {
   auto filter = std::make_unique<TestInterceptablePrefFilter>();
   filter->InterceptNextFilterOnLoad(base::BindOnce(&NoOpIntercept));
   filter->FilterOnLoad(base::BindOnce(&DeleteFilter, &filter),
-                       std::make_unique<base::DictionaryValue>());
+                       base::Value::Dict());
   EXPECT_FALSE(filter);
 }