Add domain diversity metrics for iOS

This CL moves the DomainDiversityReporter service from
//chrome/browser/history into //components/history/metrics and removes
the dependency on //content from this service. The factory for the
DomainDiversityReporter service is reimplemented for iOS.

Bug: 1132579
Change-Id: Ia4999a37a6028c48891d39c50188c3162ad4d3ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2558834Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Mingjing Zhang <mjzhang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837179}

chrome/browser/BUILD.gn

@@ -573,8 +573,6 @@ static_library("browser") {
     "history/chrome_history_backend_client.h",
     "history/chrome_history_client.cc",
     "history/chrome_history_client.h",
-    "history/domain_diversity_reporter.cc",
-    "history/domain_diversity_reporter.h",
     "history/domain_diversity_reporter_factory.cc",
     "history/domain_diversity_reporter_factory.h",
     "history/history_service_factory.cc",
@@ -2091,6 +2089,7 @@ static_library("browser") {
     "//components/history/content/browser",
     "//components/history/core/browser",
     "//components/history/core/common",
+    "//components/history/metrics",
     "//components/infobars/content",
     "//components/infobars/core",
     "//components/invalidation/impl",

chrome/browser/DEPS

@@ -147,6 +147,7 @@ include_rules = [
   "+components/history/core/browser",
   "+components/history/core/common",
   "+components/history/core/test",
+  "+components/history/metrics",
   "+components/image_fetcher/core",
   "+components/infobars/content",
   "+components/infobars/core",

chrome/browser/history/domain_diversity_reporter_factory.cc

@@ -9,10 +9,10 @@
 #include "base/bind.h"
 #include "base/time/default_clock.h"
 #include "build/build_config.h"
-#include "chrome/browser/history/domain_diversity_reporter.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
+#include "components/history/metrics/domain_diversity_reporter.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
@@ -26,7 +26,8 @@ DomainDiversityReporter* DomainDiversityReporterFactory::GetForProfile(
 
 // static
 DomainDiversityReporterFactory* DomainDiversityReporterFactory::GetInstance() {
-  return base::Singleton<DomainDiversityReporterFactory>::get();
+  static base::NoDestructor<DomainDiversityReporterFactory> instance;
+  return instance.get();
 }
 
 // static

chrome/browser/history/domain_diversity_reporter_factory.h

@@ -7,7 +7,7 @@
 
 #include <memory>
 
-#include "base/memory/singleton.h"
+#include "base/no_destructor.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 
 class DomainDiversityReporter;
@@ -28,7 +28,7 @@ class DomainDiversityReporterFactory
       content::BrowserContext* profile);
 
  private:
-  friend struct base::DefaultSingletonTraits<DomainDiversityReporterFactory>;
+  friend class base::NoDestructor<DomainDiversityReporterFactory>;
 
   DomainDiversityReporterFactory();
   ~DomainDiversityReporterFactory() override;

chrome/test/BUILD.gn

@@ -3496,7 +3496,6 @@ test("unit_tests") {
     "../browser/google/google_search_domain_mixing_metrics_emitter_unittest.cc",
     "../browser/google/google_update_settings_unittest.cc",
     "../browser/heavy_ad_intervention/heavy_ad_blocklist_unittest.cc",
-    "../browser/history/domain_diversity_reporter_unittest.cc",
     "../browser/history/history_tab_helper_unittest.cc",
     "../browser/idle/idle_detection_permission_context_unittest.cc",
     "../browser/infobars/mock_infobar_service.cc",

components/BUILD.gn

@@ -100,6 +100,7 @@ test("components_unittests") {
     "//components/heap_profiling/in_process:unit_tests",
     "//components/history/core/browser:unit_tests",
     "//components/history/core/common:unit_tests",
+    "//components/history/metrics:unit_tests",
     "//components/image_fetcher/core:unit_tests",
     "//components/keyed_service/core:unit_tests",
     "//components/language/core/browser:unit_tests",

components/history/metrics/BUILD.gn

+# Copyright 2020 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+source_set("metrics") {
+  sources = [
+    "domain_diversity_reporter.cc",
+    "domain_diversity_reporter.h",
+  ]
+
+  deps = [
+    "//base",
+    "//components/history/core/browser",
+    "//components/pref_registry",
+    "//components/prefs",
+  ]
+}
+
+source_set("unit_tests") {
+  testonly = true
+  sources = [ "domain_diversity_reporter_unittest.cc" ]
+
+  deps = [
+    ":metrics",
+    "//base",
+    "//base/test:test_support",
+    "//components/history/core/browser",
+    "//components/history/core/test",
+    "//components/sync_preferences:test_support",
+    "//testing/gtest",
+  ]
+}

components/history/metrics/DEPS

+include_rules = [
+  "+components/pref_registry",
+  "+components/sync_preferences",
+]
\ No newline at end of file

chrome/browser/history/domain_diversity_reporter.cc → components/history/metrics/domain_diversity_reporter.cc

@@ -2,14 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/history/domain_diversity_reporter.h"
+#include "components/history/metrics/domain_diversity_reporter.h"
 
 #include "base/metrics/histogram_macros.h"
+#include "base/task/post_task.h"
+#include "base/task/task_traits.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
-#include "content/public/browser/browser_task_traits.h"
-#include "content/public/browser/browser_thread.h"
 
 namespace {
 // The interval between two successive domain metrics reports.
@@ -31,10 +31,9 @@ DomainDiversityReporter::DomainDiversityReporter(
       clock_(clock),
       history_service_observer_(this) {
   DCHECK_NE(prefs_, nullptr);
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  content::GetUIThreadTaskRunner({base::TaskPriority::BEST_EFFORT})
-      ->PostTask(
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(&DomainDiversityReporter::MaybeComputeDomainMetrics,
                      weak_ptr_factory_.GetWeakPtr()));
@@ -49,6 +48,8 @@ void DomainDiversityReporter::RegisterProfilePrefs(
 }
 
 void DomainDiversityReporter::MaybeComputeDomainMetrics() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (history_service_->BackendLoaded()) {
     // HistoryService is ready; proceed to start the domain metrics
     // computation task.
@@ -56,11 +57,13 @@ void DomainDiversityReporter::MaybeComputeDomainMetrics() {
   }
   // Observe history service and start reporting as soon as
   // the former is ready.
-  DCHECK(!history_service_observer_.IsObserving(history_service_));
-  history_service_observer_.Add(history_service_);
+  DCHECK(!history_service_observer_.IsObservingSource(history_service_));
+  history_service_observer_.Observe(history_service_);
 }
 
 void DomainDiversityReporter::ComputeDomainMetrics() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   base::Time time_last_report_triggered =
       prefs_->GetTime(kDomainDiversityReportingTimestamp);
   base::Time time_current_report_triggered = clock_->Now();
@@ -117,7 +120,7 @@ void DomainDiversityReporter::ComputeDomainMetrics() {
   }
 
   // The next reporting task is scheduled to run 24 hours later.
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::BindOnce(&DomainDiversityReporter::ComputeDomainMetrics,
                      weak_ptr_factory_.GetWeakPtr()),
@@ -127,6 +130,8 @@ void DomainDiversityReporter::ComputeDomainMetrics() {
 void DomainDiversityReporter::ReportDomainMetrics(
     base::Time time_current_report_triggered,
     history::DomainDiversityResults result) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   // An empty DomainDiversityResults indicates that |db_| is null in
   // HistoryBackend.
   if (result.empty())
@@ -149,11 +154,15 @@ void DomainDiversityReporter::ReportDomainMetrics(
 void DomainDiversityReporter::OnHistoryServiceLoaded(
     history::HistoryService* history_service) {
   DCHECK_EQ(history_service, history_service_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   ComputeDomainMetrics();
 }
 
 void DomainDiversityReporter::HistoryServiceBeingDeleted(
     history::HistoryService* history_service) {
-  history_service_observer_.RemoveAll();
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  history_service_observer_.Reset();
   cancelable_task_tracker_.TryCancelAll();
 }

chrome/browser/history/domain_diversity_reporter.h → components/history/metrics/domain_diversity_reporter.h

@@ -2,12 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_HISTORY_DOMAIN_DIVERSITY_REPORTER_H_
-#define CHROME_BROWSER_HISTORY_DOMAIN_DIVERSITY_REPORTER_H_
+#ifndef COMPONENTS_HISTORY_METRICS_DOMAIN_DIVERSITY_REPORTER_H_
+#define COMPONENTS_HISTORY_METRICS_DOMAIN_DIVERSITY_REPORTER_H_
 
 #include <vector>
 
-#include "base/scoped_observer.h"
+#include "base/scoped_observation.h"
+#include "base/sequence_checker.h"
 #include "base/time/clock.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/history_service_observer.h"
@@ -58,13 +59,16 @@ class DomainDiversityReporter : public KeyedService,
   PrefService* prefs_;
   base::Clock* clock_;
 
-  ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
+  base::ScopedObservation<history::HistoryService,
+                          history::HistoryServiceObserver>
       history_service_observer_;
   base::CancelableTaskTracker cancelable_task_tracker_;
 
+  SEQUENCE_CHECKER(sequence_checker_);
+
   base::WeakPtrFactory<DomainDiversityReporter> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(DomainDiversityReporter);
 };
 
-#endif  // CHROME_BROWSER_HISTORY_DOMAIN_DIVERSITY_REPORTER_H_
+#endif  // COMPONENTS_HISTORY_MERICS_DOMAIN_DIVERSITY_REPORTER_H_

chrome/browser/history/domain_diversity_reporter_unittest.cc → components/history/metrics/domain_diversity_reporter_unittest.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/history/domain_diversity_reporter.h"
+#include "components/history/metrics/domain_diversity_reporter.h"
 
 #include "base/feature_list.h"
 #include "base/files/scoped_temp_dir.h"
@@ -17,11 +17,7 @@
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/test/history_service_test_util.h"
 #include "components/history/core/test/test_history_database.h"
-#include "components/offline_pages/core/offline_clock.h"
-#include "components/prefs/pref_registry_simple.h"
-#include "components/prefs/testing_pref_service.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
-#include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -112,8 +108,8 @@ class DomainDiversityReporterTest : public testing::Test {
   // to fire. DomainDiversityReporter internally uses a |test_clock_| instead of
   // |task_environment_|'s clock because it needs to test very specific times
   // rather than just advance in deltas from Now().
-  content::BrowserTaskEnvironment task_environment_{
-      content::BrowserTaskEnvironment::TimeSource::MOCK_TIME};
+  base::test::TaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
 
   std::unique_ptr<DomainDiversityReporter> reporter_;
 

ios/chrome/browser/DEPS

@@ -32,6 +32,7 @@ include_rules = [
   "+components/history/core/browser",
   "+components/history/core/common",
   "+components/history/ios/browser",
+  "+components/history/metrics",
   "+components/image_fetcher/core",
   "+components/image_fetcher/ios",
   "+components/infobars/core",

ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm

@@ -26,6 +26,7 @@
 #include "ios/chrome/browser/feature_engagement/tracker_factory.h"
 #include "ios/chrome/browser/gcm/ios_chrome_gcm_profile_service_factory.h"
 #include "ios/chrome/browser/google/google_logo_service_factory.h"
+#include "ios/chrome/browser/history/domain_diversity_reporter_factory.h"
 #include "ios/chrome/browser/history/history_service_factory.h"
 #include "ios/chrome/browser/history/top_sites_factory.h"
 #include "ios/chrome/browser/history/web_history_service_factory.h"
@@ -115,6 +116,7 @@ void EnsureBrowserStateKeyedServiceFactoriesBuilt() {
   ConsentAuditorFactory::GetInstance();
   DeviceSharingManagerFactory::GetInstance();
   DiscoverFeedServiceFactory::GetInstance();
+  DomainDiversityReporterFactory::GetInstance();
   GoogleLogoServiceFactory::GetInstance();
   IdentityManagerFactory::GetInstance();
   IOSChromeContentSuggestionsServiceFactory::GetInstance();

ios/chrome/browser/history/BUILD.gn

@@ -4,6 +4,8 @@
 
 source_set("history") {
   sources = [
+    "domain_diversity_reporter_factory.h",
+    "domain_diversity_reporter_factory.mm",
     "history_backend_client_impl.cc",
     "history_backend_client_impl.h",
     "history_client_impl.cc",
@@ -24,6 +26,7 @@ source_set("history") {
     "//components/dom_distiller/core",
     "//components/history/core/browser",
     "//components/history/ios/browser",
+    "//components/history/metrics",
     "//components/keyed_service/core",
     "//components/keyed_service/ios",
     "//components/pref_registry",
@@ -37,6 +40,7 @@ source_set("history") {
     "//net",
     "//url",
   ]
+  configs += [ "//build/config/compiler:enable_arc" ]
 }
 
 source_set("tab_helper") {

ios/chrome/browser/history/domain_diversity_reporter_factory.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_CHROME_BROWSER_HISTORY_DOMAIN_DIVERSITY_REPORTER_FACTORY_H_
+#define IOS_CHROME_BROWSER_HISTORY_DOMAIN_DIVERSITY_REPORTER_FACTORY_H_
+
+#include <memory>
+
+#include "base/no_destructor.h"
+#include "components/keyed_service/ios/browser_state_keyed_service_factory.h"
+
+class DomainDiversityReporter;
+
+namespace user_prefs {
+class PrefRegistrySyncable;
+}
+
+namespace web {
+class BrowserState;
+}
+
+// Singleton that creates all DomainDiversityReporter instances and associates
+// them with BrowserState.
+class DomainDiversityReporterFactory : public BrowserStateKeyedServiceFactory {
+ public:
+  static DomainDiversityReporter* GetForBrowserState(
+      web::BrowserState* browser_state);
+
+  static DomainDiversityReporterFactory* GetInstance();
+
+ private:
+  friend class base::NoDestructor<DomainDiversityReporterFactory>;
+
+  DomainDiversityReporterFactory();
+  ~DomainDiversityReporterFactory() override;
+
+  // BrowserStateKeyedServiceFactory implementation
+  void RegisterBrowserStatePrefs(
+      user_prefs::PrefRegistrySyncable* registry) override;
+
+  std::unique_ptr<KeyedService> BuildServiceInstanceFor(
+      web::BrowserState* browser_state) const override;
+
+  web::BrowserState* GetBrowserStateToUse(
+      web::BrowserState* browser_state) const override;
+  bool ServiceIsNULLWhileTesting() const override;
+  bool ServiceIsCreatedWithBrowserState() const override;
+};
+
+#endif  // IOS_CHROME_BROWSER_HISTORY_DOMAIN_DIVERSITY_REPORTER_FACTORY_H_
\ No newline at end of file

ios/chrome/browser/history/domain_diversity_reporter_factory.mm

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/chrome/browser/history/domain_diversity_reporter_factory.h"
+
+#import "base/bind.h"
+#import "base/time/default_clock.h"
+#import "build/build_config.h"
+#import "components/history/metrics/domain_diversity_reporter.h"
+#import "components/keyed_service/core/service_access_type.h"
+#import "components/keyed_service/ios/browser_state_dependency_manager.h"
+#import "components/pref_registry/pref_registry_syncable.h"
+#import "components/prefs/pref_service.h"
+#import "ios/chrome/browser/browser_state/browser_state_otr_helper.h"
+#import "ios/chrome/browser/browser_state/chrome_browser_state.h"
+#import "ios/chrome/browser/history/history_service_factory.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+// static
+DomainDiversityReporter* DomainDiversityReporterFactory::GetForBrowserState(
+    web::BrowserState* browser_state) {
+  return static_cast<DomainDiversityReporter*>(
+      GetInstance()->GetServiceForBrowserState(browser_state, /*create=*/true));
+}
+
+// static
+DomainDiversityReporterFactory* DomainDiversityReporterFactory::GetInstance() {
+  static base::NoDestructor<DomainDiversityReporterFactory> instance;
+  return instance.get();
+}
+
+DomainDiversityReporterFactory::DomainDiversityReporterFactory()
+    : BrowserStateKeyedServiceFactory(
+          "DomainDiversityReporter",
+          BrowserStateDependencyManager::GetInstance()) {
+  DependsOn(ios::HistoryServiceFactory::GetInstance());
+}
+
+DomainDiversityReporterFactory::~DomainDiversityReporterFactory() = default;
+
+std::unique_ptr<KeyedService>
+DomainDiversityReporterFactory::BuildServiceInstanceFor(
+    web::BrowserState* browser_state) const {
+  ChromeBrowserState* chrome_browser_state =
+      ChromeBrowserState::FromBrowserState(browser_state);
+  if (chrome_browser_state->IsOffTheRecord())
+    return nullptr;
+
+  history::HistoryService* history_service =
+      ios::HistoryServiceFactory::GetForBrowserState(
+          chrome_browser_state, ServiceAccessType::EXPLICIT_ACCESS);
+
+  return std::make_unique<DomainDiversityReporter>(
+      history_service, chrome_browser_state->GetPrefs(),
+      base::DefaultClock::GetInstance());
+}
+
+void DomainDiversityReporterFactory::RegisterBrowserStatePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  DomainDiversityReporter::RegisterProfilePrefs(registry);
+}
+
+web::BrowserState* DomainDiversityReporterFactory::GetBrowserStateToUse(
+    web::BrowserState* context) const {
+  return GetBrowserStateRedirectedInIncognito(context);
+}
+
+bool DomainDiversityReporterFactory::ServiceIsNULLWhileTesting() const {
+  return true;
+}
+
+bool DomainDiversityReporterFactory::ServiceIsCreatedWithBrowserState() const {
+  return true;
+}