Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834613003:
Adds error handling support for the SwReporter launcher. (Closed)
|
Created:
3 years, 8 months ago by ftirelo 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 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds error handling support for the SwReporter launcher.
BUG=690020
Review-Url: https://codereview.chromium.org/2834613003
Cr-Commit-Position: refs/heads/master@{#467471}
Committed: https://chromium.googlesource.com/chromium/src/+/e012fe8a8e4d60ed8eba17fbac5df6d9fc4bcad3
Patch Set 1 #
Total comments: 26
Patch Set 2 : Code review #Patch Set 3 : Code review #
Total comments: 9
Patch Set 4 : Move error handlers out of impl class #Patch Set 5 : Code review #
Total comments: 36
Patch Set 6 : More reviews #
Total comments: 2
Patch Set 7 : Final comments #Patch Set 8 : Fix win_clang failure #Patch Set 9 : Really? #
Messages
Total messages: 33 (12 generated)
ftirelo@chromium.org changed reviewers: + grt@chromium.org, joenotcharles@chromium.org
https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.cc (right): https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_chrome_prompt_impl.cc:20: &ChromePromptImpl::OnConnectionClosed, base::Unretained(this))); please document why Unretained is safe https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:16: // Implements specific mocking actions for tests. this is an interface for such, not an implementation. :-) https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:44: static TestDelegate* test_delegate_; is it possible to inject the delegate without using a global? perhaps by making the methods in ChromePromptImpl you care about virtual, and allowing the test to provide a factory function that will create its test-specific subclass of it? using SwReporterTestingDelegate somehow? https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:45: }; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:281: std::tuple<bool, ElevationStatus, MockedReporterFailure>> { please add a note about this param to the doc comment above https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:298: chrome_prompt_test_delegate_.reset(); isn't this a noop? https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:370: chrome_prompt_test_delegate_ = base::MakeUnique<BadMessageTestDelegate>(); nit: braces around this https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:371: ChromePromptImpl::SetTestDelegate(chrome_prompt_test_delegate_.get()); clear the delegate in TearDownInProcessBrowserTestFixture(). alternatively, put both calls to SetTestDelegate in BadMessageTestDelegate's ctor and dtor so that it is self-registering. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:594: // reporter. Must be run on the IO thread. "...reporter and establishes the mojo connection to it." or somesuch https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:683: base::OnceClosure f = base::BindOnce( why introduce a local for this? if you really think it's cleaner, i think "auto" is appropriate, and "f" must be replaced with something more clear as per "Names should be descriptive; avoid abbreviation." (https://google.github.io/styleguide/cppguide.html#General_Naming_Rules) https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:685: reporter_process.Handle(), please document why it's safe to pass the naked handle value here. its validity is bound by the lifetime of |reporter_process|. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:712: base::MakeUnique<ChromePromptImpl>(std::move(chrome_prompt_request)); perhaps this could call a function on the testing delegate to create the CPI so that a test can override methods on it. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:717: base::Unretained(chrome_prompt_impl_.get()))); please document why this Unretained is safe. is it impossible for a connection error to be posted and waiting to run while a previously-posted task deletes this SwReporterProcess?
lgtm with nits https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:100: // Potential failures on the reporter process that should be handled by the Nit: "in the reporter process". Or "of the reporter process". https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:303: // cleanup; The most trivial nit ever: Since these lines are already delimited with "-" at the start, I suggest ending them all with . instead of ; so that you don't need to update line endings again if another bullet point is added. https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:683: // It's safe to pass the process handle here Nit: End this line with a period, and either put it on the same line as the previous sentence or add a blank line to separate paragraphs. https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:722: base::Unretained(chrome_prompt_impl_.get()))); Because I know grt will ask: please add a comment explaining why Unretained is safe here.
Since most error handling is related to what to do if the dialog is shown, I'd rather move the error handling functions out of the class, so we don't need to deal with retaining pointers. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.cc (right): https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_chrome_prompt_impl.cc:20: &ChromePromptImpl::OnConnectionClosed, base::Unretained(this))); On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > please document why Unretained is safe Obsolete. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:16: // Implements specific mocking actions for tests. On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > this is an interface for such, not an implementation. :-) Done. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:44: static TestDelegate* test_delegate_; On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > is it possible to inject the delegate without using a global? perhaps by making > the methods in ChromePromptImpl you care about virtual, and allowing the test to > provide a factory function that will create its test-specific subclass of it? > using SwReporterTestingDelegate somehow? Good idea. Changed as suggested in srt_fetcher_win.cc to have the SwReporterTestingDelegate set it. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:45: }; On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:281: std::tuple<bool, ElevationStatus, MockedReporterFailure>> { On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > please add a note about this param to the doc comment above Done. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:298: chrome_prompt_test_delegate_.reset(); On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > isn't this a noop? This was misplaced. Moved to LaunchReporter() since I want it to reset the delegate object in case of tests involving multiple reporter runs. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:370: chrome_prompt_test_delegate_ = base::MakeUnique<BadMessageTestDelegate>(); On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > nit: braces around this Done. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:371: ChromePromptImpl::SetTestDelegate(chrome_prompt_test_delegate_.get()); On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > clear the delegate in TearDownInProcessBrowserTestFixture(). alternatively, put > both calls to SetTestDelegate in BadMessageTestDelegate's ctor and dtor so that > it is self-registering. Moved cleanup to LaunchReporter(), since we only need to explicitly replace it on tests involving multiple runs of the reporter. It will be a noop in some cases, but at least it's safer. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:594: // reporter. Must be run on the IO thread. On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > "...reporter and establishes the mojo connection to it." or somesuch Done. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:683: base::OnceClosure f = base::BindOnce( On 2017/04/21 11:10:59, grt (UTC plus 2) wrote: > why introduce a local for this? if you really think it's cleaner, i think "auto" > is appropriate, and "f" must be replaced with something more clear as per "Names > should be descriptive; avoid abbreviation." > (https://google.github.io/styleguide/cppguide.html#General_Naming_Rules) Left-over from a previous test; after it passed, I forgot to cleanup. Thanks for pointing it out. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:685: reporter_process.Handle(), On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > please document why it's safe to pass the naked handle value here. its validity > is bound by the lifetime of |reporter_process|. Obsolete. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:712: base::MakeUnique<ChromePromptImpl>(std::move(chrome_prompt_request)); On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > perhaps this could call a function on the testing delegate to create the CPI so > that a test can override methods on it. Done. https://codereview.chromium.org/2834613003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/srt_fetcher_win.cc:717: base::Unretained(chrome_prompt_impl_.get()))); On 2017/04/21 11:10:58, grt (UTC plus 2) wrote: > please document why this Unretained is safe. is it impossible for a connection > error to be posted and waiting to run while a previously-posted task deletes > this SwReporterProcess? Obsolete. https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:100: // Potential failures on the reporter process that should be handled by the On 2017/04/24 14:53:48, Joe Mason wrote: > Nit: "in the reporter process". Or "of the reporter process". Done. https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:303: // cleanup; On 2017/04/24 14:53:48, Joe Mason wrote: > The most trivial nit ever: Since these lines are already delimited with "-" at > the start, I suggest ending them all with . instead of ; so that you don't need > to update line endings again if another bullet point is added. Done. https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:683: // It's safe to pass the process handle here On 2017/04/24 14:53:48, Joe Mason wrote: > Nit: End this line with a period, and either put it on the same line as the > previous sentence or add a blank line to separate paragraphs. This was still wip, not needed anymore. https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:722: base::Unretained(chrome_prompt_impl_.get()))); On 2017/04/24 14:53:48, Joe Mason wrote: > Because I know grt will ask: please add a comment explaining why Unretained is > safe here. Obsolete.
https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2834613003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:722: base::Unretained(chrome_prompt_impl_.get()))); On 2017/04/24 14:53:48, Joe Mason wrote: > Because I know grt will ask... Impact! Influence! ;-) https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.cc (right): https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_chrome_prompt_impl.cc:20: binding_.set_connection_error_handler(on_connection_closed); may as well std::move the closure, no? https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:17: explicit ChromePromptImpl(chrome_cleaner::mojom::ChromePromptRequest request, remove "explicit" since there are two args now https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:18: base::Closure on_connection_closed); #include "base/callback.h" https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:260: // parent process and keep track of errors handled. By default, deletes all nit: "keep" -> "keeps"? https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:260: // parent process and keep track of errors handled. By default, deletes all it's not clear to me what you mean by "deletes all actions to...". Do you mean "delegates"? Or something else? https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:268: : ChromePromptImpl(std::move(request), on_connection_closed), std::move callback? https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:285: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:294: // - MockedReporterFailure expected_reporter_failure_: indicates errors that nit: remove trailing underscore https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:295: // should be simulated on the reporter process. "in" the reporter process? https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:408: chrome_cleaner::mojom::ChromePromptRequest request) { override https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:410: std::move(request), bad_message_expected_, /*&bad_message_reported_*/ ? /*&bad_message_reported_*/ ? https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:570: void OnConnectionClosed() override {} please group all of the overridden methods together, maintaining their order in the interface they're implementing. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:590: // Set by ReportBadMessageChromePromptImpl if a bad message error is reported. nit: blank line before comment https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:604: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:19: #include "chrome/browser/safe_browsing/srt_chrome_prompt_impl.h" can you replace this with a forward decl for ChromePromptImpl and #include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"? https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:107: // Mocks and callbacks for the unit tests. nittynit: // A delegate used by tests to implement test doubles (e.g., stubs, fakes, or mocks). https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:112: // Test mock for launching the reporter. the use of "mock" here deviates from normal naming conventions. since this is just an interface, neither it nor its methods are mocks in and of themselves (go/mock-objects#mocks-stubs-fakes). i think it's best to document when each of these will be called and what the expectation is. for example: // Invoked during tests in place of base::LaunchProcess; see SwReporterProcess::LaunchReporterProcess.
PTAnL https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.cc (right): https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_chrome_prompt_impl.cc:20: binding_.set_connection_error_handler(on_connection_closed); On 2017/04/25 07:06:39, grt (UTC plus 2) wrote: > may as well std::move the closure, no? Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:17: explicit ChromePromptImpl(chrome_cleaner::mojom::ChromePromptRequest request, On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > remove "explicit" since there are two args now Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:18: base::Closure on_connection_closed); On 2017/04/25 07:06:39, grt (UTC plus 2) wrote: > #include "base/callback.h" Included "base/callback_forward.h" instead that defined base::Closure. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:260: // parent process and keep track of errors handled. By default, deletes all On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > nit: "keep" -> "keeps"? Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:260: // parent process and keep track of errors handled. By default, deletes all On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > it's not clear to me what you mean by "deletes all actions to...". Do you mean > "delegates"? Or something else? Yup, delegates. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:268: : ChromePromptImpl(std::move(request), on_connection_closed), On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > std::move callback? Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:285: }; On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:294: // - MockedReporterFailure expected_reporter_failure_: indicates errors that On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > nit: remove trailing underscore Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:295: // should be simulated on the reporter process. On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > "in" the reporter process? Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:408: chrome_cleaner::mojom::ChromePromptRequest request) { On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > override Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:410: std::move(request), bad_message_expected_, /*&bad_message_reported_*/ On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > ? /*&bad_message_reported_*/ ? Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:570: void OnConnectionClosed() override {} On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > please group all of the overridden methods together, maintaining their order in > the interface they're implementing. Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:590: // Set by ReportBadMessageChromePromptImpl if a bad message error is reported. On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > nit: blank line before comment Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:604: }; On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > DISALLOW_COPY_AND_ASSIGN Done. Needed to add a default constructor though. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:19: #include "chrome/browser/safe_browsing/srt_chrome_prompt_impl.h" On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > can you replace this with a forward decl for ChromePromptImpl and #include > "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"? Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:107: // Mocks and callbacks for the unit tests. On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > nittynit: > > // A delegate used by tests to implement test doubles (e.g., stubs, fakes, or > mocks). Done. https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:112: // Test mock for launching the reporter. On 2017/04/25 07:06:40, grt (UTC plus 2) wrote: > the use of "mock" here deviates from normal naming conventions. since this is > just an interface, neither it nor its methods are mocks in and of themselves > (go/mock-objects#mocks-stubs-fakes). i think it's best to document when each of > these will be called and what the expectation is. for example: > > // Invoked during tests in place of base::LaunchProcess; see > SwReporterProcess::LaunchReporterProcess. Done.
lgtm w/ two nits https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:18: base::Closure on_connection_closed); On 2017/04/25 22:24:05, ftirelo wrote: > On 2017/04/25 07:06:39, grt (UTC plus 2) wrote: > > #include "base/callback.h" > > Included "base/callback_forward.h" instead that defined base::Closure. That would be sufficient if the Closure were being passed by reference, but since you're taking by value here the caller really needs the full definition of the type. https://codereview.chromium.org/2834613003/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2834613003/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:48: #include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h" remove this now that it's included by the .h
Thanks for the excellent review! https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2834613003/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:18: base::Closure on_connection_closed); On 2017/04/26 08:46:46, grt (UTC plus 2) wrote: > On 2017/04/25 22:24:05, ftirelo wrote: > > On 2017/04/25 07:06:39, grt (UTC plus 2) wrote: > > > #include "base/callback.h" > > > > Included "base/callback_forward.h" instead that defined base::Closure. > > That would be sufficient if the Closure were being passed by reference, but > since you're taking by value here the caller really needs the full definition of > the type. Done. https://codereview.chromium.org/2834613003/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2834613003/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:48: #include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h" On 2017/04/26 08:46:46, grt (UTC plus 2) wrote: > remove this now that it's included by the .h Done.
ftirelo@chromium.org changed reviewers: + jialiul@chromium.org
+jialiul for OWNERS approval
lgtm
The CQ bit was checked by ftirelo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from joenotcharles@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2834613003/#ps120001 (title: "Final comments")
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
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by ftirelo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from joenotcharles@chromium.org, jialiul@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2834613003/#ps140001 (title: "Fix win_clang failure")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by ftirelo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from joenotcharles@chromium.org, jialiul@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2834613003/#ps160001 (title: "Really?")
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": 160001, "attempt_start_ts": 1493239077343790,
"parent_rev": "72267c39aaa4368d68c078a23bd4eb14bf22a030", "commit_rev":
"e012fe8a8e4d60ed8eba17fbac5df6d9fc4bcad3"}
Message was sent while issue was closed.
Description was changed from ========== Adds error handling support for the SwReporter launcher. BUG=690020 ========== to ========== Adds error handling support for the SwReporter launcher. BUG=690020 Review-Url: https://codereview.chromium.org/2834613003 Cr-Commit-Position: refs/heads/master@{#467471} Committed: https://chromium.googlesource.com/chromium/src/+/e012fe8a8e4d60ed8eba17fbac5d... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/e012fe8a8e4d60ed8eba17fbac5d...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2844113002/ by alph@chromium.org. The reason for reverting is: Broke several browser tests https://build.chromium.org/p/chromium.win/builders/Win7%20%2832%29%20Tests/bu....
Message was sent while issue was closed.
On 2017/04/26 22:51:16, alph wrote: > A revert of this CL (patchset #9 id:160001) has been created in > https://codereview.chromium.org/2844113002/ by mailto:alph@chromium.org. > > The reason for reverting is: Broke several browser tests > https://build.chromium.org/p/chromium.win/builders/Win7%20%2832%29%20Tests/bu.... Findit detected this CL to have introduced flakiness in test NoInBrowserCleanerUI/SRTFetcherTest.Failure/1 according to analysis https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... Can someone please help confirm? Thanks, Jeff on behalf of Findit team
Message was sent while issue was closed.
I'll look into it. On Mon, 1 May 2017, 01:14 , <lijeffrey@google.com> wrote: > On 2017/04/26 22:51:16, alph wrote: > > A revert of this CL (patchset #9 id:160001) has been created in > > https://codereview.chromium.org/2844113002/ by mailto:alph@chromium.org. > > > > > The reason for reverting is: Broke several browser tests > > > > https://build.chromium.org/p/chromium.win/builders/Win7%20%2832%29%20Tests/bu... > . > > Findit detected this CL to have introduced flakiness in test > NoInBrowserCleanerUI/SRTFetcherTest.Failure/1 according to analysis > > https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... > > Can someone please help confirm? > > Thanks, > Jeff on behalf of Findit team > > https://codereview.chromium.org/2834613003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
> > Can someone please help confirm? This CL was reverted. Is the flakiness still going on? If so, the cause might be this CL: https://codereview.chromium.org/2849433002/ I'm trying to reproduce now on my machine. As soon as I have an update, I'll ping this again.
Message was sent while issue was closed.
As discussed, the flakiness was due to the same reason that made us revert this CL. The issue doesn't seem to exist in the build after it was relanded. On Mon, 1 May 2017, 13:22 , <ftirelo@chromium.org> wrote: > > > Can someone please help confirm? > > This CL was reverted. Is the flakiness still going on? If so, the cause > might be > this CL: https://codereview.chromium.org/2849433002/ > > I'm trying to reproduce now on my machine. As soon as I have an update, > I'll > ping this again. > > https://codereview.chromium.org/2834613003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
