Revert "Remove ShouldReportUser from UserEventReporterHelper."

This reverts commit 5ebe371. Reason for revert: b/284100279 Original change's description: > Remove ShouldReportUser from UserEventReporterHelper. > > Instead of depending on global dependency, each caller class > has explicit dependency to ReportingUserTracker. > > BUG=b/267685577 > TEST=Tryjob > > Change-Id: I10db2721b029eae1dd0403fb2422563afa20f082 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4482483 > Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> > Commit-Queue: Hidehiko Abe <hidehiko@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1136680} (cherry picked from commit c2b1c04) Bug: b/267685577 Change-Id: Idfe0c3d3329d08c4fde7d32ddc31a05acb95ef72 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4560634 Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Commit-Queue: Xiyuan Xia <xiyuan@chromium.org> Auto-Submit: Hidehiko Abe <hidehiko@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#1148629} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4617478 Commit-Queue: Hidehiko Abe <hidehiko@chromium.org> Cr-Commit-Position: refs/branch-heads/5790@{#832} Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
chromium · Jun 16, 2023 · b524e14 · b524e14
1 parent cd02093
commit b524e14
Show file tree

Hide file tree

Showing 18 changed files with 271 additions and 381 deletions.
diff --git a/chrome/browser/ash/login/reporting/lock_unlock_reporter.cc b/chrome/browser/ash/login/reporting/lock_unlock_reporter.cc
@@ -9,7 +9,6 @@
 #include "base/logging.h"
 #include "base/task/bind_post_task.h"
 #include "chrome/browser/ash/policy/core/device_local_account.h"
-#include "chrome/browser/ash/policy/core/reporting_user_tracker.h"
 #include "chrome/browser/ash/policy/reporting/user_event_reporter_helper.h"
 #include "chrome/browser/browser_process.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -55,12 +54,9 @@ UnlockType GetUnlockTypeForEvent(
 
 LockUnlockReporter::LockUnlockReporter(
     std::unique_ptr<::reporting::UserEventReporterHelper> helper,
-    policy::ReportingUserTracker* reporting_user_tracker,
     policy::ManagedSessionService* managed_session_service,
     base::Clock* clock)
