Integrate registry_hash_store_contents with the rest of tracked prefs.

components/user_prefs/tracked/pref_hash_filter.cc

Index: components/user_prefs/tracked/pref_hash_filter.cc
diff --git a/components/user_prefs/tracked/pref_hash_filter.cc b/components/user_prefs/tracked/pref_hash_filter.cc
index 2f0c6e71f5f38fb0f878f5d0c3c64fb2207efe83..f0dac5255a84b7c783a5f931673951eec9ce5777 100644
--- a/components/user_prefs/tracked/pref_hash_filter.cc
+++ b/components/user_prefs/tracked/pref_hash_filter.cc
@@ -8,8 +8,11 @@
 #include <algorithm>
 #include <utility>
 
+#include "base/bind.h"
+#include "base/callback.h"

[gab, 2016/08/08 04:37:45]

callback.h is already in header, remove here

[proberge, 2016/08/31 17:30:16]

Done.

 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
@@ -51,12 +54,18 @@ void CleanupDeprecatedTrackedPreferences(
 
 PrefHashFilter::PrefHashFilter(
     std::unique_ptr<PrefHashStore> pref_hash_store,
+    std::unique_ptr<PrefHashStore> external_validation_hash_store,
+    std::unique_ptr<HashStoreContents> external_validation_hash_store_contents,
     const std::vector<TrackedPreferenceMetadata>& tracked_preferences,
     const base::Closure& on_reset_on_load,
     TrackedPreferenceValidationDelegate* delegate,
     size_t reporting_ids_count,
     bool report_super_mac_validity)
     : pref_hash_store_(std::move(pref_hash_store)),
+      external_validation_hash_store_(
+          std::move(external_validation_hash_store)),
+      external_validation_hash_store_contents_(
+          external_validation_hash_store_contents.release()),

[gab, 2016/08/08 04:37:45]

Why release? Also std::move this one.

[proberge, 2016/08/31 17:30:16]

Done.

       on_reset_on_load_(on_reset_on_load),
       report_super_mac_validity_(report_super_mac_validity) {
   DCHECK(pref_hash_store_);
@@ -132,14 +141,14 @@ void PrefHashFilter::ClearResetTime(PrefService* user_prefs) {
 }
 
 void PrefHashFilter::Initialize(base::DictionaryValue* pref_store_contents) {
+  DictionaryHashStoreContents dictionary_contents(pref_store_contents);

[gab, 2016/08/08 04:37:45]

I like this, glad it's not refcounted after all :-)

[proberge, 2016/08/31 17:30:16]

Acknowledged.

   std::unique_ptr<PrefHashStoreTransaction> hash_store_transaction(
-      pref_hash_store_->BeginTransaction(std::unique_ptr<HashStoreContents>(
-          new DictionaryHashStoreContents(pref_store_contents))));
+      pref_hash_store_->BeginTransaction(&dictionary_contents));

[gab, 2016/08/08 04:37:45]

w.r.t to splitting this CL, this API change (raw pointer in BeginTransaction()
is a good candidate for another precursor CL.

[proberge, 2016/08/31 17:30:16]

Acknowledged.

   for (TrackedPreferencesMap::const_iterator it = tracked_paths_.begin();
        it != tracked_paths_.end(); ++it) {
     const std::string& initialized_path = it->first;
     const TrackedPreference* initialized_preference = it->second;
-    const base::Value* value = NULL;
+    const base::Value* value = nullptr;
     pref_store_contents->Get(initialized_path, &value);
     initialized_preference->OnNewValue(value, hash_store_transaction.get());
   }
@@ -156,30 +165,47 @@ void PrefHashFilter::FilterUpdate(const std::string& path) {
 // Updates the stored hashes for |changed_paths_| before serializing data to
 // disk. This is required as storing the hash everytime a pref's value changes
 // is too expensive (see perf regression @ http://crbug.com/331273).
-void PrefHashFilter::FilterSerializeData(
+base::Callback<void(bool)> PrefHashFilter::FilterSerializeData(
     base::DictionaryValue* pref_store_contents) {
+  // Generate the post-write callback before modifying any hashes.

[gab, 2016/08/08 04:37:46]

s/hashes/MACs/

Also, does that order actually matter? It's the dictionary store's MACs which
are updated, but that shouldn't affect the prefs the callback is based on.

[proberge, 2016/08/31 17:30:16]

Edited the comment to better explain that changed_paths_ is what matters.

+  base::Callback<void(bool)> callback =
+      GetOnWriteSynchronousCallback(pref_store_contents);
+
   if (!changed_paths_.empty()) {
     base::TimeTicks checkpoint = base::TimeTicks::Now();
     {
+      DictionaryHashStoreContents dictionary_contents(pref_store_contents);
       std::unique_ptr<PrefHashStoreTransaction> hash_store_transaction(
-          pref_hash_store_->BeginTransaction(std::unique_ptr<HashStoreContents>(
-              new DictionaryHashStoreContents(pref_store_contents))));
+          pref_hash_store_->BeginTransaction(&dictionary_contents));
+
+      std::unique_ptr<PrefHashStoreTransaction>
+          external_validation_hash_store_transaction =
+              external_validation_hash_store_ &&
+                      external_validation_hash_store_contents_

[gab, 2016/08/08 04:37:46]

It's kind of weird to have those separate isn't it (been thinking this since
constructor but now this double if is confirming it).

How about we make the member variable a:

using StoreContentsPair = std::pair<std::unique_ptr<PrefHashStore>, 
                                    std::unique_ptr<HashStoreContents>>;

const base::Optional<StoreContentsPair> external_validation_hash_store_pair_;

and have constructor take a:

StoreContentsPair* external_validation_hash_store_pair,

and in impl:

external_validation_hash_store_pair_(
    external_validation_hash_store_pair 
        ? make_optional<StoreContentsPair>(
              std::move(external_validation_hash_store_pair))
        : base::Optional::nullopt_t)

then here you can:

std::unique_ptr<PrefHashStoreTransaction>
external_validation_hash_store_transaction;
if(external_validation_hash_store_pair) {
  external_validation_hash_store_transaction = 
      external_validation_hash_store_pair->first->BeginTransaction(
          external_validation_hash_store_pair->second.get());
}

I guess it's not much less code, but having them go together as a pair makes
more sense as that's really what they are (the main reason they're split in the
DictionaryHashStoreContents use-case is that the |pref_store_contents| is
obtained from the outside and is different on every call -- in this case we own
the contents so...).
Another option would be to have an ExternalPrefHashStore which just embeds a
HashStoreContents for use-cases like any other store I can imagine where the
data is owned by it not by its caller -- but I think just keeping it as a pair
is good enough for now per this being a limited use case.

[proberge, 2016/08/31 17:30:16]

Done, but the raw pointer in the PrefHashFilter constructor is making me
uncomfortable. Can you double-check that I'm not leaking any memory? I can't
tell if ownership is probably passed to pref_hash_filter.

Would it make sense to pass in a std::unique_ptr<StoreContentsPair> instead of a
raw pointer?

+                  ? external_validation_hash_store_->BeginTransaction(
+                        external_validation_hash_store_contents_.get())
+                  : nullptr;
+
       for (ChangedPathsMap::const_iterator it = changed_paths_.begin();
            it != changed_paths_.end(); ++it) {
         const std::string& changed_path = it->first;
         const TrackedPreference* changed_preference = it->second;
-        const base::Value* value = NULL;
+        const base::Value* value = nullptr;
         pref_store_contents->Get(changed_path, &value);
         changed_preference->OnNewValue(value, hash_store_transaction.get());
+
+        // Clear prefs in the external store. They will be repopulated when

[gab, 2016/08/08 04:37:46]

// Clear MACs for modified prefs in the external store. (...)

[proberge, 2016/08/31 17:30:16]

Done.

+        // |callback| is run.
+        if (external_validation_hash_store_transaction)
+          external_validation_hash_store_transaction->ClearHash(changed_path);
       }
       changed_paths_.clear();
     }
-    // TODO(gab): Remove this histogram by Feb 21 2014; after sufficient timing
-    // data has been gathered from the wild to be confident this doesn't
-    // significantly affect performance on the UI thread.
     UMA_HISTOGRAM_TIMES("Settings.FilterSerializeDataTime",
                         base::TimeTicks::Now() - checkpoint);
   }
+
+  return callback;
 }
 
 void PrefHashFilter::FinalizeFilterOnLoad(
@@ -191,9 +217,17 @@ void PrefHashFilter::FinalizeFilterOnLoad(
 
   bool did_reset = false;
   {
+    DictionaryHashStoreContents dictionary_contents(pref_store_contents.get());
     std::unique_ptr<PrefHashStoreTransaction> hash_store_transaction(
-        pref_hash_store_->BeginTransaction(std::unique_ptr<HashStoreContents>(
-            new DictionaryHashStoreContents(pref_store_contents.get()))));
+        pref_hash_store_->BeginTransaction(&dictionary_contents));
+
+    std::unique_ptr<PrefHashStoreTransaction>
+        external_validation_hash_store_transaction =
+            external_validation_hash_store_ &&
+                    external_validation_hash_store_contents_
+                ? external_validation_hash_store_->BeginTransaction(
+                      external_validation_hash_store_contents_.get())
+                : nullptr;
 
     CleanupDeprecatedTrackedPreferences(
         pref_store_contents.get(), hash_store_transaction.get());
@@ -205,8 +239,9 @@ void PrefHashFilter::FinalizeFilterOnLoad(
 
     for (TrackedPreferencesMap::const_iterator it = tracked_paths_.begin();
          it != tracked_paths_.end(); ++it) {
-      if (it->second->EnforceAndReport(pref_store_contents.get(),
-                                       hash_store_transaction.get())) {
+      if (it->second->EnforceAndReport(
+              pref_store_contents.get(), hash_store_transaction.get(),
+              external_validation_hash_store_transaction.get())) {
         did_reset = true;
         prefs_altered = true;
       }
@@ -225,12 +260,84 @@ void PrefHashFilter::FinalizeFilterOnLoad(
       on_reset_on_load_.Run();
   }
 
-  // TODO(gab): Remove this histogram by Feb 21 2014; after sufficient timing
-  // data has been gathered from the wild to be confident this doesn't
-  // significantly affect startup.
   UMA_HISTOGRAM_TIMES("Settings.FilterOnLoadTime",
                       base::TimeTicks::Now() - checkpoint);
 
   post_filter_on_load_callback.Run(std::move(pref_store_contents),
                                    prefs_altered);
 }
+
+// static
+void PrefHashFilter::FlushToExternalStore(
+    std::unique_ptr<HashStoreContents> external_validation_hash_store_contents,
+    std::unique_ptr<base::DictionaryValue> changed_paths_and_macs,
+    bool write_success) {
+  DCHECK(!changed_paths_and_macs->empty());
+  DCHECK(external_validation_hash_store_contents);
+  if (!write_success)
+    return;
+
+  for (base::DictionaryValue::Iterator it(*changed_paths_and_macs);
+       !it.IsAtEnd(); it.Advance()) {
+    const std::string& changed_path = it.key();
+
+    const base::DictionaryValue* split_values = nullptr;
+    if (it.value().GetAsDictionary(&split_values)) {
+      for (base::DictionaryValue::Iterator inner_it(*split_values);
+           !inner_it.IsAtEnd(); inner_it.Advance()) {
+        std::string mac;
+        bool is_string = inner_it.value().GetAsString(&mac);
+        DCHECK(is_string);
+
+        external_validation_hash_store_contents->SetSplitMac(
+            changed_path, inner_it.key(), mac);
+      }
+    } else {
+      const base::StringValue* value_as_string;
+      bool is_string = it.value().GetAsString(&value_as_string);
+      DCHECK(is_string);
+
+      external_validation_hash_store_contents->SetMac(
+          changed_path, value_as_string->GetString());
+    }
+  }
+}
+
+base::Callback<void(bool)> PrefHashFilter::GetOnWriteSynchronousCallback(
+    base::DictionaryValue* pref_store_contents) {
+  if (changed_paths_.empty() || !external_validation_hash_store_ ||
+      !external_validation_hash_store_contents_) {
+    return base::Callback<void(bool)>();
+  }
+
+  std::unique_ptr<base::DictionaryValue> changed_paths_macs(
+      new base::DictionaryValue);
+
+  for (ChangedPathsMap::const_iterator it = changed_paths_.begin();
+       it != changed_paths_.end(); ++it) {
+    const std::string& changed_path = it->first;
+    const TrackedPreference* changed_preference = it->second;
+    const base::Value* value = nullptr;

[gab, 2016/08/08 04:37:45]

s/value/new_value/

[proberge, 2016/08/31 17:30:16]

Done.

+    pref_store_contents->Get(changed_path, &value);
+
+    if (changed_preference->GetType() == TrackedPreferenceType::ATOMIC) {

[gab, 2016/08/08 04:37:45]

Use a switch() statement with no default case so that compile breaks here should
this enum ever be expanded (and then you don't need a NOTREACHED() :-))

[proberge, 2016/08/31 17:30:16]

Done.

+      changed_paths_macs->SetStringWithoutPathExpansion(
+          changed_path,
+          external_validation_hash_store_->ComputeMac(changed_path, value));
+    } else if (changed_preference->GetType() == TrackedPreferenceType::SPLIT) {
+      const base::DictionaryValue* dict_value = nullptr;
+      if (value && value->GetAsDictionary(&dict_value)) {
+        changed_paths_macs->SetWithoutPathExpansion(
+            changed_path, external_validation_hash_store_->ComputeSplitMacs(
+                              changed_path, dict_value));
+      }
+    } else {
+      NOTREACHED() << "Unsupported TrackedPreferenceType";
+    }
+  }
+
+  return base::Bind(

[gab, 2016/08/08 04:37:45]

Just above this add:

DCHECK(external_validation_hash_store_contents_->IsCopyable()) 
    << "External HashStoreContents must be copyable at it needs to be used "
    << "off-thread.";

[proberge, 2016/08/31 17:30:16]

Done.

+      &FlushToExternalStore,
+      base::Passed(external_validation_hash_store_contents_->MakeCopy()),
+      base::Passed(&changed_paths_macs));
+}