Fix a race condition in preference metric reporting.

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
new file mode 100644
index 0000000000000000000000000000000000000000..2300a6247c63f1cee6d85bc7a4f48af61dd259fd
--- /dev/null
+++ b/chrome/browser/prefs/pref_hash_filter.cc
@@ -0,0 +1,91 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/prefs/pref_hash_filter.h"
+
+#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "base/prefs/persistent_pref_store.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 NULL.
+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
+  NULL,
+#endif
+  prefs::kExtensionKnownDisabled,
+};
+
+}  // namespace
+
+PrefHashFilter::PrefHashFilter(scoped_ptr<PrefHashStore> pref_hash_store)
+    : pref_hash_store_(pref_hash_store.Pass()) {}
+
+PrefHashFilter::~PrefHashFilter() {}
+
+void PrefHashFilter::FilterUpdate(PersistentPrefStore* pref_store,
+                                  const std::string& path) {
+  const base::Value* value = NULL;
+  pref_store->GetValue(path, &value);
+
+  pref_hash_store_->OnPrefValueChanged(path, value);
+}
+
+void PrefHashFilter::FilterOnLoad(PersistentPrefStore* pref_store) {
+  for (size_t i = 0; i < arraysize(kTrackedPrefs); ++i) {
+    if (kTrackedPrefs[i] == NULL)
+      continue;
+    const base::Value* value = NULL;
+    pref_store->GetValue(kTrackedPrefs[i], &value);
+    PrefHashStore::InitializationResult initialization_result =
+        pref_hash_store_->InitializeTrackedValue(kTrackedPrefs[i], value);

[gab, 2013/12/04 22:22:30]

How does this work in a world where we do more than reporting? Is
InitializeTrackedValue() expected to do the fixups and return an even broader
enum to describe what it did?

I think it will be simpler in the long run if PrefHashFilter and PrefHashStore
are folded in the same class.

Currently PrefHashFilter is pretty much a middle-man class for which most of the
logic just forwarding calls to PrefHashStore and reporting back on findings
(which PrefHashStore could easily do directly).

[erikwright (departed), 2013/12/05 23:48:00]

See my comments elsewhere on the motivation for this factorization. But I've
shifted the responsibility for storing the value from PrefHashStore to
PrefHashFilter.

+
+    switch (initialization_result) {
+      case PrefHashStore::UNCHANGED:
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceUnchanged",
+                                  i, arraysize(kTrackedPrefs));
+        break;
+      case PrefHashStore::CLEARED:
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceCleared",
+                                  i, arraysize(kTrackedPrefs));
+        break;
+      case PrefHashStore::MIGRATED:
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceMigrated",
+                                  i, arraysize(kTrackedPrefs));
+        break;
+      case PrefHashStore::CHANGED:
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChanged",
+                                  i, arraysize(kTrackedPrefs));
+        break;
+      case PrefHashStore::INITIALIZED:
+        UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceInitialized",
+                                  i, arraysize(kTrackedPrefs));
+        break;
+      default:
+        NOTREACHED() << "Unexpected PrefHashStore::InitializationResult: "
+                     << initialization_result;
+        break;
+    }
+  }
+}