Refactor sw_reporter_installer and reporter_runner

The sw_reporter_installer was getting too large and confusing.
SwReporterInstallerPolicy is not an appropriate class to hold information
about user-initiated cleanups. This CL is preliminary work for a
redesigned version of
https://chromium-review.googlesource.com/c/chromium/src/+/810086

Bug: 776538
Change-Id: I3833a06aec23fc83eba05ed2a36e014875730d3b
Reviewed-on: https://chromium-review.googlesource.com/817620Reviewed-by: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Commit-Queue: proberge <proberge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524450}

chrome/browser/component_updater/sw_reporter_installer_win.h

@@ -49,15 +49,12 @@ enum SoftwareReporterExperimentError {
 };
 
 // Callback for running the software reporter after it is downloaded.
-using SwReporterRunner = base::Callback<void(
-    safe_browsing::SwReporterInvocationType invocation_type,
+using OnComponentReadyCallback = base::Callback<void(
     safe_browsing::SwReporterInvocationSequence&& invocations)>;
 
 class SwReporterInstallerPolicy : public ComponentInstallerPolicy {
  public:
-  SwReporterInstallerPolicy(
-      const SwReporterRunner& reporter_runner,
-      safe_browsing::SwReporterInvocationType invocation_type);
+  explicit SwReporterInstallerPolicy(const OnComponentReadyCallback& callback);
   ~SwReporterInstallerPolicy() override;
 
   // ComponentInstallerPolicy implementation.
@@ -81,20 +78,11 @@ class SwReporterInstallerPolicy : public ComponentInstallerPolicy {
  private:
   friend class SwReporterInstallerTest;
 
-  SwReporterRunner reporter_runner_;
-
-  const safe_browsing::SwReporterInvocationType invocation_type_;
+  OnComponentReadyCallback on_component_ready_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(SwReporterInstallerPolicy);
 };
 
-// Installs the SwReporter component and runs the reporter once it's available.
-// Once ready, this may trigger either a periodic or a user-initiated run of
-// the reporter, depending on |invocation_type|.
-void RegisterSwReporterComponentWithParams(
-    safe_browsing::SwReporterInvocationType invocation_type,
-    ComponentUpdateService* cus);
-
 // Call once during startup to make the component update service aware of the
 // SwReporter. Once ready, this may trigger a periodic run of the reporter.
 void RegisterSwReporterComponent(ComponentUpdateService* cus);

chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc

@@ -178,7 +178,9 @@ class ReporterRunnerTest
 
   SwReporterInvocationSequence CreateInvocationSequence(
       const SwReporterInvocationSequence::Queue& container) {
-    return SwReporterInvocationSequence(base::Version("1.2.3"), container);
+    SwReporterInvocationSequence sequence(base::Version("1.2.3"));
+    sequence.mutable_container() = container;
+    return sequence;
   }
 
   // Schedules a single reporter to run.
@@ -209,7 +211,7 @@ class ReporterRunnerTest
                 OnReporterSequenceDone(Eq(expected_result)))
         .WillOnce(InvokeWithoutArgs(&on_sequence_done, &Waiter::Signal));
     auto invocations = CreateInvocationSequence(container);
-    RunSwReporters(invocation_type_, std::move(invocations));
+    RunSwReportersForTesting(invocation_type_, std::move(invocations));
     on_sequence_done.Wait();
   }
 
@@ -413,7 +415,8 @@ class ReporterRunnerTest
                   InvokeWithoutArgs(&first_sequence_done, &Waiter::Signal)));
 
     SwReporterInvocationSequence::Queue invocations1({CreateInvocation(path1)});
