Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2890023005:
Chrome Cleaner UI: reporter no longer uses mojo. (Closed)
|
Created:
3 years, 7 months ago by alito Modified:
3 years, 7 months ago CC:
chromium-reviews, vakh+watch_chromium.org, joenotcharles+watch_chromium.org, grt+watch_chromium.org, timvolodine, csharp+watch_chromium.org, alito+watch_chromium.org, ftirelo+watch_chromium.org, grt (UTC plus 2) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionChrome Cleaner UI: Reporter no longer uses mojo.
The reporter will no longer use mojo to communicate with Chrome.
That will instead be the responsibility of the Chrome Cleaner process.
BUG=690020
Review-Url: https://codereview.chromium.org/2890023005
Cr-Commit-Position: refs/heads/master@{#474778}
Committed: https://chromium.googlesource.com/chromium/src/+/bb6caf2e8acec41914ace77fdd974ca7a8b1c261
Patch Set 1 : Rip out the mojo code from the reporter runner. #
Total comments: 2
Patch Set 2 : Add ChromeCleanerRunner class #
Total comments: 20
Patch Set 3 : Addressed Fabio's comments and added documentation. #Patch Set 4 : Fix an include. #Patch Set 5 : Move main run function to inside the ChromeCleanerRunner class. #
Total comments: 32
Patch Set 6 : Addressed comments and fixed a race condition #Patch Set 7 : Add tests for ChromeCleanerRunner. #
Total comments: 6
Patch Set 8 : Rebase #
Total comments: 19
Patch Set 9 : Addressed Joe's comments #Patch Set 10 : Fixes clang compile error #
Total comments: 16
Patch Set 11 : Addressed Fabio's comments. #
Total comments: 24
Patch Set 12 : Addressed Chris's comments. #
Total comments: 4
Patch Set 13 : Nits #Dependent Patchsets: Messages
Total messages: 53 (27 generated)
Description was changed from ========== Chrome Cleaner UI: reporter no longer uses mojo. BUG= ========== to ========== Chrome Cleaner UI: Reporter no longer uses mojo. The reporter will no longer use mojo to communicate with Chrome. That will instead be the responsibility of the Chrome Cleaner process. BUG=690020 ==========
Description was changed from ========== Chrome Cleaner UI: Reporter no longer uses mojo. The reporter will no longer use mojo to communicate with Chrome. That will instead be the responsibility of the Chrome Cleaner process. BUG=690020 ========== to ========== Chrome Cleaner UI: Reporter no longer uses mojo. The reporter will no longer use mojo to communicate with Chrome. That will instead be the responsibility of the Chrome Cleaner process. BUG=690020 ==========
alito@chromium.org changed reviewers: + ftirelo@chromium.org, joenotcharles@chromium.org
PTAL! I plan on sending related patches to incrementally build this CL. The first patch more or less reverts this one: https://codereview.chromium.org/2780873002/ The next patch will add a ChromeCleanerRunner that will incorporate the mojo code in the SwReporterProcess class. But for now, please help me verify that the code goes back to running the reporter without an IPC even when the InBrowserCleanerUI feature is enabled.
Only a small nit. So far, it looks fine. https://codereview.chromium.org/2890023005/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc (right): https://codereview.chromium.org/2890023005/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc:40: #include "components/safe_browsing_db/safe_browsing_prefs.h" Shouldn't this be common/safe_browsing_prefs.h ?
https://codereview.chromium.org/2890023005/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc (right): https://codereview.chromium.org/2890023005/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc:40: #include "components/safe_browsing_db/safe_browsing_prefs.h" On 2017/05/18 13:43:07, ftirelo wrote: > Shouldn't this be common/safe_browsing_prefs.h ? That file was moved in this cl: https://codereview.chromium.org/2869383002
I've uploaded a second patch adding the ChromeCleanerRunner class. Still missing proper documentation for public functions, but PTAL and I will add those later.
A few comments. Overall it's good, feel free to continue. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:80: // Add switches that pass information about this Chrome installation. No kChromePromptSwitch? https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:103: chrome_cleaner::kExtendedSafeBrowsingEnabledSwitch); As discussed, this can become kCleanerLoggingEnabled. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:117: static_cast<int>(chrome_cleaner::ExecutionMode::kScanning))); Please add this in the method above with the other non-mojo flags. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:143: // base::Bind(&OnConnectionError); Please delete. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:165: DCHECK_CURRENTLY_ON(BrowserThread::IO); Worth DCHECK'ing that the feature is enabled? https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h (right): https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:22: void RunChromeCleanerAndReplyWithExitCode( Since we need to forward some switches from the reporter to the cleaner (such as the engine), would it make sense to pass the reporter invocation as a parameter here? https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:25: bool sber_enabled, As discussed, this is only used today for logging if the user is in SBER. Since we don't need to act upon that data anyway, this should be replaced with sber2_enabled; when this is true we should add a new flag --enable-cleaner-logging to the command line too. Note: please add the flag to //components/chrome_cleaner/constants/constants.{cc|h}. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:31: class ChromeCleanerRunner Please document lifetime for this object. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h:23: using OnPromptUserCallback = base::Callback<void( Shouldn't this be a OnceCallback as well, since it can only invoke the IPC callback once? https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h:25: chrome_cleaner::mojom::ChromePrompt::PromptUserCallback callback)>; I'm afraid OnPromptUserCallback is too similar to PromptUserCallback. What about renaming it to something like PresentCleanupPrompt? Or maybe ActuallyPromptTheUser? :-)
Addressed Fabio's comments. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:80: // Add switches that pass information about this Chrome installation. On 2017/05/18 18:27:50, ftirelo wrote: > No kChromePromptSwitch? Well, the user hasn't been prompted at this point. We are just running to scan. So I'm thinking we shouldn't add that flag when the cleaner is running in scan mode. When the process relaunches itself in cleanup mode, then we know that the user has accepted the prompt and can pass that there. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:103: chrome_cleaner::kExtendedSafeBrowsingEnabledSwitch); On 2017/05/18 18:27:50, ftirelo wrote: > As discussed, this can become kCleanerLoggingEnabled. Done. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:117: static_cast<int>(chrome_cleaner::ExecutionMode::kScanning))); On 2017/05/18 18:27:50, ftirelo wrote: > Please add this in the method above with the other non-mojo flags. Done. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:143: // base::Bind(&OnConnectionError); On 2017/05/18 18:27:50, ftirelo wrote: > Please delete. Done. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:165: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/05/18 18:27:50, ftirelo wrote: > Worth DCHECK'ing that the feature is enabled? Since this class should only be used if the feature is enabled, I added a DCHECK in the constructor. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h (right): https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:22: void RunChromeCleanerAndReplyWithExitCode( On 2017/05/18 18:27:51, ftirelo wrote: > Since we need to forward some switches from the reporter to the cleaner (such as > the engine), would it make sense to pass the reporter invocation as a parameter > here? Done. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:25: bool sber_enabled, On 2017/05/18 18:27:51, ftirelo wrote: > As discussed, this is only used today for logging if the user is in SBER. Since > we don't need to act upon that data anyway, this should be replaced with > sber2_enabled; when this is true we should add a new flag > --enable-cleaner-logging to the command line too. > > Note: please add the flag to > //components/chrome_cleaner/constants/constants.{cc|h}. Done. Also renamed the variable to cleaner_logs_enabled. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:31: class ChromeCleanerRunner On 2017/05/18 18:27:51, ftirelo wrote: > Please document lifetime for this object. Done. Added a bunch of documentation. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h:23: using OnPromptUserCallback = base::Callback<void( On 2017/05/18 18:27:51, ftirelo wrote: > Shouldn't this be a OnceCallback as well, since it can only invoke the IPC > callback once? Done. https://codereview.chromium.org/2890023005/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h:25: chrome_cleaner::mojom::ChromePrompt::PromptUserCallback callback)>; On 2017/05/18 18:27:51, ftirelo wrote: > I'm afraid OnPromptUserCallback is too similar to PromptUserCallback. What about > renaming it to something like PresentCleanupPrompt? > > Or maybe ActuallyPromptTheUser? :-) As discussed, using OnPromptUser for the type name.
alito@chromium.org changed reviewers: + csharp@chromium.org
Adding csharp@ as reviewer.
https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:52: auto cleaner_runner = make_scoped_refptr(new ChromeCleanerRunner( I wonder if it'd be cleaner to move the creation of the cleaner_runner outside of this function (instead of passing in that list of variables, you'd just need on_process_done and a pointer to the ChromeCleanerRunner) https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:154: return kNotLaunchedExitCode; I wonder if it might be cleaner to have bool in addition to the exit code int to make it clearer (instead of overloading the exit code value) https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h:26: prompt_user_response_callback)>; No need for the variable name https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h:28: // When enabled, moves all user interaction with the Software Reporter and the nit: Drop "the Software Reporter", since this flag won't really change how it interacts at all (right?)
Reviewed everything but the test. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:49: ChromeCleanerRunner::ProcessDoneCallback on_process_done_) { on_process_done_ shouldn't have a trailing _ https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:50: DCHECK_CURRENTLY_ON(BrowserThread::UI); I don't think this is necessary. We don't really care what sequence this is on, as long as all the callbacks get called on the same sequence. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:87: DCHECK_CURRENTLY_ON(BrowserThread::UI); base::SequenceChecker can replace this. You'd also want to save "task_runner_ = SequencedTaskRunnerHandle::Get()" and replace all the "GetTaskRunnerForThread(BrowserThread::UI)->PostTask" with task_runner_->PostTask https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:184: DCHECK_CURRENTLY_ON(BrowserThread::UI); Can you also DCHECK that chrome_prompt_impl_ is null here? Otherwise it'll be released on the wrong thread, if ReleaseChromePromptImpl wasn't called. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:29: // - Created on the UI thread. 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. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:36: // - deleted on the UI thread when the Chrome Cleaner process exits, at which Which would make this "deleted on the CleanerRunner sequence..." https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:47: // to the caller by calling the given callbacks on the UI thread. 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.) https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:51: // |matrics_enabled| and |cleaner_logs_enabled| parameters. The Cleaner Typo: metrics_enabled. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:63: bool metrics_enabled, 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? https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:102: std::unique_ptr<ChromePromptImpl> chrome_prompt_impl_; Can you add a base::SequenceChecker and check CALLED_ON_VALID_SEQUENCE instead of explicitly checking for BrowserThread::UI? https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.h (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.h:16: #include "base/feature_list.h" I don't think this is needed anymore. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc:43: .Run(std::move(files_to_delete), std::move(callback)); Need to check if on_prompt_user_ is still valid here, in case the cleaner sends more than one PromptUser message.
No comments on the browser test changes.
Patchset #6 (id:100001) has been deleted
PTAnL at the implementation of ChromeCleanerRunner class. The following changes were made: - According to Ken, Mojo's process error callback can be dispatched at any time and on any thread (it is not sequenced on the IO thread). This meant that there was a race condition in the previous code, which should now be fixed. - Being a bit paranoid, I'm manually ensuring that the runner class cannot be destroyed before the ChromePromptImpl object has been destroyed on the UI thread. - The runner class no longer needs to be destroyed on a specific thread or sequence. Once the Cleaner process has exited and the ChromePromptImpl object has been released on the IO thread, the object will be destroyed when the last reference to it goes out of scope. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:49: ChromeCleanerRunner::ProcessDoneCallback on_process_done_) { On 2017/05/19 22:02:25, Joe Mason wrote: > on_process_done_ shouldn't have a trailing _ Done. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:50: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2017/05/19 22:02:25, Joe Mason wrote: > I don't think this is necessary. We don't really care what sequence this is on, > as long as all the callbacks get called on the same sequence. Done. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:52: auto cleaner_runner = make_scoped_refptr(new ChromeCleanerRunner( On 2017/05/19 20:44:17, csharp wrote: > I wonder if it'd be cleaner to move the creation of the cleaner_runner outside > of this function (instead of passing in that list of variables, you'd just need > on_process_done and a pointer to the ChromeCleanerRunner) One benefit of having the static function create the instance is that users of ChromeCleanerRunner won't have to manage the lifetime of ChromeCleanerRunner instances. At least, this way, passing these arguments to the constructor are an implementation detail that are hidden away from clients. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:87: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2017/05/19 22:02:25, Joe Mason wrote: > base::SequenceChecker can replace this. > > You'd also want to save "task_runner_ = SequencedTaskRunnerHandle::Get()" and > replace all the "GetTaskRunnerForThread(BrowserThread::UI)->PostTask" with > task_runner_->PostTask Now using ThreadTaskRunnerHandle::Get() to get a SingleThreadTaskRunner. See my response in the .h file for justification. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:154: return kNotLaunchedExitCode; On 2017/05/19 20:44:17, csharp wrote: > I wonder if it might be cleaner to have bool in addition to the exit code int > to make it clearer (instead of overloading the exit code value) Done. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:184: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2017/05/19 22:02:25, Joe Mason wrote: > Can you also DCHECK that chrome_prompt_impl_ is null here? Otherwise it'll be > released on the wrong thread, if ReleaseChromePromptImpl wasn't called. Done. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:29: // - Created on the UI thread. On 2017/05/19 22:02:25, Joe Mason wrote: > 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. 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. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:36: // - deleted on the UI thread when the Chrome Cleaner process exits, at which On 2017/05/19 22:02:25, Joe Mason wrote: > Which would make this "deleted on the CleanerRunner sequence..." Reformulated. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:47: // to the caller by calling the given callbacks on the UI thread. On 2017/05/19 22:02:25, Joe Mason wrote: > 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.) Reformulated the comments. PTAnL. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:51: // |matrics_enabled| and |cleaner_logs_enabled| parameters. The Cleaner On 2017/05/19 22:02:25, Joe Mason wrote: > Typo: metrics_enabled. Done. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:63: bool metrics_enabled, On 2017/05/19 22:02:25, Joe Mason wrote: > 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? Done. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:102: std::unique_ptr<ChromePromptImpl> chrome_prompt_impl_; On 2017/05/19 22:02:25, Joe Mason wrote: > Can you add a base::SequenceChecker and check CALLED_ON_VALID_SEQUENCE instead > of explicitly checking for BrowserThread::UI? I decided to use a SingleThreadTaskRunner instead for dispatching callbacks and am no longer checking for sequenced tasks. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.h (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.h:16: #include "base/feature_list.h" On 2017/05/19 22:02:26, Joe Mason wrote: > I don't think this is needed anymore. Done. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc:43: .Run(std::move(files_to_delete), std::move(callback)); On 2017/05/19 22:02:26, Joe Mason wrote: > Need to check if on_prompt_user_ is still valid here, in case the cleaner sends > more than one PromptUser message. Good catch! Done. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h:26: prompt_user_response_callback)>; On 2017/05/19 20:44:17, csharp wrote: > No need for the variable name Done. https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h (right): https://codereview.chromium.org/2890023005/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h:28: // When enabled, moves all user interaction with the Software Reporter and the On 2017/05/19 20:44:17, csharp wrote: > nit: Drop "the Software Reporter", since this flag won't really change how it > interacts at all (right?) Done.
CC:ing Greg only in case he has some time to take an extra look.
Alright, I've added tests now. This should now complete the CL modulo any requests for comments. PTAL. /ali
The CQ bit was checked by alito@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by alito@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2890023005/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:59: std::move(on_process_done), base::ThreadTaskRunnerHandle::Get())); I think it makes sense to either use BrowserThread::UI explicitly since the caller of this requires that functions be called on the UI thread, or sequenced_task_runner since this code itself doesn't depend on running on a single thread (and so if the caller needs functions to be called on a specific thread, it's the caller's responsibility to pass that thread's task_runner). https://codereview.chromium.org/2890023005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:177: // Send() function. I would say no. There's no point in having this until we design what's done when a message fails validation. This is a natural thing to file a separate bug for. https://codereview.chromium.org/2890023005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:206: AddRef(); Nice solution. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc (right): https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:162: TEST_P(ChromeCleanerRunnerSimpleTest, InterceptLaunchParams) { Nit: I would just name this test "LaunchParams". Normally tests are named after what they're testing, not the action they take. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:174: ReporterEngineToString(reporter_engine_)); It took me a while to realize this wasn't a useless check, because CallRunChromeCleaner (which is not the code being tested) sets kEngineSwitch on "command_line". This could use a comment saying exactly what's being tested. Nit: Also I think it would be clearer in this case to put the conversion from kEngineSwitch to string in this function instead of having to jump up to a helper function to see exactly what's being tested. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:332: if (::testing::Test::HasFailure()) When can this happen? https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:366: Values(false), It would be easier to read if this were an enum instead of bool. (I suggest FoundNothing, FoundUwSWithoutReboot, FoundUwSWithReboot instead of separate enums.) https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc (right): https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:69: CrashPoint crash_point) Nit: This constructor's never actually used. Might as well get rid of it. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:150: if (::testing::Test::HasFailure()) Can you comment on what this pattern means? https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:214: FROM_HERE, base::Bind([]() { exit(kDeliberateCrashExitCode); })); Nit: could avoid the lambda with "base::Bind(&exit, kDeliverateCrashExitCode)" https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.h (right): https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.h:72: Options& operator=(const Options&) = delete; Coding style says to define both copy constructor and assignment operator, or neither: https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.h:92: // Function that recieves the Mojo response to the PromptUser message. Nit: typo "receives"
Addressed Joe's comments. https://codereview.chromium.org/2890023005/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:59: std::move(on_process_done), base::ThreadTaskRunnerHandle::Get())); On 2017/05/23 15:39:35, Joe Mason wrote: > I think it makes sense to either use BrowserThread::UI explicitly since the > caller of this requires that functions be called on the UI thread, or > sequenced_task_runner since this code itself doesn't depend on running on a > single thread (and so if the caller needs functions to be called on a specific > thread, it's the caller's responsibility to pass that thread's task_runner). As discussed, the static function will now let the caller provide it with a sequenced task runner where all callbacks are posted to. https://codereview.chromium.org/2890023005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:177: // Send() function. On 2017/05/23 15:39:35, Joe Mason wrote: > I would say no. There's no point in having this until we design what's done when > a message fails validation. This is a natural thing to file a separate bug for. Removed this comment and opened crbug.com/725570. Added as much detail as is available to me right now in the bug description. https://codereview.chromium.org/2890023005/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:206: AddRef(); On 2017/05/23 15:39:35, Joe Mason wrote: > Nice solution. Thx :). https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc (right): https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:162: TEST_P(ChromeCleanerRunnerSimpleTest, InterceptLaunchParams) { On 2017/05/23 15:39:36, Joe Mason wrote: > Nit: I would just name this test "LaunchParams". Normally tests are named after > what they're testing, not the action they take. Done. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:174: ReporterEngineToString(reporter_engine_)); On 2017/05/23 15:39:36, Joe Mason wrote: > It took me a while to realize this wasn't a useless check, because > CallRunChromeCleaner (which is not the code being tested) sets kEngineSwitch on > "command_line". This could use a comment saying exactly what's being tested. > > Nit: Also I think it would be clearer in this case to put the conversion from > kEngineSwitch to string in this function instead of having to jump up to a > helper function to see exactly what's being tested. Done. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:332: if (::testing::Test::HasFailure()) On 2017/05/23 15:39:36, Joe Mason wrote: > When can this happen? Added a comment explaining this here as well. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:366: Values(false), On 2017/05/23 15:39:36, Joe Mason wrote: > It would be easier to read if this were an enum instead of bool. (I suggest > FoundNothing, FoundUwSWithoutReboot, FoundUwSWithReboot instead of separate > enums.) Done. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc (right): https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:69: CrashPoint crash_point) On 2017/05/23 15:39:36, Joe Mason wrote: > Nit: This constructor's never actually used. Might as well get rid of it. Done. Now only a default constructor without arguments. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:150: if (::testing::Test::HasFailure()) On 2017/05/23 15:39:36, Joe Mason wrote: > Can you comment on what this pattern means? Done. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:214: FROM_HERE, base::Bind([]() { exit(kDeliberateCrashExitCode); })); On 2017/05/23 15:39:36, Joe Mason wrote: > Nit: could avoid the lambda with "base::Bind(&exit, kDeliverateCrashExitCode)" Done. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.h (right): https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.h:72: Options& operator=(const Options&) = delete; On 2017/05/23 15:39:36, Joe Mason wrote: > Coding style says to define both copy constructor and assignment operator, or > neither: > https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types Done. https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.h:92: // Function that recieves the Mojo response to the PromptUser message. On 2017/05/23 15:39:36, Joe Mason wrote: > Nit: typo "receives" Done.
The CQ bit was checked by alito@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by alito@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:200001) has been deleted
The CQ bit was checked by alito@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed clang error (not caught by compiler on win :(). https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc (right): https://codereview.chromium.org/2890023005/diff/160001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:214: FROM_HERE, base::Bind([]() { exit(kDeliberateCrashExitCode); })); On 2017/05/23 18:58:57, alito wrote: > On 2017/05/23 15:39:36, Joe Mason wrote: > > Nit: could avoid the lambda with "base::Bind(&exit, kDeliverateCrashExitCode)" > > Done. Turns out, the clang compiler does not like binding exit() directly. In stdlib.h, exit() is annotated with "__attribute__ ((__noreturn__))", which then does not allow for correct return type deduction in Bind(). So keeping the lambda...
This looks amazing. https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:59: std::move(on_process_done), task_runner)); Maybe std::move(task_runner) as well? https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:62: cleaner_runner); Should we use base::RetainedRef(cleaner_runner) here? https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:64: base::BindOnce(&ChromeCleanerRunner::OnProcessDone, cleaner_runner); Should we use base::RetainedRef(cleaner_runner) here? https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:84: : task_runner_(task_runner), Ditto. https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:125: : base::IntToString(1)); I think it'd be easier to read this if you saved the switch value to an auxiliary variable and check if it's empty. Something like this: const std::string& engine = reporter_invocation.command_line.GetSwitchValueASCII( chrome_cleaner::kEngineSwitch); command_line_.AppendSwitchASCII(chrome_cleaner::kEngineSwitch, engine.empty() ? "1" : engine); https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h (right): https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:106: ChromeCleanerRunner(const base::FilePath& executable_path, What about renaming this to cleaner_executable_path? Same for command_line_ and the param in the factory method above. https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc (right): https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:162: // process), we explicitly use ::testing::Test::HasFailure() return an error I think this sentence is missing something. https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h:24: std::unique_ptr<std::set<base::FilePath>>, Would it make sense to use std::set<base::FilePath>&& instead to force move semantics? Not sure if base::BindOnce would accept that though.
Addressed Fabio's comments. https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:59: std::move(on_process_done), task_runner)); On 2017/05/24 01:45:20, ftirelo wrote: > Maybe std::move(task_runner) as well? Done. https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:62: cleaner_runner); On 2017/05/24 01:45:20, ftirelo wrote: > Should we use base::RetainedRef(cleaner_runner) here? Actually, I think the way it is written here is the default intended use of Bind. See the docs here: https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Bind.... https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:64: base::BindOnce(&ChromeCleanerRunner::OnProcessDone, cleaner_runner); On 2017/05/24 01:45:20, ftirelo wrote: > Should we use base::RetainedRef(cleaner_runner) here? See previous comment. https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:84: : task_runner_(task_runner), On 2017/05/24 01:45:20, ftirelo wrote: > Ditto. Done. https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:125: : base::IntToString(1)); On 2017/05/24 01:45:20, ftirelo wrote: > I think it'd be easier to read this if you saved the switch value to an > auxiliary variable and check if it's empty. Something like this: > > const std::string& engine = > reporter_invocation.command_line.GetSwitchValueASCII( > chrome_cleaner::kEngineSwitch); > command_line_.AppendSwitchASCII(chrome_cleaner::kEngineSwitch, engine.empty() ? > "1" : engine); Done. https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h (right): https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:106: ChromeCleanerRunner(const base::FilePath& executable_path, On 2017/05/24 01:45:20, ftirelo wrote: > What about renaming this to cleaner_executable_path? Same for command_line_ and > the param in the factory method above. Done. https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc (right): https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:162: // process), we explicitly use ::testing::Test::HasFailure() return an error On 2017/05/24 01:45:20, ftirelo wrote: > I think this sentence is missing something. Yup. Reformulated. https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2890023005/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h:24: std::unique_ptr<std::set<base::FilePath>>, On 2017/05/24 01:45:20, ftirelo wrote: > Would it make sense to use std::set<base::FilePath>&& instead to force move > semantics? Not sure if base::BindOnce would accept that though. My reading of the style guide is that this isn't allowed in our codebase: https://google.github.io/styleguide/cppguide.html#Rvalue_references
https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:107: PathService::Get(base::FILE_EXE, &chrome_exe_path); This can fail and then chrome_exe_path would keep the default value, what happens if we append a switch with a empty value for this flag? https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:146: constexpr int kBadProcessExitCode = std::numeric_limits<int>::max(); nit: move to line 167 since it isn't needed until then? https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:206: // Ensure that this object is not destroyed before chrome_prompt_impl_ has As discussed offline, it is probably cleaner and clear to not destroy chrome_prompt_impl_ when OnConnectionClosed is called, but instead to just post it's destruction to the IO thread whenever ~ChromeCleanerRunner is called. https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:150: virtual base::CommandLine GetTestBaseCommandLine() = 0; I think you can remove this and just do this work in LaunchTestProcess https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:51: // Chrome sends to the Chrome Cleaner process. Worth mentioning it returns an invalid process and what that means it can't do? https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:161: static_cast<int>(chrome_cleaner::ExecutionMode::kScanning))); nit: I wonder if it is worth adding a helper function to convert these enums to ints https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:291: void ContinueTestIfCommunicationDone() { I don't quite understand what this name means. This function just returns, would the name be clearer as WaitUntilCommunicationDone? https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:347: TEST_P(ChromeCleanerRunnerTest, MockChromeCleanerProcess) { Maybe change MockChromeCleanerProcess so it isn't the exact same as the class name? :) https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:133: files_to_delete_.insert(filepath); Why not insert all the files for each uws at once? files_to_delete.insert(uws_ptr->files_to_delete.begin(), uws_ptr->files_to_delete.end()) https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:151: return kCanceledExitCode; nit: I think this might be clearer as a new exit code, kDeclined (or something like that), since it is different from the current cancelled behaviour https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc:39: files_to_delete->insert(filepath); Insert the whole list at once?
Addressed Chris's comments. PTAL. https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:107: PathService::Get(base::FILE_EXE, &chrome_exe_path); On 2017/05/24 20:49:35, csharp wrote: > This can fail and then chrome_exe_path would keep the default value, what > happens if we append a switch with a empty value for this flag? In the Cleaner process, that is handled the same way as when the path cannot be found: the Cleaner will try to look for Chrome's executable in other ways. https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:146: constexpr int kBadProcessExitCode = std::numeric_limits<int>::max(); On 2017/05/24 20:49:35, csharp wrote: > nit: move to line 167 since it isn't needed until then? Done. https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:206: // Ensure that this object is not destroyed before chrome_prompt_impl_ has On 2017/05/24 20:49:35, csharp wrote: > As discussed offline, it is probably cleaner and clear to not destroy > chrome_prompt_impl_ when OnConnectionClosed is called, but instead to just post > it's destruction to the IO thread whenever ~ChromeCleanerRunner is called. Done. https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h:150: virtual base::CommandLine GetTestBaseCommandLine() = 0; On 2017/05/24 20:49:35, csharp wrote: > I think you can remove this and just do this work in LaunchTestProcess Done. https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:51: // Chrome sends to the Chrome Cleaner process. On 2017/05/24 20:49:35, csharp wrote: > Worth mentioning it returns an invalid process and what that means it can't do? Done. https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:161: static_cast<int>(chrome_cleaner::ExecutionMode::kScanning))); On 2017/05/24 20:49:35, csharp wrote: > nit: I wonder if it is worth adding a helper function to convert these enums to > ints There is just this instance in this file... so probably not? https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:291: void ContinueTestIfCommunicationDone() { On 2017/05/24 20:49:35, csharp wrote: > I don't quite understand what this name means. This function just returns, would > the name be clearer as WaitUntilCommunicationDone? It is not really waiting... so I tried, QuitTestRunLoopIfCommunicationDone. better? https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:347: TEST_P(ChromeCleanerRunnerTest, MockChromeCleanerProcess) { On 2017/05/24 20:49:35, csharp wrote: > Maybe change MockChromeCleanerProcess so it isn't the exact same as the class > name? :) Done. https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:133: files_to_delete_.insert(filepath); On 2017/05/24 20:49:35, csharp wrote: > Why not insert all the files for each uws at once? > files_to_delete.insert(uws_ptr->files_to_delete.begin(), > uws_ptr->files_to_delete.end()) Indeed. It's just that I love for loops so much. https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:151: return kCanceledExitCode; On 2017/05/24 20:49:35, csharp wrote: > nit: I think this might be clearer as a new exit code, kDeclined (or something > like that), since it is different from the current cancelled behaviour Done. https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc:39: files_to_delete->insert(filepath); On 2017/05/24 20:49:36, csharp wrote: > Insert the whole list at once? Done.
lgtm https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:161: static_cast<int>(chrome_cleaner::ExecutionMode::kScanning))); On 2017/05/24 22:20:50, alito wrote: > On 2017/05/24 20:49:35, csharp wrote: > > nit: I wonder if it is worth adding a helper function to convert these enums > to > > ints > > There is just this instance in this file... so probably not? For this file yes, but I think there are 3 or 4 cases total in this cl, so it would be a helper with the same scope as ExecutionMode (same basic idea as channel as int)
ftirelo@google.com changed reviewers: + ftirelo@google.com
lgtm
lgtm with nits https://codereview.chromium.org/2890023005/diff/260001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:49: // scope. Nit: I like to use an explicit .release() call instead of a comment here. https://codereview.chromium.org/2890023005/diff/260001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc (right): https://codereview.chromium.org/2890023005/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:42: std::vector<UwSPtr> clone_uwsptr_vector(const std::vector<UwSPtr>& uws_ptrs) { Nit: this should be in CamelCase, no?
Thanks y'all for the reviews. All done now. Will try to land. https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc (right): https://codereview.chromium.org/2890023005/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc:161: static_cast<int>(chrome_cleaner::ExecutionMode::kScanning))); On 2017/05/25 14:44:46, csharp wrote: > On 2017/05/24 22:20:50, alito wrote: > > On 2017/05/24 20:49:35, csharp wrote: > > > nit: I wonder if it is worth adding a helper function to convert these enums > > to > > > ints > > > > There is just this instance in this file... so probably not? > > For this file yes, but I think there are 3 or 4 cases total in this cl, so it > would be a helper with the same scope as ExecutionMode (same basic idea as > channel as int) As discussed, this particular enum is cast only once in this directory, so no need for a helper function. https://codereview.chromium.org/2890023005/diff/260001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc (right): https://codereview.chromium.org/2890023005/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc:49: // scope. On 2017/05/25 16:40:33, Joe Mason wrote: > Nit: I like to use an explicit .release() call instead of a comment here. Done. https://codereview.chromium.org/2890023005/diff/260001/chrome/browser/safe_br... File chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc (right): https://codereview.chromium.org/2890023005/diff/260001/chrome/browser/safe_br... chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:42: std::vector<UwSPtr> clone_uwsptr_vector(const std::vector<UwSPtr>& uws_ptrs) { On 2017/05/25 16:40:33, Joe Mason wrote: > Nit: this should be in CamelCase, no? yes, indeed. Done.
The CQ bit was checked by alito@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ftirelo@google.com, csharp@chromium.org, joenotcharles@chromium.org Link to the patchset: https://codereview.chromium.org/2890023005/#ps280001 (title: "Nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1495737601274150,
"parent_rev": "66e4a1e3d234f8151ef22fcada3e28a82e45fa5d", "commit_rev":
"bb6caf2e8acec41914ace77fdd974ca7a8b1c261"}
Message was sent while issue was closed.
Description was changed from ========== Chrome Cleaner UI: Reporter no longer uses mojo. The reporter will no longer use mojo to communicate with Chrome. That will instead be the responsibility of the Chrome Cleaner process. BUG=690020 ========== to ========== Chrome Cleaner UI: Reporter no longer uses mojo. The reporter will no longer use mojo to communicate with Chrome. That will instead be the responsibility of the Chrome Cleaner process. BUG=690020 Review-Url: https://codereview.chromium.org/2890023005 Cr-Commit-Position: refs/heads/master@{#474778} Committed: https://chromium.googlesource.com/chromium/src/+/bb6caf2e8acec41914ace77fdd97... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:280001) as https://chromium.googlesource.com/chromium/src/+/bb6caf2e8acec41914ace77fdd97... |
