Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780873002:
Basic IPC communication between Chrome and the SW Reporter. (Closed)
|
Created:
3 years, 8 months ago by ftirelo Modified:
3 years, 8 months ago Reviewers:
Ken Rockot(use gerrit already), grt (UTC plus 2), joenotcharles, Fabio Tirelo, robertshield, Jialiu Lin, Joe Mason, alito CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionBasic IPC communication between Chrome and the SW Reporter.
Note to reviewers: errors and crashes in the child process
as well as other boundary conditions will be handled in
follow-up CLs, so we can unblock people working on the new
UI. Same for the cleanup of switches/constants shared between
the Chromium and the SwReporter repos.
BUG=690020
Review-Url: https://codereview.chromium.org/2780873002
Cr-Commit-Position: refs/heads/master@{#463784}
Committed: https://chromium.googlesource.com/chromium/src/+/6a532063ea0cd85c3934bcbf19037dc87a57abeb
Patch Set 1 #Patch Set 2 : Cleanup #Patch Set 3 : Rebase #
Total comments: 40
Patch Set 4 : DEPS #Patch Set 5 : First round of reviews #
Total comments: 12
Patch Set 6 : More reviews #Patch Set 7 : Fixes compilation error #
Total comments: 2
Patch Set 8 : Multiprocess browser tests #Patch Set 9 : Rebase #Patch Set 10 : RefCounted -> RefCountedThreadSafe #Patch Set 11 : Browser tests #Patch Set 12 : Comments and cleanup #Patch Set 13 : Addressing previous code review comments #Patch Set 14 : Remove debug code #Patch Set 15 : Fix const/constexpr error #
Total comments: 18
Patch Set 16 : More reviews #
Total comments: 20
Patch Set 17 : alito@'s comments #
Total comments: 2
Patch Set 18 : Rebase #Patch Set 19 : More reviews #Patch Set 20 : Fix typo #
Total comments: 10
Patch Set 21 : Non-trivial destructor in .cc file #Patch Set 22 : OWNERS reviews #Patch Set 23 : RefCounted subclasses can't have public destructors #Messages
Total messages: 53 (22 generated)
ftirelo@chromium.org changed reviewers: + alito@chromium.org, grt@chromium.org, joenotcharles@chromium.org
Hi all. This version sets up an IPC channel in Chrome for the reporter and connects both processes. The ChromePromptImpl class is only a placeholder that returns prompt denied to the reporter. As soon as we are okay with the initial design, I'll add tests.
Description was changed from ========== Basic IPC communication between Chrome and the SW Reporter. (Early review request, do not submit) BUG= ========== to ========== Basic IPC communication between Chrome and the SW Reporter. (Early review request, do not submit) BUG=690020 ==========
https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:8: #include <vector> not needed since the only use of std::vector is to override a method from chrome_cleaner::mojom::ChromePrompt https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:174: const base::Feature kInBrowserCleanerUIFeature{ constexpr https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:177: constexpr char kChromeMojoPipeTokenSwitch[] = "chrome-mojo-pipe-token"; what other component uses this switch? do the two sides share a place where the constant can be defined only once? https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:571: // Class responsible for launching the reporter process and waiting for its maybe here is a good place to document this class's relation to threads -- created and destroyed on UI thread, launching on BG thread, IPC hookup/teardown on IO thread, etc. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:579: virtual ~SwReporterProcess(); why are these methods virutal? https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:611: ->PostTask(FROM_HERE, base::Bind(&base::DeletePointer<ChromePromptImpl>, nit: ->DeleteSoon(FROM_HERE, chrome_prompt_impl_.release()); https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:615: int SwReporterProcess::LaunchAndWaitForExit() { nit: consider renaming this LaunchAndWaitForExitOnBackgroundThread https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:658: ? if (!reporter_process.IsValid()) return reporter_process; ? https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:669: base::Unretained(this), base::Passed(&mojo_pipe))); i think this could result in walking off the end of the earth under the following contrived scenario: - process creation on line 656 appears to succeed - this task is posted to the IO thread - remote process dies a horrible death, causing line 631 to return immediately - ReporterDone is posted to and run on the UI thread - SwReporterProcess instance is destroyed on UI thread - IO thread wakes up and tries to run the posted task - UAF https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:680: chrome_prompt_impl_ = you have a data race here between this store (on the IO thread) and the load on line 607 (on the UI thread). you could possibly fix it by shunting destruction of the SwReporterProcess instance itself to the IO thread whenever kInBrowserCleanerUIFeature is enabled. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:879: scoped_refptr<SwReporterProcess> sw_reporter_process( please document the expected lifetime of SwReporterProcess. it appears that it's meant to be destroyed on the UI thread when ReporterDone exits. what, other than the callback below, holds a ref to it? must it be refcounted, or can you use a more clear/obvious ownership model? https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:886: version_, sw_reporter_process)); nit: std::move(sw_reporter_process)
https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:603: std::unique_ptr<ChromePromptImpl> chrome_prompt_impl_; This file could use a comment ... somewhere ... explaining the lifetime of chrome_prompt_impl_. It's not very obvious from reading the code that the pipe is only used from within LaunchAndWaitForExit so it can be destroyed as soon as that function finishes. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:657: base::LaunchProcess(invocation_.command_line, launch_options); Should return early here if !reporter_process.IsValid. Otherwise LaunchAndWaitForExit will return immediately, but CreateChromePromptImpl will still be scheduled. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:669: base::Unretained(this), base::Passed(&mojo_pipe))); And there's another race condition here - what happens if the reporter subprocess exits before it connects to the mojo pipe? (A crash on start, for instance.) Then we have: UI thread: launch subprocess post to IO thread -> CreateChromePromptImpl reporter_process.WaitForExit (returns quickly) PostTaskAndReplyWithResult calls ReporterDone on exit from ReporterDone, SwReporterProcess is deleted IO thread: CreateChromePromptImpl called with invalid this pointer I think this can be fixed by removing "Unretained" so the CreateChromePromptImpl task retains a reference, as well as ReporterDone. So if the child process exits immediately, we create a useless chrome_prompt_impl_ and then immediately free it, but that's ok in this corner case. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:880: new SwReporterProcess(next_invocation)); I think make_scoped_refptr is preferred here.
https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:177: constexpr char kChromeMojoPipeTokenSwitch[] = "chrome-mojo-pipe-token"; On 2017/03/29 13:01:43, grt (UTC plus 2) wrote: > what other component uses this switch? do the two sides share a place where the > constant can be defined only once? The SRT, which is in a separate repo. The constant could be defined in the pipa repo alongside chrome_prompt.mojom, but I'm not sure it's worth adding another header file to that repo just for this. (Although we could move all the other SRT command-line flags to that repo too if we do that.) https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:879: scoped_refptr<SwReporterProcess> sw_reporter_process( On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > please document the expected lifetime of SwReporterProcess. it appears that it's > meant to be destroyed on the UI thread when ReporterDone exits. what, other than > the callback below, holds a ref to it? must it be refcounted, or can you use a > more clear/obvious ownership model? With scoped_refptr we can have references to it in ReporterDone (which always runs after LaunchAndWaitForExit due to PostTaskAndReply) and CreateChromePromptImpl, which ensures SwReporterProcess isn't deleted too early even on launch failures. A unique_ptr passed to ReporterDone, which then deletes the SwReporterProcess on the IO thread, would also work because it would ensure that the SwReporterProcess delete is posted after any CreateChromePromptImpl. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:884: base::Unretained(sw_reporter_process.get())), Nit: if you do keep scoped_refptr, I think it makes sense to get rid of Unretained here. It's not totally necessary since we know ReporterDone runs after LaunchAndWait, but it's a bit safer in case of refactoring.
Thanks for the comments. PTAL before I start browser tests. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:8: #include <vector> On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > not needed since the only use of std::vector is to override a method from > chrome_cleaner::mojom::ChromePrompt Done. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:174: const base::Feature kInBrowserCleanerUIFeature{ On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > constexpr This is also defined in the header file (I'll need in tests), so kept as const. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:177: constexpr char kChromeMojoPipeTokenSwitch[] = "chrome-mojo-pipe-token"; On 2017/03/29 13:31:01, Joe Mason wrote: > On 2017/03/29 13:01:43, grt (UTC plus 2) wrote: > > what other component uses this switch? do the two sides share a place where > the > > constant can be defined only once? > > The SRT, which is in a separate repo. > > The constant could be defined in the pipa repo alongside chrome_prompt.mojom, > but I'm not sure it's worth adding another header file to that repo just for > this. > > (Although we could move all the other SRT command-line flags to that repo too if > we do that.) As soon as I land crrev/2782243002, I will move shared constants to that subdir, so they can be shared with the SRT. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:177: constexpr char kChromeMojoPipeTokenSwitch[] = "chrome-mojo-pipe-token"; On 2017/03/29 13:01:43, grt (UTC plus 2) wrote: > what other component uses this switch? do the two sides share a place where the > constant can be defined only once? This will be landed in //components/chrome_cleaner as soon as I land crrev/2782243002. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:571: // Class responsible for launching the reporter process and waiting for its On 2017/03/29 13:01:43, grt (UTC plus 2) wrote: > maybe here is a good place to document this class's relation to threads -- > created and destroyed on UI thread, launching on BG thread, IPC hookup/teardown > on IO thread, etc. Done. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:579: virtual ~SwReporterProcess(); On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > why are these methods virutal? No need to, since they are in the anonymous namespace. I may move them out for testing and want to mock the class, but if needed I'll bring the virtuals back. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:603: std::unique_ptr<ChromePromptImpl> chrome_prompt_impl_; On 2017/03/29 13:04:31, Joe Mason wrote: > This file could use a comment ... somewhere ... explaining the lifetime of > chrome_prompt_impl_. It's not very obvious from reading the code that the pipe > is only used from within LaunchAndWaitForExit so it can be destroyed as soon as > that function finishes. Done. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:611: ->PostTask(FROM_HERE, base::Bind(&base::DeletePointer<ChromePromptImpl>, On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > nit: > ->DeleteSoon(FROM_HERE, chrome_prompt_impl_.release()); Done. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:615: int SwReporterProcess::LaunchAndWaitForExit() { On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > nit: consider renaming this LaunchAndWaitForExitOnBackgroundThread Done. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:657: base::LaunchProcess(invocation_.command_line, launch_options); On 2017/03/29 13:04:31, Joe Mason wrote: > Should return early here if !reporter_process.IsValid. Otherwise > LaunchAndWaitForExit will return immediately, but CreateChromePromptImpl will > still be scheduled. Done. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:658: On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > ? > if (!reporter_process.IsValid()) > return reporter_process; > ? Done. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:669: base::Unretained(this), base::Passed(&mojo_pipe))); On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > i think this could result in walking off the end of the earth under the > following contrived scenario: > - process creation on line 656 appears to succeed > - this task is posted to the IO thread > - remote process dies a horrible death, causing line 631 to return immediately > - ReporterDone is posted to and run on the UI thread > - SwReporterProcess instance is destroyed on UI thread > - IO thread wakes up and tries to run the posted task > - UAF Please take a look at the new implementation to see if all race conditions have been fixed. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:669: base::Unretained(this), base::Passed(&mojo_pipe))); On 2017/03/29 13:04:31, Joe Mason wrote: > And there's another race condition here - what happens if the reporter > subprocess exits before it connects to the mojo pipe? (A crash on start, for > instance.) Then we have: > > UI thread: > launch subprocess > post to IO thread -> CreateChromePromptImpl > reporter_process.WaitForExit (returns quickly) > PostTaskAndReplyWithResult calls ReporterDone > on exit from ReporterDone, SwReporterProcess is deleted > > IO thread: > CreateChromePromptImpl called with invalid this pointer > > I think this can be fixed by removing "Unretained" so the CreateChromePromptImpl > task retains a reference, as well as ReporterDone. So if the child process exits > immediately, we create a useless chrome_prompt_impl_ and then immediately free > it, but that's ok in this corner case. Please take a look at the new implementation to see if all race conditions have been fixed. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:680: chrome_prompt_impl_ = On 2017/03/29 13:01:43, grt (UTC plus 2) wrote: > you have a data race here between this store (on the IO thread) and the load on > line 607 (on the UI thread). you could possibly fix it by shunting destruction > of the SwReporterProcess instance itself to the IO thread whenever > kInBrowserCleanerUIFeature is enabled. Please take a look at the new implementation to see if all race conditions have been fixed. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:879: scoped_refptr<SwReporterProcess> sw_reporter_process( On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > please document the expected lifetime of SwReporterProcess. it appears that it's > meant to be destroyed on the UI thread when ReporterDone exits. what, other than > the callback below, holds a ref to it? must it be refcounted, or can you use a > more clear/obvious ownership model? Done. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:879: scoped_refptr<SwReporterProcess> sw_reporter_process( On 2017/03/29 13:31:01, Joe Mason wrote: > On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > > please document the expected lifetime of SwReporterProcess. it appears that > it's > > meant to be destroyed on the UI thread when ReporterDone exits. what, other > than > > the callback below, holds a ref to it? must it be refcounted, or can you use a > > more clear/obvious ownership model? > > With scoped_refptr we can have references to it in ReporterDone (which always > runs after LaunchAndWaitForExit due to PostTaskAndReply) and > CreateChromePromptImpl, which ensures SwReporterProcess isn't deleted too early > even on launch failures. > > A unique_ptr passed to ReporterDone, which then deletes the SwReporterProcess on > the IO thread, would also work because it would ensure that the > SwReporterProcess delete is posted after any CreateChromePromptImpl. Done. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:880: new SwReporterProcess(next_invocation)); On 2017/03/29 13:04:31, Joe Mason wrote: > I think make_scoped_refptr is preferred here. Done. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:884: base::Unretained(sw_reporter_process.get())), On 2017/03/29 13:31:00, Joe Mason wrote: > Nit: if you do keep scoped_refptr, I think it makes sense to get rid of > Unretained here. It's not totally necessary since we know ReporterDone runs > after LaunchAndWait, but it's a bit safer in case of refactoring. Done. https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:886: version_, sw_reporter_process)); On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > nit: std::move(sw_reporter_process) Saved both callbacks in auxiliary variables to enforce evaluation order (sw_reporter_process) was used by both methods posted by PostTaskAndReplyWithResult().
Everything else looks fine to me. https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:585: // - creation of a ChromePromptImpl object right the reporter process is Looks like there are some missing words in this sentence.
lgtm with nits https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:174: const base::Feature kInBrowserCleanerUIFeature{ On 2017/03/30 22:13:51, ftirelo wrote: > On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > > constexpr > > This is also defined in the header file (I'll need in tests), so kept as const. can you not leave the declaration in the .h as-is and make the definition on line 79 constexpr? https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:577: // - created in the UI thread before the reporter process launch is posted nit: "on" a thread rather than "in" (prepositions, how do they work? :-) ) https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:622: // reporter process. please document that this member may only be accessed on the IO thread. https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:698: void SwReporterProcess::ReleaseChromePromptImpl() { nit: method definitions should be in the same order as their in-class declarations. https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:900: scoped_refptr<SwReporterProcess> sw_reporter_process = nit: this can become "auto sw_reporter_process" since the type is obvious from make_scoped_refptr. https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:922: if (base::FeatureList::IsEnabled(kInBrowserCleanerUIFeature)) { nit: putting this conditional out here seems to be exposing what is otherwise an implementation detail out of SwReporterProcess. how about moving this into a method that is blindly called here (OnReporterDone or something)?
https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:577: // - created in the UI thread before the reporter process launch is posted On 2017/03/31 11:32:33, grt (UTC plus 2) wrote: > nit: "on" a thread rather than "in" (prepositions, how do they work? :-) ) Done :-) (I'm afraid that now I don't know how to properly use prepositions anymore even in Portuguese :-) ) https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:585: // - creation of a ChromePromptImpl object right the reporter process is On 2017/03/31 08:06:23, Joe Mason wrote: > Looks like there are some missing words in this sentence. Done. https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:622: // reporter process. On 2017/03/31 11:32:33, grt (UTC plus 2) wrote: > please document that this member may only be accessed on the IO thread. Done. https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:698: void SwReporterProcess::ReleaseChromePromptImpl() { On 2017/03/31 11:32:33, grt (UTC plus 2) wrote: > nit: method definitions should be in the same order as their in-class > declarations. It's private now, so keeping it here since it was declared after CreateChromePromptImpl on the class. https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:900: scoped_refptr<SwReporterProcess> sw_reporter_process = On 2017/03/31 11:32:33, grt (UTC plus 2) wrote: > nit: this can become "auto sw_reporter_process" since the type is obvious from > make_scoped_refptr. Done. https://codereview.chromium.org/2780873002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:922: if (base::FeatureList::IsEnabled(kInBrowserCleanerUIFeature)) { On 2017/03/31 11:32:33, grt (UTC plus 2) wrote: > nit: putting this conditional out here seems to be exposing what is otherwise an > implementation detail out of SwReporterProcess. how about moving this into a > method that is blindly called here (OnReporterDone or something)? Done.
I'll start browser tests now.
https://codereview.chromium.org/2780873002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:964: auto finished_invocation = sw_reporter_process->invocation(); nit: "const auto&"
This is ready for final review. PTAnL https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:174: const base::Feature kInBrowserCleanerUIFeature{ On 2017/03/31 11:32:33, grt (no reviews Apr 7-17) wrote: > On 2017/03/30 22:13:51, ftirelo wrote: > > On 2017/03/29 13:01:42, grt (UTC plus 2) wrote: > > > constexpr > > > > This is also defined in the header file (I'll need in tests), so kept as > const. > > can you not leave the declaration in the .h as-is and make the definition on > line 79 constexpr? If I use constexpr here and not in the header file, I get a compilation error. I didn't understand the second half of the comment, since line 79 doesn't seem related to this CL. https://codereview.chromium.org/2780873002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:964: auto finished_invocation = sw_reporter_process->invocation(); On 2017/03/31 21:27:28, grt (no reviews Apr 7-17) wrote: > nit: "const auto&" Done.
Description was changed from ========== Basic IPC communication between Chrome and the SW Reporter. (Early review request, do not submit) BUG=690020 ========== to ========== Basic IPC communication between Chrome and the SW Reporter. Note to reviewers: errors and crashes in the child process as well as other boundary conditions will be handled in follow-up CLs, so we can unblock people working on the new UI. Same the cleanup of switches/constants shared between the Chromium and the SwReporter repos. BUG=690020 ==========
ftirelo@chromium.org changed reviewers: + robertshield@chromium.org
Description was changed from ========== Basic IPC communication between Chrome and the SW Reporter. Note to reviewers: errors and crashes in the child process as well as other boundary conditions will be handled in follow-up CLs, so we can unblock people working on the new UI. Same the cleanup of switches/constants shared between the Chromium and the SwReporter repos. BUG=690020 ========== to ========== Basic IPC communication between Chrome and the SW Reporter. Note to reviewers: errors and crashes in the child process as well as other boundary conditions will be handled in follow-up CLs, so we can unblock people working on the new UI. Same for the cleanup of switches/constants shared between the Chromium and the SwReporter repos. BUG=690020 ==========
joenotcharles@google.com changed reviewers: + joenotcharles@google.com
joenotcharles@google.com changed reviewers: + joenotcharles@google.com
https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:8: #include <iterator> No longer needed (was used for std::begin / std::end) https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:62: // This should never passed as the expected exit code to be report by tests. Typo: "reported" https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:113: // |command_line| sets |expected_value_received| as true if the parent I think "sets" should start a new sentence. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:124: // This pointer will be deleted by callback |done|. That's not exactly true, it's deleted by PromptUserCallback. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:198: (!command_line->HasSwitch(kChromeMojoPipeTokenSwitch) || Nit: I think it'd be cleaner to read chrome_mojo_pipe_token in this func, and then pass it to ConnectAndSendDataToParentProcess. That way you're not reading kChromeMojoPipeTokenSwitch in two separate functions. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:212: public ::testing::WithParamInterface<std::tuple<bool, bool>> { Please include <tuple> for this. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:439: // checked if both sets should have the same size. This comment doesn't match the code anymore. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:448: scoped_feature_list.reset(new base::test::ScopedFeatureList()); Prefer "auto scoped_feature_list = base::MakeUnique<...>" https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:493: auto scoped_feature_list = GetScopedFeatureList(); Instead of putting this at the start of every test, can you make it a member var of SRTFetcherTest and initialize it in the constructor?
https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:8: #include <iterator> No longer needed (was used for std::begin / std::end) https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:62: // This should never passed as the expected exit code to be report by tests. Typo: "reported" https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:113: // |command_line| sets |expected_value_received| as true if the parent I think "sets" should start a new sentence. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:124: // This pointer will be deleted by callback |done|. That's not exactly true, it's deleted by PromptUserCallback. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:198: (!command_line->HasSwitch(kChromeMojoPipeTokenSwitch) || Nit: I think it'd be cleaner to read chrome_mojo_pipe_token in this func, and then pass it to ConnectAndSendDataToParentProcess. That way you're not reading kChromeMojoPipeTokenSwitch in two separate functions. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:212: public ::testing::WithParamInterface<std::tuple<bool, bool>> { Please include <tuple> for this. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:439: // checked if both sets should have the same size. This comment doesn't match the code anymore. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:448: scoped_feature_list.reset(new base::test::ScopedFeatureList()); Prefer "auto scoped_feature_list = base::MakeUnique<...>" https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:493: auto scoped_feature_list = GetScopedFeatureList(); Instead of putting this at the start of every test, can you make it a member var of SRTFetcherTest and initialize it in the constructor?
PTAnL https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:8: #include <iterator> On 2017/04/06 17:57:15, joenotcharles wrote: > No longer needed (was used for std::begin / std::end) Done. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:62: // This should never passed as the expected exit code to be report by tests. On 2017/04/06 17:57:14, joenotcharles wrote: > Typo: "reported" Done. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:113: // |command_line| sets |expected_value_received| as true if the parent On 2017/04/06 17:57:15, joenotcharles wrote: > I think "sets" should start a new sentence. Done. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:124: // This pointer will be deleted by callback |done|. On 2017/04/06 17:57:14, joenotcharles wrote: > That's not exactly true, it's deleted by PromptUserCallback. Done. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:198: (!command_line->HasSwitch(kChromeMojoPipeTokenSwitch) || On 2017/04/06 17:57:14, joenotcharles wrote: > Nit: I think it'd be cleaner to read chrome_mojo_pipe_token in this func, and > then pass it to ConnectAndSendDataToParentProcess. That way you're not reading > kChromeMojoPipeTokenSwitch in two separate functions. Done. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:212: public ::testing::WithParamInterface<std::tuple<bool, bool>> { On 2017/04/06 17:57:14, joenotcharles wrote: > Please include <tuple> for this. Done. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:439: // checked if both sets should have the same size. On 2017/04/06 17:57:15, joenotcharles wrote: > This comment doesn't match the code anymore. Done. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:448: scoped_feature_list.reset(new base::test::ScopedFeatureList()); On 2017/04/06 17:57:15, joenotcharles wrote: > Prefer "auto scoped_feature_list = base::MakeUnique<...>" Not a unique_ptr anymore. https://codereview.chromium.org/2780873002/diff/280001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:493: auto scoped_feature_list = GetScopedFeatureList(); On 2017/04/06 17:57:14, joenotcharles wrote: > Instead of putting this at the start of every test, can you make it a member var > of SRTFetcherTest and initialize it in the constructor? Done.
The CQ bit was checked by ftirelo@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...
Just a couple of minor questions. https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:96: // the prompt accepted returned by the parent process is equals to nit: equals -> equal https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:149: // Connects to the parent process and send mocked scan results. Returns true if nit: send -> sends https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:167: base::MessageLoop message_loop; Is message_loop supposed to be used somewhere? https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:196: const std::string& chrome_mojo_pipe_token = Could getting the token from the command line be done in ConnectAndSendDataToParentProcess()?
https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:167: base::MessageLoop message_loop; On 2017/04/06 21:13:26, alito wrote: > Is message_loop supposed to be used somewhere? MessageLoop's constructor registers itself with the current thread, and RunLoop wraps "the current message loop". So, yeah, strange as it seems this is correct.
https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:96: // the prompt accepted returned by the parent process is equals to On 2017/04/06 21:13:26, alito wrote: > nit: equals -> equal Done. https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:149: // Connects to the parent process and send mocked scan results. Returns true if On 2017/04/06 21:13:26, alito wrote: > nit: send -> sends Done. https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:167: base::MessageLoop message_loop; On 2017/04/06 21:21:06, joenotcharles wrote: > On 2017/04/06 21:13:26, alito wrote: > > Is message_loop supposed to be used somewhere? > > MessageLoop's constructor registers itself with the current thread, and RunLoop > wraps "the current message loop". So, yeah, strange as it seems this is correct. Done. https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:196: const std::string& chrome_mojo_pipe_token = On 2017/04/06 21:13:26, alito wrote: > Could getting the token from the command line be done in > ConnectAndSendDataToParentProcess()? As discussed, by getting the switch value here, we don't need to check it twice.
lgtm https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:167: base::MessageLoop message_loop; On 2017/04/06 21:21:06, joenotcharles wrote: > On 2017/04/06 21:13:26, alito wrote: > > Is message_loop supposed to be used somewhere? > > MessageLoop's constructor registers itself with the current thread, and RunLoop > wraps "the current message loop". So, yeah, strange as it seems this is correct. In other words, magic!
lgtm with nits https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:62: // This should never passed as the expected exit code to be reported by tests. Another typo: "never be passed" https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:107: delete g_chrome_prompt_ptr; Should set to null after deleting it. https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:156: return false; Nit: I'd prefer a DCHECK here since the caller already checks if the token is empty. https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:170: // task to unblock the child's process main thread. Nit: I think you mean "child process's main thread"? https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:263: // the parameters given in |invocation| and that will return Typo: "and that" should just be "that", I think.
quick-drive by thought (lgtm, otherwise) https://codereview.chromium.org/2780873002/diff/320001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2780873002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:21: bool elevation_required, maybe in a follow up CL, it would be nice if this was an enum rather than a bool
https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right): https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:62: // This should never passed as the expected exit code to be reported by tests. On 2017/04/06 21:41:56, joenotcharles wrote: > Another typo: "never be passed" Done. https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:107: delete g_chrome_prompt_ptr; On 2017/04/06 21:41:56, joenotcharles wrote: > Should set to null after deleting it. Done. https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:156: return false; On 2017/04/06 21:41:56, joenotcharles wrote: > Nit: I'd prefer a DCHECK here since the caller already checks if the token is > empty. Done. https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:170: // task to unblock the child's process main thread. On 2017/04/06 21:41:56, joenotcharles wrote: > Nit: I think you mean "child process's main thread"? Done. https://codereview.chromium.org/2780873002/diff/300001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:263: // the parameters given in |invocation| and that will return On 2017/04/06 21:41:56, joenotcharles wrote: > Typo: "and that" should just be "that", I think. Done. https://codereview.chromium.org/2780873002/diff/320001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2780873002/diff/320001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:21: bool elevation_required, On 2017/04/07 02:04:58, robertshield wrote: > maybe in a follow up CL, it would be nice if this was an enum rather than a bool Acknowledged.
ftirelo@chromium.org changed reviewers: + rockot@chromium.org
+rockot for Mojo OWNERS.
lgtm
The CQ bit was checked by ftirelo@google.com 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...
ftirelo@google.com changed reviewers: + jialiul@chromium.org
+jialiul for OWNERS approval.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM for c/b/safe_browsing/ I didn't review these browser tests very closely. I rely on other reviewers to make sure there is enough test coverage. https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:17: ~ChromePromptImpl() override = default; [chromium-style] Complex destructor has an inline body. https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:566: // Class responsible for launching the reporter process and waiting for its optional: maybe moving this class into separate files for easier testing? can be done in a later CL.
Mojo stuff LGTM https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:17: ~ChromePromptImpl() override = default; nit: (compiler caught this) You need to move this out-of-line. You can still write: ChromePromptImpl::~ChromePromptImpl() = default; in the cc file https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:691: base::Bind(&SwReporterProcess::CreateChromePromptImpl, nit: I think you could use BindOnce here and then just std::move(foo) a movable argument foo instead of using base::Passed(&foo). https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:706: mojo::ScopedMessagePipeHandle mojo_pipe) { nit: I'd make this arg a ChromePromptRequest rather than a generic pipe handle. Interface requests are just typesafe pipe handles and are thus completely safe move across threads. You can create a ChromePromptRequest in LaunchConnectedReporterProcess and bind it to the pipe there.
Thanks for the reviews! https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_chrome_prompt_impl.h (right): https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:17: ~ChromePromptImpl() override = default; On 2017/04/10 21:44:40, Ken Rockot wrote: > nit: (compiler caught this) You need to move this out-of-line. You can still > write: > > ChromePromptImpl::~ChromePromptImpl() = default; > > in the cc file Done. https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_chrome_prompt_impl.h:17: ~ChromePromptImpl() override = default; On 2017/04/10 17:32:54, Jialiu Lin wrote: > [chromium-style] Complex destructor has an inline body. Done. https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:566: // Class responsible for launching the reporter process and waiting for its On 2017/04/10 17:32:54, Jialiu Lin wrote: > optional: maybe moving this class into separate files for easier testing? can be > done in a later CL. For the moment, this class can be considered an implementation detail (I need an active object to hold the ChromePromptImpl pointer created while I launch the process), so I think it's okay to keep in the anonymous namespace. However, I'll keep an eye on it and move if it starts getting more responsibilities. https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:691: base::Bind(&SwReporterProcess::CreateChromePromptImpl, On 2017/04/10 21:44:40, Ken Rockot wrote: > nit: I think you could use BindOnce here and then just std::move(foo) a movable > argument foo instead of using base::Passed(&foo). Done. https://codereview.chromium.org/2780873002/diff/380001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:706: mojo::ScopedMessagePipeHandle mojo_pipe) { On 2017/04/10 21:44:40, Ken Rockot wrote: > nit: I'd make this arg a ChromePromptRequest rather than a generic pipe handle. > Interface requests are just typesafe pipe handles and are thus completely safe > move across threads. You can create a ChromePromptRequest in > LaunchConnectedReporterProcess and bind it to the pipe there. Done.
The CQ bit was checked by ftirelo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, joenotcharles@google.com, robertshield@chromium.org, alito@chromium.org, joenotcharles@chromium.org, jialiul@chromium.org, rockot@chromium.org Link to the patchset: https://codereview.chromium.org/2780873002/#ps420001 (title: "OWNERS reviews")
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: 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 ftirelo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, grt@chromium.org, joenotcharles@google.com, robertshield@chromium.org, jialiul@chromium.org, joenotcharles@chromium.org, alito@chromium.org Link to the patchset: https://codereview.chromium.org/2780873002/#ps440001 (title: "RefCounted subclasses can't have public destructors")
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": 440001, "attempt_start_ts": 1491942406737610,
"parent_rev": "6e4bc12318fac3839ddf6f0b69dcbd5bfd962b56", "commit_rev":
"6a532063ea0cd85c3934bcbf19037dc87a57abeb"}
Message was sent while issue was closed.
Description was changed from ========== Basic IPC communication between Chrome and the SW Reporter. Note to reviewers: errors and crashes in the child process as well as other boundary conditions will be handled in follow-up CLs, so we can unblock people working on the new UI. Same for the cleanup of switches/constants shared between the Chromium and the SwReporter repos. BUG=690020 ========== to ========== Basic IPC communication between Chrome and the SW Reporter. Note to reviewers: errors and crashes in the child process as well as other boundary conditions will be handled in follow-up CLs, so we can unblock people working on the new UI. Same for the cleanup of switches/constants shared between the Chromium and the SwReporter repos. BUG=690020 Review-Url: https://codereview.chromium.org/2780873002 Cr-Commit-Position: refs/heads/master@{#463784} Committed: https://chromium.googlesource.com/chromium/src/+/6a532063ea0cd85c3934bcbf1903... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001) as https://chromium.googlesource.com/chromium/src/+/6a532063ea0cd85c3934bcbf1903... |
