Merge pull request #10625 from brave/p3a-search-switch-1.32.x

Uplift P3A search engine switch P3A to 1.32.x

browser/search_engines/search_engine_tracker.cc

@@ -10,9 +10,14 @@
 #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 {
 
+// Preference name switch events are stored under.
+constexpr char kSwitchSearchEngineP3AStorage[] =
+    "brave.search.p3a_default_switch";
+
 // Deduces the search engine from |type|, if nothing is found - from |url|.
 // Not all engines added by Brave are present in |SearchEngineType| enumeration.
 void RecordSearchEngineP3A(const GURL& search_engine_url,
@@ -44,6 +49,39 @@ void RecordSearchEngineP3A(const GURL& search_engine_url,
   UMA_HISTOGRAM_ENUMERATION(kDefaultSearchEngineMetric, answer);
 }
 
+SearchEngineSwitchP3A SearchEngineSwitchP3AMapAnswer(const GURL& to,
+                                                     const GURL& from) {
+  SearchEngineSwitchP3A answer;
+
+  DCHECK(from.is_valid());
+  DCHECK(to.is_valid());
+
+  if (from.DomainIs("brave.com")) {
+    // Switching away from Brave Search.
+    if (to.DomainIs("google.com")) {
+      answer = SearchEngineSwitchP3A::kBraveToGoogle;
+    } else if (to.DomainIs("duckduckgo.com")) {
+      answer = SearchEngineSwitchP3A::kBraveToDDG;
+    } else {
+      answer = SearchEngineSwitchP3A::kBraveToOther;
+    }
+  } else if (to.DomainIs("brave.com")) {
+    // Switching to Brave Search.
+    if (from.DomainIs("google.com")) {
+      answer = SearchEngineSwitchP3A::kGoogleToBrave;
+    } else if (from.DomainIs("duckduckgo.com")) {
+      answer = SearchEngineSwitchP3A::kDDGToBrave;
+    } else {
+      answer = SearchEngineSwitchP3A::kOtherToBrave;
+    }
+  } else {
+    // Any other transition.
+    answer = SearchEngineSwitchP3A::kOtherToOther;
+  }
+
+  return answer;
+}
+
 }  // namespace
 
 // static
@@ -62,10 +100,12 @@ 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);
+  auto* user_prefs = profile->GetPrefs();
+  if (template_url_service && user_prefs) {
+    return new SearchEngineTracker(template_url_service, user_prefs);
   }
   return nullptr;
 }
@@ -74,9 +114,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();
@@ -89,7 +136,9 @@ SearchEngineTracker::SearchEngineTracker(
     const GURL url = template_url->GenerateSearchURL(search_terms);
     if (!url.is_empty()) {
       default_search_url_ = url;
+      previous_search_url_ = url;
       RecordSearchEngineP3A(url, template_url->GetEngineType(search_terms));
+      RecordSwitchP3A(url);
     }
   }
 }
@@ -106,5 +155,27 @@ void SearchEngineTracker::OnTemplateURLServiceChanged() {
     if (url != default_search_url_) {
       RecordSearchEngineP3A(url, template_url->GetEngineType(search_terms));
     }
+    RecordSwitchP3A(url);
+  }
+}
+
+void SearchEngineTracker::RecordSwitchP3A(const GURL& url) {
+  // 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 answer = SearchEngineSwitchP3A::kNoSwitch;
+  auto last = switch_record_.GetLatest();
+  if (last) {
+    answer = static_cast<SearchEngineSwitchP3A>(last.value());
+    DCHECK(answer <= SearchEngineSwitchP3A::kMaxValue);
   }
+
+  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(static_cast<int>(answer));
+  }
+
+  UMA_HISTOGRAM_ENUMERATION(kSwitchSearchEngineMetric, answer);
 }

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"
@@ -17,6 +18,7 @@
 
 // Exposed for tests.
 constexpr char kDefaultSearchEngineMetric[] = "Brave.Search.DefaultEngine.4";
+constexpr char kSwitchSearchEngineMetric[] = "Brave.Search.SwitchEngine";
 
 // Note: append-only enumeration! Never remove any existing values, as this enum
 // is used to bucket a UMA histogram, and removing values breaks that.
@@ -33,6 +35,20 @@ enum class SearchEngineP3A {
   kMaxValue = kBrave,
 };
 
+// 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,
+  kBraveToDDG,
+  kBraveToOther,
+  kGoogleToBrave,
+  kDDGToBrave,
+  kOtherToBrave,
+  kOtherToOther,
+  kMaxValue = kOtherToOther,
+};
+
 class SearchEngineTrackerFactory : public BrowserContextKeyedServiceFactory {
  public:
   static SearchEngineTrackerFactory* GetInstance();
@@ -50,13 +66,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;
@@ -69,8 +89,12 @@ class SearchEngineTracker : public KeyedService,
   base::ScopedObservation<TemplateURLService, TemplateURLServiceObserver>
       observer_{this};
 
+  void RecordSwitchP3A(const GURL& url);
+
   // Keeping this to check for changes in |OnTemplateURLServiceChanged|.
   GURL default_search_url_;
+  GURL previous_search_url_;
+  WeeklyEventStorage switch_record_;
 
   TemplateURLService* template_url_service_;
 };

browser/search_engines/search_engine_tracker_browsertest.cc

