Chrome Cleaner UI: reporter no longer uses mojo.

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h

+// Copyright 2017 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_SAFE_BROWSING_CHROME_CLEANER_CHROME_CLEANER_RUNNER_WIN_H_
+#define CHROME_BROWSER_SAFE_BROWSING_CHROME_CLEANER_CHROME_CLEANER_RUNNER_WIN_H_
+
+#include <limits>
+#include <memory>
+#include <set>
+#include <string>
+
+#include "base/callback.h"
+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/memory/ref_counted.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h"
+#include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"
+
+namespace safe_browsing {
+
+// Class responsible for launching the cleaner process and waiting for its
+// completion when the InBrowserCleanerUI feature is enabled. This object is
+// also responsible for starting the ChromePromptImpl object on the IO thread
+// and controlling its lifetime.
+//
+// Expected lifecycle of a ChromeCleanerRunner:
+//  - Created on the UI thread.

[Joe Mason, 2017/05/19 22:02:25]

Can you rephrase this as something like "created on a sequence called the
CleanerRunner sequence"? Then you can refer to the "CleanerRunner sequence" in
the rest of the comments. It happens that right now the CleanerRunner sequence
will be the UI thread because that's the thread that calls
RunChromeCleanerAndReplyWithExitCode, but if I understand right this class
should keep working if that changes.

[alito, 2017/05/22 23:26:31]

I made another pass over the comments in this file to update them wrt the
changes. PTAnL to see if there is anything that is still unclear.

Regarding the UI thread and sequences: I might be wrong, but based on the
documentation available, I don't see any guarantees that functions posted on the
same sequence will be posted on the same thread. The ChromeCleanerController
class will be the main client of ChromeCleanerRunner. That class lives on the UI
thread because it interacts with the cleaner dialog and the webui page; it
explicitly expects its callbacks to be called on the UI thread (makes life so
much simpler while we still have an explicit UI thread). So instead of a
sequence I decided to use a SingleThreadTaskRunner and post all callbacks to
that.

I might be wrong about the assumptions about sequences and threads, so let me
know if you know otherwise.

Thanks.

+//  - Launches the Chrome Cleaner process when the
+//    LaunchAndWaitForExitOnBackgroundThread() function is called, creates a
+//    ChromePromptImpl object on the IO thread and waits for the process to
+//    terminate. That function must be called on a sequence with the correct set
+//    of traits. See the implementation of
+//    RunChromeCleanerAndReplyWithExitCode() for details.
+//  - deleted on the UI thread when the Chrome Cleaner process exits, at which

[Joe Mason, 2017/05/19 22:02:25]

Which would make this "deleted on the CleanerRunner sequence..."

[alito, 2017/05/22 23:26:31]

Reformulated.

+//    point the ChromePromptImpl object will also be scheduled for deletion on
+//    the IO thread.
+class ChromeCleanerRunner
+    : public base::RefCountedThreadSafe<ChromeCleanerRunner> {
+ public:
+  using ProcessDoneCallback = base::OnceCallback<void(int /*exit_code*/)>;
+
+  static constexpr int kNotLaunchedExitCode = std::numeric_limits<int>::max();
+
+  // Executes the Chrome Cleaner in the background and passes information back
+  // to the caller by calling the given callbacks on the UI thread.

[Joe Mason, 2017/05/19 22:02:25]

I would phrase this as something like "...by calling the given callbacks on the
calling sequence, which becomes the CleanerRunner sequence." (However you choose
to word things, I think it's important to specify what sequence this function is
expected to be called on, as well as what sequence the callbacks are on.)

[alito, 2017/05/22 23:26:31]

Reformulated the comments. PTAnL.

+  //
+  // This function will pass command line flags to the Chrome Cleaner executable
+  // as appropriate based on the flags in |reporter_invocation| and the
+  // |matrics_enabled| and |cleaner_logs_enabled| parameters. The Cleaner

[Joe Mason, 2017/05/19 22:02:25]

Typo: metrics_enabled.

[alito, 2017/05/22 23:26:31]

Done.

+  // process will communicate with Chrome via a mojo IPC interface and any IPC
+  // requests or notifications are passed to the caller on the UI thread via the
+  // |on_prompt_user|, |on_connection_closed|, and |on_connection_error|
+  // callbacks. The details of the mojo interface are documented in
+  // "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h".
+  //
+  // Finally, when the Chrome Cleaner process terminates, the exit code is
+  // passed along to |done_callback|, also on the UI thread.
+  static void RunChromeCleanerAndReplyWithExitCode(
+      const base::FilePath& executable_path,
+      const SwReporterInvocation& reporter_invocation,
+      bool metrics_enabled,

[Joe Mason, 2017/05/19 22:02:25]

How about enums for metrics_enabled and cleaner_logs_enabled (or a bitmask
covering both) to avoid having to comment the meaning of the bools at the call
site?

[alito, 2017/05/22 23:26:31]

Done.

+      bool cleaner_logs_enabled,
+      ChromePromptImpl::OnPromptUser on_prompt_user,
+      base::OnceClosure on_connection_closed,
+      base::OnceClosure on_connection_error,
+      ChromeCleanerRunner::ProcessDoneCallback on_process_done);
+
+ private:
+  friend class base::RefCountedThreadSafe<ChromeCleanerRunner>;
+  ~ChromeCleanerRunner();
+
+  ChromeCleanerRunner(const base::FilePath& executable_path,
+                      const SwReporterInvocation& reporter_invocation,
+                      bool metrics_enabled,
+                      bool cleaner_logs_enabled,
+                      ChromePromptImpl::OnPromptUser on_prompt_user,
+                      base::OnceClosure on_connection_closed,
+                      base::OnceClosure on_connection_error,
+                      ProcessDoneCallback on_process_done);
+
+  int LaunchAndWaitForExitOnBackgroundThread();
+
+  void CreateChromePromptImpl(
+      chrome_cleaner::mojom::ChromePromptRequest chrome_prompt_request);
+  void ReleaseChromePromptImpl();
+
+  void OnPromptUser(std::unique_ptr<std::set<base::FilePath>> files_to_delete,
+                    chrome_cleaner::mojom::ChromePrompt::PromptUserCallback
+                        prompt_user_callback);
+  void OnConnectionClosed();
+  void OnConnectionError(const std::string& error);
+  void OnProcessDone(int exit_code);
+
+  base::CommandLine command_line_;
+  ChromePromptImpl::OnPromptUser on_prompt_user_;
+  base::OnceClosure on_connection_closed_;
+  base::OnceClosure on_connection_error_;
+  ProcessDoneCallback on_process_done_;
+
+  std::unique_ptr<ChromePromptImpl> chrome_prompt_impl_;

[Joe Mason, 2017/05/19 22:02:25]

Can you add a base::SequenceChecker and check CALLED_ON_VALID_SEQUENCE instead
of explicitly checking for BrowserThread::UI?

[alito, 2017/05/22 23:26:31]

I decided to use a SingleThreadTaskRunner instead for dispatching callbacks and
am no longer checking for sequenced tasks.

+};
+
+}  // namespace safe_browsing
+
+#endif  // CHROME_BROWSER_SAFE_BROWSING_CHROME_CLEANER_CHROME_CLEANER_RUNNER_WIN_H_