Remove ChromeCleanupProtobufIPC feature which is launched to 100%

* Removes the ChromePromptChannelMojo class and supporting code.
* Merges ChromePromptChannelProtobuf into the base ChromePromptChannel.
* Removes ChromePromptActions::PromptAcceptance which duplicated the
  PromptAcceptance enum from the protobuf.

R=proberge

Bug: 969139
Change-Id: I11350428dfddee2ec942ceda2fdb2dbc67aac863
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1903947
Commit-Queue: Joe Mason <joenotcharles@chromium.org>
Reviewed-by: proberge <proberge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713456}

chrome/browser/BUILD.gn

@@ -3819,7 +3819,6 @@ jumbo_static_library("browser") {
       "//components/browser_watcher:browser_watcher_client",
       "//components/browser_watcher:stability_client",
       "//components/chrome_cleaner/public/constants",
-      "//components/chrome_cleaner/public/interfaces",
       "//components/download/quarantine",
       "//third_party/crashpad/crashpad/client:client",
       "//third_party/iaccessible2",

chrome/browser/safe_browsing/chrome_cleaner/BUILD.gn

@@ -22,7 +22,6 @@ source_set("public") {
   deps = [
     "//base",
     "//components/chrome_cleaner/public/constants",
-    "//components/chrome_cleaner/public/interfaces",
     "//components/variations",
     "//url",
   ]
@@ -63,7 +62,6 @@ jumbo_static_library("chrome_cleaner") {
     "//chrome/common",
     "//chrome/installer/util:with_no_strings",
     "//components/chrome_cleaner/public/constants",
-    "//components/chrome_cleaner/public/interfaces",
     "//components/chrome_cleaner/public/proto",
     "//components/component_updater",
     "//components/crx_file",

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_impl_win.cc

@@ -38,6 +38,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/installer/util/scoped_token_privilege.h"
 #include "components/chrome_cleaner/public/constants/constants.h"
+#include "components/chrome_cleaner/public/proto/chrome_prompt.pb.h"
 #include "components/component_updater/component_updater_service.h"
 #include "components/component_updater/pref_names.h"
 #include "components/prefs/pref_service.h"
@@ -53,7 +54,7 @@ namespace safe_browsing {
 namespace {
 
 using ::content::BrowserThread;
-using PromptAcceptance = ChromePromptActions::PromptAcceptance;
+using PromptUserResponse = chrome_cleaner::PromptUserResponse;
 
 // The global singleton instance. Exposed outside of GetInstance() so that it
 // can be reset by tests.
@@ -470,11 +471,11 @@ void ChromeCleanerControllerImpl::ReplyWithUserResponse(
 
   DCHECK(prompt_user_reply_callback_);
 
-  PromptAcceptance acceptance = PromptAcceptance::DENIED;
+  PromptUserResponse::PromptAcceptance acceptance = PromptUserResponse::DENIED;
   State new_state = State::kIdle;
   switch (user_response) {
     case UserResponse::kAcceptedWithLogs:
-      acceptance = PromptAcceptance::ACCEPTED_WITH_LOGS;
+      acceptance = PromptUserResponse::ACCEPTED_WITH_LOGS;
       SetLogsEnabled(profile, true);
       RecordCleanerLogsAcceptanceHistogram(true);
       new_state = State::kCleaning;
@@ -483,7 +484,7 @@ void ChromeCleanerControllerImpl::ReplyWithUserResponse(
       extension_registry_ = extensions::ExtensionRegistry::Get(profile);
       break;
     case UserResponse::kAcceptedWithoutLogs:
-      acceptance = PromptAcceptance::ACCEPTED_WITHOUT_LOGS;
+      acceptance = PromptUserResponse::ACCEPTED_WITHOUT_LOGS;
       SetLogsEnabled(profile, false);
       RecordCleanerLogsAcceptanceHistogram(false);
       new_state = State::kCleaning;
@@ -493,7 +494,7 @@ void ChromeCleanerControllerImpl::ReplyWithUserResponse(
       break;
     case UserResponse::kDenied:  // Fallthrough
     case UserResponse::kDismissed:
-      acceptance = PromptAcceptance::DENIED;
+      acceptance = PromptUserResponse::DENIED;
       idle_reason_ = IdleReason::kUserDeclinedCleanup;
       new_state = State::kIdle;
       break;
@@ -631,9 +632,9 @@ void ChromeCleanerControllerImpl::WeakOnPromptUser(
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // If the weak pointer has been invalidated, the controller is no longer able
-  // to receive callbacks, so respond with PromptAcceptance::Denied immediately.
+  // to receive callbacks, so respond with DENIED immediately.
   if (!controller) {
-    std::move(reply_callback).Run(PromptAcceptance::DENIED);
+    std::move(reply_callback).Run(PromptUserResponse::DENIED);
     return;
   }
 
@@ -656,7 +657,7 @@ void ChromeCleanerControllerImpl::OnPromptUser(
                                base::Time::Now() - time_scanning_started_);
 
   if (scanner_results.files_to_delete().empty()) {
-    std::move(reply_callback).Run(PromptAcceptance::DENIED);
+    std::move(reply_callback).Run(PromptUserResponse::DENIED);
     idle_reason_ = IdleReason::kScanningFoundNothing;
     SetStateAndNotifyObservers(State::kIdle);
     RecordPromptNotShownWithReasonHistogram(NO_PROMPT_REASON_NOTHING_FOUND);

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_impl_win_unittest.cc

@@ -30,6 +30,7 @@
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "components/chrome_cleaner/public/constants/constants.h"
+#include "components/chrome_cleaner/public/proto/chrome_prompt.pb.h"
 #include "components/chrome_cleaner/test/test_name_helper.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/browser_task_environment.h"
@@ -54,11 +55,9 @@ using ::testing::ValuesIn;
 using CrashPoint = MockChromeCleanerProcess::CrashPoint;
 using ExtensionCleaningFeatureStatus =
     MockChromeCleanerProcess::ExtensionCleaningFeatureStatus;
-using ProtobufIPCFeatureStatus =
-    MockChromeCleanerProcess::ProtobufIPCFeatureStatus;
 using IdleReason = ChromeCleanerController::IdleReason;
 using ItemsReporting = MockChromeCleanerProcess::ItemsReporting;
-using PromptAcceptance = ChromePromptActions::PromptAcceptance;
+using PromptUserResponse = chrome_cleaner::PromptUserResponse;
 using State = ChromeCleanerController::State;
 using UserResponse = ChromeCleanerController::UserResponse;
 using UwsFoundStatus = MockChromeCleanerProcess::UwsFoundStatus;
@@ -66,19 +65,20 @@ using UwsFoundStatus = MockChromeCleanerProcess::UwsFoundStatus;
 // Returns the PromptAcceptance value that ChromeCleanerController is supposed
 // to send to the Chrome Cleaner process when ReplyWithUserResponse() is
 // called with |user_response|.
-PromptAcceptance UserResponseToPromptAcceptance(UserResponse user_response) {
+PromptUserResponse::PromptAcceptance UserResponseToPromptAcceptance(
+    UserResponse user_response) {
   switch (user_response) {
     case UserResponse::kAcceptedWithLogs:
-      return PromptAcceptance::ACCEPTED_WITH_LOGS;
+      return PromptUserResponse::ACCEPTED_WITH_LOGS;
     case UserResponse::kAcceptedWithoutLogs:
-      return PromptAcceptance::ACCEPTED_WITHOUT_LOGS;
+      return PromptUserResponse::ACCEPTED_WITHOUT_LOGS;
     case UserResponse::kDenied:  // Fallthrough
     case UserResponse::kDismissed:
-      return PromptAcceptance::DENIED;
+      return PromptUserResponse::DENIED;
   }
 
   NOTREACHED();
-  return PromptAcceptance::UNSPECIFIED;
+  return PromptUserResponse::UNSPECIFIED;
 }
 
 class MockChromeCleanerControllerObserver
@@ -232,7 +232,6 @@ typedef std::tuple<CleanerProcessStatus,
                    CrashPoint,
                    UwsFoundStatus,
                    ExtensionCleaningFeatureStatus,
-                   ProtobufIPCFeatureStatus,
                    ItemsReporting,
                    ItemsReporting,
                    UserResponse>
@@ -251,9 +250,8 @@ class ChromeCleanerControllerTest
 
   void SetUp() override {
     std::tie(process_status_, crash_point_, uws_found_status_,
-             extension_cleaning_feature_status_, protobuf_ipc_feature_status_,
-             registry_keys_reporting_, extensions_reporting_, user_response_) =
-        GetParam();
+             extension_cleaning_feature_status_, registry_keys_reporting_,
+             extensions_reporting_, user_response_) = GetParam();
 
     std::vector<base::Feature> enabled_features;
     std::vector<base::Feature> disabled_features;
@@ -263,11 +261,6 @@ class ChromeCleanerControllerTest
     } else {
       disabled_features.push_back(kChromeCleanupExtensionsFeature);
     }
-    if (protobuf_ipc_feature_status_ == ProtobufIPCFeatureStatus::kEnabled) {
-      enabled_features.push_back(kChromeCleanupProtobufIPCFeature);
-    } else {
-      disabled_features.push_back(kChromeCleanupProtobufIPCFeature);
-    }
     features_.InitWithFeatures(enabled_features, disabled_features);
 
     InitializeEmptyExtensionService();
@@ -280,7 +273,7 @@ class ChromeCleanerControllerTest
     cleaner_process_options_.set_crash_point(crash_point_);
     cleaner_process_options_.set_expected_user_response(
         uws_found_status_ == UwsFoundStatus::kNoUwsFound
-            ? PromptAcceptance::DENIED
+            ? PromptUserResponse::DENIED
             : UserResponseToPromptAcceptance(user_response_));
 
     ChromeCleanerControllerImpl::ResetInstanceForTesting();
@@ -471,7 +464,6 @@ class ChromeCleanerControllerTest
   MockChromeCleanerProcess::CrashPoint crash_point_;
   UwsFoundStatus uws_found_status_;
   ExtensionCleaningFeatureStatus extension_cleaning_feature_status_;
-  ProtobufIPCFeatureStatus protobuf_ipc_feature_status_;
   ItemsReporting registry_keys_reporting_;
   ItemsReporting extensions_reporting_;
   ChromeCleanerController::UserResponse user_response_;
@@ -650,8 +642,6 @@ INSTANTIATE_TEST_SUITE_P(
                    UwsFoundStatus::kUwsFoundNoRebootRequired),
             Values(ExtensionCleaningFeatureStatus::kEnabled,
                    ExtensionCleaningFeatureStatus::kDisabled),
-            Values(ProtobufIPCFeatureStatus::kEnabled,
-                   ProtobufIPCFeatureStatus::kDisabled),
             Values(ItemsReporting::kUnsupported,
                    ItemsReporting::kNotReported,
                    ItemsReporting::kReported),
@@ -671,8 +661,6 @@ INSTANTIATE_TEST_SUITE_P(
             ValuesIn(kCrashPointsAfterStartup),
             Values(UwsFoundStatus::kUwsFoundRebootRequired),
             Values(ExtensionCleaningFeatureStatus::kEnabled),
-            Values(ProtobufIPCFeatureStatus::kEnabled,
-                   ProtobufIPCFeatureStatus::kDisabled),
             Values(ItemsReporting::kReported),
             Values(ItemsReporting::kReported),
             Values(UserResponse::kDenied, UserResponse::kDismissed)),
@@ -689,8 +677,6 @@ INSTANTIATE_TEST_SUITE_P(
             ValuesIn(kCrashPointsAfterStartup),
             Values(UwsFoundStatus::kNoUwsFound),
             Values(ExtensionCleaningFeatureStatus::kDisabled),
-            Values(ProtobufIPCFeatureStatus::kEnabled,
-                   ProtobufIPCFeatureStatus::kDisabled),
             Values(ItemsReporting::kNotReported),
             Values(ItemsReporting::kNotReported),
             Values(UserResponse::kDismissed)),
@@ -706,7 +692,6 @@ INSTANTIATE_TEST_SUITE_P(
             Values(CrashPoint::kNone),
             Values(UwsFoundStatus::kNoUwsFound),
             Values(ExtensionCleaningFeatureStatus::kDisabled),
-            Values(ProtobufIPCFeatureStatus::kDisabled),
             Values(ItemsReporting::kUnsupported),
             Values(ItemsReporting::kUnsupported),
             Values(UserResponse::kAcceptedWithLogs)),
@@ -726,8 +711,6 @@ INSTANTIATE_TEST_SUITE_P(
             Values(CrashPoint::kOnStartup),
             Values(UwsFoundStatus::kNoUwsFound),
             Values(ExtensionCleaningFeatureStatus::kDisabled),
-            Values(ProtobufIPCFeatureStatus::kEnabled,
-                   ProtobufIPCFeatureStatus::kDisabled),
             Values(ItemsReporting::kUnsupported),
             Values(ItemsReporting::kUnsupported),
             Values(UserResponse::kAcceptedWithLogs)),

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc

@@ -29,8 +29,6 @@
 #include "chrome/installer/util/install_util.h"
 #include "components/chrome_cleaner/public/constants/constants.h"
 #include "components/version_info/version_info.h"
-#include "content/public/browser/browser_task_traits.h"
-#include "content/public/browser/browser_thread.h"
 
 namespace safe_browsing {
 
@@ -166,30 +164,16 @@ ChromeCleanerRunner::LaunchAndWaitForExitOnBackgroundThread(
       base::BindOnce(&ChromeCleanerRunner::OnPromptUser,
                      base::RetainedRef(this)));
 
+  // The channel will make blocking calls to ::WriteFile.
+  scoped_refptr<base::SequencedTaskRunner> channel_task_runner =
+      base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock()});
+
   // ChromePromptChannel method calls will be posted to this sequence using
   // WeakPtr's, so the channel must be deleted on the same sequence.
-  scoped_refptr<base::SequencedTaskRunner> channel_task_runner;
-  using ChromePromptChannelPtr =
-      std::unique_ptr<ChromePromptChannel, base::OnTaskRunnerDeleter>;
-  ChromePromptChannelPtr channel(nullptr, base::OnTaskRunnerDeleter(nullptr));
-  if (base::FeatureList::IsEnabled(kChromeCleanupProtobufIPCFeature)) {
-    // The channel will make blocking calls to ::WriteFile.
-    channel_task_runner =
-        base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock()});
-    channel =
-        ChromePromptChannelPtr(new ChromePromptChannelProtobuf(
-                                   std::move(on_connection_closed),
-                                   std::move(actions), channel_task_runner),
-                               base::OnTaskRunnerDeleter(channel_task_runner));
-  } else {
-    // Mojo uses the IO thread.
-    channel_task_runner =
-        base::CreateSingleThreadTaskRunner({content::BrowserThread::IO});
-    channel = ChromePromptChannelPtr(
-        new ChromePromptChannelMojo(std::move(on_connection_closed),
-                                    std::move(actions), channel_task_runner),
-        base::OnTaskRunnerDeleter(channel_task_runner));
-  }
+  std::unique_ptr<ChromePromptChannel, base::OnTaskRunnerDeleter> channel(
+      new ChromePromptChannel(std::move(on_connection_closed),
+                              std::move(actions), channel_task_runner),
+      base::OnTaskRunnerDeleter(channel_task_runner));
 
   base::LaunchOptions launch_options;
   if (!channel->PrepareForCleaner(&cleaner_command_line_,

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h

@@ -98,20 +98,15 @@ class ChromeCleanerRunner
   // This function will pass command line flags to the Chrome Cleaner
   // executable as appropriate based on the flags in |reporter_invocation| and
   // the |metrics_status| parameters. The Cleaner process will communicate with
-  // Chrome via a Mojo or protobuf IPC interface and any IPC requests or
-  // notifications are passed to the caller via the |on_prompt_user| and
-  // |on_connection_closed| callbacks. Finally, when the Chrome Cleaner process
-  // terminates, a ProcessStatus is passed along to |on_process_done|.
+  // Chrome via an IPC interface and any IPC requests or notifications are
+  // passed to the caller via the |on_prompt_user| and |on_connection_closed|
+  // callbacks. Finally, when the Chrome Cleaner process terminates, a
+  // ProcessStatus is passed along to |on_process_done|.
   //
   // This IPC interface needs the |extension_service| in order to disable
   // extensions that the Cleaner process wants to disable.
   //
-  // The details of the Mojo interface are documented in
-  // "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h".
-  //
-  // TODO(crbug.com/969139): Add a reference to the protobuf interface. Once
-  // the experiment is over, update this comment to only reference the
-  // interface that's actually used.
+  // See ChromePromptChannel for more details of the IPC interface.
   static void RunChromeCleanerAndReplyWithExitCode(
       extensions::ExtensionService* extension_service,
       extensions::ExtensionRegistry* extension_registry,

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc

@@ -41,9 +41,7 @@ using ::testing::Values;
 using ChromeMetricsStatus = ChromeCleanerRunner::ChromeMetricsStatus;
 using ExtensionCleaningFeatureStatus =
     MockChromeCleanerProcess::ExtensionCleaningFeatureStatus;
-using ProtobufIPCFeatureStatus =
-    MockChromeCleanerProcess::ProtobufIPCFeatureStatus;
-using PromptAcceptance = ChromePromptActions::PromptAcceptance;
+using PromptUserResponse = chrome_cleaner::PromptUserResponse;
 using UwsFoundStatus = MockChromeCleanerProcess::UwsFoundStatus;
 
 enum class ReporterEngine {
@@ -214,11 +212,10 @@ INSTANTIATE_TEST_SUITE_P(All,
 
 typedef std::tuple<UwsFoundStatus,
                    ExtensionCleaningFeatureStatus,
-                   ProtobufIPCFeatureStatus,
                    MockChromeCleanerProcess::ItemsReporting,
                    MockChromeCleanerProcess::ItemsReporting,
                    MockChromeCleanerProcess::CrashPoint,
-                   PromptAcceptance>
+                   PromptUserResponse::PromptAcceptance>
     ChromeCleanerRunnerTestParams;
 
 // Test fixture for testing ChromeCleanerRunner with a mock Chrome Cleaner
@@ -243,12 +240,11 @@ class ChromeCleanerRunnerTest
     MockChromeCleanerProcess::ItemsReporting extensions_reporting;
     MockChromeCleanerProcess::CrashPoint crash_point;
     std::tie(uws_found_state, extension_cleaning_feature_status_,
-             protobuf_ipc_feature_status_, registry_keys_reporting,
-             extensions_reporting, crash_point, prompt_acceptance_to_send_) =
-        GetParam();
+             registry_keys_reporting, extensions_reporting, crash_point,
+             prompt_acceptance_to_send_) = GetParam();
 
     ASSERT_FALSE(uws_found_state == UwsFoundStatus::kNoUwsFound &&
-                 prompt_acceptance_to_send_ != PromptAcceptance::DENIED);
+                 prompt_acceptance_to_send_ != PromptUserResponse::DENIED);
 
     std::vector<base::Feature> enabled_features;
     std::vector<base::Feature> disabled_features;
@@ -258,11 +254,6 @@ class ChromeCleanerRunnerTest
     } else {
       disabled_features.push_back(kChromeCleanupExtensionsFeature);
     }
-    if (protobuf_ipc_feature_status_ == ProtobufIPCFeatureStatus::kEnabled) {
-      enabled_features.push_back(kChromeCleanupProtobufIPCFeature);
-    } else {
-      disabled_features.push_back(kChromeCleanupProtobufIPCFeature);
-    }
     features_.InitWithFeatures(enabled_features, disabled_features);
 
     cleaner_process_options_.SetReportedResults(
@@ -353,9 +344,9 @@ class ChromeCleanerRunnerTest
   base::RunLoop run_loop_;
 
   MockChromeCleanerProcess::Options cleaner_process_options_;
-  PromptAcceptance prompt_acceptance_to_send_ = PromptAcceptance::UNSPECIFIED;
+  PromptUserResponse::PromptAcceptance prompt_acceptance_to_send_ =
+      PromptUserResponse::UNSPECIFIED;
   ExtensionCleaningFeatureStatus extension_cleaning_feature_status_;
-  ProtobufIPCFeatureStatus protobuf_ipc_feature_status_;
 
   // Set by OnProcessDone().
   ChromeCleanerRunner::ProcessStatus process_status_;
@@ -450,8 +441,6 @@ INSTANTIATE_TEST_SUITE_P(
     Combine(Values(UwsFoundStatus::kNoUwsFound),
             // When no UwS is found we don't care about extension removel.
             Values(ExtensionCleaningFeatureStatus::kDisabled),
-            // When no UwS is found there is no prompt.
-            Values(ProtobufIPCFeatureStatus::kDisabled),
             Values(MockChromeCleanerProcess::ItemsReporting::kUnsupported,
                    MockChromeCleanerProcess::ItemsReporting::kNotReported,
                    MockChromeCleanerProcess::ItemsReporting::kReported),
@@ -459,7 +448,7 @@ INSTANTIATE_TEST_SUITE_P(
                    MockChromeCleanerProcess::ItemsReporting::kNotReported,
                    MockChromeCleanerProcess::ItemsReporting::kReported),
             Values(MockChromeCleanerProcess::CrashPoint::kNone),
-            Values(PromptAcceptance::DENIED)),
+            Values(PromptUserResponse::DENIED)),
     chrome_cleaner::GetParamNameForTest());
 
 INSTANTIATE_TEST_SUITE_P(
@@ -469,15 +458,13 @@ INSTANTIATE_TEST_SUITE_P(
         Values(UwsFoundStatus::kNoUwsFound),
         // When no UwS is found we don't care about extension removel.
         Values(ExtensionCleaningFeatureStatus::kDisabled),
-        // When no UwS is found there is no prompt.
-        Values(ProtobufIPCFeatureStatus::kDisabled),
         Values(MockChromeCleanerProcess::ItemsReporting::kReported),
         Values(MockChromeCleanerProcess::ItemsReporting::kReported),
         Values(MockChromeCleanerProcess::CrashPoint::kOnStartup,
                MockChromeCleanerProcess::CrashPoint::kAfterConnection,
                MockChromeCleanerProcess::CrashPoint::kAfterRequestSent,
                MockChromeCleanerProcess::CrashPoint::kAfterResponseReceived),
-        Values(PromptAcceptance::DENIED)),
+        Values(PromptUserResponse::DENIED)),
     chrome_cleaner::GetParamNameForTest());
 
 INSTANTIATE_TEST_SUITE_P(
@@ -487,8 +474,6 @@ INSTANTIATE_TEST_SUITE_P(
                    UwsFoundStatus::kUwsFoundNoRebootRequired),
             Values(ExtensionCleaningFeatureStatus::kEnabled,
                    ExtensionCleaningFeatureStatus::kDisabled),
-            Values(ProtobufIPCFeatureStatus::kEnabled,
-                   ProtobufIPCFeatureStatus::kDisabled),
             Values(MockChromeCleanerProcess::ItemsReporting::kUnsupported,
                    MockChromeCleanerProcess::ItemsReporting::kNotReported,
                    MockChromeCleanerProcess::ItemsReporting::kReported),
@@ -496,9 +481,9 @@ INSTANTIATE_TEST_SUITE_P(
                    MockChromeCleanerProcess::ItemsReporting::kNotReported,
                    MockChromeCleanerProcess::ItemsReporting::kReported),
             Values(MockChromeCleanerProcess::CrashPoint::kNone),
-            Values(PromptAcceptance::DENIED,
-                   PromptAcceptance::ACCEPTED_WITH_LOGS,
-                   PromptAcceptance::ACCEPTED_WITHOUT_LOGS)),
+            Values(PromptUserResponse::DENIED,
+                   PromptUserResponse::ACCEPTED_WITH_LOGS,
+                   PromptUserResponse::ACCEPTED_WITHOUT_LOGS)),
     chrome_cleaner::GetParamNameForTest());
 
 INSTANTIATE_TEST_SUITE_P(
@@ -507,15 +492,13 @@ INSTANTIATE_TEST_SUITE_P(
     Combine(
         Values(UwsFoundStatus::kUwsFoundRebootRequired),
         Values(ExtensionCleaningFeatureStatus::kDisabled),
-        Values(ProtobufIPCFeatureStatus::kEnabled,
-               ProtobufIPCFeatureStatus::kDisabled),
         Values(MockChromeCleanerProcess::ItemsReporting::kReported),
         Values(MockChromeCleanerProcess::ItemsReporting::kReported),
         Values(MockChromeCleanerProcess::CrashPoint::kOnStartup,
                MockChromeCleanerProcess::CrashPoint::kAfterConnection,
                MockChromeCleanerProcess::CrashPoint::kAfterRequestSent,
                MockChromeCleanerProcess::CrashPoint::kAfterResponseReceived),
-        Values(PromptAcceptance::ACCEPTED_WITH_LOGS)),
+        Values(PromptUserResponse::ACCEPTED_WITH_LOGS)),
     chrome_cleaner::GetParamNameForTest());
 
 }  // namespace

chrome/browser/safe_browsing/chrome_cleaner/chrome_prompt_actions_win.cc

@@ -101,12 +101,4 @@ bool ChromePromptActions::DisableExtensions(
   return result;
 }
 
-// Keep the printable name short since it's used in tests with very long
-// parameter lists.
-std::ostream& operator<<(
-    std::ostream& out,
-    ChromePromptActions::PromptAcceptance prompt_acceptance) {
-  return out << "Accept" << static_cast<int>(prompt_acceptance);
-}
-
 }  // namespace safe_browsing

chrome/browser/safe_browsing/chrome_cleaner/chrome_prompt_actions_win.h

@@ -13,6 +13,7 @@
 #include "base/files/file_path.h"
 #include "base/optional.h"
 #include "base/strings/string16.h"
+#include "components/chrome_cleaner/public/proto/chrome_prompt.pb.h"
 
 namespace extensions {
 class ExtensionRegistry;
@@ -30,24 +31,9 @@ class ChromeCleanerScannerResults;
 // message received.
 class ChromePromptActions {
  public:
-  // TODO(crbug.com/969139): This mirrors the PromptAcceptance enums from
-  // chrome_prompt.mojom and chrome_prompt.proto. Once the
-  // ChromeCleanupProtobufIPC experiment is over remove this and use the proto
-  // version directly.
-  enum class PromptAcceptance {
-    UNSPECIFIED,
-    // The user explicitly accepted the cleanup operation and cleaner logs
-    // upload is allowed.
-    ACCEPTED_WITH_LOGS,
-    // The user explicitly accepted the cleanup operation and cleaner logs
-    // upload is not allowed.
-    ACCEPTED_WITHOUT_LOGS,
-    // The user explicitly denied the Chrome prompt.
-    DENIED,
-  };
-
   // A callback to be called after showing the prompt, with the user's choice.
-  using PromptUserReplyCallback = base::OnceCallback<void(PromptAcceptance)>;
+  using PromptUserReplyCallback = base::OnceCallback<void(
+      chrome_cleaner::PromptUserResponse::PromptAcceptance)>;
 
   // A callback to show the prompt. The ChromeCleanerScannerResults contains
   // the items that were detected by the scanner, for display in the prompt.
@@ -77,9 +63,6 @@ class ChromePromptActions {
   // Deletes the given |extension_ids|. Returns false on an error, including if
   // not all |extension_ids| were displayed to the user in the last PromptUser
   // call.
-  //
-  // TODO(crbug.com/969139): Now that we're updating the IPC interface, rename
-  // this DeleteExtensions to match the implementation.
   bool DisableExtensions(const std::vector<base::string16>& extension_ids);
 
  private:
@@ -102,11 +85,6 @@ class ChromePromptActions {
   std::vector<base::string16> extension_ids_;
 };
 
-//  Format for debug output in tests.
-std::ostream& operator<<(
-    std::ostream& out,
-    ChromePromptActions::PromptAcceptance prompt_acceptance);
-
 }  // namespace safe_browsing
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_CHROME_CLEANER_CHROME_PROMPT_ACTIONS_WIN_H_

chrome/browser/safe_browsing/chrome_cleaner/chrome_prompt_channel_win.h

@@ -16,11 +16,7 @@
 #include "base/sequenced_task_runner.h"
 #include "base/win/scoped_handle.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_prompt_actions_win.h"
-#include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"
 #include "components/chrome_cleaner/public/proto/chrome_prompt.pb.h"
-#include "mojo/public/cpp/platform/platform_channel.h"
-#include "mojo/public/cpp/system/invitation.h"
-#include "mojo/public/cpp/system/message_pipe.h"
 #include "third_party/protobuf/src/google/protobuf/message_lite.h"
 
 namespace base {
@@ -29,15 +25,18 @@ class CommandLine;
 
 namespace safe_browsing {
 
-namespace internal {
-class ChromePromptImpl;
-}  // namespace internal
-
 class ChromePromptActions;
 
 // Handles IPC to the Chrome Cleaner process. The Chrome Cleaner process will
 // send requests and ChromePromptChannel will handle the request, often by
 // using ChromePromptActions, and write the response.
+//
+// This implementation serializes protobufs over a pipe, instead of using Mojo,
+// because the Chrome Cleaner process might be built with a different version
+// of Mojo that isn't wire-compatible.
+//
+// The interface specification is in
+// "components/chrome_cleaner/public/proto/chrome_prompt.proto".
 class ChromePromptChannel {
  public:
   // Gives access to the Chrome Cleaner process that the channel communicates
@@ -51,91 +50,6 @@ class ChromePromptChannel {
     virtual void TerminateOnError() const = 0;
   };
 
-  // Returns a CleanerProcessDelegate that wraps |process|.
-  static std::unique_ptr<CleanerProcessDelegate> CreateDelegateForProcess(
-      const base::Process& process);
-
-  // Creates a ChromePromptChannel that calls |on_connection_closed| when the
-  // IPC channel closes (either normally or on error) and uses |actions| to
-  // fulfill requests. |task_runner| can be used to run any tasks that must be
-  // seqeuenced with destruction of the ChromePromptChannel.
-  ChromePromptChannel(base::OnceClosure on_connection_closed,
-                      std::unique_ptr<ChromePromptActions> actions,
-                      scoped_refptr<base::SequencedTaskRunner> task_runner);
-
-  virtual ~ChromePromptChannel();
-
-  // Prepares an IPC channel to be used by the cleaner process that is about to
-  // be launched. Adds all handles used by the channel to |handles_to_inherit|
-  // so that the cleaner process can access them, and adds switches to
-  // |command_line| that the cleaner process can use to connect to the channel.
-  virtual bool PrepareForCleaner(
-      base::CommandLine* command_line,
-      base::HandlesToInheritVector* handles_to_inherit) = 0;
-
-  // Does any cleanup required if the cleaner process fails to launch after
-  // PrepareForCleaner was called.
-  virtual void CleanupAfterCleanerLaunchFailed() = 0;
-
-  // Kicks off communication between the IPC channel prepared by
-  // PrepareForCleaner and the process in |cleaner_process|. If the connection
-  // fails, |connection_closed_callback_| should be called.
-  virtual void ConnectToCleaner(
-      std::unique_ptr<CleanerProcessDelegate> cleaner_process) = 0;
-
-  scoped_refptr<base::SequencedTaskRunner> task_runner() const {
-    return task_runner_;
-  }
-
- protected:
-  base::OnceClosure on_connection_closed_;
-  std::unique_ptr<ChromePromptActions> actions_;
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
-
- private:
-  ChromePromptChannel(const ChromePromptChannel& other) = delete;
-  ChromePromptChannel& operator=(const ChromePromptChannel& other) = delete;
-};
-
-// Handles IPC to the Chrome Cleaner process using Mojo.
-class ChromePromptChannelMojo : public ChromePromptChannel {
- public:
-  ChromePromptChannelMojo(base::OnceClosure on_connection_closed,
-                          std::unique_ptr<ChromePromptActions> actions,
-                          scoped_refptr<base::SequencedTaskRunner> task_runner);
-
-  ~ChromePromptChannelMojo() override;
-
-  bool PrepareForCleaner(
-      base::CommandLine* command_line,
-      base::HandlesToInheritVector* handles_to_inherit) override;
-
-  void CleanupAfterCleanerLaunchFailed() override;
-
-  void ConnectToCleaner(
-      std::unique_ptr<CleanerProcessDelegate> cleaner_process) override;
-
- private:
-  ChromePromptChannelMojo(const ChromePromptChannelMojo& other) = delete;
-  ChromePromptChannelMojo& operator=(const ChromePromptChannelMojo& other) =
-      delete;
-
-  void CreateChromePromptImpl(
-      chrome_cleaner::mojom::ChromePromptRequest chrome_prompt_request);
-
-  mojo::OutgoingInvitation invitation_;
-  mojo::PlatformChannel mojo_channel_;
-  mojo::ScopedMessagePipeHandle request_pipe_;
-
-  std::unique_ptr<internal::ChromePromptImpl> chrome_prompt_impl_;
-
-  base::WeakPtrFactory<ChromePromptChannelMojo> weak_factory_{this};
-};
-
-// Handles IPC to the Chrome Cleaner process by serializing protobufs over
-// a pipe.
-class ChromePromptChannelProtobuf : public ChromePromptChannel {
- public:
   static const char kErrorHistogramName[];
   static constexpr uint32_t kMaxMessageLength = 1 * 1024 * 1024;  // 1M bytes
 
@@ -193,21 +107,40 @@ class ChromePromptChannelProtobuf : public ChromePromptChannel {
     return category_and_code;
   }
 
-  ChromePromptChannelProtobuf(
-      base::OnceClosure on_connection_closed,
-      std::unique_ptr<ChromePromptActions> actions,
-      scoped_refptr<base::SequencedTaskRunner> task_runner);
+  // Returns a CleanerProcessDelegate that wraps |process|.
+  static std::unique_ptr<CleanerProcessDelegate> CreateDelegateForProcess(
+      const base::Process& process);
+
+  // Creates a ChromePromptChannel that calls |on_connection_closed| when the
+  // IPC channel closes (either normally or on error) and uses |actions| to
+  // fulfill requests. |task_runner| can be used to run any tasks that must be
+  // seqeuenced with destruction of the ChromePromptChannel.
+  ChromePromptChannel(base::OnceClosure on_connection_closed,
+                      std::unique_ptr<ChromePromptActions> actions,
+                      scoped_refptr<base::SequencedTaskRunner> task_runner);
 
-  ~ChromePromptChannelProtobuf() override;
+  ~ChromePromptChannel();
 
-  bool PrepareForCleaner(
-      base::CommandLine* command_line,
-      base::HandlesToInheritVector* handles_to_inherit) override;
+  // Prepares an IPC channel to be used by the cleaner process that is about to
+  // be launched. Adds all handles used by the channel to |handles_to_inherit|
+  // so that the cleaner process can access them, and adds switches to
+  // |command_line| that the cleaner process can use to connect to the channel.
+  bool PrepareForCleaner(base::CommandLine* command_line,
+                         base::HandlesToInheritVector* handles_to_inherit);
 
-  void CleanupAfterCleanerLaunchFailed() override;
+  // Does any cleanup required if the cleaner process fails to launch after
+  // PrepareForCleaner was called.
+  void CleanupAfterCleanerLaunchFailed();
 
+  // Kicks off communication between the IPC channel prepared by
+  // PrepareForCleaner and the process in |cleaner_process|. If the connection
+  // fails, |connection_closed_callback_| should be called.
   void ConnectToCleaner(
-      std::unique_ptr<CleanerProcessDelegate> cleaner_process) override;
+      std::unique_ptr<CleanerProcessDelegate> cleaner_process);
+
+  scoped_refptr<base::SequencedTaskRunner> task_runner() const {
+    return task_runner_;
+  }
 
   // Handles |request| and sends a QueryCapabilityResponse in reply.
   void HandleQueryCapabilityRequest(
@@ -226,20 +159,23 @@ class ChromePromptChannelProtobuf : public ChromePromptChannel {
   void CloseHandles();
 
  private:
-  ChromePromptChannelProtobuf(const ChromePromptChannelProtobuf& other) =
-      delete;
-  ChromePromptChannelProtobuf& operator=(
-      const ChromePromptChannelProtobuf& other) = delete;
+  ChromePromptChannel(const ChromePromptChannel& other) = delete;
+  ChromePromptChannel& operator=(const ChromePromptChannel& other) = delete;
 
   // Serializes |message| to response_write_handle_. Calls CloseHandles on
   // error.
   void WriteResponseMessage(const google::protobuf::MessageLite& message);
 
   // Sends a PromptUserResponse with the given |acceptance| value.
-  void SendPromptUserResponse(ChromePromptActions::PromptAcceptance acceptance);
+  void SendPromptUserResponse(
+      chrome_cleaner::PromptUserResponse::PromptAcceptance acceptance);
 
   SEQUENCE_CHECKER(sequence_checker_);
 
+  base::OnceClosure on_connection_closed_;
+  std::unique_ptr<ChromePromptActions> actions_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
   // Requests always flow from the Chrome Cleanup tool to Chrome.
   // This class owns request_read_handle_ but request_write_handle_ will be
   // closed once it is passed to the child process.
@@ -252,7 +188,7 @@ class ChromePromptChannelProtobuf : public ChromePromptChannel {
   base::win::ScopedHandle response_read_handle_;
   base::win::ScopedHandle response_write_handle_;
 
-  base::WeakPtrFactory<ChromePromptChannelProtobuf> weak_factory_{this};
+  base::WeakPtrFactory<ChromePromptChannel> weak_factory_{this};
 };
 
 }  // namespace safe_browsing

chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.h

@@ -70,11 +70,6 @@ class MockChromeCleanerProcess {
     kDisabled,
   };
 
-  enum class ProtobufIPCFeatureStatus {
-    kEnabled,
-    kDisabled,
-  };
-
   static constexpr int kInternalTestFailureExitCode = 100001;
   static constexpr int kDeliberateCrashExitCode = 100002;
   static constexpr int kNothingFoundExitCode = 2;
@@ -130,11 +125,13 @@ class MockChromeCleanerProcess {
     CrashPoint crash_point() const { return crash_point_; }
 
     void set_expected_user_response(
-        ChromePromptActions::PromptAcceptance expected_user_response) {
+        chrome_cleaner::PromptUserResponse::PromptAcceptance
+            expected_user_response) {
       expected_user_response_ = expected_user_response;
     }
 
-    ChromePromptActions::PromptAcceptance expected_user_response() const {
+    chrome_cleaner::PromptUserResponse::PromptAcceptance
+    expected_user_response() const {
       return expected_user_response_;
     }
 
@@ -146,8 +143,8 @@ class MockChromeCleanerProcess {
       return extensions_reporting_;
     }
 
-    int ExpectedExitCode(
-        ChromePromptActions::PromptAcceptance received_prompt_acceptance) const;
+    int ExpectedExitCode(chrome_cleaner::PromptUserResponse::PromptAcceptance
+                             received_prompt_acceptance) const;
 
    private:
     std::vector<base::FilePath> files_to_delete_;
@@ -158,8 +155,9 @@ class MockChromeCleanerProcess {
     CrashPoint crash_point_ = CrashPoint::kNone;
     ItemsReporting registry_keys_reporting_ = ItemsReporting::kUnsupported;
     ItemsReporting extensions_reporting_ = ItemsReporting::kUnsupported;
-    ChromePromptActions::PromptAcceptance expected_user_response_ =
-        ChromePromptActions::PromptAcceptance::UNSPECIFIED;
+    chrome_cleaner::PromptUserResponse::PromptAcceptance
+        expected_user_response_ =
+            chrome_cleaner::PromptUserResponse::UNSPECIFIED;
   };
 
   MockChromeCleanerProcess();
@@ -194,10 +192,6 @@ std::ostream& operator<<(
     std::ostream& out,
     MockChromeCleanerProcess::ExtensionCleaningFeatureStatus status);
 
-std::ostream& operator<<(
-    std::ostream& out,
-    MockChromeCleanerProcess::ProtobufIPCFeatureStatus status);
-
 std::ostream& operator<<(
     std::ostream& out,
     MockChromeCleanerProcess::ItemsReporting items_reporting);

chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.cc

@@ -42,9 +42,6 @@ const base::Feature kChromeCleanupDistributionFeature{
 const base::Feature kChromeCleanupExtensionsFeature{
     "ChromeCleanupExtensions", base::FEATURE_DISABLED_BY_DEFAULT};
 
-const base::Feature kChromeCleanupProtobufIPCFeature{
-    "ChromeCleanupProtobufIPC", base::FEATURE_ENABLED_BY_DEFAULT};
-
 bool IsSRTPromptFeatureEnabled() {
   return base::FeatureList::IsEnabled(kChromeCleanupInBrowserPromptFeature);
 }

chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h

@@ -64,11 +64,6 @@ extern const base::Feature kChromeCleanupDistributionFeature;
 // for, and cleanup, bad extensions.
 extern const base::Feature kChromeCleanupExtensionsFeature;
 
-// Protobuf IPC feature. When enabled, Chrome Cleaner will communicate by
-// serializing protobufs over a custom IPC pipe that isn't tied to the Mojo
-// version.
-extern const base::Feature kChromeCleanupProtobufIPCFeature;
-
 // Returns true if this Chrome is in a field trial group which shows the SRT
 // prompt.
 bool IsSRTPromptFeatureEnabled();