@@ -14,24 +14,45 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/search_test_utils.h"
+#include "components/country_codes/country_codes.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/search_engines/template_url_prepopulate_data.h"
 #include "content/public/test/browser_test.h"
 
 class SearchEngineProviderP3ATest : public InProcessBrowserTest {
  public:
   SearchEngineProviderP3ATest() {
     histogram_tester_.reset(new base::HistogramTester);
+
+    // Override the default region. Defaults vary and this
+    // ties the expected test results to a specific region
+    // so everyone sees the same behaviour.
+    //
+    // May also help with unstable test results in ci.
+    create_services_subscription_ =
+        BrowserContextDependencyManager::GetInstance()
+            ->RegisterCreateServicesCallbackForTesting(
+                base::BindRepeating(&OverrideCountryID, "US"));
+  }
+
+ private:
+  static void OverrideCountryID(const std::string& country_id,
+                                content::BrowserContext* context) {
+    const int32_t id = country_codes::CountryCharsToCountryID(country_id.at(0),
+                                                              country_id.at(1));
+    Profile::FromBrowserContext(context)->GetPrefs()->SetInteger(
+        country_codes::kCountryIDAtInstall, id);
   }
 
  protected:
   std::unique_ptr<base::HistogramTester> histogram_tester_;
+  base::CallbackListSubscription create_services_subscription_;
 };
 
-IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest,
-                       DISABLED_DefaultSearchEngineP3A) {
+IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest, DefaultSearchEngineP3A) {
   // Check that the metric is reported on startup.
   histogram_tester_->ExpectUniqueSample(kDefaultSearchEngineMetric,
-                                        SearchEngineP3A::kGoogle, 1);
+                                        SearchEngineP3A::kBrave, 1);
 
   auto* service =
       TemplateURLServiceFactory::GetForProfile(browser()->profile());
@@ -55,3 +76,56 @@ IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest,
 
   histogram_tester_->ExpectTotalCount(kDefaultSearchEngineMetric, 2);
 }
+
+IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest, SwitchSearchEngineP3A) {
+  // Check that the metric is reported on startup.
+  // For some reason we record kNoSwitch twice, even though
+  // kDefaultSearchEngineMetric is only updated once at this point.
+  histogram_tester_->ExpectUniqueSample(kSwitchSearchEngineMetric,
+                                        SearchEngineSwitchP3A::kNoSwitch, 2);
+
+  // Load service for switching the default search engine.
+  auto* service =
+      TemplateURLServiceFactory::GetForProfile(browser()->profile());
+  search_test_utils::WaitForTemplateURLServiceToLoad(service);
+
+  // Check that changing the default engine triggers emission of a new value.
+  auto ddg_data = TemplateURLPrepopulateData::GetPrepopulatedEngine(
+      browser()->profile()->GetPrefs(),
+      TemplateURLPrepopulateData::PREPOPULATED_ENGINE_ID_DUCKDUCKGO);
+  TemplateURL ddg_url(*ddg_data);
+
+  service->SetUserSelectedDefaultSearchProvider(&ddg_url);
+  // This assumes Brave Search is the default!
+  histogram_tester_->ExpectBucketCount(kSwitchSearchEngineMetric,
+                                       SearchEngineSwitchP3A::kBraveToDDG, 1);
+
+  // Check additional changes.
+  auto brave_data = TemplateURLPrepopulateData::GetPrepopulatedEngine(
+      browser()->profile()->GetPrefs(),
+      TemplateURLPrepopulateData::PREPOPULATED_ENGINE_ID_BRAVE);
+  TemplateURL brave_url(*brave_data);
+
+  service->SetUserSelectedDefaultSearchProvider(&brave_url);
+  histogram_tester_->ExpectBucketCount(kSwitchSearchEngineMetric,
+                                       SearchEngineSwitchP3A::kDDGToBrave, 1);
+
+  // Check additional changes.
+  auto bing_data = TemplateURLPrepopulateData::GetPrepopulatedEngine(
+      browser()->profile()->GetPrefs(),
+      TemplateURLPrepopulateData::PREPOPULATED_ENGINE_ID_BING);
+  TemplateURL bing_url(*bing_data);
+
+  service->SetUserSelectedDefaultSearchProvider(&bing_url);
+  histogram_tester_->ExpectBucketCount(kSwitchSearchEngineMetric,
+                                       SearchEngineSwitchP3A::kBraveToOther, 1);
+
+  // Check that incognito or TOR profiles do not emit the metric.
+  histogram_tester_->ExpectTotalCount(kSwitchSearchEngineMetric, 5);
+  CreateIncognitoBrowser();
+#if BUILDFLAG(ENABLE_TOR)
+  brave::NewOffTheRecordWindowTor(browser());
+#endif
+
+  histogram_tester_->ExpectTotalCount(kSwitchSearchEngineMetric, 5);
+}

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",

components/p3a/brave_p3a_service.cc

@@ -82,6 +82,7 @@ constexpr const char* kCollectedHistograms[] = {
     "Brave.Rewards.WalletState",
     "Brave.Savings.BandwidthSavingsMB",
     "Brave.Search.DefaultEngine.4",
+    "Brave.Search.SwitchEngine",
     "Brave.Shields.UsageStatus",
     "Brave.SpeedReader.Enabled",
     "Brave.SpeedReader.ToggleCount",

components/weekly_storage/BUILD.gn

@@ -2,6 +2,8 @@ static_library("weekly_storage") {
   sources = [
     "daily_storage.cc",
     "daily_storage.h",
+    "weekly_event_storage.cc",
+    "weekly_event_storage.h",
     "weekly_storage.cc",
     "weekly_storage.h",
   ]