Pref service: Add a ScopedDictionaryPrefUpdate to track value changes.

services/preferences/persistent_pref_store_impl.cc

Index: services/preferences/persistent_pref_store_impl.cc
diff --git a/services/preferences/persistent_pref_store_impl.cc b/services/preferences/persistent_pref_store_impl.cc
index 95930387654f5c875d845330787287e8bbb2b662..ed0d1c824df963dfc70b274eb89b4487901775ac 100644
--- a/services/preferences/persistent_pref_store_impl.cc
+++ b/services/preferences/persistent_pref_store_impl.cc
@@ -11,7 +11,9 @@
 #include "base/stl_util.h"
 #include "base/values.h"
 #include "components/prefs/persistent_pref_store.h"
+#include "mojo/common/values_struct_traits.h"
 #include "mojo/public/cpp/bindings/binding.h"
+#include "services/preferences/public/cpp/lib/util.h"
 
 namespace prefs {
 
@@ -41,9 +43,7 @@ class PersistentPrefStoreImpl::Connection : public mojom::PersistentPrefStore {
     std::vector<mojom::PrefUpdatePtr> filtered_updates;
     for (const auto& update : updates) {
       if (base::ContainsKey(observed_keys_, update->key)) {
-        filtered_updates.push_back(mojom::PrefUpdate::New(
-            update->key,
-            update->value ? update->value->CreateDeepCopy() : nullptr, 0));
+        filtered_updates.push_back(update->Clone());
       }
     }
     if (!filtered_updates.empty())
@@ -133,11 +133,39 @@ void PersistentPrefStoreImpl::SetValues(
     entry.first->OnPrefValuesChanged(updates);
 
   for (auto& update : updates) {
-    if (update->value) {
-      backing_pref_store_->SetValue(update->key, std::move(update->value),
+    if (update->value->is_atomic_update()) {
+      auto& value = update->value->get_atomic_update();
+      if (value) {
+        backing_pref_store_->SetValue(update->key, value->CreateDeepCopy(),

[tibell, 2017/04/12 07:13:52]

Can't we move out value?

+                                      update->flags);
+      } else {
+        backing_pref_store_->RemoveValue(update->key, update->flags);
+      }
+      continue;
+    }
+
+    DCHECK(update->value->is_split_updates());

[tibell, 2017/04/12 07:13:52]

I'd prefer that this was in an else block without the continue statement above.
If that's too much nesting pull it out into a helper function/method.

+    base::Value* mutable_value = nullptr;
+    base::DictionaryValue* dictionary_value = nullptr;
+    std::unique_ptr<base::DictionaryValue> pending_dictionary;
+    if (!backing_pref_store_->GetMutableValue(update->key, &mutable_value) ||
+        !mutable_value->GetAsDictionary(&dictionary_value)) {
+      pending_dictionary = base::MakeUnique<base::DictionaryValue>();
+      dictionary_value = pending_dictionary.get();
+    }
+    for (auto& split_update : update->value->get_split_updates()) {
+      if (split_update->path.empty())
+        continue;
+
+      SetValue(dictionary_value,
+               {split_update->path.begin(), split_update->path.end()},
+               std::move(split_update->value));
+    }
+    if (pending_dictionary) {
+      backing_pref_store_->SetValue(update->key, std::move(pending_dictionary),
                                     update->flags);
     } else {
-      backing_pref_store_->RemoveValue(update->key, update->flags);
+      backing_pref_store_->ReportValueChanged(update->key, update->flags);
     }
   }
 }