[CBCM] Refactor ReportScheduler to use delegate pattern

Moves all platform-specific logic in ReportScheduler to a delegate
class. A few things to note:

- For now, ReportScheduler takes the desktop-specific factory in its
  constructor because it can't use the base factory until
  ReportScheduler is moved to components.
- Observing the build state is now done by the desktop delegate. To
  notify ReportScheduler of an update, ReportScheduler passes a
  callback to its delegate, which the delegate can use to trigger
  report uploads. For this to work, the ReportTrigger enum had to be
  made visible to the delegate.
- ReportScheduler still has one chrome/browser dependency for the name
  of the kLastUploadTimestamp pref. This pref name will be moved to
  components in the next CL.
- On Chrome OS, ReportScheduler was being given a specific profile to
  watch. Because Profile cannot be used in Components, the Profile*
  must now be passed to the delegate directly. So, on Chrome OS, the
  ReportSchedulerDesktop delegate is instantiated directly and passed
  to ReportScheduler, instead of ReportScheduler taking the delegate
  from the factory.

Bug: 1092432
Change-Id: Ia3169410c8ca83151556fed4319db512bc9fc258
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2303589
Commit-Queue: Guillaume Jenkins <gujen@google.com>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790848}

chrome/browser/BUILD.gn

@@ -3140,6 +3140,8 @@ static_library("browser") {
       "enterprise/reporting/report_generator_desktop.h",
       "enterprise/reporting/report_scheduler.cc",
       "enterprise/reporting/report_scheduler.h",
+      "enterprise/reporting/report_scheduler_desktop.cc",
+      "enterprise/reporting/report_scheduler_desktop.h",
       "enterprise/reporting/reporting_delegate_factory_desktop.cc",
       "enterprise/reporting/reporting_delegate_factory_desktop.h",
       "feedback/feedback_dialog_utils.cc",

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc

@@ -31,6 +31,7 @@
 #include "chrome/browser/chromeos/policy/remote_commands/user_commands_factory_chromeos.h"
 #include "chrome/browser/chromeos/policy/wildcard_login_checker.h"
 #include "chrome/browser/enterprise/reporting/report_scheduler.h"
+#include "chrome/browser/enterprise/reporting/report_scheduler_desktop.h"
 #include "chrome/browser/enterprise/reporting/reporting_delegate_factory_desktop.h"
 #include "chrome/browser/invalidation/profile_invalidation_provider_factory.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
@@ -785,7 +786,7 @@ void UserCloudPolicyManagerChromeOS::StartReportSchedulerIfReady(
       client(),
       std::make_unique<enterprise_reporting::ReportGenerator>(
           &delegate_factory),
-      profile_);
+      std::make_unique<enterprise_reporting::ReportSchedulerDesktop>(profile_));
 
   report_scheduler_->OnDMTokenUpdated();
 }

chrome/browser/enterprise/reporting/report_scheduler.cc

@@ -13,64 +13,60 @@
 #include "base/task/post_task.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
-#include "chrome/browser/browser_process.h"
 #include "chrome/browser/enterprise/reporting/prefs.h"
-#include "chrome/browser/net/system_network_context_manager.h"
-#include "chrome/browser/policy/chrome_browser_policy_connector.h"
-#include "chrome/browser/upgrade_detector/build_state.h"
-#include "chrome/common/chrome_constants.h"
+#include "chrome/browser/enterprise/reporting/reporting_delegate_factory_desktop.h"
 #include "chrome/common/pref_names.h"
 #include "components/enterprise/browser/controller/browser_dm_token_storage.h"
 #include "components/policy/core/common/cloud/cloud_policy_client.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
 #include "components/prefs/pref_service.h"
