[Background Fetch] Use UkmBackgroundRecorderService for recording UKM.

Replace manual privacy checks in Background Fetch code for recording
background events with calls to UkmBackgroundRecorder, which handles
them.

Bug: 952870
Change-Id: Ib48ce0f5d52447070926c026d398e7acbb5e35dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1601268
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658590}

chrome/browser/background_fetch/background_fetch_delegate_factory.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/background_fetch/background_fetch_delegate_impl.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/download/download_service_factory.h"
+#include "chrome/browser/metrics/ukm_background_recorder_service.h"
 #include "chrome/browser/offline_items_collection/offline_content_aggregator_factory.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
@@ -38,6 +39,7 @@ BackgroundFetchDelegateFactory::BackgroundFetchDelegateFactory()
   DependsOn(DownloadServiceFactory::GetInstance());
   DependsOn(HostContentSettingsMapFactory::GetInstance());
   DependsOn(OfflineContentAggregatorFactory::GetInstance());
+  DependsOn(ukm::UkmBackgroundRecorderFactory::GetInstance());
 }
 
 BackgroundFetchDelegateFactory::~BackgroundFetchDelegateFactory() {}

chrome/browser/background_fetch/background_fetch_delegate_impl.cc

@@ -17,14 +17,13 @@
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/download/download_request_limiter.h"
 #include "chrome/browser/download/download_service_factory.h"
-#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/metrics/ukm_background_recorder_service.h"
 #include "chrome/browser/offline_items_collection/offline_content_aggregator_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings_types.h"
 #include "components/download/public/background_service/download_params.h"
 #include "components/download/public/background_service/download_service.h"
-#include "components/history/core/browser/history_service.h"
 #include "components/offline_items_collection/core/offline_content_aggregator.h"
 #include "components/offline_items_collection/core/offline_item.h"
 #include "content/public/browser/background_fetch_description.h"
