From a86c7f70bdffbe9dfc4d48fa3a33e7375a7075f3 Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Mon, 9 Mar 2026 11:21:51 -0500 Subject: [PATCH] [CodeHealth] SERP metrics (#34518) As part of general code health improvements, apply IWYU fixes, replace SerpClassifier with free functions, improve TestHttpsServerBuilder, update tests whose behavior did not match our expectations, and deduplicate the search engine allowlist. --- ...etrics_all_profiles_aggregator_unittest.cc | 1 - .../serp_metrics/serp_metrics_tab_helper.cc | 20 +- .../serp_metrics/serp_metrics_tab_helper.h | 5 +- .../serp_metrics_tab_helper_browsertest.cc | 172 ++++++++++-------- components/serp_metrics/BUILD.gn | 3 + components/serp_metrics/serp_classifier.cc | 87 +++++---- components/serp_metrics/serp_classifier.h | 35 +--- .../serp_metrics/serp_classifier_unittest.cc | 68 +++---- .../serp_metrics/serp_classifier_utils.cc | 26 +++ .../serp_metrics/serp_classifier_utils.h | 18 ++ .../serp_classifier_utils_unittest.cc | 30 +++ 11 files changed, 264 insertions(+), 201 deletions(-) create mode 100644 components/serp_metrics/serp_classifier_utils.cc create mode 100644 components/serp_metrics/serp_classifier_utils.h create mode 100644 components/serp_metrics/serp_classifier_utils_unittest.cc diff --git a/browser/serp_metrics/serp_metrics_all_profiles_aggregator_unittest.cc b/browser/serp_metrics/serp_metrics_all_profiles_aggregator_unittest.cc index de2595c0a13..d7eddd70cf4 100644 --- a/browser/serp_metrics/serp_metrics_all_profiles_aggregator_unittest.cc +++ b/browser/serp_metrics/serp_metrics_all_profiles_aggregator_unittest.cc @@ -15,7 +15,6 @@ #include "components/prefs/pref_registry_simple.h" #include "components/prefs/testing_pref_service.h" #include "content/public/test/browser_task_environment.h" -#include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" namespace serp_metrics { diff --git a/browser/serp_metrics/serp_metrics_tab_helper.cc b/browser/serp_metrics/serp_metrics_tab_helper.cc index fe43bb28ce2..0c508dcf8bd 100644 --- a/browser/serp_metrics/serp_metrics_tab_helper.cc +++ b/browser/serp_metrics/serp_metrics_tab_helper.cc @@ -18,12 +18,10 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/profiles/profile.h" #include "components/prefs/pref_service.h" -#include "components/search_engines/search_engine_type.h" #include "components/search_engines/search_engine_utils.h" #include "content/public/browser/navigation_handle.h" #include "content/public/browser/web_contents.h" #include "ui/base/page_transition_types.h" -#include "url/gurl.h" namespace serp_metrics { @@ -67,21 +65,19 @@ SerpMetricsTabHelper::~SerpMetricsTabHelper() = default; /////////////////////////////////////////////////////////////////////////////// -bool SerpMetricsTabHelper::IsSameSearchQuery(const GURL& url) const { +bool SerpMetricsTabHelper::IsSameSerpAsLastRecorded(const GURL& url) const { return last_recorded_serp_url_ && - serp_classifier_.IsSameSearchQuery(url, *last_recorded_serp_url_); + IsSameSearchQuery(url, *last_recorded_serp_url_); } void SerpMetricsTabHelper::MaybeClassifyAndRecordSearchEngineForUrl( const GURL& url) { - if (IsSameSearchQuery(url)) { - // The navigation repeats the same search query as the last recorded SERP, - // so do not double-count it. + if (IsSameSerpAsLastRecorded(url)) { return; } std::optional search_engine_type = - serp_classifier_.MaybeClassify(url); + MaybeClassifySearchEngine(url); if (!search_engine_type) { return; } @@ -134,10 +130,10 @@ void SerpMetricsTabHelper::DidFinishNavigation( const GURL& url = navigation_handle->GetURL(); - if (!is_new_navigation || !IsSameSearchQuery(url)) { - // Any navigation that doesn't match the previous search engine results page - // should reset it along with any user initiated navigation (omnibox, - // bookmarks, etc...) whether it matches or not. + if (!is_new_navigation || !IsSameSerpAsLastRecorded(url)) { + // If this isn't a new navigation or it doesn't go to the same SERP as the + // last recorded one, clear the last recorded SERP URL so the next visit to + // that SERP can be recorded again. last_recorded_serp_url_.reset(); } diff --git a/browser/serp_metrics/serp_metrics_tab_helper.h b/browser/serp_metrics/serp_metrics_tab_helper.h index bee9f0e8c60..ed649cd6a10 100644 --- a/browser/serp_metrics/serp_metrics_tab_helper.h +++ b/browser/serp_metrics/serp_metrics_tab_helper.h @@ -9,7 +9,6 @@ #include #include "base/memory/raw_ptr.h" -#include "brave/components/serp_metrics/serp_classifier.h" #include "components/search_engines/search_engine_type.h" #include "content/public/browser/web_contents_observer.h" #include "content/public/browser/web_contents_user_data.h" @@ -40,7 +39,7 @@ class SerpMetricsTabHelper final private: friend class content::WebContentsUserData; - bool IsSameSearchQuery(const GURL& url) const; + bool IsSameSerpAsLastRecorded(const GURL& url) const; void MaybeClassifyAndRecordSearchEngineForUrl(const GURL& url); void RecordSearchEngine(SearchEngineType search_engine_type); @@ -49,8 +48,6 @@ class SerpMetricsTabHelper final void DidFinishNavigation( content::NavigationHandle* navigation_handle) override; - SerpClassifier serp_classifier_; - raw_ptr serp_metrics_ = nullptr; // Not owned. std::optional last_recorded_serp_url_; diff --git a/browser/serp_metrics/serp_metrics_tab_helper_browsertest.cc b/browser/serp_metrics/serp_metrics_tab_helper_browsertest.cc index 19bef7b6c43..55a97d132ff 100644 --- a/browser/serp_metrics/serp_metrics_tab_helper_browsertest.cc +++ b/browser/serp_metrics/serp_metrics_tab_helper_browsertest.cc @@ -11,6 +11,8 @@ #include #include "absl/strings/str_format.h" +#include "base/check.h" +#include "base/containers/flat_map.h" #include "base/test/bind.h" #include "base/test/scoped_feature_list.h" #include "base/test/test_future.h" @@ -42,7 +44,6 @@ #include "net/http/http_status_code.h" #include "net/test/embedded_test_server/embedded_test_server.h" #include "testing/gtest/include/gtest/gtest.h" -#include "url/gurl.h" #if !BUILDFLAG(IS_ANDROID) #include "chrome/browser/profiles/keep_alive/profile_keep_alive_types.h" @@ -58,6 +59,14 @@ namespace serp_metrics { namespace { +constexpr char kHtmlWithAnchorLinkContent[] = R"HTML( + + + Link + + + )HTML"; + // Builder class to help construct an HTTPS `EmbeddedTestServer` with various // configurations for testing. class TestHttpsServerBuilder { @@ -75,8 +84,15 @@ class TestHttpsServerBuilder { return *this; } - TestHttpsServerBuilder& WithAnchorLink(GURL anchor_link) { - anchor_link_ = std::move(anchor_link); + TestHttpsServerBuilder& WithContentOverrideForPath( + const std::string& path, + const std::string& content) { + CHECK(!path.empty()); + CHECK(!content.empty()); + CHECK(override_http_response_content_.find(path) == + override_http_response_content_.cend()) + << "Duplicate content override for path: " << path; + override_http_response_content_[path] = content; return *this; } @@ -96,7 +112,8 @@ class TestHttpsServerBuilder { } https_server->RegisterRequestHandler(base::BindLambdaForTesting( - [http_status_code = http_status_code_, anchor_link = anchor_link_, + [http_status_code = http_status_code_, + override_http_response_content = override_http_response_content_, source_path = source_path_, destination_path = destination_path_]( const net::test_server::HttpRequest& http_request) -> std::unique_ptr { @@ -112,20 +129,20 @@ class TestHttpsServerBuilder { return http_response; } - // Serve a basic HTML page with the specified status code and optional - // anchor link. + // Serve a HTML page with the specified status code. http_response->set_code(http_status_code); http_response->set_content_type("text/html"); - std::string_view http_reason_phrase = - net::GetHttpReasonPhrase(http_status_code); - if (anchor_link) { - http_response->set_content( - absl::StrFormat("%sLink", - http_reason_phrase, anchor_link->spec())); + const auto iter = + override_http_response_content.find(http_request.GetURL().path()); + if (iter != override_http_response_content.cend()) { + // Override the default response content. + http_response->set_content(iter->second); } else { - http_response->set_content(absl::StrFormat( - "%s", http_reason_phrase)); + // Default response content is a simple HTML page showing the status + // code reason phrase. + http_response->set_content( + absl::StrFormat("%s", + net::GetHttpReasonPhrase(http_status_code))); } return http_response; @@ -141,9 +158,22 @@ class TestHttpsServerBuilder { std::optional destination_path_; net::HttpStatusCode http_status_code_ = net::HTTP_OK; - std::optional anchor_link_; + base::flat_map + override_http_response_content_; }; +#if !BUILDFLAG(IS_ANDROID) +Browser* CreateProfileAndOpenBrowser() { + base::FilePath profile_path = + g_browser_process->profile_manager()->GenerateNextProfileDirectoryPath(); + base::test::TestFuture browser_test_future; + profiles::SwitchToProfile(profile_path, /*always_create=*/false, + browser_test_future.GetCallback()); + EXPECT_TRUE(browser_test_future.Wait()); + return browser_test_future.Take(); +} +#endif // !BUILDFLAG(IS_ANDROID) + } // namespace // TODO(https://github.com/brave/brave-browser/issues/52599): Migrate SERP @@ -171,7 +201,7 @@ class SerpMetricsTabHelperTest : public PlatformBrowserTest { ASSERT_TRUE(https_server_); ASSERT_TRUE(https_server_->Start()); - // Wait for TemplateURLService to finish loading so that SerpClassifier can + // Wait for TemplateURLService to finish loading so that SERP classifier can // classify search URLs. Without this, navigations that complete before the // service is loaded will not be classified. search_test_utils::WaitForTemplateURLServiceToLoad( @@ -185,17 +215,19 @@ class SerpMetricsTabHelperTest : public PlatformBrowserTest { } void SimulateClickingAnchorLink() const { - content::TestNavigationObserver observer(GetWebContents()); + content::TestNavigationObserver navigation_observer(GetWebContents()); ASSERT_TRUE(content::ExecJs( GetWebContents(), "document.getElementById('anchor_link').click();")); - observer.Wait(); + navigation_observer.Wait(); } void Reload() const { - content::TestNavigationObserver observer(GetWebContents()); + ASSERT_TRUE(GetWebContents()->GetController().GetLastCommittedEntry()); + content::TestNavigationManager navigation_manager( + GetWebContents(), GetWebContents()->GetLastCommittedURL()); GetWebContents()->GetController().Reload(content::ReloadType::NORMAL, /*check_for_repost=*/false); - observer.Wait(); + ASSERT_TRUE(navigation_manager.WaitForNavigationFinished()); ASSERT_TRUE(content::WaitForLoadStop(GetWebContents())); } @@ -207,22 +239,6 @@ class SerpMetricsTabHelperTest : public PlatformBrowserTest { return profile_misc_metrics_service->GetSerpMetrics(); } - ProfileManager* profile_manager() { - return g_browser_process->profile_manager(); - } - -#if !BUILDFLAG(IS_ANDROID) - Browser* CreateProfileAndOpenBrowser() { - base::FilePath profile_path = - profile_manager()->GenerateNextProfileDirectoryPath(); - base::test::TestFuture browser_future; - profiles::SwitchToProfile(profile_path, /*always_create=*/false, - browser_future.GetCallback()); - EXPECT_TRUE(browser_future.Wait()); - return browser_future.Take(); - } -#endif // !BUILDFLAG(IS_ANDROID) - base::test::ScopedFeatureList scoped_feature_list_; content::ContentMockCertVerifier mock_cert_verifier_; @@ -278,12 +294,16 @@ IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, } IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, - RecordSearchWhenNavigatingViaLinkClick) { - auto https_server = + RecordWhenNavigatingViaLinkClick) { + const auto https_server = TestHttpsServerBuilder() .WithCertHostnames({"search.brave.com", "plugh.xyzzy.com"}) - .WithAnchorLink( - https_server_->GetURL("search.brave.com", "/search?q=test")) + .WithContentOverrideForPath( + "/thud", + absl::StrFormat( + kHtmlWithAnchorLinkContent, + https_server_->GetURL("search.brave.com", "/search?q=test") + .spec())) .Build(); ASSERT_TRUE(https_server); ASSERT_TRUE(https_server->Start()); @@ -301,10 +321,10 @@ IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, } IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, RecordForHttp4xxResponse) { - auto https_server = TestHttpsServerBuilder() - .WithStatusCode(net::HTTP_NOT_FOUND) - .WithCertHostnames({"search.brave.com"}) - .Build(); + const auto https_server = TestHttpsServerBuilder() + .WithStatusCode(net::HTTP_NOT_FOUND) + .WithCertHostnames({"search.brave.com"}) + .Build(); ASSERT_TRUE(https_server); ASSERT_TRUE(https_server->Start()); @@ -317,10 +337,10 @@ IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, RecordForHttp4xxResponse) { } IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, RecordForHttp5xxResponse) { - auto https_server = TestHttpsServerBuilder() - .WithStatusCode(net::HTTP_INTERNAL_SERVER_ERROR) - .WithCertHostnames({"www.google.com"}) - .Build(); + const auto https_server = TestHttpsServerBuilder() + .WithStatusCode(net::HTTP_INTERNAL_SERVER_ERROR) + .WithCertHostnames({"www.google.com"}) + .Build(); ASSERT_TRUE(https_server); ASSERT_TRUE(https_server->Start()); @@ -360,7 +380,7 @@ IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, DoNotRecordRedirectNavigationForSameSearch) { - auto https_server = + const auto https_server = TestHttpsServerBuilder() .WithCertHostnames({"search.brave.com"}) .WithRedirect(/*source_path=*/"/search?q=test", @@ -380,12 +400,13 @@ IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, } IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, - RecordSearchAfterRedirectNavigation) { - auto https_server = TestHttpsServerBuilder() - .WithCertHostnames({"search.brave.com"}) - .WithRedirect(/*source_path=*/"/a/redirect", - /*destination_path=*/"/search?q=test") - .Build(); + RecordAfterRedirectNavigation) { + const auto https_server = + TestHttpsServerBuilder() + .WithCertHostnames({"search.brave.com"}) + .WithRedirect(/*source_path=*/"/a/redirect", + /*destination_path=*/"/search?q=test") + .Build(); ASSERT_TRUE(https_server); ASSERT_TRUE(https_server->Start()); @@ -502,11 +523,15 @@ IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, RecordSameSearchAfterReloadNavigationAndLinkClick) { - auto https_server = + const auto https_server = TestHttpsServerBuilder() .WithCertHostnames({"search.brave.com", "plugh.xyzzy.com"}) - .WithAnchorLink( - https_server_->GetURL("search.brave.com", "/search?q=test")) + .WithContentOverrideForPath( + "/thud", + absl::StrFormat( + kHtmlWithAnchorLinkContent, + https_server_->GetURL("search.brave.com", "/search?q=test") + .spec())) .Build(); ASSERT_TRUE(https_server); ASSERT_TRUE(https_server->Start()); @@ -524,9 +549,10 @@ IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, Reload(); + ASSERT_TRUE(content::HistoryGoBack(GetWebContents())); SimulateClickingAnchorLink(); - EXPECT_EQ(2U, + EXPECT_EQ(3U, GetSerpMetrics()->GetSearchCountForTesting(SerpMetricType::kBrave)); } @@ -643,14 +669,17 @@ IN_PROC_BROWSER_TEST_F( 2U, GetSerpMetrics()->GetSearchCountForTesting(SerpMetricType::kGoogle)); } -IN_PROC_BROWSER_TEST_F( - SerpMetricsTabHelperTest, - DoNotRecordSameSearchAfterBackForwardNavigationAndLinkClick) { - auto https_server = +IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, + RecordSameSearchAfterBackForwardNavigationAndLinkClick) { + const auto https_server = TestHttpsServerBuilder() .WithCertHostnames({"search.brave.com", "plugh.xyzzy.com"}) - .WithAnchorLink( - https_server_->GetURL("search.brave.com", "/search?q=test")) + .WithContentOverrideForPath( + "/thud", + absl::StrFormat( + kHtmlWithAnchorLinkContent, + https_server_->GetURL("search.brave.com", "/search?q=test") + .spec())) .Build(); ASSERT_TRUE(https_server); ASSERT_TRUE(https_server->Start()); @@ -669,9 +698,10 @@ IN_PROC_BROWSER_TEST_F( ASSERT_TRUE(content::HistoryGoBack(GetWebContents())); ASSERT_TRUE(content::HistoryGoForward(GetWebContents())); + ASSERT_TRUE(content::HistoryGoBack(GetWebContents())); SimulateClickingAnchorLink(); - EXPECT_EQ(2U, + EXPECT_EQ(3U, GetSerpMetrics()->GetSearchCountForTesting(SerpMetricType::kBrave)); } @@ -702,11 +732,11 @@ IN_PROC_BROWSER_TEST_F( } IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, RecordWithoutUserGesture) { - content::TestNavigationObserver observer(GetWebContents()); + content::TestNavigationObserver navigation_observer(GetWebContents()); ASSERT_TRUE(NavigateToURLFromRendererWithoutUserGesture( GetWebContents(), https_server_->GetURL("www.google.com", "/search?q=test"))); - observer.Wait(); + navigation_observer.Wait(); EXPECT_EQ( 1U, GetSerpMetrics()->GetSearchCountForTesting(SerpMetricType::kGoogle)); @@ -762,7 +792,6 @@ IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, RecordIfTabWasRestored) { session_restore_test_helper.Wait(); } SetBrowser(browser_created_observer.Wait()); - ASSERT_TRUE(content::WaitForLoadStop(GetWebContents())); EXPECT_EQ( @@ -792,7 +821,7 @@ IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, RecordForMultipleProfiles) { SerpMetricsAllProfilesAggregator all_profiles_aggregator( g_browser_process->local_state(), - profile_manager()->GetProfileAttributesStorage()); + g_browser_process->profile_manager()->GetProfileAttributesStorage()); EXPECT_EQ(2U, all_profiles_aggregator.GetSearchCountForTesting( SerpMetricType::kBrave)); EXPECT_EQ(1U, all_profiles_aggregator.GetSearchCountForTesting( @@ -800,7 +829,6 @@ IN_PROC_BROWSER_TEST_F(SerpMetricsTabHelperTest, RecordForMultipleProfiles) { EXPECT_EQ(1U, all_profiles_aggregator.GetSearchCountForTesting( SerpMetricType::kOther)); } - #endif // !BUILDFLAG(IS_ANDROID) } // namespace serp_metrics diff --git a/components/serp_metrics/BUILD.gn b/components/serp_metrics/BUILD.gn index 60287b6aa68..af7c8cd8db9 100644 --- a/components/serp_metrics/BUILD.gn +++ b/components/serp_metrics/BUILD.gn @@ -6,12 +6,14 @@ static_library("serp_metrics") { public = [ "serp_classifier.h", + "serp_classifier_utils.h", "serp_metric_type.h", "serp_metrics.h", ] sources = [ "serp_classifier.cc", + "serp_classifier_utils.cc", "serp_metrics.cc", ] @@ -42,6 +44,7 @@ source_set("unit_tests") { sources = [ "serp_classifier_unittest.cc", + "serp_classifier_utils_unittest.cc", "serp_metrics_unittest.cc", ] diff --git a/components/serp_metrics/serp_classifier.cc b/components/serp_metrics/serp_classifier.cc index 32bff711eb4..55eb80ac363 100644 --- a/components/serp_metrics/serp_classifier.cc +++ b/components/serp_metrics/serp_classifier.cc @@ -5,9 +5,11 @@ #include "brave/components/serp_metrics/serp_classifier.h" -#include "base/containers/fixed_flat_set.h" +#include +#include + #include "brave/components/search_engines/brave_prepopulated_engines.h" -#include "components/search_engines/search_engine_type.h" +#include "brave/components/serp_metrics/serp_classifier_utils.h" #include "components/search_engines/search_terms_data.h" #include "components/search_engines/template_url.h" #include "components/search_engines/template_url_data_util.h" @@ -18,19 +20,15 @@ namespace serp_metrics { namespace { -constexpr auto kAllowedPrepopulatedEngines = - base::MakeFixedFlatSet( - base::sorted_unique, - {SEARCH_ENGINE_BING, SEARCH_ENGINE_GOOGLE, SEARCH_ENGINE_YAHOO, - SEARCH_ENGINE_DUCKDUCKGO, SEARCH_ENGINE_QWANT, SEARCH_ENGINE_ECOSIA, - SEARCH_ENGINE_BRAVE, SEARCH_ENGINE_STARTPAGE}); +constexpr std::string_view kStartpageUrlHost = "www.startpage.com"; +constexpr std::string_view kStartpageUrlPath = "/sp/search"; // Returns a `TemplateURL` if `url` matches the search engine results page for // `prepopulated_engine`. std::unique_ptr MaybeGetTemplateURLForPrepopulatedEngine( const TemplateURLPrepopulateData::PrepopulatedEngine& prepopulated_engine, const GURL& url) { - if (!kAllowedPrepopulatedEngines.contains(prepopulated_engine.type)) { + if (!IsAllowedSearchEngine(prepopulated_engine.type)) { return nullptr; } @@ -40,7 +38,7 @@ std::unique_ptr MaybeGetTemplateURLForPrepopulatedEngine( if (!template_url->IsSearchURL(url, SearchTermsData())) { if (prepopulated_engine.type == SEARCH_ENGINE_STARTPAGE && - url.host() == "www.startpage.com" && url.path() == "/sp/search") { + url.host() == kStartpageUrlHost && url.path() == kStartpageUrlPath) { // Startpage uses a path-based SERP URL. Chromium still checks the legacy // query-based format and does not support the new one. Even if we update // the search URL, `TemplateURL::IsSearchURL` still fails because it @@ -54,31 +52,32 @@ std::unique_ptr MaybeGetTemplateURLForPrepopulatedEngine( return template_url; } -} // namespace - -bool SerpClassifier::IsSameSearchQuery(const GURL& lhs, const GURL& rhs) const { - if (lhs.host() == "www.startpage.com") { - // For Startpage, we cannot determine whether two URLs represent the same - // search results page, so these pages are always classified. - return false; +// Returns a `TemplateURL` if `url` matches the search engine results page for +// any prepopulated engine in the allow list. +std::unique_ptr MaybeGetTemplateUrl(const GURL& url) { + for (const auto* prepopulated_engine : + TemplateURLPrepopulateData::GetAllPrepopulatedEngines()) { + if (auto search_engine = MaybeGetTemplateURLForPrepopulatedEngine( + *prepopulated_engine, url)) { + return search_engine; + } } - return NormalizeUrl(lhs) == NormalizeUrl(rhs); -} - -std::optional SerpClassifier::MaybeClassify(const GURL& url) { - const GURL normalized_url = NormalizeUrl(url); - - if (const auto template_url = MaybeGetTemplateUrl(normalized_url)) { - return template_url->GetEngineType(SearchTermsData()); + for (const auto& [_, prepopulated_engine] : + TemplateURLPrepopulateData::kBraveEngines) { + if (auto search_engine = MaybeGetTemplateURLForPrepopulatedEngine( + *prepopulated_engine, url)) { + return search_engine; + } } - return std::nullopt; + return nullptr; } -/////////////////////////////////////////////////////////////////////////////// - -GURL SerpClassifier::NormalizeUrl(const GURL& url) const { +// Normalizes a SERP URL so equivalent search queries produce the same URL for +// comparison. Strips ports, removes non-search parameters, and canonicalizes +// the search terms. +GURL NormalizeUrl(const GURL& url) { if (!url.is_valid()) { return url; } @@ -102,25 +101,25 @@ GURL SerpClassifier::NormalizeUrl(const GURL& url) const { return normalized_url; } -std::unique_ptr SerpClassifier::MaybeGetTemplateUrl( - const GURL& url) const { - for (const auto* prepopulated_engine : - TemplateURLPrepopulateData::GetAllPrepopulatedEngines()) { - if (auto search_engine = MaybeGetTemplateURLForPrepopulatedEngine( - *prepopulated_engine, url)) { - return search_engine; - } +} // namespace + +bool IsSameSearchQuery(const GURL& lhs, const GURL& rhs) { + if (lhs.host() == kStartpageUrlHost) { + // For Startpage, we cannot determine whether two URLs represent the same + // search results page, so these pages are always classified. + return false; } - for (const auto& [_, prepopulated_engine] : - TemplateURLPrepopulateData::kBraveEngines) { - if (auto search_engine = MaybeGetTemplateURLForPrepopulatedEngine( - *prepopulated_engine, url)) { - return search_engine; - } + return NormalizeUrl(lhs) == NormalizeUrl(rhs); +} + +std::optional MaybeClassifySearchEngine(const GURL& url) { + const GURL normalized_url = NormalizeUrl(url); + if (const auto template_url = MaybeGetTemplateUrl(normalized_url)) { + return template_url->GetEngineType(SearchTermsData()); } - return nullptr; + return std::nullopt; } } // namespace serp_metrics diff --git a/components/serp_metrics/serp_classifier.h b/components/serp_metrics/serp_classifier.h index c804ccf4550..b6ec34139cc 100644 --- a/components/serp_metrics/serp_classifier.h +++ b/components/serp_metrics/serp_classifier.h @@ -6,42 +6,23 @@ #ifndef BRAVE_COMPONENTS_SERP_METRICS_SERP_CLASSIFIER_H_ #define BRAVE_COMPONENTS_SERP_METRICS_SERP_CLASSIFIER_H_ -#include #include #include "components/search_engines/search_engine_type.h" class GURL; -class TemplateURL; + +// SERP classifier determines whether a URL is a search engine results page and, +// if so, identifies the corresponding search engine. namespace serp_metrics { -// SerpClassifier determines whether a URL is a search engine results page and, -// if so, identifies the corresponding search engine. +// Returns `true` if `lhs` and `rhs` represent the same search results page. +bool IsSameSearchQuery(const GURL& lhs, const GURL& rhs); -class SerpClassifier final { - public: - SerpClassifier() = default; - ~SerpClassifier() = default; - - SerpClassifier(const SerpClassifier&) = delete; - SerpClassifier& operator=(const SerpClassifier&) = delete; - - // Returns `true` if `lhs` and `rhs` represent the same search results page. - bool IsSameSearchQuery(const GURL& lhs, const GURL& rhs) const; - - // Returns the corresponding search engine type if `url` is a SERP. Returns - // `std::nullopt` if `url` is not a SERP. - std::optional MaybeClassify(const GURL& url); - - private: - // Normalizes a URL so equivalent search results pages compare equal. - GURL NormalizeUrl(const GURL& url) const; - - // Returns a `TemplateURL` if `url` matches the search engine results page for - // any prepopulated engine in the allow list. - std::unique_ptr MaybeGetTemplateUrl(const GURL& url) const; -}; +// Returns the corresponding search engine type if `url` is a SERP. Returns +// `std::nullopt` if `url` is not a SERP. +std::optional MaybeClassifySearchEngine(const GURL& url); } // namespace serp_metrics diff --git a/components/serp_metrics/serp_classifier_unittest.cc b/components/serp_metrics/serp_classifier_unittest.cc index b6d7be1ca2b..a3fa757f8c5 100644 --- a/components/serp_metrics/serp_classifier_unittest.cc +++ b/components/serp_metrics/serp_classifier_unittest.cc @@ -7,8 +7,8 @@ #include -#include "base/containers/fixed_flat_set.h" #include "brave/components/search_engines/brave_prepopulated_engines.h" +#include "brave/components/serp_metrics/serp_classifier_utils.h" #include "components/search_engines/search_engine_type.h" #include "components/search_engines/search_terms_data.h" #include "components/search_engines/template_url.h" @@ -21,56 +21,44 @@ namespace serp_metrics { namespace { -constexpr auto kAllowedPrepopulatedEngines = - base::MakeFixedFlatSet( - base::sorted_unique, - {SEARCH_ENGINE_BING, SEARCH_ENGINE_GOOGLE, SEARCH_ENGINE_YAHOO, - SEARCH_ENGINE_DUCKDUCKGO, SEARCH_ENGINE_QWANT, SEARCH_ENGINE_ECOSIA, - SEARCH_ENGINE_BRAVE, SEARCH_ENGINE_STARTPAGE}); - void VerifySerpClassifierExpectation( const TemplateURLPrepopulateData::PrepopulatedEngine& prepopulated_engine) { const auto template_url_data = TemplateURLDataFromPrepopulatedEngine(prepopulated_engine); - TemplateURL template_url(*template_url_data); + const TemplateURL template_url(*template_url_data); - GURL url = template_url.GenerateSearchURL(SearchTermsData(), u"test"); + const GURL url = template_url.GenerateSearchURL(SearchTermsData(), u"test"); ASSERT_TRUE(url.is_valid()); - SerpClassifier classifier; if (std::optional search_engine_type = - classifier.MaybeClassify(url)) { - EXPECT_TRUE(kAllowedPrepopulatedEngines.contains(*search_engine_type)); + MaybeClassifySearchEngine(url)) { + EXPECT_TRUE(IsAllowedSearchEngine(*search_engine_type)); } } } // namespace TEST(SerpClassifierTest, IsSameSearchQuery) { - SerpClassifier classifier; - EXPECT_TRUE(classifier.IsSameSearchQuery( - GURL(R"(https://www.qwant.com/?q=foobar)"), - GURL(R"(https://www.qwant.com/?q=foobar&t=web)"))); + EXPECT_TRUE(IsSameSearchQuery( + GURL(R"(https://search.brave.com/search?q=foobar)"), + GURL(R"(https://search.brave.com/search?q=foobar&t=web)"))); } TEST(SerpClassifierTest, IsSameSearchQueryWithDifferentParamOrder) { - SerpClassifier classifier; - EXPECT_TRUE(classifier.IsSameSearchQuery( - GURL(R"(https://www.qwant.com/?q=foobar)"), - GURL(R"(https://www.qwant.com/?t=web&q=foobar)"))); + EXPECT_TRUE(IsSameSearchQuery( + GURL(R"(https://search.brave.com/search?q=foobar)"), + GURL(R"(https://search.brave.com/search?t=web&q=foobar)"))); } TEST(SerpClassifierTest, IsNotSameSearchQuery) { - SerpClassifier classifier; - EXPECT_FALSE(classifier.IsSameSearchQuery( - GURL(R"(https://www.qwant.com/?q=foo&t=web)"), - GURL(R"(https://www.qwant.com/?q=bar&t=web")"))); + 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")"))); } TEST(SerpClassifierTest, IsNotSameSearchQueryWithInvalidUrl) { - SerpClassifier classifier; - EXPECT_FALSE(classifier.IsSameSearchQuery( - GURL(R"(https://www.qwant.com/?q=foobar)"), GURL("foobar"))); + EXPECT_FALSE(IsSameSearchQuery( + GURL(R"(https://search.brave.com/search?q=foobar)"), GURL("invalid"))); } TEST(SerpClassifierTest, OnlyClassifyAllowedSearchEngines) { @@ -87,26 +75,24 @@ TEST(SerpClassifierTest, OnlyClassifyAllowedSearchEngines) { TEST(SerpClassifierTest, ClassifyStartpageSearchEngine) { // Startpage uses a path-based SERP URL that Chromium's query-based detection - // does not support for real-world navigations to the results page. - SerpClassifier classifier; - EXPECT_TRUE( - classifier.MaybeClassify(GURL(R"(https://www.startpage.com/sp/search)"))); + // does not support. + EXPECT_TRUE(MaybeClassifySearchEngine( + GURL(R"(https://www.startpage.com/sp/search)"))); } TEST(SerpClassifierTest, DoNotClassifyNonSearchEngine) { - SerpClassifier classifier; - - EXPECT_FALSE(classifier.MaybeClassify( + EXPECT_FALSE(MaybeClassifySearchEngine( GURL(R"(https://www.perplexity.ai/search/new/foo)"))); - EXPECT_FALSE(classifier.MaybeClassify(GURL(R"(https://brave.com/)"))); - EXPECT_FALSE(classifier.MaybeClassify(GURL(R"(https://bar.com/baz)"))); - EXPECT_FALSE(classifier.MaybeClassify(GURL(R"(https://qux.quux.com/corge)"))); + EXPECT_FALSE(MaybeClassifySearchEngine(GURL(R"(https://brave.com/)"))); + EXPECT_FALSE(MaybeClassifySearchEngine(GURL(R"(https://bar.com/baz)"))); EXPECT_FALSE( - classifier.MaybeClassify(GURL(R"(https://startpage.com/grault)"))); + MaybeClassifySearchEngine(GURL(R"(https://qux.quux.com/corge)"))); EXPECT_FALSE( - classifier.MaybeClassify(GURL(R"(https://uk.search.yahoo.com/garply)"))); + MaybeClassifySearchEngine(GURL(R"(https://startpage.com/grault)"))); EXPECT_FALSE( - classifier.MaybeClassify(GURL(R"(https://search.yahoo.com/waldo)"))); + MaybeClassifySearchEngine(GURL(R"(https://uk.search.yahoo.com/garply)"))); + EXPECT_FALSE( + MaybeClassifySearchEngine(GURL(R"(https://search.yahoo.com/waldo)"))); } } // namespace serp_metrics diff --git a/components/serp_metrics/serp_classifier_utils.cc b/components/serp_metrics/serp_classifier_utils.cc new file mode 100644 index 00000000000..d0134912e94 --- /dev/null +++ b/components/serp_metrics/serp_classifier_utils.cc @@ -0,0 +1,26 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/components/serp_metrics/serp_classifier_utils.h" + +#include "base/containers/fixed_flat_set.h" + +namespace serp_metrics { + +namespace { + +constexpr auto kAllowedSearchEngines = base::MakeFixedFlatSet( + base::sorted_unique, + {SEARCH_ENGINE_BING, SEARCH_ENGINE_GOOGLE, SEARCH_ENGINE_YAHOO, + SEARCH_ENGINE_DUCKDUCKGO, SEARCH_ENGINE_QWANT, SEARCH_ENGINE_ECOSIA, + SEARCH_ENGINE_BRAVE, SEARCH_ENGINE_STARTPAGE}); + +} // namespace + +bool IsAllowedSearchEngine(SearchEngineType type) { + return kAllowedSearchEngines.contains(type); +} + +} // namespace serp_metrics diff --git a/components/serp_metrics/serp_classifier_utils.h b/components/serp_metrics/serp_classifier_utils.h new file mode 100644 index 00000000000..caee342df74 --- /dev/null +++ b/components/serp_metrics/serp_classifier_utils.h @@ -0,0 +1,18 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_COMPONENTS_SERP_METRICS_SERP_CLASSIFIER_UTILS_H_ +#define BRAVE_COMPONENTS_SERP_METRICS_SERP_CLASSIFIER_UTILS_H_ + +#include "components/search_engines/search_engine_type.h" + +namespace serp_metrics { + +// Returns `true` if the search engine type is allowed to be classified. +bool IsAllowedSearchEngine(SearchEngineType type); + +} // namespace serp_metrics + +#endif // BRAVE_COMPONENTS_SERP_METRICS_SERP_CLASSIFIER_UTILS_H_ diff --git a/components/serp_metrics/serp_classifier_utils_unittest.cc b/components/serp_metrics/serp_classifier_utils_unittest.cc new file mode 100644 index 00000000000..5dea0c67be3 --- /dev/null +++ b/components/serp_metrics/serp_classifier_utils_unittest.cc @@ -0,0 +1,30 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/components/serp_metrics/serp_classifier_utils.h" + +#include "base/containers/fixed_flat_set.h" +#include "components/search_engines/search_engine_type.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace serp_metrics { + +TEST(SerpClassifierUtilsTest, IsAllowedSearchEngine) { + constexpr auto kAllowedSearchEngines = + base::MakeFixedFlatSet( + base::sorted_unique, + {SEARCH_ENGINE_BING, SEARCH_ENGINE_GOOGLE, SEARCH_ENGINE_YAHOO, + SEARCH_ENGINE_DUCKDUCKGO, SEARCH_ENGINE_QWANT, SEARCH_ENGINE_ECOSIA, + SEARCH_ENGINE_BRAVE, SEARCH_ENGINE_STARTPAGE}); + + for (int i = 0; i <= SEARCH_ENGINE_MAX; ++i) { + const SearchEngineType search_engine_type = + static_cast(i); + EXPECT_EQ(IsAllowedSearchEngine(search_engine_type), + kAllowedSearchEngines.contains(search_engine_type)); + } +} + +} // namespace serp_metrics