-    RunSwReporters(invocation_type1, CreateInvocationSequence(invocations1));
+    RunSwReportersForTesting(invocation_type1,
+                             CreateInvocationSequence(invocations1));
 
     EXPECT_CALL(
         mock_chrome_cleaner_controller_,
@@ -421,7 +424,8 @@ class ReporterRunnerTest
         .WillOnce(InvokeWithoutArgs(&second_sequence_done, &Waiter::Signal));
 
     SwReporterInvocationSequence::Queue invocations2({CreateInvocation(path2)});
-    RunSwReporters(invocation_type2, CreateInvocationSequence(invocations2));
+    RunSwReportersForTesting(invocation_type2,
+                             CreateInvocationSequence(invocations2));
 
     first_sequence_done.Wait();
 

chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.cc

@@ -1044,9 +1044,8 @@ void SwReporterInvocation::set_cleaner_logs_upload_enabled(
 }
 
 SwReporterInvocationSequence::SwReporterInvocationSequence(
-    const base::Version& version,
-    const Queue& container)
-    : version_(version), container_(container) {
+    const base::Version& version)
+    : version_(version) {
   // Notify the cleaner controller once this sequence completes. Don't retain
   // a reference to the controller object, since it's guaranteed to outlive the
   // sequence.
@@ -1070,6 +1069,11 @@ void SwReporterInvocationSequence::operator=(
   on_sequence_done_ = std::move(invocations_sequence.on_sequence_done_);
 }
 
+void SwReporterInvocationSequence::PushInvocation(
+    const SwReporterInvocation& invocation) {
+  container_.push(invocation);
+}
+
 void SwReporterInvocationSequence::NotifySequenceDone(
     SwReporterInvocationResult result) {
   if (on_sequence_done_)
@@ -1090,8 +1094,8 @@ SwReporterInvocationSequence::mutable_container() {
   return container_;
 }
 
-void RunSwReporters(SwReporterInvocationType invocation_type,
-                    SwReporterInvocationSequence&& invocations) {
+void RunSwReportersForTesting(SwReporterInvocationType invocation_type,
+                              SwReporterInvocationSequence&& invocations) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!invocations.container().empty());
 
@@ -1099,6 +1103,15 @@ void RunSwReporters(SwReporterInvocationType invocation_type,
                                         std::move(invocations));
 }
 
+void OnSwReporterReady(SwReporterInvocationSequence&& invocations) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(!invocations.container().empty());
+
+  // TODO(crbug.com/776538): Handle user-initiated cleanups.
+  ReporterRunner::MaybeStartInvocations(SwReporterInvocationType::kPeriodicRun,
+                                        std::move(invocations));
+}
+
 void SetSwReporterTestingDelegate(SwReporterTestingDelegate* delegate) {
   g_testing_delegate_ = delegate;
 }

chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.h

@@ -160,11 +160,13 @@ class SwReporterInvocationSequence {
  public:
   using Queue = std::queue<SwReporterInvocation>;
 
-  SwReporterInvocationSequence(const base::Version& version = base::Version(),
-                               const Queue& container = Queue());
+  explicit SwReporterInvocationSequence(
+      const base::Version& version = base::Version());
   SwReporterInvocationSequence(SwReporterInvocationSequence&& queue);
   virtual ~SwReporterInvocationSequence();
 
+  void PushInvocation(const SwReporterInvocation& invocation);
+
   void operator=(SwReporterInvocationSequence&& queue);
 
   void NotifySequenceDone(SwReporterInvocationResult result);
@@ -182,7 +184,13 @@ class SwReporterInvocationSequence {
   OnReporterSequenceDone on_sequence_done_;
 };
 
-// Tries to run the sw_reporter component. If this runs successfully, than any
+// This is used only by tests and exists solely to make the browser tests happy
+// while we implement user-initiated runs.
+// TODO(proberge): Remove this by Q1 2018.
+void RunSwReportersForTesting(SwReporterInvocationType invocation_type,
+                              SwReporterInvocationSequence&& invocations);
+
+// Tries to run the given invocations. If this runs successfully, than any
 // calls made in the next |kDaysBetweenSuccessfulSwReporterRuns| days will be
 // ignored.
 //
@@ -190,8 +198,7 @@ class SwReporterInvocationSequence {
 // executions of the tool with different command lines. |invocations| is the
 // queue of SwReporters to execute as a single "run". When a new try is
 // scheduled the entire queue is executed.
-void RunSwReporters(SwReporterInvocationType invocation_type,
-                    SwReporterInvocationSequence&& invocations);
+void OnSwReporterReady(SwReporterInvocationSequence&& invocations);
 
 // A delegate used by tests to implement test doubles (e.g., stubs, fakes, or
 // mocks).

chrome/browser/ui/webui/settings/chrome_cleanup_handler.cc

@@ -210,12 +210,8 @@ void ChromeCleanupHandler::HandleStartScanning(const base::ListValue* args) {
   // The state is propagated to all open tabs and should be consistent.
   DCHECK_EQ(controller_->logs_enabled(), allow_logs_upload);
 
-  // TODO(crbug.com/776538): Force an on-demand update of the component.
-  component_updater::RegisterSwReporterComponentWithParams(
-      allow_logs_upload ? safe_browsing::SwReporterInvocationType::
-                              kUserInitiatedWithLogsAllowed
-                        : safe_browsing::SwReporterInvocationType::
-                              kUserInitiatedWithLogsDisallowed,
+  // TODO(crbug.com/776538): Force a proper on-demand update of the component.
+  component_updater::RegisterSwReporterComponent(
       g_browser_process->component_updater());
 
   base::RecordAction(