diff --git a/browser/search_engines/search_engine_tracker.cc b/browser/search_engines/search_engine_tracker.cc index a0e07bf8cbec..068c836bd6d3 100644 --- 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 index 5f49ad7e9fd6..dc36e907c1e4 100644 --- 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 switch_record_; TemplateURLService* template_url_service_; }; diff --git a/browser/search_engines/sources.gni b/browser/search_engines/sources.gni index bfb3a9a401b2..6dcae544d901 100644 --- 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",