[Background Sync] Use the UKM background service for recording.

Use the ukm service for recording background events instead of
manually querying the history service.

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

chrome/browser/background_sync/background_sync_controller_factory.cc

@@ -6,7 +6,7 @@
 
 #include "chrome/browser/background_sync/background_sync_controller_impl.h"
 #include "chrome/browser/engagement/site_engagement_service_factory.h"
-#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/metrics/ukm_background_recorder_service.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
@@ -28,7 +28,7 @@ BackgroundSyncControllerFactory::BackgroundSyncControllerFactory()
     : BrowserContextKeyedServiceFactory(
           "BackgroundSyncService",
           BrowserContextDependencyManager::GetInstance()) {
-  DependsOn(HistoryServiceFactory::GetInstance());
+  DependsOn(ukm::UkmBackgroundRecorderFactory::GetInstance());
   DependsOn(SiteEngagementServiceFactory::GetInstance());
 }
 

chrome/browser/background_sync/background_sync_controller_impl.cc

@@ -9,7 +9,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/engagement/site_engagement_service.h"
-#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/metrics/ukm_background_recorder_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/variations/variations_associated_data.h"
@@ -48,9 +48,8 @@ const char BackgroundSyncControllerImpl::kMaxSyncEventDurationName[] =
 BackgroundSyncControllerImpl::BackgroundSyncControllerImpl(Profile* profile)
     : profile_(profile),
       site_engagement_service_(SiteEngagementService::Get(profile)),