-#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace em = enterprise_management;
 
 namespace enterprise_reporting {
+
 namespace {
 
 constexpr base::TimeDelta kDefaultUploadInterval =
     base::TimeDelta::FromHours(24);  // Default upload interval is 24 hours.
 const int kMaximumRetry = 10;  // Retry 10 times takes about 15 to 19 hours.
 
-// Returns true if cloud reporting is enabled.
-bool IsReportingEnabled() {
-  return g_browser_process->local_state()->GetBoolean(
-      prefs::kCloudReportingEnabled);
-}
+}  // namespace
 
-// Returns true if this build should generate basic reports when an update is
-// detected.
-constexpr bool ShouldReportUpdates() {
-#if defined(OS_CHROMEOS)
-  return false;
-#else
-  return true;
-#endif
+ReportScheduler::Delegate::Delegate() = default;
+ReportScheduler::Delegate::~Delegate() = default;
+
+void ReportScheduler::Delegate::SetReportTriggerCallback(
+    ReportScheduler::ReportTriggerCallback callback) {
+  DCHECK(trigger_report_callback_.is_null());
+  trigger_report_callback_ = std::move(callback);
 }
 
-}  // namespace
+ReportScheduler::ReportScheduler(
+    policy::CloudPolicyClient* client,
+    std::unique_ptr<ReportGenerator> report_generator,
+    ReportingDelegateFactoryDesktop* delegate_factory)
+    : ReportScheduler(std::move(client),
+                      std::move(report_generator),
+                      delegate_factory->GetReportSchedulerDelegate()) {}
 
 ReportScheduler::ReportScheduler(
     policy::CloudPolicyClient* client,
     std::unique_ptr<ReportGenerator> report_generator,
-    Profile* profile)
-    : cloud_policy_client_(std::move(client)),
-      report_generator_(std::move(report_generator)),
-      extension_request_observer_factory_(profile) {
+    std::unique_ptr<ReportScheduler::Delegate> delegate)
+    : delegate_(std::move(delegate)),
+      cloud_policy_client_(std::move(client)),
+      report_generator_(std::move(report_generator)) {
+  delegate_->SetReportTriggerCallback(
+      base::BindRepeating(&ReportScheduler::GenerateAndUploadReport,
+                          weak_ptr_factory_.GetWeakPtr()));
   RegisterPrefObserver();
 }
 
-ReportScheduler::~ReportScheduler() {
-  // If new profiles have been added since the last report was sent, they won't
-  // be reported until the next launch, since Chrome is shutting down. However,
-  // the (now obsolete) Enterprise.CloudReportingStaleProfileCount metric has
-  // shown that this very rarely happens, with 99.23% of samples reporting no
-  // stale profiles and 0.72% reporting a single stale profile.
-  if (ShouldReportUpdates())
-    g_browser_process->GetBuildState()->RemoveObserver(this);
+ReportScheduler::~ReportScheduler() = default;
+
+bool ReportScheduler::IsReportingEnabled() const {
+  return delegate_->GetLocalState()->GetBoolean(prefs::kCloudReportingEnabled);
 }
 
 bool ReportScheduler::IsNextReportScheduledForTesting() const {
@@ -86,16 +82,8 @@ void ReportScheduler::OnDMTokenUpdated() {
   OnReportEnabledPrefChanged();
 }
 
-void ReportScheduler::OnUpdate(const BuildState* build_state) {
-  DCHECK(ShouldReportUpdates());
-  // A new version has been detected on the machine and a restart is now needed
-  // for it to take effect. Send a basic report (without profile info)
-  // immediately.
-  GenerateAndUploadReport(kTriggerUpdate);
-}
-
 void ReportScheduler::RegisterPrefObserver() {
-  pref_change_registrar_.Init(g_browser_process->local_state());
+  pref_change_registrar_.Init(delegate_->GetLocalState());
   pref_change_registrar_.Add(
       prefs::kCloudReportingEnabled,
       base::BindRepeating(&ReportScheduler::OnReportEnabledPrefChanged,
@@ -122,31 +110,16 @@ void ReportScheduler::OnReportEnabledPrefChanged() {
 
   // Start the periodic report timer.
   const base::Time last_upload_timestamp =
-      g_browser_process->local_state()->GetTime(kLastUploadTimestamp);
+      delegate_->GetLocalState()->GetTime(kLastUploadTimestamp);
   Start(last_upload_timestamp);
 
-  if (ShouldReportUpdates()) {
-    // Watch for browser updates if not already doing so.
-    auto* build_state = g_browser_process->GetBuildState();
-    if (!build_state->HasObserver(this)) {
-      build_state->AddObserver(this);
-
-      // Generate and upload a basic report immediately if the version has
-      // changed since the last upload and the last upload was less than 24h
-      // ago.
-      if (g_browser_process->local_state()->GetString(kLastUploadVersion) !=
-              chrome::kChromeVersion &&
-          last_upload_timestamp + kDefaultUploadInterval > base::Time::Now()) {
-        GenerateAndUploadReport(kTriggerNewVersion);
-      }
-    }
-  }
+  delegate_->StartWatchingUpdatesIfNeeded(last_upload_timestamp,
+                                          kDefaultUploadInterval);
 }
 
 void ReportScheduler::Stop() {
   request_timer_.Stop();
-  if (ShouldReportUpdates())
-    g_browser_process->GetBuildState()->RemoveObserver(this);
+  delegate_->StopWatchingUpdates();
 }
 
 bool ReportScheduler::SetupBrowserPolicyClientRegistration() {
@@ -246,18 +219,14 @@ void ReportScheduler::OnReportUploaded(ReportUploader::ReportStatus status) {
       // Schedule the next report for success. Reset uploader to reset failure
       // count.
       report_uploader_.reset();
-      if (ShouldReportUpdates()) {
-        // Remember what browser version made this upload.
-        g_browser_process->local_state()->SetString(kLastUploadVersion,
-                                                    chrome::kChromeVersion);
-      }
+      delegate_->SaveLastUploadVersion();
       FALLTHROUGH;
     case ReportUploader::kTransientError:
       // Stop retrying and schedule the next report to avoid stale report.
       // Failure count is not reset so retry delay remains.
       if (active_trigger_ == kTriggerTimer) {
         const base::Time now = base::Time::Now();
-        g_browser_process->local_state()->SetTime(kLastUploadTimestamp, now);
+        delegate_->GetLocalState()->SetTime(kLastUploadTimestamp, now);
         if (IsReportingEnabled())
           Start(now);
       }

chrome/browser/enterprise/reporting/report_scheduler.h

@@ -8,10 +8,12 @@
 #include <stdint.h>
 #include <memory>
 
+#include "base/callback.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/observer_list_types.h"
+#include "base/time/time.h"
 #include "base/util/timer/wall_clock_timer.h"
-#include "chrome/browser/enterprise/reporting/notification/extension_request_observer_factory.h"
-#include "chrome/browser/upgrade_detector/build_state_observer.h"
 #include "components/enterprise/browser/reporting/report_generator.h"
 #include "components/enterprise/browser/reporting/report_uploader.h"
 #include "components/prefs/pref_change_registrar.h"
@@ -22,17 +24,58 @@ class CloudPolicyClient;
 
 namespace enterprise_reporting {
 
+class ReportingDelegateFactoryDesktop;
+
 // Schedules report generation and upload every 24 hours and upon browser update
 // for desktop Chrome while cloud reporting is enabled via administrative
 // policy. If either of these triggers fires while a report is being generated,
 // processing is deferred until the existing processing completes.
-class ReportScheduler : public BuildStateObserver {
+class ReportScheduler {
  public:
+  // The trigger leading to report generation. Values are bitmasks in the
+  // |pending_triggers_| bitfield.
+  enum ReportTrigger : uint32_t {
+    kTriggerNone = 0,              // No trigger.
+    kTriggerTimer = 1U << 0,       // The periodic timer expired.
+    kTriggerUpdate = 1U << 1,      // An update was detected.
+    kTriggerNewVersion = 1U << 2,  // A new version is running.
+  };
+
+  using ReportTriggerCallback = base::RepeatingCallback<void(ReportTrigger)>;
+
+  class Delegate {
+   public:
+    Delegate();
+    Delegate(const Delegate&) = delete;
+    Delegate& operator=(const Delegate&) = delete;
+
+    virtual ~Delegate();
+
+    void SetReportTriggerCallback(ReportTriggerCallback callback);
+
+    virtual PrefService* GetLocalState() = 0;
+    virtual void StartWatchingUpdatesIfNeeded(
+        base::Time last_upload,
+        base::TimeDelta upload_interval) = 0;
+    virtual void StopWatchingUpdates() = 0;
+    virtual void SaveLastUploadVersion() = 0;
+
+   protected:
+    ReportTriggerCallback trigger_report_callback_;
+  };
+
   ReportScheduler(policy::CloudPolicyClient* client,
                   std::unique_ptr<ReportGenerator> report_generator,
-                  Profile* profile = nullptr);
+                  ReportingDelegateFactoryDesktop* delegate_factory);
 
-  ~ReportScheduler() override;
+  ReportScheduler(policy::CloudPolicyClient* client,
+                  std::unique_ptr<ReportGenerator> report_generator,
+                  std::unique_ptr<ReportScheduler::Delegate> delegate);
+
+  ~ReportScheduler();
+
+  // Returns true if cloud reporting is enabled.
+  bool IsReportingEnabled() const;
 
   // Returns true if next report has been scheduled. The report will be
   // scheduled only if the previous report is uploaded successfully and the
@@ -43,19 +86,7 @@ class ReportScheduler : public BuildStateObserver {
 
   void OnDMTokenUpdated();
 
-  // BuildStateObserver:
-  void OnUpdate(const BuildState* build_state) override;
-
  private:
-  // The trigger leading to report generation. Values are bitmasks in the
-  // |pending_triggers_| bitfield.
-  enum ReportTrigger : uint32_t {
-    kTriggerNone = 0,              // No trigger.
-    kTriggerTimer = 1U << 0,       // The periodic timer expired.
-    kTriggerUpdate = 1U << 1,      // An update was detected.
-    kTriggerNewVersion = 1U << 2,  // A new version is running.
-  };
-
   // Observes CloudReportingEnabled policy.
   void RegisterPrefObserver();
 
@@ -91,6 +122,8 @@ class ReportScheduler : public BuildStateObserver {
   // Records that |trigger| was responsible for an upload attempt.
   static void RecordUploadTrigger(ReportTrigger trigger);
 
+  std::unique_ptr<Delegate> delegate_;
+
   // Policy value watcher
   PrefChangeRegistrar pref_change_registrar_;
 
@@ -102,8 +135,6 @@ class ReportScheduler : public BuildStateObserver {
 
   std::unique_ptr<ReportGenerator> report_generator_;
 
-  ExtensionRequestObserverFactory extension_request_observer_factory_;
-
   // The trigger responsible for initiating active report generation.
   ReportTrigger active_trigger_ = kTriggerNone;
 
@@ -112,6 +143,8 @@ class ReportScheduler : public BuildStateObserver {
   // in-process report.
   uint32_t pending_triggers_ = 0;
 
+  base::WeakPtrFactory<ReportScheduler> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(ReportScheduler);
 };
 

chrome/browser/enterprise/reporting/report_scheduler_desktop.cc

+// 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.
+
+#include "chrome/browser/enterprise/reporting/report_scheduler_desktop.h"
+
+#include "base/bind.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/enterprise/reporting/prefs.h"
+#include "chrome/browser/upgrade_detector/build_state.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/pref_names.h"
+#include "components/prefs/pref_service.h"
+
+namespace em = enterprise_management;
+
+namespace enterprise_reporting {
+
+namespace {
+
+// Returns true if this build should generate basic reports when an update is
+// detected.
+// TODO(crbug.com/1102047): Get rid of this function after Chrome OS reporting
+// logic has been split to its own delegates.
+constexpr bool ShouldReportUpdates() {
+#if defined(OS_CHROMEOS)
+  return false;
+#else
+  return true;
+#endif
+}
+
+}  // namespace
+
+ReportSchedulerDesktop::ReportSchedulerDesktop(Profile* profile)
+    : extension_request_observer_factory_(profile) {}
+
+ReportSchedulerDesktop::~ReportSchedulerDesktop() {
+  // If new profiles have been added since the last report was sent, they won't
+  // be reported until the next launch, since Chrome is shutting down. However,
+  // the (now obsolete) Enterprise.CloudReportingStaleProfileCount metric has
+  // shown that this very rarely happens, with 99.23% of samples reporting no
+  // stale profiles and 0.72% reporting a single stale profile.
+  if (ShouldReportUpdates())
+    g_browser_process->GetBuildState()->RemoveObserver(this);
+}
+
+PrefService* ReportSchedulerDesktop::GetLocalState() {
+  return g_browser_process->local_state();
+}
+
+void ReportSchedulerDesktop::StartWatchingUpdatesIfNeeded(
+    base::Time last_upload,
+    base::TimeDelta upload_interval) {
+  if (!ShouldReportUpdates())
+    return;
+
+  auto* build_state = g_browser_process->GetBuildState();
+  if (build_state->HasObserver(this))
+    // Already watching browser updates.
+    return;
+
+  build_state->AddObserver(this);
+
+  // Generate and upload a basic report immediately if the version has
+  // changed since the last upload and the last upload was less than 24h
+  // ago.
+  if (GetLocalState()->GetString(kLastUploadVersion) !=
+          chrome::kChromeVersion &&
+      last_upload + upload_interval > base::Time::Now() &&
+      !trigger_report_callback_.is_null()) {
+    trigger_report_callback_.Run(
+        ReportScheduler::ReportTrigger::kTriggerNewVersion);
+  }
+}
+
+void ReportSchedulerDesktop::StopWatchingUpdates() {
+  if (ShouldReportUpdates()) {
+    g_browser_process->GetBuildState()->RemoveObserver(this);
+  }
+}
+
+void ReportSchedulerDesktop::SaveLastUploadVersion() {
+  if (ShouldReportUpdates()) {
+    // Remember what browser version made this upload.
+    GetLocalState()->SetString(kLastUploadVersion, chrome::kChromeVersion);
+  }
+}
+
+void ReportSchedulerDesktop::OnUpdate(const BuildState* build_state) {
+  DCHECK(ShouldReportUpdates());
+  // A new version has been detected on the machine and a restart is now needed
+  // for it to take effect. Send a basic report (without profile info)
+  // immediately.
+  if (!trigger_report_callback_.is_null()) {
+    trigger_report_callback_.Run(
+        ReportScheduler::ReportTrigger::kTriggerUpdate);
+  }
+}
+
+}  // namespace enterprise_reporting

chrome/browser/enterprise/reporting/report_scheduler_desktop.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 CHROME_BROWSER_ENTERPRISE_REPORTING_REPORT_SCHEDULER_DESKTOP_H_
+#define CHROME_BROWSER_ENTERPRISE_REPORTING_REPORT_SCHEDULER_DESKTOP_H_
+
+#include "chrome/browser/enterprise/reporting/notification/extension_request_observer_factory.h"
+#include "chrome/browser/enterprise/reporting/report_scheduler.h"
+#include "chrome/browser/upgrade_detector/build_state_observer.h"
+
+class Profile;
+
+namespace enterprise_reporting {
+
+// Desktop implementation of the ReportScheduler delegate.
+class ReportSchedulerDesktop : public ReportScheduler::Delegate,
+                               public BuildStateObserver {
+ public:
+  explicit ReportSchedulerDesktop(Profile* profile = nullptr);
+  ReportSchedulerDesktop(const ReportSchedulerDesktop&) = delete;
+  ReportSchedulerDesktop& operator=(const ReportSchedulerDesktop&) = delete;
+
+  ~ReportSchedulerDesktop() override;
+
+  // ReportScheduler::Delegate implementation.
+  PrefService* GetLocalState() override;
+  void StartWatchingUpdatesIfNeeded(base::Time last_upload,
+                                    base::TimeDelta upload_interval) override;
+  void StopWatchingUpdates() override;
+  void SaveLastUploadVersion() override;
+
+  // BuildStateObserver implementation.
+  void OnUpdate(const BuildState* build_state) override;
+
+ private:
+  ExtensionRequestObserverFactory extension_request_observer_factory_;
+};
+
+}  // namespace enterprise_reporting
+
+#endif  // CHROME_BROWSER_ENTERPRISE_REPORTING_REPORT_SCHEDULER_DESKTOP_H_

chrome/browser/enterprise/reporting/report_scheduler_unittest.cc

@@ -116,8 +116,8 @@ class ReportSchedulerTest : public ::testing::Test {
   }
 
   void CreateScheduler() {
-    scheduler_ =
-        std::make_unique<ReportScheduler>(client_, std::move(generator_ptr_));
+    scheduler_ = std::make_unique<ReportScheduler>(
+        client_, std::move(generator_ptr_), &report_delegate_factory_);
     scheduler_->SetReportUploaderForTesting(std::move(uploader_ptr_));
   }
 

chrome/browser/enterprise/reporting/reporting_delegate_factory_desktop.cc

@@ -7,6 +7,7 @@
 #include "chrome/browser/enterprise/reporting/browser_report_generator_desktop.h"
 #include "chrome/browser/enterprise/reporting/profile_report_generator_desktop.h"
 #include "chrome/browser/enterprise/reporting/report_generator_desktop.h"
+#include "chrome/browser/enterprise/reporting/report_scheduler_desktop.h"
 
 namespace enterprise_reporting {
 
@@ -25,4 +26,9 @@ ReportingDelegateFactoryDesktop::GetReportGeneratorDelegate() {
   return std::make_unique<ReportGeneratorDesktop>();
 }
 
+std::unique_ptr<ReportScheduler::Delegate>
+ReportingDelegateFactoryDesktop::GetReportSchedulerDelegate() {
+  return std::make_unique<ReportSchedulerDesktop>();
+}
+
 }  // namespace enterprise_reporting

chrome/browser/enterprise/reporting/reporting_delegate_factory_desktop.h

@@ -9,6 +9,7 @@
 
 #include <memory>
 
+#include "chrome/browser/enterprise/reporting/report_scheduler_desktop.h"
 #include "components/enterprise/browser/reporting/browser_report_generator.h"
 #include "components/enterprise/browser/reporting/profile_report_generator.h"
 #include "components/enterprise/browser/reporting/report_generator.h"
@@ -34,6 +35,8 @@ class ReportingDelegateFactoryDesktop : public ReportingDelegateFactory {
 
   std::unique_ptr<ReportGenerator::Delegate> GetReportGeneratorDelegate()
       override;
+
+  std::unique_ptr<ReportScheduler::Delegate> GetReportSchedulerDelegate();
 };
 
 }  // namespace enterprise_reporting

chrome/browser/policy/chrome_browser_cloud_management_controller.cc

@@ -643,7 +643,7 @@ void ChromeBrowserCloudManagementController::CreateReportScheduler() {
   auto generator = std::make_unique<enterprise_reporting::ReportGenerator>(
       &delegate_factory_);
   report_scheduler_ = std::make_unique<enterprise_reporting::ReportScheduler>(
-      cloud_policy_client_.get(), std::move(generator));
+      cloud_policy_client_.get(), std::move(generator), &delegate_factory_);
 
   NotifyCloudReportingLaunched();
 }