[Merge to M101][Topics] Implement BrowsingTopicsServiceImpl

- Implement the BrowsingTopicsServiceImpl responsible for scheduling the topics calculation, observing and handling history/cookies deletion, and calculating the topics for the JS API and for the UX. - Integrate with the renderer side API. - Update the BrowsingTopicsSiteDataManager/Storage::OnBrowsingTopicsApiUsed API() to let it get the timestamp from the main thread and pass it to the backend thread. This way, it's consistent with the query operation, so that there's no races (i.e. a Query(/*end_time=*/Now()) is guaranteed to return previously stored entries). - Update model_version type from "int" to "int64_t": the model version is expected to exceed the limit of int (i.e. it's using the timestamp). (cherry picked from commit cc37939) Binary-Size: Size increase is unavoidable due to new feature. Bug: 1310012 Change-Id: I45bc0ff3e7b8e9df28c940d14a95f076d9a69944 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3526273 Reviewed-by: Josh Karlin <jkarlin@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Yao Xiao <yaoxia@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#985500} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3552463 Reviewed-by: Yao Xiao <yaoxia@chromium.org> Cr-Commit-Position: refs/branch-heads/4951@{#220} Cr-Branched-From: 27de622-refs/heads/main@{#982481}
chromium · Mar 29, 2022 · f230ea6 · f230ea6
1 parent b00be27
commit f230ea6
Show file tree

Hide file tree

Showing 52 changed files with 3,591 additions and 182 deletions.
diff --git a/chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc b/chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc
diff --git a/chrome/browser/browsing_topics/browsing_topics_service_factory.cc b/chrome/browser/browsing_topics/browsing_topics_service_factory.cc
@@ -4,10 +4,19 @@
 
 #include "chrome/browser/browsing_topics/browsing_topics_service_factory.h"
 
+#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/optimization_guide/page_content_annotations_service_factory.h"
+#include "chrome/browser/privacy_sandbox/privacy_sandbox_settings_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "components/browsing_topics/browsing_topics_service.h"
 #include "components/browsing_topics/browsing_topics_service_impl.h"
+#include "components/history/core/browser/history_service.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/keyed_service/core/service_access_type.h"
+#include "components/optimization_guide/content/browser/page_content_annotations_service.h"
+#include "components/privacy_sandbox/privacy_sandbox_settings.h"
+#include "content/public/browser/browsing_topics_site_data_manager.h"
+#include "content/public/browser/storage_partition.h"
 #include "third_party/blink/public/common/features.h"
 
 namespace browsing_topics {
@@ -28,7 +37,11 @@ BrowsingTopicsServiceFactory* BrowsingTopicsServiceFactory::GetInstance() {
 BrowsingTopicsServiceFactory::BrowsingTopicsServiceFactory()
     : BrowserContextKeyedServiceFactory(
           "BrowsingTopicsService",
-          BrowserContextDependencyManager::GetInstance()) {}
+          BrowserContextDependencyManager::GetInstance()) {
+  DependsOn(PrivacySandboxSettingsFactory::GetInstance());
+  DependsOn(HistoryServiceFactory::GetInstance());
+  DependsOn(PageContentAnnotationsServiceFactory::GetInstance());
+}
 
 BrowsingTopicsServiceFactory::~BrowsingTopicsServiceFactory() = default;
 
@@ -37,7 +50,32 @@ KeyedService* BrowsingTopicsServiceFactory::BuildServiceInstanceFor(
   if (!base::FeatureList::IsEnabled(blink::features::kBrowsingTopics))
     return nullptr;
 
-  return new BrowsingTopicsServiceImpl();
+  Profile* profile = Profile::FromBrowserContext(context);
+
+  privacy_sandbox::PrivacySandboxSettings* privacy_sandbox_settings =
+      PrivacySandboxSettingsFactory::GetForProfile(profile);
+  if (!privacy_sandbox_settings)
+    return nullptr;
+
+  history::HistoryService* history_service =
+      HistoryServiceFactory::GetForProfile(profile,
+                                           ServiceAccessType::IMPLICIT_ACCESS);
+  if (!history_service)
+    return nullptr;
+
+  content::BrowsingTopicsSiteDataManager* site_data_manager =
+      context->GetDefaultStoragePartition()->GetBrowsingTopicsSiteDataManager();
+  if (!site_data_manager)
+    return nullptr;
+
+  optimization_guide::PageContentAnnotationsService* annotations_service =
+      PageContentAnnotationsServiceFactory::GetForProfile(profile);
+  if (!annotations_service)
+    return nullptr;
+
+  return new BrowsingTopicsServiceImpl(
+      profile->GetPath(), privacy_sandbox_settings, history_service,
+      site_data_manager, annotations_service);
 }
 
 bool BrowsingTopicsServiceFactory::ServiceIsCreatedWithBrowserContext() const {

diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
@@ -35,6 +35,7 @@
 #include "chrome/browser/browser_about_handler.h"
 #include "chrome/browser/browser_features.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/browsing_topics/browsing_topics_service_factory.h"
 #include "chrome/browser/captive_portal/captive_portal_service_factory.h"
 #include "chrome/browser/chrome_content_browser_client_binder_policies.h"
 #include "chrome/browser/chrome_content_browser_client_parts.h"
@@ -173,6 +174,7 @@
 #include "chrome/installer/util/google_update_settings.h"
 #include "components/autofill/core/common/autofill_switches.h"
 #include "components/blocked_content/popup_blocker.h"
+#include "components/browsing_topics/browsing_topics_service.h"
 #include "components/captive_portal/core/buildflags.h"
 #include "components/cloud_devices/common/cloud_devices_switches.h"
 #include "components/content_settings/browser/page_specific_content_settings.h"
@@ -308,6 +310,7 @@
 #include "third_party/blink/public/common/loader/url_loader_throttle.h"
 #include "third_party/blink/public/common/navigation/navigation_policy.h"
 #include "third_party/blink/public/common/switches.h"
+#include "third_party/blink/public/mojom/browsing_topics/browsing_topics.mojom.h"
 #include "third_party/blink/public/public_buildflags.h"
 #include "third_party/widevine/cdm/buildflags.h"
 #include "ui/base/clipboard/clipboard_format_type.h"
@@ -5974,6 +5977,23 @@ void ChromeContentBrowserClient::AugmentNavigationDownloadPolicy(
   }
 }
 
+std::vector<blink::mojom::EpochTopicPtr>
+ChromeContentBrowserClient::GetBrowsingTopicsForJsApi(
+    const url::Origin& context_origin,
+    content::RenderFrameHost* main_frame) {
+  browsing_topics::BrowsingTopicsService* browsing_topics_service =
+      browsing_topics::BrowsingTopicsServiceFactory::GetForProfile(
+          Profile::FromBrowserContext(
+              content::WebContents::FromRenderFrameHost(main_frame)
+                  ->GetBrowserContext()));
+
+  if (!browsing_topics_service)
+    return {};
+
+  return browsing_topics_service->GetBrowsingTopicsForJsApi(context_origin,
+                                                            main_frame);
+}
+
 bool ChromeContentBrowserClient::IsBluetoothScanningBlocked(
     content::BrowserContext* browser_context,
     const url::Origin& requesting_origin,

diff --git a/chrome/browser/chrome_content_browser_client.h b/chrome/browser/chrome_content_browser_client.h
@@ -680,6 +680,10 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
       bool user_gesture,
       blink::NavigationDownloadPolicy* download_policy) override;
 
+  std::vector<blink::mojom::EpochTopicPtr> GetBrowsingTopicsForJsApi(
+      const url::Origin& context_origin,
+      content::RenderFrameHost* main_frame) override;
+
   bool IsBluetoothScanningBlocked(content::BrowserContext* browser_context,
                                   const url::Origin& requesting_origin,
                                   const url::Origin& embedding_origin) override;

diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
@@ -221,6 +221,7 @@ static_library("test_support") {
     "//chrome/utility",
     "//components/autofill/core/browser:test_support",
     "//components/bookmarks/test",
+    "//components/browsing_topics:test_support",
     "//components/captive_portal/core:test_support",
     "//components/consent_auditor:test_support",
     "//components/content_settings/core/browser",
@@ -238,6 +239,7 @@ static_library("test_support") {
     "//components/network_time",
     "//components/network_time:network_time_test_support",
     "//components/omnibox/browser:test_support",
+    "//components/optimization_guide/content/browser:test_support",
     "//components/os_crypt",
     "//components/password_manager/core/browser:test_support",
     "//components/payments/core:test_support",

diff --git a/chrome/test/data/browsing_topics/one_iframe_page_sandboxed.html.mock-http-headers b/chrome/test/data/browsing_topics/one_iframe_page_sandboxed.html.mock-http-headers
@@ -0,0 +1,2 @@
+HTTP/1.1 200 OK
+Content-Security-Policy: sandbox allow-scripts;
diff --git a/chrome/test/data/browsing_topics/one_sandboxed_iframe_page.html b/chrome/test/data/browsing_topics/one_sandboxed_iframe_page.html
@@ -0,0 +1,6 @@
+<!DOCTYPE html>
+<html>
+<body>
+  <iframe src="empty_page.html" sandbox="allow-scripts" id="frame"></iframe>
+</body>
+</html>
diff --git a/components/browsing_topics/BUILD.gn b/components/browsing_topics/BUILD.gn
@@ -6,6 +6,8 @@ source_set("browsing_topics") {
   sources = [
     "browsing_topics_calculator.cc",
     "browsing_topics_calculator.h",
+    "browsing_topics_page_load_data_tracker.cc",
+    "browsing_topics_page_load_data_tracker.h",
     "browsing_topics_service.h",
     "browsing_topics_service_impl.cc",
     "browsing_topics_service_impl.h",
@@ -28,7 +30,9 @@ source_set("browsing_topics") {
     "//components/optimization_guide/content/browser",
     "//components/privacy_sandbox",
     "//content/public/browser",
+    "//content/public/common:common",
     "//crypto",
+    "//net/base/registry_controlled_domains",
     "//third_party/blink/public/common",
   ]
 }
@@ -42,13 +46,19 @@ source_set("test_support") {
 
   public_deps = [ "//base" ]
 
-  deps = [ ":browsing_topics" ]
+  deps = [
+    ":browsing_topics",
+    "//base/test:test_support",
+    "//components/history/core/browser:browser",
+  ]
 }
 
 source_set("unit_tests") {
   testonly = true
   sources = [
     "browsing_topics_calculator_unittest.cc",
+    "browsing_topics_page_load_data_tracker_unittest.cc",
+    "browsing_topics_service_impl_unittest.cc",
     "browsing_topics_state_unittest.cc",
     "epoch_topics_unittest.cc",
     "topic_and_domains_unittest.cc",
@@ -61,6 +71,7 @@ source_set("unit_tests") {
     "//base",
     "//base/test:test_support",
     "//components/content_settings/core/test:test_support",
+    "//components/history/content/browser:browser",
     "//components/history/core/browser:browser",
     "//components/history/core/test",
     "//components/optimization_guide/content/browser:browser",

diff --git a/components/browsing_topics/DEPS b/components/browsing_topics/DEPS
@@ -5,8 +5,10 @@ include_rules = [
   "+components/privacy_sandbox",
   "+content/public/browser",
   "+content/public/test",
+  "+content/test",
   "+crypto",
-  "+third_party/blink/public/common",
+  "+net/base/registry_controlled_domains",
+  "+third_party/blink/public",
 ]
 
 specific_include_rules = {

diff --git a/components/browsing_topics/browsing_topics_calculator.cc b/components/browsing_topics/browsing_topics_calculator.cc
@@ -277,6 +277,11 @@ void BrowsingTopicsCalculator::OnGetRecentlyVisitedURLsCompleted(
 
   std::vector<std::string> raw_hosts_vector(raw_hosts.begin(), raw_hosts.end());
 
+  if (raw_hosts_vector.empty()) {
+    OnGetTopicsForHostsCompleted(/*raw_hosts=*/{}, /*results=*/{});
+    return;
+  }
+
   annotations_service_->BatchAnnotatePageTopics(
       base::BindOnce(&BrowsingTopicsCalculator::OnGetTopicsForHostsCompleted,
                      weak_ptr_factory_.GetWeakPtr(), raw_hosts_vector),
@@ -303,7 +308,7 @@ void BrowsingTopicsCalculator::OnGetTopicsForHostsCompleted(
     return;
   }
 
-  const int model_version = base::checked_cast<int>(model_info->GetVersion());
+  const int64_t model_version = model_info->GetVersion();
   DCHECK_GT(model_version, 0);
 
   std::map<HashedHost, std::set<Topic>> host_topics_map;
@@ -356,8 +361,7 @@ void BrowsingTopicsCalculator::OnGetTopicsForHostsCompleted(
 void BrowsingTopicsCalculator::OnCalculateCompleted(
     CalculatorResultStatus status,
     EpochTopics epoch_topics) {
-  DCHECK(status != CalculatorResultStatus::kSuccess ||
-         epoch_topics.HasValidTopics());
+  DCHECK(status != CalculatorResultStatus::kSuccess || !epoch_topics.empty());
 
   base::UmaHistogramEnumeration(
       "BrowsingTopics.EpochTopicsCalculation.CalculatorResultStatus", status);

diff --git a/components/browsing_topics/browsing_topics_calculator.h b/components/browsing_topics/browsing_topics_calculator.h
@@ -76,6 +76,7 @@ class BrowsingTopicsCalculator {
  protected:
   // This method exists for the purposes of overriding in tests.
   virtual uint64_t GenerateRandUint64();
+  virtual void CheckCanCalculate();
 
  private:
   // Get the top `kBrowsingTopicsNumberOfTopTopicsPerEpoch` topics. If there
@@ -91,8 +92,6 @@ class BrowsingTopicsCalculator {
       std::vector<Topic>& top_topics,
       size_t& padded_top_topics_start_index);
 
-  void CheckCanCalculate();
-
   void OnGetRecentBrowsingTopicsApiUsagesCompleted(
       browsing_topics::ApiUsageContextQueryResult result);
 

diff --git a/components/browsing_topics/browsing_topics_calculator_unittest.cc b/components/browsing_topics/browsing_topics_calculator_unittest.cc
@@ -33,22 +33,22 @@ namespace browsing_topics {
 
 namespace {
 
-const size_t kTaxonomySize = 349;
-const int kTaxonomyVersion = 1;
-
-const std::string kHost1 = "www.foo1.com";
-const std::string kHost2 = "www.foo2.com";
-const std::string kHost3 = "www.foo3.com";
-const std::string kHost4 = "www.foo4.com";
-const std::string kHost5 = "www.foo5.com";
-const std::string kHost6 = "www.foo6.com";
-
-const std::string kTokenizedHost1 = "foo1 com";
-const std::string kTokenizedHost2 = "foo2 com";
-const std::string kTokenizedHost3 = "foo3 com";
-const std::string kTokenizedHost4 = "foo4 com";
-const std::string kTokenizedHost5 = "foo5 com";
-const std::string kTokenizedHost6 = "foo6 com";
+constexpr size_t kTaxonomySize = 349;
+constexpr int kTaxonomyVersion = 1;
+
+constexpr char kHost1[] = "www.foo1.com";
+constexpr char kHost2[] = "www.foo2.com";
+constexpr char kHost3[] = "www.foo3.com";
+constexpr char kHost4[] = "www.foo4.com";
+constexpr char kHost5[] = "www.foo5.com";
+constexpr char kHost6[] = "www.foo6.com";
+
+constexpr char kTokenizedHost1[] = "foo1 com";
+constexpr char kTokenizedHost2[] = "foo2 com";
+constexpr char kTokenizedHost3[] = "foo3 com";
+constexpr char kTokenizedHost4[] = "foo4 com";
+constexpr char kTokenizedHost5[] = "foo5 com";
+constexpr char kTokenizedHost6[] = "foo6 com";
 
 }  // namespace
 
@@ -62,10 +62,10 @@ class BrowsingTopicsCalculatorTest : public testing::Test {
     HostContentSettingsMap::RegisterProfilePrefs(prefs_.registry());
     privacy_sandbox::RegisterProfilePrefs(prefs_.registry());
 
-    host_content_settings_map_ = new HostContentSettingsMap(
+    host_content_settings_map_ = base::MakeRefCounted<HostContentSettingsMap>(
         &prefs_, /*is_off_the_record=*/false, /*store_last_modified=*/false,
         /*restore_session=*/false);
-    cookie_settings_ = new content_settings::CookieSettings(
+    cookie_settings_ = base::MakeRefCounted<content_settings::CookieSettings>(
         host_content_settings_map_.get(), &prefs_, false, "chrome-extension");
     auto privacy_sandbox_delegate = std::make_unique<
         privacy_sandbox_test_util::MockPrivacySandboxSettingsDelegate>();
@@ -146,21 +146,22 @@ class BrowsingTopicsCalculatorTest : public testing::Test {
   }
 
   void AddApiUsageContextEntries(
-      std::vector<std::pair<std::string, std::set<HashedDomain>>>
+      const std::vector<std::pair<std::string, std::set<HashedDomain>>>&
           main_frame_hosts_with_context_domains) {
     for (auto& [main_frame_host, context_domains] :
          main_frame_hosts_with_context_domains) {
       topics_site_data_manager_->OnBrowsingTopicsApiUsed(
           HashMainFrameHostForStorage(main_frame_host),
           base::flat_set<HashedDomain>(context_domains.begin(),
-                                       context_domains.end()));
+                                       context_domains.end()),
+          base::Time::Now());
     }
 
     task_environment_.RunUntilIdle();
   }
 
   std::vector<optimization_guide::WeightedIdentifier> TopicsAndWeight(
-      std::vector<int32_t> topics,
+      const std::vector<int32_t>& topics,
       double weight) {
     std::vector<optimization_guide::WeightedIdentifier> result;
     for (int32_t topic : topics) {
@@ -213,7 +214,7 @@ TEST_F(BrowsingTopicsCalculatorTest, PermissionDenied) {
   privacy_sandbox_settings_->SetPrivacySandboxEnabled(false);
 
   EpochTopics result = CalculateTopics();
-  EXPECT_FALSE(result.HasValidTopics());
+  EXPECT_TRUE(result.empty());
 
   histograms.ExpectUniqueSample(
       "BrowsingTopics.EpochTopicsCalculation.CalculatorResultStatus",
@@ -227,7 +228,7 @@ TEST_F(BrowsingTopicsCalculatorTest, ApiUsageContextQueryError) {
   topics_site_data_manager_->SetQueryFailureOverride();
 
   EpochTopics result = CalculateTopics();
-  EXPECT_FALSE(result.HasValidTopics());
+  EXPECT_TRUE(result.empty());
 
   histograms.ExpectUniqueSample(
       "BrowsingTopics.EpochTopicsCalculation.CalculatorResultStatus",
@@ -239,7 +240,7 @@ TEST_F(BrowsingTopicsCalculatorTest, AnnotationExecutionError) {
   base::HistogramTester histograms;
 
   EpochTopics result = CalculateTopics();
-  EXPECT_FALSE(result.HasValidTopics());
+  EXPECT_TRUE(result.empty());
 
   histograms.ExpectUniqueSample(
       "BrowsingTopics.EpochTopicsCalculation.CalculatorResultStatus",
@@ -267,7 +268,7 @@ TEST_F(BrowsingTopicsCalculatorUnsupporedTaxonomyVersionTest,
       *optimization_guide::TestModelInfoBuilder().SetVersion(1).Build(), {});
 
   EpochTopics result = CalculateTopics();
-  EXPECT_FALSE(result.HasValidTopics());
+  EXPECT_TRUE(result.empty());
 
   histograms.ExpectUniqueSample(
       "BrowsingTopics.EpochTopicsCalculation.CalculatorResultStatus",
@@ -283,7 +284,7 @@ TEST_F(BrowsingTopicsCalculatorTest, TopicsMetadata) {
       *optimization_guide::TestModelInfoBuilder().SetVersion(1).Build(), {});
 
   EpochTopics result1 = CalculateTopics();
-  EXPECT_TRUE(result1.HasValidTopics());
+  EXPECT_FALSE(result1.empty());
   EXPECT_EQ(result1.taxonomy_size(), kTaxonomySize);
   EXPECT_EQ(result1.taxonomy_version(), kTaxonomyVersion);
   EXPECT_EQ(result1.model_version(), 1);
@@ -300,7 +301,7 @@ TEST_F(BrowsingTopicsCalculatorTest, TopicsMetadata) {
       *optimization_guide::TestModelInfoBuilder().SetVersion(50).Build(), {});
 
   EpochTopics result2 = CalculateTopics();
-  EXPECT_TRUE(result2.HasValidTopics());
+  EXPECT_FALSE(result2.empty());
   EXPECT_EQ(result2.taxonomy_size(), kTaxonomySize);
   EXPECT_EQ(result2.taxonomy_version(), kTaxonomyVersion);
   EXPECT_EQ(result2.model_version(), 50);