Break dependency loop between ui and safe_browsing/chrome_cleaner

* Move safe_browsing::GetExtensionNamesFromIds into
  settings::ChromeCleanupHandler. Its only function is to format
  extension names for display; this code should live in browser/ui. This
  lets us completely remove chrome_cleaner_extension_util_win.*

* Rename chrome_cleanup_handler.* to chrome_cleanup_handler_win.* to
  enforce that it's only built on Windows.

* Add a safe_browsing:chrome_cleaner_types target containing only files
  that don't depend on anything else in browser:
  * Move SwReporterInvocation and related from reporter_runner_win.h to
    a new sw_reporter_invocation_win.*.
  * Move some functions that are declared in
    chrome_cleaner_controller_win.h from
    chrome_cleaner_controller_impl_win.cc to
    chrome_cleaner_controller_win.cc, which they should have been in the
    first place.
  * Remove ChromeCleanerScannerResults::FetchExtensionNames, which was a
    thin wrapper around GetExtensionNamesFromIds and was only called from
    browser/ui and from tests.
  * Remove SwReporterInvocationSequence::NotifySequenceDone, which was a
    thin wrapper around ChromeCleanerController::OnReporterSequenceDone.
    This lets us leave GetCleanerController, which has a dependency on
    chrome_cleaner_controller_impl_win.h, in reporter_runner.cc.

* Also remove unused OnSequenceDoneCallback in
  reporter_runner_browsertest_win.cc.

Now the only files from safe_browsing/chrome_cleaner that ui depends on
are in safe_browsing:chrome_cleaner_types. A followup will update the
build files to break cycles in the safe_browsing dir.

