[CodeHealth] SERP metrics (#34602)

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;
     }

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:

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));
 }

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;

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()) {

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<SearchEngineType>(
           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<SearchEngineType>(i);
     EXPECT_EQ(IsAllowedSearchEngine(search_engine_type),
-              kAllowedSearchEngines.contains(search_engine_type));
+              kExpectedSearchEngines.contains(search_engine_type));
   }
 }
 

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));