Multi-strategy based tracking.

chrome/browser/prefs/pref_hash_filter.cc

Index: chrome/browser/prefs/pref_hash_filter.cc
diff --git a/chrome/browser/prefs/pref_hash_filter.cc b/chrome/browser/prefs/pref_hash_filter.cc
index d50d66c0e550ad3b6cb8f73994592d1b09a321e6..8376b0359e2a9dd2d470bc9383d34a3f7a59b2a7 100644
--- a/chrome/browser/prefs/pref_hash_filter.cc
+++ b/chrome/browser/prefs/pref_hash_filter.cc
@@ -7,90 +7,179 @@
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/values.h"
-#include "chrome/browser/prefs/pref_hash_store.h"
 #include "chrome/common/pref_names.h"
 
 namespace {
 
-// These preferences must be kept in sync with the TrackedPreference enum in
-// tools/metrics/histograms/histograms.xml. To add a new preference, append it
-// to the array and add a corresponding value to the histogram enum. Replace
-// removed preferences with "".
-const char* kTrackedPrefs[] = {
-  prefs::kShowHomeButton,
-  prefs::kHomePageIsNewTabPage,
-  prefs::kHomePage,
-  prefs::kRestoreOnStartup,
-  prefs::kURLsToRestoreOnStartup,
-  prefs::kExtensionsPref,
-  prefs::kGoogleServicesLastUsername,
-  prefs::kSearchProviderOverrides,
-  prefs::kDefaultSearchProviderSearchURL,
-  prefs::kDefaultSearchProviderKeyword,
-  prefs::kDefaultSearchProviderName,
-#if !defined(OS_ANDROID)
-  prefs::kPinnedTabs,
-#else
-  "",
-#endif
-  prefs::kExtensionKnownDisabled,
-  prefs::kProfileResetPromptMemento,
-};
-
 void ReportValidationResult(PrefHashStore::ValueState value_state,
-                            size_t value_index) {
+                            size_t value_index,
+                            size_t num_values) {
   switch (value_state) {
     case PrefHashStore::UNCHANGED:
       UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceUnchanged",
-                                value_index, arraysize(kTrackedPrefs));
+                                value_index, num_values);
       return;
     case PrefHashStore::CLEARED:
       UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceCleared",
-                                value_index, arraysize(kTrackedPrefs));
+                                value_index, num_values);
       return;
     case PrefHashStore::MIGRATED:
       UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceMigrated",
-                                value_index, arraysize(kTrackedPrefs));
+                                value_index, num_values);
       return;
     case PrefHashStore::CHANGED:
       UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChanged",
-                                value_index, arraysize(kTrackedPrefs));
+                                value_index, num_values);
       return;
     case PrefHashStore::UNKNOWN_VALUE:
       UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceInitialized",
-                                value_index, arraysize(kTrackedPrefs));
+                                value_index, num_values);
       return;
   }
   NOTREACHED() << "Unexpected PrefHashStore::ValueState: " << value_state;
 }
 
+void ReportSplitPreferenceChangedCount(const std::string& path, size_t count) {
+  // Handle each known split preference explicitly as UMA macros result in
+  // static blocks which aren't compatible with dynamically generated names.
+  if (path == prefs::kExtensionsPref) {
+    UMA_HISTOGRAM_COUNTS_100(
+        "Settings.TrackedSplitPreferenceChangedCount.kExtensionsPref",
+        count);
+    return;
+  }
+  // All split preferences need to be handled above.
+  NOTREACHED();
+}
+
 }  // namespace
 
 PrefHashFilter::PrefHashFilter(scoped_ptr<PrefHashStore> pref_hash_store)