-    : clock_(clock),
-      helper_(std::move(helper)),
-      reporting_user_tracker_(reporting_user_tracker) {
+    : clock_(clock), helper_(std::move(helper)) {
   if (managed_session_service) {
     managed_session_observation_.Observe(managed_session_service);
   }
@@ -70,23 +66,20 @@ LockUnlockReporter::~LockUnlockReporter() = default;
 
 // static
 std::unique_ptr<LockUnlockReporter> LockUnlockReporter::Create(
-    policy::ReportingUserTracker* reporting_user_tracker,
     policy::ManagedSessionService* managed_session_service) {
   return base::WrapUnique(new LockUnlockReporter(
       std::make_unique<::reporting::UserEventReporterHelper>(
           ::reporting::Destination::LOCK_UNLOCK_EVENTS),
-      reporting_user_tracker, managed_session_service));
+      managed_session_service));
 }
 
 // static
 std::unique_ptr<LockUnlockReporter> LockUnlockReporter::CreateForTest(
     std::unique_ptr<::reporting::UserEventReporterHelper> reporter_helper,
-    policy::ReportingUserTracker* reporting_user_tracker,
     policy::ManagedSessionService* managed_session_service,
     base::Clock* clock) {
-  return base::WrapUnique(
-      new LockUnlockReporter(std::move(reporter_helper), reporting_user_tracker,
-                             managed_session_service, clock));
+  return base::WrapUnique(new LockUnlockReporter(
+      std::move(reporter_helper), managed_session_service, clock));
 }
 
 void LockUnlockReporter::MaybeReportEvent(LockUnlockRecord record) {
@@ -95,7 +88,7 @@ void LockUnlockReporter::MaybeReportEvent(LockUnlockRecord record) {
   }
   const std::string& user_email =
       user_manager::UserManager::Get()->GetPrimaryUser()->GetDisplayEmail();
-  if (!reporting_user_tracker_->ShouldReportUser(user_email)) {
+  if (!helper_->ShouldReportUser(user_email)) {
     return;
   }
 

diff --git a/chrome/browser/ash/login/reporting/lock_unlock_reporter.h b/chrome/browser/ash/login/reporting/lock_unlock_reporter.h
@@ -14,13 +14,9 @@
 #include "chrome/browser/ash/policy/status_collector/managed_session_service.h"
 #include "chrome/browser/policy/messaging_layer/proto/synced/lock_unlock_event.pb.h"
 
-namespace policy {
-class ReportingUserTracker;
-}  // namespace policy
-
 namespace reporting {
 class UserEventReporterHelper;
-}  // namespace reporting
+}
 
 namespace ash {
 namespace reporting {
@@ -37,13 +33,11 @@ class LockUnlockReporter : public policy::ManagedSessionService::Observer {
 
   // For prod. Uses the default implementation of UserEventReporterHelper.
   static std::unique_ptr<LockUnlockReporter> Create(
-      policy::ReportingUserTracker* reporting_user_tracker,
       policy::ManagedSessionService* managed_session_service);
 
   // For use in testing only. Allows user to pass in a test helper.
   static std::unique_ptr<LockUnlockReporter> CreateForTest(
       std::unique_ptr<::reporting::UserEventReporterHelper> reporter_helper,
-      policy::ReportingUserTracker* reporting_user_tracker,
       policy::ManagedSessionService* managed_session_service,
       base::Clock* clock = base::DefaultClock::GetInstance());
 
@@ -56,7 +50,6 @@ class LockUnlockReporter : public policy::ManagedSessionService::Observer {
  private:
   LockUnlockReporter(
       std::unique_ptr<::reporting::UserEventReporterHelper> helper,
-      policy::ReportingUserTracker* reporting_user_tracker,
       policy::ManagedSessionService* managed_session_service,
       base::Clock* clock = base::DefaultClock::GetInstance());
 
@@ -66,9 +59,6 @@ class LockUnlockReporter : public policy::ManagedSessionService::Observer {
 
   std::unique_ptr<::reporting::UserEventReporterHelper> helper_;
 
-  const base::raw_ptr<policy::ReportingUserTracker, ExperimentalAsh>
-      reporting_user_tracker_;
-
   base::ScopedObservation<policy::ManagedSessionService,
                           policy::ManagedSessionService::Observer>
       managed_session_observation_{this};

diff --git a/chrome/browser/ash/login/reporting/lock_unlock_reporter_unittest.cc b/chrome/browser/ash/login/reporting/lock_unlock_reporter_unittest.cc
@@ -9,11 +9,9 @@
 #include "base/test/simple_test_clock.h"
 #include "chrome/browser/ash/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/ash/policy/core/device_local_account.h"
-#include "chrome/browser/ash/policy/core/reporting_user_tracker.h"
 #include "chrome/browser/ash/policy/reporting/user_event_reporter_helper_testing.h"
 #include "chrome/browser/ash/profiles/profile_helper.h"
 #include "chrome/browser/policy/messaging_layer/proto/synced/lock_unlock_event.pb.h"
-#include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chromeos/ash/components/login/auth/public/auth_failure.h"
@@ -56,39 +54,30 @@ class LockUnlockTestHelper {
     user_manager_ = user_manager.get();
     user_manager_enabler_ = std::make_unique<user_manager::ScopedUserManager>(
         std::move(user_manager));
-
-    reporting_user_tracker_ =
-        std::make_unique<policy::ReportingUserTracker>(user_manager_);
   }
 
-  void Shutdown() {
-    reporting_user_tracker_.reset();
-    user_manager_enabler_.reset();
-    SessionManagerClient::Shutdown();
-    chromeos::PowerManagerClient::Shutdown();
-  }
+  void Shutdown() { chromeos::PowerManagerClient::Shutdown(); }
 
   session_manager::SessionManager* session_manager() {
     return &session_manager_;
   }
 
   std::unique_ptr<TestingProfile> CreateRegularUserProfile() {
-    const AccountId account_id = AccountId::FromUserEmail(kFakeEmail);
-
+    AccountId account_id = AccountId::FromUserEmail(kFakeEmail);
+    auto* const user = user_manager_->AddUser(account_id);
     TestingProfile::Builder profile_builder;
-    profile_builder.SetProfileName(account_id.GetUserEmail());
+    profile_builder.SetProfileName(user->GetAccountId().GetUserEmail());
     auto profile = profile_builder.Build();
-
-    auto* const user = user_manager_->AddUserWithAffiliationAndTypeAndProfile(
-        account_id, /*is_affiliated=*/true, user_manager::USER_TYPE_REGULAR,
-        profile.get());
-
+    ProfileHelper::Get()->SetProfileToUserMappingForTesting(user);
+    ProfileHelper::Get()->SetUserToProfileMappingForTesting(user,
+                                                            profile.get());
     user_manager_->LoginUser(user->GetAccountId(), true);
     return profile;
   }
 
   std::unique_ptr<::reporting::UserEventReporterHelperTesting>
   GetReporterHelper(bool reporting_enabled,
+                    bool should_report_user,
                     ::reporting::Status status = ::reporting::Status()) {
     record_.Clear();
     report_count_ = 0;
@@ -111,23 +100,18 @@ class LockUnlockTestHelper {
 
     auto reporter_helper =
         std::make_unique<::reporting::UserEventReporterHelperTesting>(
-            reporting_enabled, /*is_kiosk_user=*/false, std::move(mock_queue));
+            reporting_enabled, should_report_user, /*is_kiosk_user=*/false,
+            std::move(mock_queue));
     return reporter_helper;
   }
 
   LockUnlockRecord GetRecord() { return record_; }
 
   int GetReportCount() { return report_count_; }
 
-  policy::ReportingUserTracker* reporting_user_tracker() {
-    return reporting_user_tracker_.get();
-  }
-
  private:
-  ScopedTestingLocalState local_state_{TestingBrowserProcess::GetGlobal()};
   raw_ptr<FakeChromeUserManager, ExperimentalAsh> user_manager_;
   std::unique_ptr<user_manager::ScopedUserManager> user_manager_enabler_;
-  std::unique_ptr<policy::ReportingUserTracker> reporting_user_tracker_;
   content::BrowserTaskEnvironment task_environment_;
 
   LockUnlockRecord record_;
@@ -150,11 +134,11 @@ class LockUnlockReporterTest
 TEST_F(LockUnlockReporterTest, ReportLockPolicyEnabled) {
   policy::ManagedSessionService managed_session_service;
   auto reporter_helper = test_helper_.GetReporterHelper(
-      /*reporting_enabled=*/true);
+      /*reporting_enabled=*/true,
+      /*should_report_user=*/true);
 
-  auto reporter = LockUnlockReporter::CreateForTest(
-      std::move(reporter_helper), test_helper_.reporting_user_tracker(),
-      &managed_session_service);
+  auto reporter = LockUnlockReporter::CreateForTest(std::move(reporter_helper),
+                                                    &managed_session_service);
 
   auto profile = test_helper_.CreateRegularUserProfile();
   auto* const user = ProfileHelper::Get()->GetUserByProfile(profile.get());
@@ -177,11 +161,11 @@ TEST_F(LockUnlockReporterTest, ReportLockPolicyEnabled) {
 TEST_F(LockUnlockReporterTest, ReportLockPolicyDisabled) {
   policy::ManagedSessionService managed_session_service;
   auto reporter_helper = test_helper_.GetReporterHelper(
-      /*reporting_enabled=*/false);
+      /*reporting_enabled=*/false,
+      /*should_report_user=*/true);
 
-  auto reporter = LockUnlockReporter::CreateForTest(
-      std::move(reporter_helper), test_helper_.reporting_user_tracker(),
-      &managed_session_service);
+  auto reporter = LockUnlockReporter::CreateForTest(std::move(reporter_helper),
+                                                    &managed_session_service);
 
   auto profile = test_helper_.CreateRegularUserProfile();
   auto* const user = ProfileHelper::Get()->GetUserByProfile(profile.get());
@@ -198,11 +182,11 @@ TEST_P(LockUnlockReporterTest, ReportUnlockPolicyDisabled) {
   const auto test_case = GetParam();
   policy::ManagedSessionService managed_session_service;
   auto reporter_helper = test_helper_.GetReporterHelper(
-      /*reporting_enabled=*/false);
+      /*reporting_enabled=*/false,
+      /*should_report_user=*/true);
 
-  auto reporter = LockUnlockReporter::CreateForTest(
-      std::move(reporter_helper), test_helper_.reporting_user_tracker(),
-      &managed_session_service);
+  auto reporter = LockUnlockReporter::CreateForTest(std::move(reporter_helper),
+                                                    &managed_session_service);
 
   auto profile = test_helper_.CreateRegularUserProfile();
   auto* const user = ProfileHelper::Get()->GetUserByProfile(profile.get());
@@ -218,11 +202,11 @@ TEST_P(LockUnlockReporterTest, ReportUnlockPolicyEnabled) {
   const auto test_case = GetParam();
   policy::ManagedSessionService managed_session_service;
   auto reporter_helper = test_helper_.GetReporterHelper(
-      /*reporting_enabled=*/true);
+      /*reporting_enabled=*/true,
+      /*should_report_user=*/true);
 
-  auto reporter = LockUnlockReporter::CreateForTest(
-      std::move(reporter_helper), test_helper_.reporting_user_tracker(),
-      &managed_session_service);
+  auto reporter = LockUnlockReporter::CreateForTest(std::move(reporter_helper),
+                                                    &managed_session_service);
 
   auto profile = test_helper_.CreateRegularUserProfile();
   auto* const user = ProfileHelper::Get()->GetUserByProfile(profile.get());

diff --git a/chrome/browser/ash/login/reporting/login_logout_reporter.cc b/chrome/browser/ash/login/reporting/login_logout_reporter.cc
@@ -9,7 +9,6 @@
 #include "base/task/single_thread_task_runner.h"
 #include "chrome/browser/ash/login/existing_user_controller.h"
 #include "chrome/browser/ash/policy/core/device_local_account.h"
-#include "chrome/browser/ash/policy/core/reporting_user_tracker.h"
 #include "chrome/browser/ash/policy/reporting/user_event_reporter_helper.h"
 #include "chrome/browser/ash/profiles/profile_helper.h"
 #include "chrome/browser/browser_process.h"
@@ -120,12 +119,10 @@ void LoginLogoutReporter::RegisterPrefs(PrefRegistrySimple* registry) {
 LoginLogoutReporter::LoginLogoutReporter(
     std::unique_ptr<::reporting::UserEventReporterHelper> reporter_helper,
     std::unique_ptr<Delegate> delegate,
-    policy::ReportingUserTracker* reporting_user_tracker,
     policy::ManagedSessionService* managed_session_service,
     base::Clock* clock)
     : reporter_helper_(std::move(reporter_helper)),
       delegate_(std::move(delegate)),
-      reporting_user_tracker_(reporting_user_tracker),
       clock_(clock) {
   if (managed_session_service) {
     managed_session_observation_.Observe(managed_session_service);
@@ -137,26 +134,24 @@ LoginLogoutReporter::~LoginLogoutReporter() = default;
 
 // static
 std::unique_ptr<LoginLogoutReporter> LoginLogoutReporter::Create(
-    policy::ReportingUserTracker* reporting_user_tracker,
     policy::ManagedSessionService* managed_session_service) {
   auto reporter_helper = std::make_unique<::reporting::UserEventReporterHelper>(
       ::reporting::Destination::LOGIN_LOGOUT_EVENTS);
   auto delegate = std::make_unique<LoginLogoutReporter::Delegate>();
-  return base::WrapUnique(
-      new LoginLogoutReporter(std::move(reporter_helper), std::move(delegate),
-                              reporting_user_tracker, managed_session_service));
+  return base::WrapUnique(new LoginLogoutReporter(std::move(reporter_helper),
+                                                  std::move(delegate),
+                                                  managed_session_service));
 }
 
 // static
 std::unique_ptr<LoginLogoutReporter> LoginLogoutReporter::CreateForTest(
     std::unique_ptr<::reporting::UserEventReporterHelper> reporter_helper,
     std::unique_ptr<LoginLogoutReporter::Delegate> delegate,
-    policy::ReportingUserTracker* reporting_user_tracker,
     policy::ManagedSessionService* managed_session_service,
     base::Clock* clock) {
-  return base::WrapUnique(new LoginLogoutReporter(
-      std::move(reporter_helper), std::move(delegate), reporting_user_tracker,
-      managed_session_service, clock));
+  return base::WrapUnique(
+      new LoginLogoutReporter(std::move(reporter_helper), std::move(delegate),
+                              managed_session_service, clock));
 }
 
 void LoginLogoutReporter::MaybeReportEvent(LoginLogoutRecord record,
@@ -173,7 +168,7 @@ void LoginLogoutReporter::MaybeReportEvent(LoginLogoutRecord record,
       session_type == LoginLogoutSessionType::GUEST_SESSION) {
     record.set_is_guest_session(true);
   } else if (session_type == LoginLogoutSessionType::REGULAR_USER_SESSION &&
-             reporting_user_tracker_->ShouldReportUser(user_email)) {
+             reporter_helper_->ShouldReportUser(user_email)) {
     record.mutable_affiliated_user()->set_user_email(user_email);
   }
   reporter_helper_->ReportEvent(

diff --git a/chrome/browser/ash/login/reporting/login_logout_reporter.h b/chrome/browser/ash/login/reporting/login_logout_reporter.h
@@ -18,12 +18,10 @@
 
 class PrefRegistrySimple;
 
-namespace policy {
-class ReportingUserTracker;
-}  // namespace policy
-
 namespace reporting {
+
 class UserEventReporterHelper;
+
 }  // namespace reporting
 
 namespace ash {
@@ -54,13 +52,11 @@ class LoginLogoutReporter : public policy::ManagedSessionService::Observer {
   ~LoginLogoutReporter() override;
 
   static std::unique_ptr<LoginLogoutReporter> Create(
-      policy::ReportingUserTracker* reporting_user_tracker,
       policy::ManagedSessionService* managed_session_service);
 
   static std::unique_ptr<LoginLogoutReporter> CreateForTest(
       std::unique_ptr<::reporting::UserEventReporterHelper> reporter_helper,
       std::unique_ptr<Delegate> delegate,
-      policy::ReportingUserTracker* reporting_user_tracker,
       policy::ManagedSessionService* managed_session_service,
       base::Clock* clock = base::DefaultClock::GetInstance());
 
@@ -81,7 +77,6 @@ class LoginLogoutReporter : public policy::ManagedSessionService::Observer {
   LoginLogoutReporter(
       std::unique_ptr<::reporting::UserEventReporterHelper> reporter_helper,
       std::unique_ptr<Delegate> delegate,
-      policy::ReportingUserTracker* reporting_user_tracker,
       policy::ManagedSessionService* managed_session_service,
       base::Clock* clock = base::DefaultClock::GetInstance());
 
@@ -93,9 +88,6 @@ class LoginLogoutReporter : public policy::ManagedSessionService::Observer {
 
   std::unique_ptr<Delegate> delegate_;
 
-  const base::raw_ptr<policy::ReportingUserTracker, ExperimentalAsh>
-      reporting_user_tracker_;
-
   base::ScopedObservation<policy::ManagedSessionService,
                           policy::ManagedSessionService::Observer>
       managed_session_observation_{this};