Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
981223004:
Add test for ProcessSingleton hung rendezvous case. (Closed)
|
Created:
5 years, 9 months ago by Sigurður Ásgeirsson Modified:
5 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a test for ProcessSingleton hung rendezvous case.
BUG=464733
Committed: https://crrev.com/2a0214bf578d1ccc77aa216fe36acfe865499bec
Cr-Commit-Position: refs/heads/master@{#320277}
Patch Set 1 #Patch Set 2 : A bit of cleanup, now passes first test. #Patch Set 3 : Add appropriate test seams to make the test run quickly, and without user interaction. #Patch Set 4 : Now tests the user interaction cases. #
Total comments: 20
Patch Set 5 : Address Erik's comments. #Patch Set 6 : Fix speling[sic]. #
Total comments: 40
Patch Set 7 : Address Gab's comments. #
Total comments: 8
Patch Set 8 : Rebase to ToT. #Patch Set 9 : Address Gab's remaining comments. #
Total comments: 13
Patch Set 10 : Address Scott's comments. #Patch Set 11 : Clean up in short-lived test process. Moar comments. #
Total comments: 2
Patch Set 12 : Address Scott's remaining comment. #
Messages
Total messages: 32 (7 generated)
siggi@chromium.org changed reviewers: + erikwright@chromium.org, thakis@chromium.org
Erik for content, please. Nico for owners, please.
https://codereview.chromium.org/981223004/diff/60001/chrome/browser/chrome_pr... File chrome/browser/chrome_process_finder_win.h (right): https://codereview.chromium.org/981223004/diff/60001/chrome/browser/chrome_pr... chrome/browser/chrome_process_finder_win.h:33: int SetNotificationTimeoutForTesting(int new_timeout); The name should include "InSeconds" https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win.cc:447: void ProcessSingleton::OverrideKillMessageBoxCallbackForTesting( extra space after void https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Fix the copyright. I believe the (c) should go, and 2012->2015 https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:107: ::SetEvent(ready_event.Get()); Check the result of SetEvent and WaitForSingleObject (i.e. WAIT_OBJECT_0)? https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:122: // Drop the process finder notification timeout to a second for testing. a -> one or 1 https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:154: // Create the named "ready" event, this is unique to our process. ready->continue https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:246: EXPECT_TRUE(browser_victim().WaitForExit(&exit_code)); Should this be WaitForExitWithTimeout with a delay of 0? Presumably it's a key part of the contract that the other process is dead when Notify... returns. https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:260: // windows. comment in error https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:278: // The message box should not have been invoked, as the "browser" had no comment in error https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:283: EXPECT_TRUE(browser_victim().WaitForExit(&exit_code)); same question about timeout
Thanks, PTAL? https://codereview.chromium.org/981223004/diff/60001/chrome/browser/chrome_pr... File chrome/browser/chrome_process_finder_win.h (right): https://codereview.chromium.org/981223004/diff/60001/chrome/browser/chrome_pr... chrome/browser/chrome_process_finder_win.h:33: int SetNotificationTimeoutForTesting(int new_timeout); On 2015/03/10 19:22:49, erikwright wrote: > The name should include "InSeconds" Done. https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win.cc:447: void ProcessSingleton::OverrideKillMessageBoxCallbackForTesting( On 2015/03/10 19:22:49, erikwright wrote: > extra space after void Done. https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/03/10 19:22:49, erikwright wrote: > Fix the copyright. > > I believe the (c) should go, and 2012->2015 Done. https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:107: ::SetEvent(ready_event.Get()); On 2015/03/10 19:22:49, erikwright wrote: > Check the result of SetEvent and WaitForSingleObject (i.e. WAIT_OBJECT_0)? Done. https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:122: // Drop the process finder notification timeout to a second for testing. On 2015/03/10 19:22:49, erikwright wrote: > a -> one > > or 1 Done. https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:154: // Create the named "ready" event, this is unique to our process. On 2015/03/10 19:22:49, erikwright wrote: > ready->continue Done. https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:246: EXPECT_TRUE(browser_victim().WaitForExit(&exit_code)); On 2015/03/10 19:22:49, erikwright wrote: > Should this be WaitForExitWithTimeout with a delay of 0? > > Presumably it's a key part of the contract that the other process is dead when > Notify... returns. This is true - good point! https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:260: // windows. On 2015/03/10 19:22:49, erikwright wrote: > comment in error Done. https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:278: // The message box should not have been invoked, as the "browser" had no On 2015/03/10 19:22:49, erikwright wrote: > comment in error Done. https://codereview.chromium.org/981223004/diff/60001/chrome/browser/process_s... chrome/browser/process_singleton_win_unittest.cc:283: EXPECT_TRUE(browser_victim().WaitForExit(&exit_code)); On 2015/03/10 19:22:49, erikwright wrote: > same question about timeout Done.
LGTM.
gab@chromium.org changed reviewers: + gab@chromium.org
Very very nice! Comments/suggestions below, mostly around improving readability. Cheers! Gab https://codereview.chromium.org/981223004/diff/100001/chrome/browser/chrome_p... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/981223004/diff/100001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.cc:33: int timeout_in_seconds = kDefaultTimeoutInSeconds; nit: |g_timeout_in_seconds| (g_ prefix for globals) https://codereview.chromium.org/981223004/diff/100001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.cc:167: return old_timeout; <investigate> https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton.h:96: typedef base::Callback<bool()> DisplayShouldKillMessageBoxCallback; New C++11 syntax is prefered these days, i.e.: using DisplayShouldKillMessageBoxCallback = base::Callback<bool()>; https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton.h:96: typedef base::Callback<bool()> DisplayShouldKillMessageBoxCallback; Add a comment to explain what it means for this callback to return true/false. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win.cc:293: if (visible_window && !display_should_kill_messagebox_callback_.Run()) { Actually this callback is more than "display" it's "asking", how about naming it something like "QueryUserToKillRemoteProcessCallback" or even just "ShouldKillRemoteProcessCallback" (i.e. the inline implementation just happens to use a message box, but that doesn't have to be part of the interface). https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:40: return true; Remind the reader with a comment of what it means to return true here? https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:64: if (window) Can this ever be NULL in a valid run of this test? Should you EXPECT_TRUE that it's not? https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:74: if (user_data_dir.empty()) Doesn't ASSERT semantic work here and below rather than using the kErrorResultCode? (simply asking, first time I see a base::MultiProcessTest) https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:117: typedef base::MultiProcessTest Super; I've never seen this style in Chromium, tests typically refer explicitly to their parent's type when forwarding things like SetUp() and TearDown(). Not using Super doesn't add reading complexity (if anything it reduces it as the reader has one less type to keep in his mind). Especially since you not only use Super in SetUp and TearDown (which arguably are likely to be overridable if the Super type changes), but use it in MakeCmdLine which is specific to base::MultiProcessTest -- so might as well be explicit about it. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:118: public: s/public/protected (I was recently told to do so by other reviewers as nothing in the test fixture needs to be public) https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:132: old_notification_timeout_); Why bother restoring it? Other unit tests shouldn't get involved with ProcessSingleton anyways (besides other ProcessSingleton tests in this fixture which may run in parallel in the same process, but they all desire the 1s timeout so wtv). Not doing so would simplify the setters' interface too which IMO is atypical (setters rarely return the old value) https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:143: enum WindowOption { The Google C++ style-guide requires that typedefs/enums be at the top of each class' sections: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:179: ASSERT_EQ(result, WAIT_OBJECT_0); Assert that |ready_event| was signaled? Other pieces below (i.e. line 197) seem to assume that if this returns with no fatal failure it's because the |ready_event| was signaled. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:201: base::Bind(&NotificationCallback))); nit: + single space (suggest running "git cl format") https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:208: base::Process& browser_victim() { return browser_victim_; } Use a * (non-const & are not-allowed by style guide) https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:214: bool MockDisplayKillMessageBox(bool result) { s/result/allow_kill ? (I had to piece things around to figure out that's what this meant, seems a better name would help readability?) https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:253: EXPECT_EQ(exit_code, content::RESULT_CODE_HUNG); Add a comment stating that RESULT_CODE_HUNG is expected to be the exit code on the hung process we just killed (i.e. I had to code search to figure out how this exit code got set, ideally I read a quick comment here and trust your words without having to search around). https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:292: } Should we also add a test that the normal use case for process singleton (i.e. successful rendez-vous) works?
thanks - please take another look? https://codereview.chromium.org/981223004/diff/100001/chrome/browser/chrome_p... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/981223004/diff/100001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.cc:33: int timeout_in_seconds = kDefaultTimeoutInSeconds; On 2015/03/11 13:20:39, gab wrote: > nit: |g_timeout_in_seconds| (g_ prefix for globals) Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.cc:167: return old_timeout; On 2015/03/11 13:20:39, gab wrote: > <investigate> ??? https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton.h:96: typedef base::Callback<bool()> DisplayShouldKillMessageBoxCallback; On 2015/03/11 13:20:39, gab wrote: > Add a comment to explain what it means for this callback to return true/false. Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton.h:96: typedef base::Callback<bool()> DisplayShouldKillMessageBoxCallback; On 2015/03/11 13:20:39, gab wrote: > New C++11 syntax is prefered these days, i.e.: > > using DisplayShouldKillMessageBoxCallback = base::Callback<bool()>; Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win.cc:293: if (visible_window && !display_should_kill_messagebox_callback_.Run()) { On 2015/03/11 13:20:39, gab wrote: > Actually this callback is more than "display" it's "asking", how about naming it > something like "QueryUserToKillRemoteProcessCallback" or even just > "ShouldKillRemoteProcessCallback" (i.e. the inline implementation just happens > to use a message box, but that doesn't have to be part of the interface). Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:40: return true; On 2015/03/11 13:20:40, gab wrote: > Remind the reader with a comment of what it means to return true here? Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:64: if (window) On 2015/03/11 13:20:39, gab wrote: > Can this ever be NULL in a valid run of this test? Should you EXPECT_TRUE that > it's not? This happens in the remote instance - error is flagged by the process exit code. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:74: if (user_data_dir.empty()) On 2015/03/11 13:20:40, gab wrote: > Doesn't ASSERT semantic work here and below rather than using the > kErrorResultCode? (simply asking, first time I see a base::MultiProcessTest) I just tested this, and there's no support for the ASSERTs in the subprocess. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:117: typedef base::MultiProcessTest Super; On 2015/03/11 13:20:40, gab wrote: > I've never seen this style in Chromium, tests typically refer explicitly to > their parent's type when forwarding things like SetUp() and TearDown(). > > Not using Super doesn't add reading complexity (if anything it reduces it as the > reader has one less type to keep in his mind). > > Especially since you not only use Super in SetUp and TearDown (which arguably > are likely to be overridable if the Super type changes), but use it in > MakeCmdLine which is specific to base::MultiProcessTest -- so might as well be > explicit about it. Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:118: public: On 2015/03/11 13:20:39, gab wrote: > s/public/protected > > (I was recently told to do so by other reviewers as nothing in the test fixture > needs to be public) Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:132: old_notification_timeout_); On 2015/03/11 13:20:40, gab wrote: > Why bother restoring it? Other unit tests shouldn't get involved with > ProcessSingleton anyways (besides other ProcessSingleton tests in this fixture > which may run in parallel in the same process, but they all desire the 1s > timeout so wtv). > > Not doing so would simplify the setters' interface too which IMO is atypical > (setters rarely return the old value) There are apparently cases where multiple tests run inside the same process. I've had DrMemory FYI waterfall test failures occur in unrelated tests due to coupling through histograms as a case in point. IMHO a global is a very bad idea(TM), but if you must have one, leave it as you found it. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:143: enum WindowOption { On 2015/03/11 13:20:39, gab wrote: > The Google C++ style-guide requires that typedefs/enums be at the top of each > class' sections: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:179: ASSERT_EQ(result, WAIT_OBJECT_0); On 2015/03/11 13:20:39, gab wrote: > Assert that |ready_event| was signaled? Other pieces below (i.e. line 197) seem > to assume that if this returns with no fatal failure it's because the > |ready_event| was signaled. I don't follow - WAIT_OBJECT_0 == ready_event? https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:201: base::Bind(&NotificationCallback))); On 2015/03/11 13:20:39, gab wrote: > nit: + single space (suggest running "git cl format") Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:208: base::Process& browser_victim() { return browser_victim_; } On 2015/03/11 13:20:40, gab wrote: > Use a * (non-const & are not-allowed by style guide) Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:214: bool MockDisplayKillMessageBox(bool result) { On 2015/03/11 13:20:40, gab wrote: > s/result/allow_kill ? (I had to piece things around to figure out that's what > this meant, seems a better name would help readability?) Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:253: EXPECT_EQ(exit_code, content::RESULT_CODE_HUNG); On 2015/03/11 13:20:39, gab wrote: > Add a comment stating that RESULT_CODE_HUNG is expected to be the exit code on > the hung process we just killed (i.e. I had to code search to figure out how > this exit code got set, ideally I read a quick comment here and trust your words > without having to search around). Done. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:292: } On 2015/03/11 13:20:39, gab wrote: > Should we also add a test that the normal use case for process singleton (i.e. > successful rendez-vous) works? There are other tests for that, which test the platform-independent success path.
A few more comments. Thanks, Gab https://codereview.chromium.org/981223004/diff/100001/chrome/browser/chrome_p... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/981223004/diff/100001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.cc:167: return old_timeout; On 2015/03/11 14:32:36, Sigurður Ásgeirsson wrote: > On 2015/03/11 13:20:39, gab wrote: > > <investigate> > > ??? Oops that was a note to self to figure out why we need to return the old value (forgot to remove it). https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:132: old_notification_timeout_); On 2015/03/11 14:32:37, Sigurður Ásgeirsson wrote: > On 2015/03/11 13:20:40, gab wrote: > > Why bother restoring it? Other unit tests shouldn't get involved with > > ProcessSingleton anyways (besides other ProcessSingleton tests in this fixture > > which may run in parallel in the same process, but they all desire the 1s > > timeout so wtv). > > > > Not doing so would simplify the setters' interface too which IMO is atypical > > (setters rarely return the old value) > > There are apparently cases where multiple tests run inside the same process. > I've had DrMemory FYI waterfall test failures occur in unrelated tests due to > coupling through histograms as a case in point. > > IMHO a global is a very bad idea(TM), but if you must have one, leave it as you > found it. Right, tests do run in parallel. But by setting the global back as you found it you only decrease the likelihood of tripping something that would depend on that, but if whatever barfs when this is set to 1s happens to run in parallel with your test it will still barf... So might as well never set it back IMO (making potential conflicts more deterministic). https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:179: ASSERT_EQ(result, WAIT_OBJECT_0); On 2015/03/11 14:32:37, Sigurður Ásgeirsson wrote: > On 2015/03/11 13:20:39, gab wrote: > > Assert that |ready_event| was signaled? Other pieces below (i.e. line 197) > seem > > to assume that if this returns with no fatal failure it's because the > > |ready_event| was signaled. > > I don't follow - WAIT_OBJECT_0 == ready_event? Oh actually nvm, I just realized that this is already what this line was doing (had forgot the WAIT_OBJECT_0 + n semantic (where n == 0 here)). Maybe add a comment like: // The wait should always return because |ready_event| was signaled or |browser_victim_| died unexpectedly. https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:292: } On 2015/03/11 14:32:37, Sigurður Ásgeirsson wrote: > On 2015/03/11 13:20:39, gab wrote: > > Should we also add a test that the normal use case for process singleton (i.e. > > successful rendez-vous) works? > > There are other tests for that, which test the platform-independent success > path. I see, maybe stress that in a comment on line 116 (test fixture class definition) to express the exact purpose of these tests. https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... chrome/browser/process_singleton.h:99: void OverrideKillMessageBoxCallbackForTesting( Update method name to match new callback name. https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:40: // notification was successfully handled. Add a NOTREACHED() to confirm that this is never called? (typically in tests we would use ADD_FAILURE() instead of NOTREACHED() but this is also referred form the child process which IIUC doesn't handle gtest stuff properly) https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:206: bool MockDisplayKillMessageBox(bool allow_kill) { Update method name to match new callback name. https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:240: EXPECT_FALSE(msg_box_called()); Update msg_box_called() naming (and above comment) to reflect new name.
On Wed, Mar 11, 2015 at 11:19 AM <gab@chromium.org> wrote: > A few more comments. > > Thanks, > Gab > > > https://codereview.chromium.org/981223004/diff/100001/ > chrome/browser/chrome_process_finder_win.cc > File chrome/browser/chrome_process_finder_win.cc (right): > > https://codereview.chromium.org/981223004/diff/100001/ > chrome/browser/chrome_process_finder_win.cc#newcode167 > chrome/browser/chrome_process_finder_win.cc:167 > <https://codereview.chromium.org/981223004/diff/100001/chrome/browser/chrome_p...>: > return old_timeout; > On 2015/03/11 14:32:36, Sigurður Ásgeirsson wrote: > > On 2015/03/11 13:20:39, gab wrote: > > > <investigate> > > > ??? > > Oops that was a note to self to figure out why we need to return the old > value (forgot to remove it). > > https://codereview.chromium.org/981223004/diff/100001/ > chrome/browser/process_singleton_win_unittest.cc > File chrome/browser/process_singleton_win_unittest.cc (right): > > https://codereview.chromium.org/981223004/diff/100001/ > chrome/browser/process_singleton_win_unittest.cc#newcode132 > chrome/browser/process_singleton_win_unittest.cc:132 > <https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_...> > : > old_notification_timeout_); > On 2015/03/11 14:32:37, Sigurður Ásgeirsson wrote: > > On 2015/03/11 13:20:40, gab wrote: > > > Why bother restoring it? Other unit tests shouldn't get involved > with > > > ProcessSingleton anyways (besides other ProcessSingleton tests in > this fixture > > > which may run in parallel in the same process, but they all desire > the 1s > > > timeout so wtv). > > > > > > Not doing so would simplify the setters' interface too which IMO is > atypical > > > (setters rarely return the old value) > > > There are apparently cases where multiple tests run inside the same > process. > > I've had DrMemory FYI waterfall test failures occur in unrelated tests > due to > > coupling through histograms as a case in point. > > > IMHO a global is a very bad idea(TM), but if you must have one, leave > it as you > > found it. > > Right, tests do run in parallel. I think you will find that under Dr Memory tests do not run in parallel, but rather run back-to-back from the same process instance. The reason being that Dr Memory has ungodly overhead per process launch. That being said, this is your call - changed as you request. > But by setting the global back as you > found it you only decrease the likelihood of tripping something that > would depend on that, but if whatever barfs when this is set to 1s > happens to run in parallel with your test it will still barf... So might > as well never set it back IMO (making potential conflicts more > deterministic). > > https://codereview.chromium.org/981223004/diff/100001/ > chrome/browser/process_singleton_win_unittest.cc#newcode179 > chrome/browser/process_singleton_win_unittest.cc:179 > <https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_...>: > ASSERT_EQ(result, > WAIT_OBJECT_0); > On 2015/03/11 14:32:37, Sigurður Ásgeirsson wrote: > > On 2015/03/11 13:20:39, gab wrote: > > > Assert that |ready_event| was signaled? Other pieces below (i.e. > line 197) > > seem > > > to assume that if this returns with no fatal failure it's because > the > > > |ready_event| was signaled. > > > I don't follow - WAIT_OBJECT_0 == ready_event? > > Oh actually nvm, I just realized that this is already what this line was > doing (had forgot the WAIT_OBJECT_0 + n semantic (where n == 0 here)). > > Maybe add a comment like: > // The wait should always return because |ready_event| was signaled or > |browser_victim_| died unexpectedly. > > https://codereview.chromium.org/981223004/diff/100001/ > chrome/browser/process_singleton_win_unittest.cc#newcode292 > chrome/browser/process_singleton_win_unittest.cc:292 > <https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_...>: > } > On 2015/03/11 14:32:37, Sigurður Ásgeirsson wrote: > > On 2015/03/11 13:20:39, gab wrote: > > > Should we also add a test that the normal use case for process > singleton (i.e. > > > successful rendez-vous) works? > > > There are other tests for that, which test the platform-independent > success > > path. > > I see, maybe stress that in a comment on line 116 (test fixture class > definition) to express the exact purpose of these tests. > > https://codereview.chromium.org/981223004/diff/120001/ > chrome/browser/process_singleton.h > File chrome/browser/process_singleton.h (right): > > https://codereview.chromium.org/981223004/diff/120001/ > chrome/browser/process_singleton.h#newcode99 > chrome/browser/process_singleton.h:99 > <https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_...>: > void > OverrideKillMessageBoxCallbackForTesting( > Update method name to match new callback name. > > https://codereview.chromium.org/981223004/diff/120001/ > chrome/browser/process_singleton_win_unittest.cc > File chrome/browser/process_singleton_win_unittest.cc (right): > > https://codereview.chromium.org/981223004/diff/120001/ > chrome/browser/process_singleton_win_unittest.cc#newcode40 > chrome/browser/process_singleton_win_unittest.cc:40 > <https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_...>: > // notification was > successfully handled. > Add a NOTREACHED() to confirm that this is never called? > > (typically in tests we would use ADD_FAILURE() instead of NOTREACHED() > but this is also referred form the child process which IIUC doesn't > handle gtest stuff properly) > > https://codereview.chromium.org/981223004/diff/120001/ > chrome/browser/process_singleton_win_unittest.cc#newcode206 > chrome/browser/process_singleton_win_unittest.cc:206 > <https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_...>: > bool > MockDisplayKillMessageBox(bool allow_kill) { > Update method name to match new callback name. > > https://codereview.chromium.org/981223004/diff/120001/ > chrome/browser/process_singleton_win_unittest.cc#newcode240 > chrome/browser/process_singleton_win_unittest.cc:240 > <https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_...> > : > EXPECT_FALSE(msg_box_called()); > Update msg_box_called() naming (and above comment) to reflect new name. > > https://codereview.chromium.org/981223004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Tests should leave the state the way they found it. They do not run in parallel in a single process, but may run in series. It is an error to not leave the state the way it was found. On Wed, Mar 11, 2015 at 11:44 AM, Sigurður Ásgeirsson <siggi@chromium.org> wrote: > On Wed, Mar 11, 2015 at 11:19 AM <gab@chromium.org> wrote: > >> A few more comments. >> >> Thanks, >> Gab >> >> >> https://codereview.chromium.org/981223004/diff/100001/ >> chrome/browser/chrome_process_finder_win.cc >> File chrome/browser/chrome_process_finder_win.cc (right): >> >> https://codereview.chromium.org/981223004/diff/100001/ >> chrome/browser/chrome_process_finder_win.cc#newcode167 >> chrome/browser/chrome_process_finder_win.cc:167 >> <https://codereview.chromium.org/981223004/diff/100001/chrome/browser/chrome_p...>: >> return old_timeout; >> On 2015/03/11 14:32:36, Sigurður Ásgeirsson wrote: >> > On 2015/03/11 13:20:39, gab wrote: >> > > <investigate> >> >> > ??? >> >> Oops that was a note to self to figure out why we need to return the old >> value (forgot to remove it). >> >> https://codereview.chromium.org/981223004/diff/100001/ >> chrome/browser/process_singleton_win_unittest.cc >> File chrome/browser/process_singleton_win_unittest.cc (right): >> >> https://codereview.chromium.org/981223004/diff/100001/ >> chrome/browser/process_singleton_win_unittest.cc#newcode132 >> chrome/browser/process_singleton_win_unittest.cc:132 >> <https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_...> >> : >> old_notification_timeout_); >> On 2015/03/11 14:32:37, Sigurður Ásgeirsson wrote: >> > On 2015/03/11 13:20:40, gab wrote: >> > > Why bother restoring it? Other unit tests shouldn't get involved >> with >> > > ProcessSingleton anyways (besides other ProcessSingleton tests in >> this fixture >> > > which may run in parallel in the same process, but they all desire >> the 1s >> > > timeout so wtv). >> > > >> > > Not doing so would simplify the setters' interface too which IMO is >> atypical >> > > (setters rarely return the old value) >> >> > There are apparently cases where multiple tests run inside the same >> process. >> > I've had DrMemory FYI waterfall test failures occur in unrelated tests >> due to >> > coupling through histograms as a case in point. >> >> > IMHO a global is a very bad idea(TM), but if you must have one, leave >> it as you >> > found it. >> >> Right, tests do run in parallel. > > > I think you will find that under Dr Memory tests do not run in parallel, > but rather run back-to-back from the same process instance. The reason > being that Dr Memory has ungodly overhead per process launch. > > That being said, this is your call - changed as you request. > > >> But by setting the global back as you >> found it you only decrease the likelihood of tripping something that >> would depend on that, but if whatever barfs when this is set to 1s >> happens to run in parallel with your test it will still barf... So might >> as well never set it back IMO (making potential conflicts more >> deterministic). >> >> https://codereview.chromium.org/981223004/diff/100001/ >> chrome/browser/process_singleton_win_unittest.cc#newcode179 >> chrome/browser/process_singleton_win_unittest.cc:179 >> <https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_...>: >> ASSERT_EQ(result, >> WAIT_OBJECT_0); >> On 2015/03/11 14:32:37, Sigurður Ásgeirsson wrote: >> > On 2015/03/11 13:20:39, gab wrote: >> > > Assert that |ready_event| was signaled? Other pieces below (i.e. >> line 197) >> > seem >> > > to assume that if this returns with no fatal failure it's because >> the >> > > |ready_event| was signaled. >> >> > I don't follow - WAIT_OBJECT_0 == ready_event? >> >> Oh actually nvm, I just realized that this is already what this line was >> doing (had forgot the WAIT_OBJECT_0 + n semantic (where n == 0 here)). >> >> Maybe add a comment like: >> // The wait should always return because |ready_event| was signaled or >> |browser_victim_| died unexpectedly. >> >> https://codereview.chromium.org/981223004/diff/100001/ >> chrome/browser/process_singleton_win_unittest.cc#newcode292 >> chrome/browser/process_singleton_win_unittest.cc:292 >> <https://codereview.chromium.org/981223004/diff/100001/chrome/browser/process_...>: >> } >> On 2015/03/11 14:32:37, Sigurður Ásgeirsson wrote: >> > On 2015/03/11 13:20:39, gab wrote: >> > > Should we also add a test that the normal use case for process >> singleton (i.e. >> > > successful rendez-vous) works? >> >> > There are other tests for that, which test the platform-independent >> success >> > path. >> >> I see, maybe stress that in a comment on line 116 (test fixture class >> definition) to express the exact purpose of these tests. >> >> https://codereview.chromium.org/981223004/diff/120001/ >> chrome/browser/process_singleton.h >> File chrome/browser/process_singleton.h (right): >> >> https://codereview.chromium.org/981223004/diff/120001/ >> chrome/browser/process_singleton.h#newcode99 >> chrome/browser/process_singleton.h:99 >> <https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_...>: >> void >> OverrideKillMessageBoxCallbackForTesting( >> Update method name to match new callback name. >> >> https://codereview.chromium.org/981223004/diff/120001/ >> chrome/browser/process_singleton_win_unittest.cc >> File chrome/browser/process_singleton_win_unittest.cc (right): >> >> https://codereview.chromium.org/981223004/diff/120001/ >> chrome/browser/process_singleton_win_unittest.cc#newcode40 >> chrome/browser/process_singleton_win_unittest.cc:40 >> <https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_...>: >> // notification was >> successfully handled. >> Add a NOTREACHED() to confirm that this is never called? >> >> (typically in tests we would use ADD_FAILURE() instead of NOTREACHED() >> but this is also referred form the child process which IIUC doesn't >> handle gtest stuff properly) >> >> https://codereview.chromium.org/981223004/diff/120001/ >> chrome/browser/process_singleton_win_unittest.cc#newcode206 >> chrome/browser/process_singleton_win_unittest.cc:206 >> <https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_...>: >> bool >> MockDisplayKillMessageBox(bool allow_kill) { >> Update method name to match new callback name. >> >> https://codereview.chromium.org/981223004/diff/120001/ >> chrome/browser/process_singleton_win_unittest.cc#newcode240 >> chrome/browser/process_singleton_win_unittest.cc:240 >> <https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_...> >> : >> EXPECT_FALSE(msg_box_called()); >> Update msg_box_called() naming (and above comment) to reflect new name. >> >> https://codereview.chromium.org/981223004/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/03/11 15:50:20, erikwright wrote: > Tests should leave the state the way they found it. > > They do not run in parallel in a single process, but may run in series. > > It is an error to not leave the state the way it was found. Ah right, they may be run in series within one process, the parallelization is done via multiple processes, not threads. Okay, let's recover the old state there (although I think this is overkill in this case, it's cleaner practice).
Patchset #9 (id:160001) has been deleted
'k - all comments addressed. https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... chrome/browser/process_singleton.h:99: void OverrideKillMessageBoxCallbackForTesting( On 2015/03/11 15:19:43, gab wrote: > Update method name to match new callback name. Done. https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:40: // notification was successfully handled. On 2015/03/11 15:19:43, gab wrote: > Add a NOTREACHED() to confirm that this is never called? > > (typically in tests we would use ADD_FAILURE() instead of NOTREACHED() but this > is also referred form the child process which IIUC doesn't handle gtest stuff > properly) Done. https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:206: bool MockDisplayKillMessageBox(bool allow_kill) { On 2015/03/11 15:19:43, gab wrote: > Update method name to match new callback name. Done. https://codereview.chromium.org/981223004/diff/120001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:240: EXPECT_FALSE(msg_box_called()); On 2015/03/11 15:19:43, gab wrote: > Update msg_box_called() naming (and above comment) to reflect new name. Done.
Woot, lgtm++, this is awesome :-)!
siggi@chromium.org changed reviewers: + sky@chromium.org
Scott - can I ask you for owners approval, as it appears Nico's OOO?
https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.cc:31: const int kDefaultTimeoutInSeconds = 20; There is no point in this constant now. Initialize g_timeout_in_seconds with 20 and nuke kDefault. https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.cc:33: int g_timeout_in_seconds = kDefaultTimeoutInSeconds; This isn't global, it's file local. It should be timeout_in_seconds. https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... File chrome/browser/chrome_process_finder_win.h (right): https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.h:33: int SetNotificationTimeoutInSecondsForTesting(int new_timeout); Make this take a TimeDelta so that units are clearer and its less error prone. https://codereview.chromium.org/981223004/diff/180001/chrome/browser/process_... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/180001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:64: HWND window = ::CreateWindow(reinterpret_cast<LPCWSTR>(clazz), 0, WS_POPUP, 0, Where is the window destroyed? Alos, why do you need to create a window? https://codereview.chromium.org/981223004/diff/180001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:180: ASSERT_EQ(result, WAIT_OBJECT_0); Isn't WAIT_OBJECT_0 the expected value? In general test assertions are expected, actual. I think you want: ASSERT_EQ(WAIT_OBJECT_0, result). You have this problem in other places in this file.
Patchset #10 (id:200001) has been deleted
Thanks, please take another look? Note that this is the companion test for the fix here <https://codereview.chromium.org/982323002>. The regression this fixes is ~20 months old. https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.cc:31: const int kDefaultTimeoutInSeconds = 20; On 2015/03/11 19:15:51, sky wrote: > There is no point in this constant now. Initialize g_timeout_in_seconds with 20 > and nuke kDefault. Done. https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.cc:33: int g_timeout_in_seconds = kDefaultTimeoutInSeconds; On 2015/03/11 19:15:51, sky wrote: > This isn't global, it's file local. It should be timeout_in_seconds. Done. https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... File chrome/browser/chrome_process_finder_win.h (right): https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.h:33: int SetNotificationTimeoutInSecondsForTesting(int new_timeout); On 2015/03/11 19:15:51, sky wrote: > Make this take a TimeDelta so that units are clearer and its less error prone. Done. https://codereview.chromium.org/981223004/diff/180001/chrome/browser/process_... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/180001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:64: HWND window = ::CreateWindow(reinterpret_cast<LPCWSTR>(clazz), 0, WS_POPUP, 0, On 2015/03/11 19:15:51, sky wrote: > Where is the window destroyed? > Alos, why do you need to create a window? This is testing specific behavior of the ProcessSingleton that's sensitive to whether or not a hung browser has a visible window. This window dies with the test process, I've added a comment to this effect, or I can explicitly destroy it if you think that's an improvement? Note that this is used only inside a short-lived multiprocess test main function. https://codereview.chromium.org/981223004/diff/180001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:180: ASSERT_EQ(result, WAIT_OBJECT_0); On 2015/03/11 19:15:51, sky wrote: > Isn't WAIT_OBJECT_0 the expected value? In general test assertions are expected, > actual. I think you want: > ASSERT_EQ(WAIT_OBJECT_0, result). > You have this problem in other places in this file. Yes, I've been using the macros consistently this way after being scolded for the reverse in a code review. Fixed - this is the way the macros are intended IMHO, as base gtest issues better failure messages this way.
https://codereview.chromium.org/981223004/diff/180001/chrome/browser/process_... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/180001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:64: HWND window = ::CreateWindow(reinterpret_cast<LPCWSTR>(clazz), 0, WS_POPUP, 0, On 2015/03/11 20:15:31, Sigurður Ásgeirsson wrote: > On 2015/03/11 19:15:51, sky wrote: > > Where is the window destroyed? > > Alos, why do you need to create a window? > > This is testing specific behavior of the ProcessSingleton that's sensitive to > whether or not a hung browser has a visible window. How about a good comment then? The one on 62 doesn't really describe why this is necessary. > This window dies with the test process, I've added a comment to this effect, or > I can explicitly destroy it if you think that's an improvement? Note that this > is used only inside a short-lived multiprocess test main function. Please clean up. Your argument in the extreme means we shouldn't bother cleaning anything up in tests, which is obviously bad practice.
https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.cc:33: int g_timeout_in_seconds = kDefaultTimeoutInSeconds; On 2015/03/11 19:15:51, sky wrote: > This isn't global, it's file local. It should be timeout_in_seconds. Interesting, I've seen g_ prefixes used for file local variables in chrome before (i.e. 153 instances in anonymous namespaces in chrome/ alone [1]) FWIW, I like using g_ as it's a reading hint between a local variable (i.e. local scope/on the stack) and a program-wide file local/global variable which helps the reader/reviewer think of things like thread-safety, etc. [1] https://code.google.com/p/chromium/codesearch#search/&q=file:chrome/*%20pcre:... Lastly, FWIW, the style-guide isn't very specific here beyond suggesting g_ for globals: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names
https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... File chrome/browser/chrome_process_finder_win.cc (right): https://codereview.chromium.org/981223004/diff/180001/chrome/browser/chrome_p... chrome/browser/chrome_process_finder_win.cc:33: int g_timeout_in_seconds = kDefaultTimeoutInSeconds; On 2015/03/11 20:25:35, gab wrote: > On 2015/03/11 19:15:51, sky wrote: > > This isn't global, it's file local. It should be timeout_in_seconds. > > Interesting, I've seen g_ prefixes used for file local variables in chrome > before (i.e. 153 instances in anonymous namespaces in chrome/ alone [1]) > > FWIW, I like using g_ as it's a reading hint between a local variable (i.e. > local scope/on the stack) and a program-wide file local/global variable which > helps the reader/reviewer think of things like thread-safety, etc. > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=file:chrome/*%20pcre:... > > Lastly, FWIW, the style-guide isn't very specific here beyond suggesting g_ for > globals: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names I agree that g_foo is more readable, but it's my take that this isn't a global so g_ isn't appropriate. You can ask for clarification on the style-guide alias or chromium-dev.
Cleanup enacted, comments improved (I hope) - another look?
LGTM https://codereview.chromium.org/981223004/diff/230001/chrome/browser/process_... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/230001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:91: }; DISALLOW_COPY_AND_ASSIGN.
chrome/ lgtm. Thanks much for getting back to this :-)
Thanks, committing. https://codereview.chromium.org/981223004/diff/230001/chrome/browser/process_... File chrome/browser/process_singleton_win_unittest.cc (right): https://codereview.chromium.org/981223004/diff/230001/chrome/browser/process_... chrome/browser/process_singleton_win_unittest.cc:91: }; On 2015/03/11 21:55:35, sky wrote: > DISALLOW_COPY_AND_ASSIGN. Done.
The CQ bit was checked by siggi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikwright@chromium.org, gab@chromium.org, thakis@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/981223004/#ps250001 (title: "Address Scott's remaining comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981223004/250001
Message was sent while issue was closed.
Committed patchset #12 (id:250001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/2a0214bf578d1ccc77aa216fe36acfe865499bec Cr-Commit-Position: refs/heads/master@{#320277} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