-    : pref_hash_store_(pref_hash_store.Pass()),
-      tracked_paths_(kTrackedPrefs, kTrackedPrefs + arraysize(kTrackedPrefs)) {}
+    : pref_hash_store_(pref_hash_store.Pass()) {
+  // These preferences must be kept in sync with the TrackedPreference enum in
+  // tools/metrics/histograms/histograms.xml. To add a new preference, append it
+  // to the array and add a corresponding value to the histogram enum. New split
+  // preferences must also be added to ReportSplitPreferenceChangedCount above.
+  // The index of each tracked preference must not change; replace removed
+  // preferences' strategies with TRACKING_STRATEGY_NONE rather than removing
+  // them.
+  static const TrackedPreference kTrackedPrefs[] = {
+    { prefs::kShowHomeButton, TRACKING_STRATEGY_ATOMIC },

[erikwright (departed), 2013/12/17 02:22:50]

Suggestion: Make the 'reporting id' a member of this struct, removing the
relationship between position and id. And thus removing the requirement of
placeholders for platform-specific values.

Also removing the need for 'strategy none'.

[gab, 2013/12/17 18:08:05]

I also considered this, I don't like that it adds an implicit requirement (which
is hard to verify with a simple DCHECK) that no two TrackedPreferences have the
same ID.

It does make things cleaner though (TRACKING_STRATEGY_NONE was making switches
and tests uglier).

Conversely not having a default "bad" enum value makes the error cases for
checked_value/stored_value in the tests uglier.

Implemented for now, let me know what you prefer (I can easily go back).

+    { prefs::kHomePageIsNewTabPage, TRACKING_STRATEGY_ATOMIC },
+    { prefs::kHomePage, TRACKING_STRATEGY_ATOMIC },
+    { prefs::kRestoreOnStartup, TRACKING_STRATEGY_ATOMIC },
+    { prefs::kURLsToRestoreOnStartup, TRACKING_STRATEGY_ATOMIC },
+    { prefs::kExtensionsPref, TRACKING_STRATEGY_SPLIT },
+    { prefs::kGoogleServicesLastUsername, TRACKING_STRATEGY_ATOMIC },
+    { prefs::kSearchProviderOverrides, TRACKING_STRATEGY_ATOMIC },
+    { prefs::kDefaultSearchProviderSearchURL, TRACKING_STRATEGY_ATOMIC },
+    { prefs::kDefaultSearchProviderKeyword, TRACKING_STRATEGY_ATOMIC },
+    { prefs::kDefaultSearchProviderName, TRACKING_STRATEGY_ATOMIC },
+  #if !defined(OS_ANDROID)
+    { prefs::kPinnedTabs, TRACKING_STRATEGY_ATOMIC },
+  #else
+    { NULL, TRACKING_STRATEGY_NONE },
+  #endif
+    { prefs::kExtensionKnownDisabled, TRACKING_STRATEGY_ATOMIC },
+    { prefs::kProfileResetPromptMemento, TRACKING_STRATEGY_ATOMIC },
+  };
+
+  InitTrackedPreferences(kTrackedPrefs, arraysize(kTrackedPrefs));
+}
+
+PrefHashFilter::PrefHashFilter(
+    scoped_ptr<PrefHashStore> pref_hash_store,
+    const PrefHashFilter::TrackedPreference tracked_preferences[],
+    size_t tracked_preferences_size)
+    : pref_hash_store_(pref_hash_store.Pass()) {
+  InitTrackedPreferences(tracked_preferences, tracked_preferences_size);
+}
 
 PrefHashFilter::~PrefHashFilter() {}
 
 // Updates the stored hash to correspond to the updated preference value.
 void PrefHashFilter::FilterUpdate(const std::string& path,
                                   const base::Value* value) {
-  if (tracked_paths_.find(path) != tracked_paths_.end())
-    pref_hash_store_->StoreHash(path, value);
+  TrackedPreferencesMap::const_iterator it = tracked_paths_.find(path);
+  if (it != tracked_paths_.end())
+    StoreHashBasedOnStrategy(path, value, it->second.strategy);
 }
 
 // Validates loaded preference values according to stored hashes, reports
 // validation results via UMA, and updates hashes in case of mismatch.
 void PrefHashFilter::FilterOnLoad(base::DictionaryValue* pref_store_contents) {
-  for (size_t i = 0; i < arraysize(kTrackedPrefs); ++i) {
-    if (kTrackedPrefs[i][0] == '\0')
-      continue;
+  for (TrackedPreferencesMap::const_iterator it = tracked_paths_.begin();
+       it != tracked_paths_.end(); ++it) {
+    const std::string& pref_path = it->first;
+    const TrackedPreferenceMetaData& metadata = it->second;
     const base::Value* value = NULL;
-    pref_store_contents->Get(kTrackedPrefs[i], &value);
+    pref_store_contents->Get(pref_path, &value);
     PrefHashStore::ValueState value_state =
-        pref_hash_store_->CheckValue(kTrackedPrefs[i], value);
-    ReportValidationResult(value_state, i);
+        CheckValueBasedOnStrategy(pref_path, value, metadata.strategy);
+    ReportValidationResult(value_state, metadata.index, tracked_paths_.size());
     if (value_state != PrefHashStore::UNCHANGED)
-      pref_hash_store_->StoreHash(kTrackedPrefs[i], value);
+      StoreHashBasedOnStrategy(pref_path, value, metadata.strategy);
+  }
+}
+
+void PrefHashFilter::InitTrackedPreferences(
+    const PrefHashFilter::TrackedPreference tracked_preferences[],

[erikwright (departed), 2013/12/17 02:22:50]

verify whether it's more common to use [] or * in parameter types.

[gab, 2013/12/17 18:08:05]

Here are 161 cases where an array is passed as the first argument to a method:
https://code.google.com/p/chromium/codesearch#search/&q=%5BA-Za-z:%5D%2B%5C%2...

It wouldn't be fair to compare that against uses of * since most uses of *
aren't actually arrays...

But there is precedent for taking [] + count variable as input.

I like it because it makes it explicit that you want an array, not just a
pointer to single value.

There appears to only be 7 if I exclude third_party/ though :(
https://code.google.com/p/chromium/codesearch#search/&q=%5BA-Za-z:%5D%2B%5C%2...

I'm wondering whether that's still because codesearch is ignore many packages as
it was yesterday...

I found 46 instances in my local src/ (excluding third_party), using the same
regexp:
[A-Za-z:]* [A-Za-z:]*\((\n *)?const [A-Za-z:]* [a-z_]*\[\]

+    size_t tracked_preferences_size) {
+  for (size_t i = 0; i < tracked_preferences_size; ++i) {
+    if (tracked_preferences[i].strategy == TRACKING_STRATEGY_NONE)
+      continue;
+    bool is_new = tracked_paths_.insert(std::make_pair(
+        std::string(tracked_preferences[i].name),
+        TrackedPreferenceMetaData(i, tracked_preferences[i].strategy))).second;
+    DCHECK(is_new);
+  }
+}
+
+void PrefHashFilter::StoreHashBasedOnStrategy(const std::string& path,
+                                              const base::Value* value,
+                                              PrefTrackingStrategy strategy) {
+  switch (strategy) {
+    case TRACKING_STRATEGY_NONE:
+      NOTREACHED();
+      break;
+    case TRACKING_STRATEGY_ATOMIC:
+      pref_hash_store_->StoreHash(path, value);
+      break;
+    case TRACKING_STRATEGY_SPLIT:
+      const base::DictionaryValue* dict_value = NULL;
+      if (value && !value->GetAsDictionary(&dict_value)) {
+        NOTREACHED();
+        break;

[erikwright (departed), 2013/12/17 02:22:50]

I find this a little foreign (two breaks in a case). Consider alternatives.

[gab, 2013/12/17 18:08:05]

I considered switching the break; for a return; but I don't think it adds any
benefit.

I otherwise don't see how to do it cleanly...

I don't think double-break is any worse than early returns in the same scenario
outside of a switch block.

+      }
+      pref_hash_store_->StoreSplitHash(path, dict_value);
+      break;
+  }

[erikwright (departed), 2013/12/17 02:22:50]

Are unhandled enums handled here too? Or is it only when there's a return value
and thus a path without a return?

[gab, 2013/12/17 18:08:05]

Unhandled enums will throw a couple error on gcc/clang.

There doesn't need to be a NOTREACHED() block after the switch to cover the
"potential path without a return" error in this case since the return type is
void.

+}
+
+PrefHashStore::ValueState PrefHashFilter::CheckValueBasedOnStrategy(
+    const std::string& path,
+    const base::Value* value,
+    PrefTrackingStrategy strategy) {
+  switch (strategy) {
+    case TRACKING_STRATEGY_NONE:
+      NOTREACHED();
+      return PrefHashStore::UNKNOWN_VALUE;
+    case TRACKING_STRATEGY_ATOMIC:
+      return pref_hash_store_->CheckValue(path, value);
+    case TRACKING_STRATEGY_SPLIT:
+      const base::DictionaryValue* dict_value = NULL;
+      if (value && !value->GetAsDictionary(&dict_value)) {
+        NOTREACHED();
+        return PrefHashStore::UNKNOWN_VALUE;
+      }
+
+      std::vector<std::string> invalid_keys;
+      PrefHashStore::ValueState state =
+          pref_hash_store_->CheckSplitValue(path, dict_value, &invalid_keys);
+      if (state == PrefHashStore::CHANGED)
+        ReportSplitPreferenceChangedCount(path, invalid_keys.size());
+      return state;
   }
+  NOTREACHED();
+  return PrefHashStore::UNKNOWN_VALUE;
 }