Use UkmBackgroundRecorderService for background notification events

Remove manual history/privacy checks by delgating the recording decision to
UkmBackgroundRecorderService.

Bug: 952870
Change-Id: I2bea7bbbee6a43c21959b7c60c9640d4a8ed4e8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1601042Reviewed-by: Richard Knoll <knollr@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658572}

chrome/browser/notifications/platform_notification_service_factory.cc

@@ -8,6 +8,7 @@
 
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
+#include "chrome/browser/metrics/ukm_background_recorder_service.h"
 #include "chrome/browser/notifications/metrics/notification_metrics_logger_factory.h"
 #include "chrome/browser/notifications/notification_display_service_factory.h"
 #include "chrome/browser/notifications/platform_notification_service_impl.h"
@@ -35,6 +36,7 @@ PlatformNotificationServiceFactory::PlatformNotificationServiceFactory()
   DependsOn(HostContentSettingsMapFactory::GetInstance());
   DependsOn(NotificationDisplayServiceFactory::GetInstance());
   DependsOn(NotificationMetricsLoggerFactory::GetInstance());
+  DependsOn(ukm::UkmBackgroundRecorderFactory::GetInstance());
 }
 
 KeyedService* PlatformNotificationServiceFactory::BuildServiceInstanceFor(

chrome/browser/notifications/platform_notification_service_impl.cc

@@ -18,7 +18,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
-#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/metrics/ukm_background_recorder_service.h"
 #include "chrome/browser/notifications/metrics/notification_metrics_logger.h"
 #include "chrome/browser/notifications/metrics/notification_metrics_logger_factory.h"
 #include "chrome/browser/notifications/notification_display_service.h"
@@ -52,6 +52,7 @@
 #include "ui/message_center/public/cpp/features.h"
 #include "ui/message_center/public/cpp/notification_types.h"
 #include "ui/message_center/public/cpp/notifier_id.h"
+#include "url/origin.h"
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 #include "extensions/browser/extension_registry.h"
@@ -335,18 +336,13 @@ void PlatformNotificationServiceImpl::RecordNotificationUkmEvent(
     return;
   }
 
-  // Query the HistoryService so we only record a notification if the origin is
-  // in the user's history.
-  history::HistoryService* history_service =
-      HistoryServiceFactory::GetForProfile(profile_,
-                                           ServiceAccessType::EXPLICIT_ACCESS);
-  DCHECK(history_service);
-  history_service->QueryURL(
-      data.origin, /*want_visits=*/false,
-      base::BindOnce(
-          &PlatformNotificationServiceImpl::OnUrlHistoryQueryComplete,
-          std::move(history_query_complete_closure_for_testing_), data),
-      &task_tracker_);
+  // Check if this event can be recorded via UKM.
+  auto* ukm_background_service =
+      ukm::UkmBackgroundRecorderFactory::GetForProfile(profile_);
+  ukm_background_service->GetBackgroundSourceIdIfAllowed(
+      url::Origin::Create(data.origin),
+      base::BindOnce(&PlatformNotificationServiceImpl::DidGetBackgroundSourceId,
+                     std::move(ukm_recorded_closure_for_testing_), data));
 }
 
 NotificationTriggerScheduler*
@@ -355,31 +351,15 @@ PlatformNotificationServiceImpl::GetNotificationTriggerScheduler() {
 }
 
 // static