Bug: 920223
Change-Id: If61a2e653234a958cca83de1bd61bd57d9f17352
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1614453
Reviewed-by: proberge <proberge@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Joe Mason <joenotcharles@google.com>
Cr-Commit-Position: refs/heads/master@{#661806}

chrome/browser/component_updater/sw_reporter_installer_win.h

@@ -12,7 +12,7 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
-#include "chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/sw_reporter_invocation_win.h"
 #include "components/component_updater/component_installer.h"
 #include "components/component_updater/component_updater_service.h"
 

chrome/browser/safe_browsing/BUILD.gn

@@ -48,23 +48,17 @@ jumbo_static_library("safe_browsing") {
     sources += [
       "chrome_cleaner/chrome_cleaner_controller_impl_win.cc",
       "chrome_cleaner/chrome_cleaner_controller_impl_win.h",
-      "chrome_cleaner/chrome_cleaner_controller_win.cc",
-      "chrome_cleaner/chrome_cleaner_controller_win.h",
       "chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.cc",
       "chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.h",
-      "chrome_cleaner/chrome_cleaner_dialog_controller_win.h",
       "chrome_cleaner/chrome_cleaner_extensions_util_win.h",
       "chrome_cleaner/chrome_cleaner_fetcher_win.cc",
       "chrome_cleaner/chrome_cleaner_fetcher_win.h",
       "chrome_cleaner/chrome_cleaner_navigation_util_win.cc",
       "chrome_cleaner/chrome_cleaner_navigation_util_win.h",
       "chrome_cleaner/chrome_cleaner_reboot_dialog_controller_impl_win.cc",
       "chrome_cleaner/chrome_cleaner_reboot_dialog_controller_impl_win.h",
-      "chrome_cleaner/chrome_cleaner_reboot_dialog_controller_win.h",
       "chrome_cleaner/chrome_cleaner_runner_win.cc",
       "chrome_cleaner/chrome_cleaner_runner_win.h",
-      "chrome_cleaner/chrome_cleaner_scanner_results_win.cc",
-      "chrome_cleaner/chrome_cleaner_scanner_results_win.h",
       "chrome_cleaner/reporter_runner_win.cc",
       "chrome_cleaner/reporter_runner_win.h",
       "chrome_cleaner/settings_resetter_win.cc",
@@ -73,8 +67,6 @@ jumbo_static_library("safe_browsing") {
       "chrome_cleaner/srt_chrome_prompt_impl_win.h",
       "chrome_cleaner/srt_client_info_win.cc",
       "chrome_cleaner/srt_client_info_win.h",
-      "chrome_cleaner/srt_field_trial_win.cc",
-      "chrome_cleaner/srt_field_trial_win.h",
       "settings_reset_prompt/default_settings_fetcher.cc",
       "settings_reset_prompt/default_settings_fetcher.h",
       "settings_reset_prompt/settings_reset_prompt_config.cc",
@@ -89,17 +81,12 @@ jumbo_static_library("safe_browsing") {
       "settings_reset_prompt/settings_reset_prompt_util_win.h",
     ]
     deps += [
+      ":chrome_cleaner_types",
       "//components/keep_alive_registry",
       "//extensions/browser",
     ]
   }
 
-  if (is_chrome_branded && is_win) {
-    sources += [ "chrome_cleaner/chrome_cleaner_extension_util_win.cc" ]
-  } else {
-    sources += [ "chrome_cleaner/chrome_cleaner_extension_util_win_noop.cc" ]
-  }
-
   if (safe_browsing_mode != 0) {
     # "Safe Browsing Basic" files used for safe browsing in full mode
     # (safe_browsing=1) and mobile (=2)
@@ -307,6 +294,29 @@ static_library("advanced_protection") {
   ]
 }
 
+source_set("chrome_cleaner_types") {
+  sources = [
+    "chrome_cleaner/chrome_cleaner_controller_win.cc",
+    "chrome_cleaner/chrome_cleaner_controller_win.h",
+    "chrome_cleaner/chrome_cleaner_dialog_controller_win.h",
+    "chrome_cleaner/chrome_cleaner_reboot_dialog_controller_win.h",
+    "chrome_cleaner/chrome_cleaner_scanner_results_win.cc",
+    "chrome_cleaner/chrome_cleaner_scanner_results_win.h",
+    "chrome_cleaner/srt_field_trial_win.cc",
+    "chrome_cleaner/srt_field_trial_win.h",
+    "chrome_cleaner/sw_reporter_invocation_win.cc",
+    "chrome_cleaner/sw_reporter_invocation_win.h",
+  ]
+
+  deps = [
+    "//base",
+    "//components/chrome_cleaner/public/constants",
+    "//components/chrome_cleaner/public/interfaces",
+    "//components/variations",
+    "//url",
+  ]
+}
+
 source_set("test_support") {
   if (safe_browsing_mode != 0) {
     sources = [

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_impl_win.cc

@@ -31,6 +31,7 @@
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_navigation_util_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_reboot_dialog_controller_impl_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_client_info_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
@@ -180,11 +181,6 @@ void RecordOnDemandUpdateRequiredHistogram(bool value) {
 
 }  // namespace
 
-void RecordCleanupStartedHistogram(CleanupStartedHistogramValue value) {
-  UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.CleanupStarted", value,
-                            CLEANUP_STARTED_MAX);
-}
-
 ChromeCleanerControllerDelegate::ChromeCleanerControllerDelegate() = default;
 
 ChromeCleanerControllerDelegate::~ChromeCleanerControllerDelegate() = default;
@@ -239,10 +235,6 @@ ChromeCleanerController* ChromeCleanerController::GetInstance() {
   return ChromeCleanerControllerImpl::GetInstance();
 }
 
-ChromeCleanerController::ChromeCleanerController() = default;
-
-ChromeCleanerController::~ChromeCleanerController() = default;
-
 ChromeCleanerController::State ChromeCleanerControllerImpl::state() const {
   return state_;
 }

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_impl_win_unittest.cc

@@ -594,26 +594,15 @@ TEST_P(ChromeCleanerControllerTest, WithMockCleanerProcess) {
         scanner_results_on_cleaning.registry_keys(),
         UnorderedElementsAreArray(scanner_results_on_infected.registry_keys()));
   }
-
-  std::set<base::string16> extension_names_infected;
-  scanner_results_on_infected.FetchExtensionNames(profile1,
-                                                  &extension_names_infected);
-  std::set<base::string16> extension_names_cleaning;
-  scanner_results_on_cleaning.FetchExtensionNames(profile1,
-                                                  &extension_names_cleaning);
-// Extension names only reported on Windows Chrome build.
-#if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
-  EXPECT_EQ(!extension_names_infected.empty(), ExpectedExtensionsReported());
-  EXPECT_EQ(!extension_names_cleaning.empty(),
+  EXPECT_EQ(!scanner_results_on_infected.extension_ids().empty(),
+            ExpectedExtensionsReported());
+  EXPECT_EQ(!scanner_results_on_cleaning.extension_ids().empty(),
             ExpectedExtensionsReported() && ExpectedOnCleaningCalled());
-  if (!extension_names_cleaning.empty()) {
-    EXPECT_THAT(extension_names_cleaning,
-                UnorderedElementsAreArray(extension_names_infected));
-  }
-#else
-  EXPECT_TRUE(extension_names_infected.empty());
-  EXPECT_TRUE(extension_names_cleaning.empty());
-#endif
+  if (!scanner_results_on_cleaning.extension_ids().empty()) {
+    EXPECT_THAT(
+        scanner_results_on_cleaning.extension_ids(),
+        UnorderedElementsAreArray(scanner_results_on_infected.extension_ids()));
+  }
 
   EXPECT_EQ(ExpectedRebootFlowStarted(), reboot_flow_started_);
 

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc

@@ -4,6 +4,10 @@
 
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h"
 
+#include <ostream>
+
+#include "base/metrics/histogram_macros.h"
+
 namespace safe_browsing {
 
 using UserResponse = ChromeCleanerController::UserResponse;
@@ -20,4 +24,13 @@ std::ostream& operator<<(std::ostream& out, UserResponse response) {
   return out << "Resp" << static_cast<int>(response);
 }
 
+void RecordCleanupStartedHistogram(CleanupStartedHistogramValue value) {
+  UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.CleanupStarted", value,
+                            CLEANUP_STARTED_MAX);
+}
+
+ChromeCleanerController::ChromeCleanerController() = default;
+
+ChromeCleanerController::~ChromeCleanerController() = default;
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h

@@ -11,13 +11,16 @@
 #include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
-#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_scanner_results_win.h"
-#include "chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/sw_reporter_invocation_win.h"
 #include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"
 
 class Profile;
 
+namespace extensions {
+class ExtensionService;
+}
+
 namespace safe_browsing {
 
 // These values are used to send UMA information and are replicated in the

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h

@@ -17,8 +17,8 @@
 #include "base/process/launch.h"
 #include "base/process/process.h"
 #include "base/sequenced_task_runner.h"
-#include "chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/sw_reporter_invocation_win.h"
 #include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/extension_system.h"

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc

@@ -22,6 +22,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
+#include "chrome/browser/ui/webui/settings/chrome_cleanup_handler_win.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
@@ -437,8 +438,9 @@ TEST_P(ChromeCleanerRunnerTest, WithMockCleanerProcess) {
     }
 
     std::set<base::string16> extension_names;
-    received_scanner_results_.FetchExtensionNames(testing_profile_,
-                                                  &extension_names);
+    settings::ChromeCleanupHandler::GetExtensionNamesFromIds(
+        testing_profile_, received_scanner_results_.extension_ids(),
+        &extension_names);
     if (cleaner_process_options_.extension_ids() &&
         extension_cleaning_feature_status_ ==
             ExtensionCleaningFeatureStatus::kEnabled) {

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_scanner_results_win.cc

@@ -4,9 +4,6 @@
 
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_scanner_results_win.h"
 
-#include "chrome/browser/profiles/profile_manager.h"
-#include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_extension_util_win.h"
-
 namespace safe_browsing {
 
 ChromeCleanerScannerResults::ChromeCleanerScannerResults() = default;
@@ -35,10 +32,4 @@ ChromeCleanerScannerResults& ChromeCleanerScannerResults::operator=(
   return *this;
 }
 
-void ChromeCleanerScannerResults::FetchExtensionNames(
-    Profile* profile,
-    ChromeCleanerScannerResults::ExtensionCollection* extension_names) const {
-  GetExtensionNamesFromIds(profile, extension_ids_, extension_names);
-}
-
 }  // namespace safe_browsing

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_scanner_results_win.h

@@ -9,7 +9,6 @@
 
 #include "base/files/file_path.h"
 #include "base/strings/string16.h"
-#include "chrome/browser/profiles/profile.h"
 
 namespace safe_browsing {
 
@@ -31,12 +30,6 @@ class ChromeCleanerScannerResults {
   ChromeCleanerScannerResults& operator=(
       const ChromeCleanerScannerResults& other);
 
-  // Retrieves the extension names of |extension_ids_| using the extension
-  // registry from |profile|. If a name cannot be found for an extension ID, a
-  // translated string is added stating that it is an unknown ID.
-  void FetchExtensionNames(Profile* profile,
-                           ExtensionCollection* extension_names) const;
-
   const FileCollection& files_to_delete() const { return files_to_delete_; }
   const RegistryKeyCollection& registry_keys() const { return registry_keys_; }
   const ExtensionCollection& extension_ids() const { return extension_ids_; }

chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc

@@ -29,6 +29,7 @@
 #include "chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_controller_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_client_info_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/sw_reporter_invocation_win.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/common/chrome_switches.h"
@@ -503,14 +504,6 @@ class ReporterRunnerTest
     std::move(closure).Run();
   }
 
-  OnReporterSequenceDone ExpectResultOnSequenceDoneCallback(
-      SwReporterInvocationResult expected_result,
-      base::OnceClosure closure) {
-    return base::BindOnce(&ReporterRunnerTest::ExpectResultOnSequenceDone,
-                          base::Unretained(this), expected_result,
-                          base::Passed(&closure));
-  }
-
   bool PromptDialogShouldBeShown(SwReporterInvocationType invocation_type) {
     return !IsUserInitiated(invocation_type) &&
            (incoming_seed_.empty() || (incoming_seed_ != old_seed_));