@@ -399,47 +398,27 @@ void BackgroundFetchDelegateImpl::
     RecordBackgroundFetchDeletingRegistrationUkmEvent(
         const url::Origin& origin,
         bool user_initiated_abort) {
-  // Log the UKM event anyway, if the origin can be found in the user's
-  // history database.
-  history::HistoryService* history_service =
-      HistoryServiceFactory::GetForProfile(profile_,
-                                           ServiceAccessType::EXPLICIT_ACCESS);
-  DCHECK(history_service);
-  const GURL origin_url = origin.GetURL();
-  history_service->GetVisibleVisitCountToHost(
-      origin_url,
-      base::BindRepeating(&BackgroundFetchDelegateImpl::DidQueryUrl,
-                          weak_ptr_factory_.GetWeakPtr(), origin_url,
-                          user_initiated_abort),
-      &task_tracker_);
-}
-
-void BackgroundFetchDelegateImpl::DidQueryUrl(const GURL& origin_url,
-                                              bool user_initiated_abort,
-                                              bool success,
-                                              int num_visits,
-                                              base::Time first_visit) {
-  // This notifies the tests that the history query has completed.
-  if (history_query_complete_closure_for_testing_) {
-    base::PostTask(FROM_HERE,
-                   std::move(history_query_complete_closure_for_testing_));
-  }
-
-  if (!success || !num_visits)
+  auto* ukm_background_service =
+      ukm::UkmBackgroundRecorderFactory::GetForProfile(profile_);
+  ukm_background_service->GetBackgroundSourceIdIfAllowed(
+      origin,
+      base::BindOnce(&BackgroundFetchDelegateImpl::DidGetBackgroundSourceId,
+                     weak_ptr_factory_.GetWeakPtr(), user_initiated_abort));
+}
+
+void BackgroundFetchDelegateImpl::DidGetBackgroundSourceId(
+    bool user_initiated_abort,
+    base::Optional<ukm::SourceId> source_id) {
+  // This background event did not meet the requirements for the UKM service.
+  if (!source_id)
     return;
 
-  ukm::SourceId source_id = ukm::UkmRecorder::GetNewSourceID();
-  ukm::UkmRecorder* recorder = ukm::UkmRecorder::Get();
-  DCHECK(recorder);
-
-  // This is OK from a privacy perspective since we have verified that the
-  // origin the background fetch is associated with, is in the history service
-  // database.
-  recorder->UpdateSourceURL(source_id, origin_url);
-
-  ukm::builders::BackgroundFetchDeletingRegistration(source_id)
+  ukm::builders::BackgroundFetchDeletingRegistration(*source_id)
       .SetUserInitiatedAbort(user_initiated_abort)
       .Record(ukm::UkmRecorder::Get());
+
+  if (ukm_event_recorded_for_testing_)
+    std::move(ukm_event_recorded_for_testing_).Run();
 }
 
 void BackgroundFetchDelegateImpl::MarkJobComplete(

chrome/browser/background_fetch/background_fetch_delegate_impl.h

@@ -13,7 +13,6 @@
 
 #include "base/containers/flat_set.h"
 #include "base/memory/weak_ptr.h"
-#include "base/task/cancelable_task_tracker.h"
 #include "components/download/public/background_service/download_params.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/offline_items_collection/core/offline_content_provider.h"
@@ -132,9 +131,8 @@ class BackgroundFetchDelegateImpl
   void GetUploadData(const std::string& download_guid,
                      download::GetUploadDataCallback callback);
 
-  void set_history_query_complete_closure_for_testing(
-      base::OnceClosure closure) {
-    history_query_complete_closure_for_testing_ = std::move(closure);
+  void set_ukm_event_recorded_for_testing(base::OnceClosure closure) {
+    ukm_event_recorded_for_testing_ = std::move(closure);
   }
 
   base::WeakPtr<BackgroundFetchDelegateImpl> GetWeakPtr() {
@@ -263,17 +261,13 @@ class BackgroundFetchDelegateImpl
   base::WeakPtr<Client> GetClient(const std::string& job_unique_id);
 
   // Helper methods for recording BackgroundFetchDeletingRegistration UKM event.
-  // We try to look for any URL corresponding to this origin in the user's
-  // browsing history. If we find one, we record the UKM event with a new
-  // SourceID, after associating it with |origin|.
+  // We check with UkmBackgroundRecorderService whether this event for |origin|
+  // can be recorded.
   void RecordBackgroundFetchDeletingRegistrationUkmEvent(
       const url::Origin& origin,
       bool user_initiated_abort);
-  void DidQueryUrl(const GURL& origin_url,
-                   bool user_initiated_abort,
-                   bool success,
-                   int num_visits,
-                   base::Time first_visit);
+  void DidGetBackgroundSourceId(bool user_initiated_abort,
+                                base::Optional<ukm::SourceId> source_id);
 
   // The profile this service is being created for.
   Profile* profile_;
@@ -298,12 +292,8 @@ class BackgroundFetchDelegateImpl
   // Set of Observers to be notified of any changes to the shown notifications.
   std::set<Observer*> observers_;
 
-  // Task tracker used for querying URLs in the history service.
-  base::CancelableTaskTracker task_tracker_;
-
-  // Testing-only closure to observe when the history service query has
-  // finished, and the result of logging UKM can be observed.
-  base::OnceClosure history_query_complete_closure_for_testing_;
+  // Testing-only closure to inform tests when a UKM event has been recorded.
+  base::OnceClosure ukm_event_recorded_for_testing_;
 
   base::WeakPtrFactory<BackgroundFetchDelegateImpl> weak_ptr_factory_;
 

chrome/browser/background_fetch/background_fetch_delegate_impl_unittest.cc

@@ -31,14 +31,20 @@ class BackgroundFetchDelegateImplTest : public testing::Test {
     recorder_ = std::make_unique<ukm::TestAutoSetUkmRecorder>();
     delegate_ = static_cast<BackgroundFetchDelegateImpl*>(
         profile_.GetBackgroundFetchDelegate());
-  }
 
-  BackgroundFetchDelegateImpl* delegate() { return delegate_; }
+    // Add |kOriginUrl| to |profile_|'s history so the UKM background
+    // recording conditions are met.
+    ASSERT_TRUE(profile_.CreateHistoryService(/* delete_file= */ true,
+                                              /* no_db= */ false));
+    auto* history_service = HistoryServiceFactory::GetForProfile(
+        &profile_, ServiceAccessType::EXPLICIT_ACCESS);
+    history_service->AddPage(kOriginUrl, base::Time::Now(),
+                             history::SOURCE_BROWSED);
+  }
 
-  void WaitForHistoryQueryToComplete() {
+  void WaitForUkmEvent() {
     base::RunLoop run_loop;
-    delegate()->set_history_query_complete_closure_for_testing(
-        run_loop.QuitClosure());
+    delegate_->set_ukm_event_recorded_for_testing(run_loop.QuitClosure());
     run_loop.Run();
   }
 
@@ -52,93 +58,9 @@ class BackgroundFetchDelegateImplTest : public testing::Test {
   TestingProfile profile_;
 };
 
-TEST_F(BackgroundFetchDelegateImplTest, HistoryServiceIntegration) {
-  ASSERT_TRUE(profile_.CreateHistoryService(/* delete_file= */ true,
-                                            /* no_db= */ false));
-  auto* history_service = HistoryServiceFactory::GetForProfile(
-      &profile_, ServiceAccessType::EXPLICIT_ACCESS);
-
-  bool user_initiated_abort = true;
-
-  url::Origin origin = url::Origin::Create(kOriginUrl);
-  size_t initial_entries_count = recorder_->entries_count();
-  size_t expected_entries_count = initial_entries_count + 1;
-
-  // First, attempt to record an event for |origin| before it's been added to
-  // the |history_service|. Nothing should be recorded.
-  delegate()->RecordBackgroundFetchDeletingRegistrationUkmEvent(
-      origin, user_initiated_abort);
-  WaitForHistoryQueryToComplete();
-
-  EXPECT_EQ(recorder_->entries_count(), initial_entries_count);
-
-  // Now add |origin| to the |history_service|. After this, registration
-  // deletion should cause UKM event to be logged.
-  history_service->AddPage(kOriginUrl, base::Time::Now(),
-                           history::SOURCE_BROWSED);
-  delegate()->RecordBackgroundFetchDeletingRegistrationUkmEvent(
-      origin, user_initiated_abort);
-  WaitForHistoryQueryToComplete();
-
-  EXPECT_EQ(recorder_->entries_count(), expected_entries_count);
-
-  // Delete the |origin| from the |history_service|. Subsequent events should
-  // not be logged to UKM anymore.
-  history_service->DeleteURL(kOriginUrl);
-  delegate()->RecordBackgroundFetchDeletingRegistrationUkmEvent(
-      origin, user_initiated_abort);
-  WaitForHistoryQueryToComplete();
-
-  EXPECT_EQ(recorder_->entries_count(), expected_entries_count);
-}
-
-TEST_F(BackgroundFetchDelegateImplTest, HistoryServiceIntegrationUrlVsOrigin) {
-  ASSERT_TRUE(profile_.CreateHistoryService(/* delete_file= */ true,
-                                            /* no_db= */ false));
-  auto* history_service = HistoryServiceFactory::GetForProfile(
-      &profile_, ServiceAccessType::EXPLICIT_ACCESS);
-
-  bool user_initiated_abort = true;
-
-  url::Origin origin = url::Origin::Create(kOriginUrl);
-  size_t initial_entries_count = recorder_->entries_count();
-  size_t expected_entries_count = initial_entries_count + 1;
-
-  // First, attempt to record an event for |origin| before it's been added to
-  // the |history_service|. Nothing should be recorded.
-  delegate()->RecordBackgroundFetchDeletingRegistrationUkmEvent(
-      origin, user_initiated_abort);
-  WaitForHistoryQueryToComplete();
-
-  EXPECT_EQ(recorder_->entries_count(), initial_entries_count);
-
-  // Now add |kPageUrl| to the |history_service|. After this, registration
-  // deletion shouldn't cause UKM event to be logged for |origin|.
-  history_service->AddPage(kPageUrl, base::Time::Now(),
-                           history::SOURCE_BROWSED);
-  delegate()->RecordBackgroundFetchDeletingRegistrationUkmEvent(
-      origin, user_initiated_abort);
-  WaitForHistoryQueryToComplete();
-
-  EXPECT_EQ(recorder_->entries_count(), expected_entries_count);
-
-  // Delete the |kPageUrl| from the |history_service|. Subsequent events should
-  // not be logged to UKM anymore.
-  history_service->DeleteURL(kPageUrl);
-  delegate()->RecordBackgroundFetchDeletingRegistrationUkmEvent(
-      origin, user_initiated_abort);
-  WaitForHistoryQueryToComplete();
-
-  EXPECT_EQ(recorder_->entries_count(), expected_entries_count);
-}
-
 TEST_F(BackgroundFetchDelegateImplTest, RecordUkmEvent) {
   url::Origin origin = url::Origin::Create(kOriginUrl);
 
-  // Origin not found in the history service, nothing is recorded.
-  delegate()->DidQueryUrl(kOriginUrl, /* user_initiated_abort= */ true,
-                          /* success= */ false, /* num_visits= */ 0,
-                          base::Time::Now());
   {
     std::vector<const ukm::mojom::UkmEntry*> entries =
         recorder_->GetEntriesByName(
@@ -146,10 +68,10 @@ TEST_F(BackgroundFetchDelegateImplTest, RecordUkmEvent) {
     EXPECT_EQ(entries.size(), 0u);
   }
 
-  // History Service finds the origin and the UKM event is logged.
-  delegate()->DidQueryUrl(kOriginUrl, /* user_initiated_abort= */ true,
-                          /* success= */ true, /* num_visits= */ 2,
-                          base::Time::Now());
+  delegate_->RecordBackgroundFetchDeletingRegistrationUkmEvent(
+      origin, /* user_initiated_abort= */ true);
+  WaitForUkmEvent();
+
   {
     std::vector<const ukm::mojom::UkmEntry*> entries =
         recorder_->GetEntriesByName(

services/metrics/public/cpp/ukm_recorder.h

@@ -17,7 +17,6 @@
 #include "services/metrics/public/mojom/ukm_interface.mojom-forward.h"
 #include "url/gurl.h"
 
-class BackgroundFetchDelegateImpl;
 class IOSChromePasswordManagerClient;
 class MediaEngagementSession;
 class PluginInfoHostImpl;
@@ -102,7 +101,6 @@ class METRICS_EXPORT UkmRecorder {
   void RecordAppURL(base::UkmSourceId source_id, const GURL& url);
 
  private:
-  friend BackgroundFetchDelegateImpl;
   friend DelegatingUkmRecorder;
   friend IOSChromePasswordManagerClient;
   friend MediaEngagementSession;