-void PlatformNotificationServiceImpl::OnUrlHistoryQueryComplete(
-    base::OnceClosure callback,
+void PlatformNotificationServiceImpl::DidGetBackgroundSourceId(
+    base::OnceClosure recorded_closure,
     const content::NotificationDatabaseData& data,
-    bool found_url,
-    const history::URLRow& url_row,
-    const history::VisitVector& visits) {
-  // Post the |callback| closure to the current task runner to inform tests that
-  // the history query has completed.
-  if (callback)
-    base::PostTask(FROM_HERE, std::move(callback));
-
-  // Only record the notification if the |data.origin| is in the history
-  // service.
-  if (!found_url)
+    base::Optional<ukm::SourceId> source_id) {
+  // This background event did not meet the requirements for the UKM service.
+  if (!source_id)
     return;
 
-  ukm::UkmRecorder* recorder = ukm::UkmRecorder::Get();
-  DCHECK(recorder);
-  // We are using UpdateSourceURL as notifications are not tied to a navigation.
-  // This is ok from a privacy perspective as we have explicitly verified that
-  // |data.origin| is in the HistoryService before recording to UKM.
-  ukm::SourceId source_id = ukm::UkmRecorder::GetNewSourceID();
-  recorder->UpdateSourceURL(source_id, data.origin);
-
-  ukm::builders::Notification builder(source_id);
+  ukm::builders::Notification builder(*source_id);
 
   int64_t time_until_first_click_millis =
       data.time_until_first_click_millis.has_value()
@@ -412,7 +392,10 @@ void PlatformNotificationServiceImpl::OnUrlHistoryQueryComplete(
       .SetTimeUntilClose(time_until_close_millis)
       .SetTimeUntilFirstClick(time_until_first_click_millis)
       .SetTimeUntilLastClick(time_until_last_click_millis)
-      .Record(recorder);
+      .Record(ukm::UkmRecorder::Get());
+
+  if (recorded_closure)
+    std::move(recorded_closure).Run();
 }
 
 message_center::Notification

chrome/browser/notifications/platform_notification_service_impl.h

@@ -20,9 +20,9 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/buildflags.h"
 #include "components/content_settings/core/browser/content_settings_observer.h"
-#include "components/history/core/browser/history_service.h"
-#include "components/history/core/browser/history_types.h"
+#include "components/keyed_service/core/keyed_service.h"
 #include "content/public/browser/platform_notification_service.h"
+#include "services/metrics/public/cpp/ukm_source_id.h"
 #include "ui/message_center/public/cpp/notification.h"
 
 class GURL;
@@ -71,9 +71,8 @@ class PlatformNotificationServiceImpl
   void RecordNotificationUkmEvent(
       const content::NotificationDatabaseData& data) override;
 
-  void set_history_query_complete_closure_for_testing(
-      base::OnceClosure closure) {
-    history_query_complete_closure_for_testing_ = std::move(closure);
+  void set_ukm_recorded_closure_for_testing(base::OnceClosure closure) {
+    ukm_recorded_closure_for_testing_ = std::move(closure);
   }
 
   NotificationTriggerScheduler* GetNotificationTriggerScheduler();
@@ -100,12 +99,10 @@ class PlatformNotificationServiceImpl
                                ContentSettingsType content_type,
                                const std::string& resource_identifier) override;
 
-  static void OnUrlHistoryQueryComplete(
-      base::OnceClosure callback,
+  static void DidGetBackgroundSourceId(
+      base::OnceClosure recorded_closure,
       const content::NotificationDatabaseData& data,
-      bool found_url,
-      const history::URLRow& url_row,
-      const history::VisitVector& visits);
+      base::Optional<ukm::SourceId> source_id);
 
   // Creates a new Web Notification-based Notification object. Should only be
   // called when the notification is first shown.
@@ -129,15 +126,11 @@ class PlatformNotificationServiceImpl
   // programmatically to avoid dispatching close events for them.
   std::unordered_set<std::string> closed_notifications_;
 
-  // Task tracker used for querying URLs in the history service.
-  base::CancelableTaskTracker task_tracker_;
-
   // Scheduler for notifications with a trigger.
   std::unique_ptr<NotificationTriggerScheduler> trigger_scheduler_;
 
-  // Testing-only closure to observe when querying the history service has been
-  // completed, and the result of logging UKM can be observed.
-  base::OnceClosure history_query_complete_closure_for_testing_;
+  // Testing-only closure to observe when a UKM event has been recorded.
+  base::OnceClosure ukm_recorded_closure_for_testing_;
 
   DISALLOW_COPY_AND_ASSIGN(PlatformNotificationServiceImpl);
 };

chrome/browser/notifications/platform_notification_service_unittest.cc

@@ -10,7 +10,6 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/feature_list.h"
-#include "base/files/scoped_temp_dir.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
@@ -23,7 +22,6 @@
 #include "chrome/browser/notifications/platform_notification_service_factory.h"
 #include "chrome/browser/notifications/platform_notification_service_impl.h"
 #include "chrome/test/base/testing_profile.h"
-#include "components/history/core/browser/history_database_params.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/test/test_history_database.h"
 #include "components/ukm/test_ukm_recorder.h"
@@ -34,6 +32,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/common/notifications/notification_resources.h"
 #include "third_party/blink/public/common/notifications/platform_notification_data.h"
+#include "url/gurl.h"
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 #include "chrome/browser/extensions/extension_service.h"
@@ -68,6 +67,7 @@ const char kRequireInteraction[] = "RequireInteraction";
 const char kTimeUntilCloseMillis[] = "TimeUntilClose";
 const char kTimeUntilFirstClickMillis[] = "TimeUntilFirstClick";
 const char kTimeUntilLastClickMillis[] = "TimeUntilLastClick";
+const GURL kOrigin = GURL("https://example.com");
 
 }  // namespace
 
@@ -244,76 +244,17 @@ TEST_F(PlatformNotificationServiceTest, DisplayPersistentPropertiesMatch) {
   EXPECT_TRUE(buttons[1].placeholder);
 }
 
