From b262d21ea593b101b15518b5d2f0fa8c15460d0f Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Tue, 10 Mar 2026 13:43:48 -0500 Subject: [PATCH] [CodeHealth] SERP metrics (#34602) --- browser/serp_metrics/serp_metrics_tab_helper.cc | 1 + browser/serp_metrics/serp_metrics_tab_helper.h | 1 - .../serp_metrics_tab_helper_browsertest.cc | 3 ++- components/serp_metrics/serp_classifier.cc | 2 +- components/serp_metrics/serp_classifier_unittest.cc | 11 ++++++++++- .../serp_metrics/serp_classifier_utils_unittest.cc | 4 ++-- components/serp_metrics/serp_metrics_unittest.cc | 4 ++-- 7 files changed, 18 insertions(+), 8 deletions(-) diff --git a/browser/serp_metrics/serp_metrics_tab_helper.cc b/browser/serp_metrics/serp_metrics_tab_helper.cc index 0c508dcf8bd..cb52c6f3a4c 100644 --- a/browser/serp_metrics/serp_metrics_tab_helper.cc +++ b/browser/serp_metrics/serp_metrics_tab_helper.cc @@ -107,6 +107,7 @@ void SerpMetricsTabHelper::RecordSearchEngine( } default: { + // All other search engines are intentionally grouped together. serp_metrics_->RecordSearch(SerpMetricType::kOther); break; } diff --git a/browser/serp_metrics/serp_metrics_tab_helper.h b/browser/serp_metrics/serp_metrics_tab_helper.h index ed649cd6a10..2014591bb1f 100644 --- a/browser/serp_metrics/serp_metrics_tab_helper.h +++ b/browser/serp_metrics/serp_metrics_tab_helper.h @@ -33,7 +33,6 @@ class SerpMetricsTabHelper final ~SerpMetricsTabHelper() override; - // static static void MaybeCreateForWebContents(content::WebContents* web_contents); private: diff --git a/browser/serp_metrics/serp_metrics_tab_helper_browsertest.cc b/browser/serp_metrics/serp_metrics_tab_helper_browsertest.cc index 55a97d132ff..604adc38364 100644 --- a/browser/serp_metrics/serp_metrics_tab_helper_browsertest.cc +++ b/browser/serp_metrics/serp_metrics_tab_helper_browsertest.cc @@ -133,7 +133,7 @@ class TestHttpsServerBuilder { http_response->set_code(http_status_code); http_response->set_content_type("text/html"); const auto iter = - override_http_response_content.find(http_request.GetURL().path()); + override_http_response_content.find(http_request.relative_url); if (iter != override_http_response_content.cend()) { // Override the default response content. http_response->set_content(iter->second); @@ -348,6 +348,7 @@ IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, RecordForHttp5xxResponse) { GetWebContents(), https_server->GetURL("www.google.com", "/search?q=test"), /*number_of_navigations=*/1, /*ignore_uncommitted_navigations=*/true); + EXPECT_EQ( 1U, GetSerpMetrics()->GetSearchCountForTesting(SerpMetricType::kGoogle)); } diff --git a/components/serp_metrics/serp_classifier.cc b/components/serp_metrics/serp_classifier.cc index 55eb80ac363..8e15f6fec92 100644 --- a/components/serp_metrics/serp_classifier.cc +++ b/components/serp_metrics/serp_classifier.cc @@ -104,7 +104,7 @@ GURL NormalizeUrl(const GURL& url) { } // namespace bool IsSameSearchQuery(const GURL& lhs, const GURL& rhs) { - if (lhs.host() == kStartpageUrlHost) { + if (lhs.host() == kStartpageUrlHost || rhs.host() == kStartpageUrlHost) { // For Startpage, we cannot determine whether two URLs represent the same // search results page, so these pages are always classified. return false; diff --git a/components/serp_metrics/serp_classifier_unittest.cc b/components/serp_metrics/serp_classifier_unittest.cc index a3fa757f8c5..df656e5b284 100644 --- a/components/serp_metrics/serp_classifier_unittest.cc +++ b/components/serp_metrics/serp_classifier_unittest.cc @@ -53,7 +53,7 @@ TEST(SerpClassifierTest, IsSameSearchQueryWithDifferentParamOrder) { TEST(SerpClassifierTest, IsNotSameSearchQuery) { EXPECT_FALSE(IsSameSearchQuery( GURL(R"(https://search.brave.com/search?q=foo&t=web)"), - GURL(R"(https://search.brave.com/search?q=bar&t=web")"))); + GURL(R"(https://search.brave.com/search?q=bar&t=web)"))); } TEST(SerpClassifierTest, IsNotSameSearchQueryWithInvalidUrl) { @@ -61,6 +61,15 @@ TEST(SerpClassifierTest, IsNotSameSearchQueryWithInvalidUrl) { GURL(R"(https://search.brave.com/search?q=foobar)"), GURL("invalid"))); } +TEST(SerpClassifierTest, IsNotSameSearchQueryWithStartpage) { + EXPECT_FALSE( + IsSameSearchQuery(GURL(R"(https://www.startpage.com/sp/search)"), + GURL(R"(https://www.startpage.com/sp/search)"))); + EXPECT_FALSE( + IsSameSearchQuery(GURL(R"(https://search.brave.com/search?q=foobar)"), + GURL(R"(https://www.startpage.com/sp/search)"))); +} + TEST(SerpClassifierTest, OnlyClassifyAllowedSearchEngines) { for (const auto* prepopulated_engine : TemplateURLPrepopulateData::GetAllPrepopulatedEngines()) { diff --git a/components/serp_metrics/serp_classifier_utils_unittest.cc b/components/serp_metrics/serp_classifier_utils_unittest.cc index 5dea0c67be3..3416b6651df 100644 --- a/components/serp_metrics/serp_classifier_utils_unittest.cc +++ b/components/serp_metrics/serp_classifier_utils_unittest.cc @@ -12,7 +12,7 @@ namespace serp_metrics { TEST(SerpClassifierUtilsTest, IsAllowedSearchEngine) { - constexpr auto kAllowedSearchEngines = + constexpr auto kExpectedSearchEngines = base::MakeFixedFlatSet( base::sorted_unique, {SEARCH_ENGINE_BING, SEARCH_ENGINE_GOOGLE, SEARCH_ENGINE_YAHOO, @@ -23,7 +23,7 @@ TEST(SerpClassifierUtilsTest, IsAllowedSearchEngine) { const SearchEngineType search_engine_type = static_cast(i); EXPECT_EQ(IsAllowedSearchEngine(search_engine_type), - kAllowedSearchEngines.contains(search_engine_type)); + kExpectedSearchEngines.contains(search_engine_type)); } } diff --git a/components/serp_metrics/serp_metrics_unittest.cc b/components/serp_metrics/serp_metrics_unittest.cc index eb4bf9e8784..dcb9500537f 100644 --- a/components/serp_metrics/serp_metrics_unittest.cc +++ b/components/serp_metrics/serp_metrics_unittest.cc @@ -657,13 +657,13 @@ TEST_F(SerpMetricsTest, ClearHistoryDoesNotRestoreClearedSearchCounts) { serp_metrics_->GetSearchCountForTesting(SerpMetricType::kBrave)); AdvanceClockToNextDay(); - // Day 2: Yesterday + // Day 1: Yesterday serp_metrics_->RecordSearch(SerpMetricType::kGoogle); ASSERT_EQ(1U, serp_metrics_->GetSearchCountForTesting(SerpMetricType::kGoogle)); AdvanceClockToNextDay(); - // Day 3: Today + // Day 2: Today serp_metrics_->RecordSearch(SerpMetricType::kOther); ASSERT_EQ(1U, serp_metrics_->GetSearchCountForTesting(SerpMetricType::kOther));