-      background_sync_metrics_(HistoryServiceFactory::GetForProfile(
-          profile_,
-          ServiceAccessType::EXPLICIT_ACCESS)) {
+      background_sync_metrics_(
+          ukm::UkmBackgroundRecorderFactory::GetForProfile(profile_)) {
   DCHECK(profile_);
   DCHECK(site_engagement_service_);
 }

chrome/browser/background_sync/background_sync_metrics.cc

@@ -5,15 +5,15 @@
 #include "chrome/browser/background_sync/background_sync_metrics.h"
 
 #include "base/bind.h"
-#include "components/history/core/browser/history_service.h"
+#include "chrome/browser/metrics/ukm_background_recorder_service.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
 #include "services/metrics/public/cpp/ukm_recorder.h"
 #include "url/origin.h"
 
 BackgroundSyncMetrics::BackgroundSyncMetrics(
-    history::HistoryService* history_service)
-    : history_service_(history_service), weak_ptr_factory_(this) {
-  DCHECK(history_service_);
+    ukm::UkmBackgroundRecorderService* ukm_background_service)
+    : ukm_background_service_(ukm_background_service), weak_ptr_factory_(this) {
+  DCHECK(ukm_background_service_);
 }
 
 BackgroundSyncMetrics::~BackgroundSyncMetrics() = default;
@@ -22,17 +22,14 @@ void BackgroundSyncMetrics::MaybeRecordRegistrationEvent(
     const url::Origin& origin,
     bool can_fire,
     bool is_reregistered) {
-  // TODO(crbug.com/952870): Move this to a better mechanism for background
-  // UKM recording.
-  history_service_->GetVisibleVisitCountToHost(
-      origin.GetURL(),
-      base::BindRepeating(
-          &BackgroundSyncMetrics::DidGetVisibleVisitCount,
+  ukm_background_service_->GetBackgroundSourceIdIfAllowed(
+      origin,
+      base::BindOnce(
+          &BackgroundSyncMetrics::DidGetBackgroundSourceId,
           weak_ptr_factory_.GetWeakPtr(),
-          base::BindRepeating(&BackgroundSyncMetrics::RecordRegistrationEvent,
-                              weak_ptr_factory_.GetWeakPtr(), origin, can_fire,
-                              is_reregistered)),
-      &task_tracker_);
+          base::BindOnce(&BackgroundSyncMetrics::RecordRegistrationEvent,
+                         weak_ptr_factory_.GetWeakPtr(), can_fire,
+                         is_reregistered)));
 }
 
 void BackgroundSyncMetrics::MaybeRecordCompletionEvent(
@@ -40,41 +37,32 @@ void BackgroundSyncMetrics::MaybeRecordCompletionEvent(
     blink::ServiceWorkerStatusCode status_code,
     int num_attempts,
     int max_attempts) {
-  // TODO(crbug.com/952870): Move this to a better mechanism for background
-  // UKM recording.
-  history_service_->GetVisibleVisitCountToHost(
-      origin.GetURL(),
-      base::BindRepeating(
-          &BackgroundSyncMetrics::DidGetVisibleVisitCount,
-          weak_ptr_factory_.GetWeakPtr(),
-          base::BindRepeating(&BackgroundSyncMetrics::RecordCompletionEvent,
-                              weak_ptr_factory_.GetWeakPtr(), origin,
-                              status_code, num_attempts, max_attempts)),
-      &task_tracker_);
+  ukm_background_service_->GetBackgroundSourceIdIfAllowed(
+      origin, base::BindOnce(
+                  &BackgroundSyncMetrics::DidGetBackgroundSourceId,
+                  weak_ptr_factory_.GetWeakPtr(),
+                  base::BindOnce(&BackgroundSyncMetrics::RecordCompletionEvent,
+                                 weak_ptr_factory_.GetWeakPtr(), status_code,
+                                 num_attempts, max_attempts)));
 }
 
-void BackgroundSyncMetrics::DidGetVisibleVisitCount(
-    base::OnceClosure visit_closure,
-    bool did_determine,
-    int num_visits,
-    base::Time first_visit_time) {
-  if (!did_determine || !num_visits)
+void BackgroundSyncMetrics::DidGetBackgroundSourceId(
+    RecordCallback record_callback,
+    base::Optional<ukm::SourceId> source_id) {
+  // This background event did not meet the requirements for the UKM service.
+  if (!source_id)
     return;
 
-  std::move(visit_closure).Run();
+  std::move(record_callback).Run(*source_id);
   if (ukm_event_recorded_for_testing_)
     std::move(ukm_event_recorded_for_testing_).Run();
 }
 
-void BackgroundSyncMetrics::RecordRegistrationEvent(const url::Origin& origin,
-                                                    bool can_fire,
-                                                    bool is_reregistered) {
-  ukm::SourceId source_id = ukm::UkmRecorder::GetNewSourceID();
+void BackgroundSyncMetrics::RecordRegistrationEvent(bool can_fire,
+                                                    bool is_reregistered,
+                                                    ukm::SourceId source_id) {
   ukm::UkmRecorder* recorder = ukm::UkmRecorder::Get();
   DCHECK(recorder);
-  // |origin| was checked to exist in the profile history. This is the origin
-  // of the Service Worker tied to the Background Sync registration.
-  recorder->UpdateSourceURL(source_id, origin.GetURL());
 
   ukm::builders::BackgroundSyncRegistered(source_id)
       .SetCanFire(can_fire)
@@ -83,16 +71,12 @@ void BackgroundSyncMetrics::RecordRegistrationEvent(const url::Origin& origin,
 }
 
 void BackgroundSyncMetrics::RecordCompletionEvent(
-    const url::Origin& origin,
     blink::ServiceWorkerStatusCode status_code,
     int num_attempts,
-    int max_attempts) {
-  ukm::SourceId source_id = ukm::UkmRecorder::GetNewSourceID();
+    int max_attempts,
+    ukm::SourceId source_id) {
   ukm::UkmRecorder* recorder = ukm::UkmRecorder::Get();
   DCHECK(recorder);
-  // |origin| was checked to exist in the profile history. This is the origin
-  // of the Service Worker tied to the Background Sync registration.
-  recorder->UpdateSourceURL(source_id, origin.GetURL());
 
   ukm::builders::BackgroundSyncCompleted(source_id)
       .SetStatus(static_cast<int>(status_code))

chrome/browser/background_sync/background_sync_metrics.h

@@ -5,14 +5,16 @@
 #ifndef CHROME_BROWSER_BACKGROUND_SYNC_BACKGROUND_SYNC_METRICS_H_
 #define CHROME_BROWSER_BACKGROUND_SYNC_BACKGROUND_SYNC_METRICS_H_
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "base/task/cancelable_task_tracker.h"
+#include "base/optional.h"
+#include "services/metrics/public/cpp/ukm_source_id.h"
 #include "third_party/blink/public/common/service_worker/service_worker_status_code.h"
 
-namespace history {
-class HistoryService;
-}  // namespace history
+namespace ukm {
+class UkmBackgroundRecorderService;
+}  // namespace ukm
 
 namespace url {
 class Origin;
@@ -21,7 +23,10 @@ class Origin;
 // Lives entirely on the UI thread.
 class BackgroundSyncMetrics {
  public:
-  explicit BackgroundSyncMetrics(history::HistoryService* history_service);
+  using RecordCallback = base::OnceCallback<void(ukm::SourceId)>;
+
+  explicit BackgroundSyncMetrics(
+      ukm::UkmBackgroundRecorderService* ukm_background_service);
   ~BackgroundSyncMetrics();
 
   void MaybeRecordRegistrationEvent(const url::Origin& origin,
@@ -36,23 +41,19 @@ class BackgroundSyncMetrics {
  private:
   friend class BackgroundSyncMetricsBrowserTest;
 
-  void DidGetVisibleVisitCount(base::OnceClosure visit_closure,
-                               bool did_determine,
-                               int num_visits,
-                               base::Time first_visit_time);
-  void RecordRegistrationEvent(const url::Origin& origin,
-                               bool can_fire,
-                               bool is_reregistered);
+  void DidGetBackgroundSourceId(RecordCallback record_callback,
+                                base::Optional<ukm::SourceId> source_id);
 
-  void RecordCompletionEvent(const url::Origin& origin,
-                             blink::ServiceWorkerStatusCode status_code,
-                             int num_attempts,
-                             int max_attempts);
+  void RecordRegistrationEvent(bool can_fire,
+                               bool is_reregistered,
+                               ukm::SourceId source_id);
 
-  history::HistoryService* history_service_;
+  void RecordCompletionEvent(blink::ServiceWorkerStatusCode status_code,
+                             int num_attempts,
+                             int max_attempts,
+                             ukm::SourceId source_id);
 
-  // Task tracker used for querying URLs in the history service.
-  base::CancelableTaskTracker task_tracker_;
+  ukm::UkmBackgroundRecorderService* ukm_background_service_;
 
   // Used to signal tests that a UKM event has been recorded.
   base::OnceClosure ukm_event_recorded_for_testing_;

chrome/browser/background_sync/background_sync_metrics_browsertest.cc

@@ -6,10 +6,10 @@
 
 #include <memory>
 
-#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/metrics/ukm_background_recorder_service.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
-#include "components/history/core/browser/history_service.h"
+#include "chrome/test/base/ui_test_utils.h"
 #include "components/ukm/test_ukm_recorder.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
 #include "url/gurl.h"
@@ -24,17 +24,18 @@ class BackgroundSyncMetricsBrowserTest : public InProcessBrowserTest {
     Profile* profile = browser()->profile();
 
     recorder_ = std::make_unique<ukm::TestAutoSetUkmRecorder>();
-    auto* history_service = HistoryServiceFactory::GetForProfile(
-        profile, ServiceAccessType::EXPLICIT_ACCESS);
-    DCHECK(history_service);
+    auto* ukm_background_service =
+        ukm::UkmBackgroundRecorderFactory::GetForProfile(profile);
+    DCHECK(ukm_background_service);
 
     background_sync_metrics_ =
-        std::make_unique<BackgroundSyncMetrics>(history_service);
+        std::make_unique<BackgroundSyncMetrics>(ukm_background_service);
 
     // Adds the URL to the history so that UKM events for this origin are
     // recorded.
-    history_service->AddPage(GURL("https://backgroundsync.com/page"),
-                             base::Time::Now(), history::SOURCE_BROWSED);
+    ASSERT_TRUE(embedded_test_server()->Start());
+    GURL url(embedded_test_server()->GetURL("/links.html"));
+    ui_test_utils::NavigateToURL(browser(), url);
   }
 
  protected:
@@ -52,14 +53,9 @@ class BackgroundSyncMetricsBrowserTest : public InProcessBrowserTest {
 };
 
 IN_PROC_BROWSER_TEST_F(BackgroundSyncMetricsBrowserTest,
-                       NoUkmRecordingsWithMissingHistory) {
+                       BackgroundSyncUkmEventsAreRecorded) {
   background_sync_metrics_->MaybeRecordRegistrationEvent(
-      url::Origin::Create(GURL("https://notinhistory.com")),
-      /* can_fire= */ false,
-      /* is_reregistered= */ false);
-
-  background_sync_metrics_->MaybeRecordRegistrationEvent(
-      url::Origin::Create(GURL("https://backgroundsync.com")),
+      url::Origin::Create(embedded_test_server()->base_url().GetOrigin()),
       /* can_fire= */ true,
       /* is_reregistered= */ false);
   WaitForUkm();
@@ -77,19 +73,11 @@ IN_PROC_BROWSER_TEST_F(BackgroundSyncMetricsBrowserTest,
   }
 
   background_sync_metrics_->MaybeRecordCompletionEvent(
-      url::Origin::Create(GURL("https://backgroundsync.com")),
+      url::Origin::Create(embedded_test_server()->base_url().GetOrigin()),
       /* status_code= */ blink::ServiceWorkerStatusCode::kOk,
       /* num_attempts= */ 2, /* max_attempts= */ 5);
   WaitForUkm();
 
-  // Sanity check that no additional BackgroundSyncRegistered events were
-  // logged.
-  {
-    auto entries = recorder_->GetEntriesByName(
-        ukm::builders::BackgroundSyncRegistered::kEntryName);
-    EXPECT_EQ(entries.size(), 1u);
-  }
-
   {
     auto entries = recorder_->GetEntriesByName(
         ukm::builders::BackgroundSyncCompleted::kEntryName);

services/metrics/public/cpp/ukm_recorder.h

@@ -18,7 +18,6 @@
 #include "url/gurl.h"
 
 class BackgroundFetchDelegateImpl;
-class BackgroundSyncMetrics;
 class IOSChromePasswordManagerClient;
 class MediaEngagementSession;
 class PlatformNotificationServiceImpl;
@@ -105,7 +104,6 @@ class METRICS_EXPORT UkmRecorder {
 
  private:
   friend BackgroundFetchDelegateImpl;
-  friend BackgroundSyncMetrics;
   friend DelegatingUkmRecorder;
   friend IOSChromePasswordManagerClient;
   friend MediaEngagementSession;