SearchEngineTracker: Remember switches for a week.

Use WeeklyEventStorage to keep track of default search engine changes over the previous week, so we can return an accurate report of whether a switch happened during the P3A measurement period. This registers a new pref to back the WeeklyEventStorage data.
brave · Oct 13, 2021 · ca79451 · ca79451
1 parent dbd01e1
commit ca79451
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 8 deletions.
diff --git a/browser/search_engines/search_engine_tracker.cc b/browser/search_engines/search_engine_tracker.cc
@@ -10,6 +10,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/pref_registry/pref_registry_syncable.h"
 
 namespace {
 
@@ -95,10 +96,14 @@ SearchEngineTrackerFactory::~SearchEngineTrackerFactory() {}
 
 KeyedService* SearchEngineTrackerFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  auto* template_url_service = TemplateURLServiceFactory::GetForProfile(
-      Profile::FromBrowserContext(context));
-  if (template_url_service) {
-    return new SearchEngineTracker(template_url_service);
+  auto* profile = Profile::FromBrowserContext(context);
+  auto* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(profile);
+  // FIXME(rillian) Since we're a KeyedService, do we need to declare a
+  // dependency on PrefService?
+  auto* user_prefs = profile->GetPrefs();
+  if (template_url_service && user_prefs) {
+    return new SearchEngineTracker(template_url_service, user_prefs);
   }
   return nullptr;
 }
@@ -107,9 +112,16 @@ bool SearchEngineTrackerFactory::ServiceIsCreatedWithBrowserContext() const {
   return true;
 }
 
+void SearchEngineTrackerFactory::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterListPref(kSwitchSearchEngineP3AStorage);
+}
+
 SearchEngineTracker::SearchEngineTracker(
-    TemplateURLService* template_url_service)
-    : template_url_service_(template_url_service) {
+    TemplateURLService* template_url_service,
+    PrefService* user_prefs)
+    : switch_record_(user_prefs, kSwitchSearchEngineP3AStorage),
+      template_url_service_(template_url_service) {
   observer_.Observe(template_url_service_);
   const TemplateURL* template_url =
       template_url_service_->GetDefaultSearchProvider();
@@ -146,11 +158,17 @@ void SearchEngineTracker::OnTemplateURLServiceChanged() {
 }
 
 void SearchEngineTracker::RecordSwitchP3A(const GURL& url) {
-  auto answer = SearchEngineSwitchP3A::kNoSwitch;
+  // Default to the last recorded switch so when we're called
+  // at start-up we initialize the histogram with whatever we
+  // remember from the previous run.
+  auto last = switch_record_.GetLatest();
+  auto answer = last.value_or(SearchEngineSwitchP3A::kNoSwitch);
 
   if (url.is_valid() && url != previous_search_url_) {
+    // The default url has been switched, record that instead.
     answer = SearchEngineSwitchP3AMapAnswer(url, previous_search_url_);
     previous_search_url_ = url;
+    switch_record_.Add(answer);
   }
 
   UMA_HISTOGRAM_ENUMERATION(kSwitchSearchEngineMetric, answer);

diff --git a/browser/search_engines/search_engine_tracker.h b/browser/search_engines/search_engine_tracker.h
@@ -8,6 +8,7 @@
 
 #include "base/memory/singleton.h"
 #include "base/scoped_observation.h"
+#include "brave/components/weekly_storage/weekly_event_storage.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/prefs/pref_member.h"
@@ -34,6 +35,12 @@ enum class SearchEngineP3A {
   kMaxValue = kBrave,
 };
 
+// Preference name switch events are stored under.
+constexpr char kSwitchSearchEngineP3AStorage[] =
+    "brave.search.p3a_default_switch";
+
+// Note: append-only enumeration! Never remove any existing values, as this enum
+// is used to bucket a UMA histogram, and removing values breaks that.
 enum class SearchEngineSwitchP3A {
   kNoSwitch,
   kBraveToGoogle,
@@ -63,13 +70,17 @@ class SearchEngineTrackerFactory : public BrowserContextKeyedServiceFactory {
   KeyedService* BuildServiceInstanceFor(
       content::BrowserContext* context) const override;
   bool ServiceIsCreatedWithBrowserContext() const override;
+
+  void RegisterProfilePrefs(
+      user_prefs::PrefRegistrySyncable* registry) override;
 };
 
 // Records P3A metrics when default search engine changes.
 class SearchEngineTracker : public KeyedService,
                             public TemplateURLServiceObserver {
  public:
-  explicit SearchEngineTracker(TemplateURLService* template_url_service);
+  SearchEngineTracker(TemplateURLService* template_url_service,
+                      PrefService* user_prefs);
   ~SearchEngineTracker() override;
 
   SearchEngineTracker(const SearchEngineTracker&) = delete;
@@ -87,6 +98,7 @@ class SearchEngineTracker : public KeyedService,
   // Keeping this to check for changes in |OnTemplateURLServiceChanged|.
   GURL default_search_url_;
   GURL previous_search_url_;
+  WeeklyEventStorage<SearchEngineSwitchP3A> switch_record_;
 
   TemplateURLService* template_url_service_;
 };

diff --git a/browser/search_engines/sources.gni b/browser/search_engines/sources.gni
@@ -31,6 +31,7 @@ if (is_android) {
 brave_browser_search_engines_deps = [
   "//brave/browser/profiles:util",
   "//brave/common:pref_names",
+  "//brave/components/weekly_storage",
   "//chrome/browser/profiles:profile",
   "//components/keyed_service/content",
   "//components/pref_registry",