-TEST_F(PlatformNotificationServiceTest, RecordNotificationUkmEventHistory) {
-  base::ScopedTempDir temp_dir;
-  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-
-  HistoryServiceFactory::GetInstance()->SetTestingFactoryAndUse(
-      &profile_, base::BindRepeating([](content::BrowserContext* context) {
-        return static_cast<std::unique_ptr<KeyedService>>(
-            std::make_unique<history::HistoryService>());
-      }));
-
-  history::HistoryService* history_service =
-      HistoryServiceFactory::GetForProfile(&profile_,
-                                           ServiceAccessType::EXPLICIT_ACCESS);
-
-  // Initialize the |history_service| based on our |temp_dir|.
-  history_service->Init(
-      history::TestHistoryDatabaseParamsForPath(temp_dir.GetPath()));
-
-  NotificationDatabaseData data;
-  data.closed_reason = NotificationDatabaseData::ClosedReason::USER;
-  data.origin = GURL("https://chrome.com/");
-
-  size_t initial_entries_count = recorder_->entries_count();
-  size_t expected_entries_count = initial_entries_count + 1;
-
-  // First attempt to record an event for |data.origin| before it has been added
-  // to the |history_service|. Nothing should be recorded.
-  {
-    base::RunLoop run_loop;
-    service()->set_history_query_complete_closure_for_testing(
-        run_loop.QuitClosure());
-    service()->RecordNotificationUkmEvent(data);
-    run_loop.Run();
-  }
-
-  EXPECT_EQ(recorder_->entries_count(), initial_entries_count);
-
-  // Now add |data.origin| to the |history_service|. After this, notification
-  // events being logged should end up in UKM.
-  history_service->AddPage(data.origin, base::Time::Now(),
-                           history::SOURCE_BROWSED);
-
-  {
-    base::RunLoop run_loop;
-    service()->set_history_query_complete_closure_for_testing(
-        run_loop.QuitClosure());
-    service()->RecordNotificationUkmEvent(data);
-    run_loop.Run();
-  }
-
-  EXPECT_EQ(recorder_->entries_count(), expected_entries_count);
-
-  // Delete the |data.origin| from the |history_service|. Subsequent events
-  // should not be logged to UKM anymore.
-  history_service->DeleteURL(data.origin);
-
-  {
-    base::RunLoop run_loop;
-    service()->set_history_query_complete_closure_for_testing(
-        run_loop.QuitClosure());
-    service()->RecordNotificationUkmEvent(data);
-    run_loop.Run();
-  }
-
-  EXPECT_EQ(recorder_->entries_count(), expected_entries_count);
-}
-
 TEST_F(PlatformNotificationServiceTest, RecordNotificationUkmEvent) {
+  // Set up UKM recording conditions.
+  ASSERT_TRUE(profile_.CreateHistoryService(/* delete_file= */ true,
+                                            /* no_db= */ false));
+  auto* history_service = HistoryServiceFactory::GetForProfile(
+      &profile_, ServiceAccessType::EXPLICIT_ACCESS);
+  history_service->AddPage(kOrigin, base::Time::Now(), history::SOURCE_BROWSED);
+
   NotificationDatabaseData data;
   data.notification_id = "notification1";
+  data.origin = kOrigin;
   data.closed_reason = NotificationDatabaseData::ClosedReason::USER;
   data.replaced_existing_notification = true;
   data.notification_data.icon = GURL("https://icon.com");
@@ -332,19 +273,18 @@ TEST_F(PlatformNotificationServiceTest, RecordNotificationUkmEvent) {
   data.time_until_first_click_millis = base::TimeDelta::FromMilliseconds(2222);
   data.time_until_last_click_millis = base::TimeDelta::FromMilliseconds(3333);
 
-  history::URLRow url_row;
-  history::VisitVector visits;
-
-  // The history service does not find the given URL and nothing is recorded.
-  PlatformNotificationServiceImpl::OnUrlHistoryQueryComplete(
-      base::DoNothing(), data, false, url_row, visits);
+  // Initially there are no UKM entries.
   std::vector<const ukm::mojom::UkmEntry*> entries =
       recorder_->GetEntriesByName(ukm::builders::Notification::kEntryName);
   EXPECT_EQ(0u, entries.size());
 
-  // The history service finds the given URL and the notification is logged.
-  PlatformNotificationServiceImpl::OnUrlHistoryQueryComplete(
-      base::DoNothing(), data, true, url_row, visits);
+  {
+    base::RunLoop run_loop;
+    service()->set_ukm_recorded_closure_for_testing(run_loop.QuitClosure());
+    service()->RecordNotificationUkmEvent(data);
+    run_loop.Run();
+  }
+
   entries =
       recorder_->GetEntriesByName(ukm::builders::Notification::kEntryName);
   ASSERT_EQ(1u, entries.size());

services/metrics/public/cpp/ukm_recorder.h

@@ -20,7 +20,6 @@
 class BackgroundFetchDelegateImpl;
 class IOSChromePasswordManagerClient;
 class MediaEngagementSession;
-class PlatformNotificationServiceImpl;
 class PluginInfoHostImpl;
 
 namespace autofill {
@@ -107,7 +106,6 @@ class METRICS_EXPORT UkmRecorder {
   friend DelegatingUkmRecorder;
   friend IOSChromePasswordManagerClient;
   friend MediaEngagementSession;
-  friend PlatformNotificationServiceImpl;
   friend PluginInfoHostImpl;
   friend TestRecordingHelper;
   friend UkmBackgroundRecorderService;