Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1538923004:
[win] Change Chrome Watcher configuration API. (Closed)
Description[win] Change Chrome Watcher configuration API.
This moves away from using bound functions as generators/interpreters, to a pair of classes with setters and getters. This will make it significantly easier to plumb in new parameters in the future. A further CL will revamp the existing code to use this new mechanism.
BUG=576299
TBR=grt@chromium.org
Committed: https://crrev.com/5a17ab25f51d52db128dae16a8d3efee441113b8
Cr-Commit-Position: refs/heads/master@{#368609}
Patch Set 1 #Patch Set 2 : Added unittests. #
Total comments: 14
Patch Set 3 : Address comments on patchset 2. #Patch Set 4 : Cleanup. #
Total comments: 18
Patch Set 5 : Address comments from patchset 4. #Patch Set 6 : Fix unittests. #Patch Set 7 : Rebased. #
Total comments: 14
Patch Set 8 : Address siggi's comments. #
Total comments: 2
Patch Set 9 : Fix outdated comment. #
Messages
Total messages: 50 (19 generated)
Description was changed from ========== [win] Change how Chrome Watcher configuration API. This moves away from using bound functions as generators/interpreters, to a pair of classes with setters and getters. This will make it significantly easier to plumb in new parameters in the future. A further CL will revamp the existing code to use this new mechanism. BUG= ========== to ========== [win] Change Chrome Watcher configuration API. This moves away from using bound functions as generators/interpreters, to a pair of classes with setters and getters. This will make it significantly easier to plumb in new parameters in the future. A further CL will revamp the existing code to use this new mechanism. BUG= ==========
chrisha@chromium.org changed reviewers: + grt@chromium.org, siggi@chromium.org
A first look at the API to see if we're on the same page? Once that's ironed out I'll worry about implementation and unittests.
Now with unittests. PTAL?
looks pretty good https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_unittest_win.cc (right): https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_unittest_win.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: this file should be called chrome_watcher_command_line_win_unittest.cc https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_win.cc (right): https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.cc:99: if (on_initialized_event_handle_.Get() || parent_process_handle_.Get()) nit: use IsValid() rather than .Get() https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.cc:100: CHECK(false); can you make a death test (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...) for this? https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.cc:114: return TakeHandle(&on_initialized_event_handle_, on_initialized_event_handle); can this be: return std::move(on_initialized_event_handle_); ? https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_win.h (right): https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.h:41: void GetInheritedHandles(std::vector<HANDLE>* inherited_handles) const; #include <vector> https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.h:75: bool InterpretCommandLine(const base::CommandLine& command_line); wdyt of: class ChromeWatcherCommandLine { public: static scoped_ptr<ChromeWatcherCommandLine> Interpret(const base::CommandLine& command_line); ... }; then the getters don't need to be documented with "Only valid after..." ? https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.h:81: bool TakeOnInitializedEventHandle( can these return the ScopedHandle (which appears to be a movable type)?
Another look? https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_unittest_win.cc (right): https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_unittest_win.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2015/12/21 20:19:06, grt wrote: > nit: this file should be called chrome_watcher_command_line_win_unittest.cc Doh! Done. https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_win.cc (right): https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.cc:99: if (on_initialized_event_handle_.Get() || parent_process_handle_.Get()) On 2015/12/21 20:19:06, grt wrote: > nit: use IsValid() rather than .Get() Done. https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.cc:100: CHECK(false); On 2015/12/21 20:19:06, grt wrote: > can you make a death test > (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...) > for this? Done. https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.cc:114: return TakeHandle(&on_initialized_event_handle_, on_initialized_event_handle); On 2015/12/21 20:19:06, grt wrote: > can this be: > return std::move(on_initialized_event_handle_); > ? Changed to using move semantics and returning a raw ScopedHandle. https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_win.h (right): https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.h:41: void GetInheritedHandles(std::vector<HANDLE>* inherited_handles) const; On 2015/12/21 20:19:07, grt wrote: > #include <vector> Done. https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.h:75: bool InterpretCommandLine(const base::CommandLine& command_line); On 2015/12/21 20:19:07, grt wrote: > wdyt of: > class ChromeWatcherCommandLine { > public: > static scoped_ptr<ChromeWatcherCommandLine> Interpret(const base::CommandLine& > command_line); > ... > }; > then the getters don't need to be documented with "Only valid after..." > ? Done. https://codereview.chromium.org/1538923004/diff/20001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.h:81: bool TakeOnInitializedEventHandle( On 2015/12/21 20:19:07, grt wrote: > can these return the ScopedHandle (which appears to be a movable type)? Yup, they sure can. Done.
The CQ bit was checked by chrisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538923004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538923004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_win.cc (right): https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.cc:106: if (on_initialized_event_handle_.IsValid() || is this the same? CHECK(!on_initialized_event_handle_.IsValid() && !parent_process_handle_.IsValid()); https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_win.h (right): https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.h:81: base::win::ScopedHandle& on_initialized_event_handle() { i like base::win::ScopedHandle TakeOnInitializedEventHandler() { return std::move(on_initialized_event_handle_); } better since you can't accidentally grab the ref to the ScopedHandle then destroy this instance, leading to badness. https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_win_unittest.cc (right): https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:36: base::FilePath(L"example.exe")); nit: FILE_PATH_LITERAL("example.exe") here and elsewhere https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:40: // Expect there to be two inherited handles created by the generator. two -> no? https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:52: EXPECT_FALSE(interpreted.get()); scoped_ptr can be used directly in boolean expressions, so this can be: EXPECT_FALSE(interpreted); please update lines 86 and 130 accordingly https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:56: base::win::ScopedHandle process(::OpenProcess( can you use base::Process for this and other process handles? base::Process process(base::Process::OpenWithAccess(base::GetCurrentProcId(), PROCESS_QUERY_INFORMATION | SYNCHRONIZE)); https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:63: ASSERT_TRUE(event.Get()); use IsValid() rather than Get() if these assertions are intended to check the validity of a ScopedHandle https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:74: EXPECT_EQ(2u, handles.size()); nit: 2U here and line 118 https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:136: on_init.Set(interpreted->on_initialized_event_handle().Take()); with moving, this can be: on_init = interpreted->TakeOnInitializedEventHandle();
Alright, all cleaned up. Another look? (Also, time for some new owners in here, given that cpu@ is off Chrome and you're the only other reviewer!) https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_win.cc (right): https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.cc:106: if (on_initialized_event_handle_.IsValid() || On 2015/12/22 16:31:31, grt (no reviews until Jan 14) wrote: > is this the same? > CHECK(!on_initialized_event_handle_.IsValid() && > !parent_process_handle_.IsValid()); Indeed... I wanted to have an explicit log statement, but I didn't realize you could log to a CHECK. Rewritten. https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_win.h (right): https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win.h:81: base::win::ScopedHandle& on_initialized_event_handle() { On 2015/12/22 16:31:31, grt (no reviews until Jan 14) wrote: > i like base::win::ScopedHandle TakeOnInitializedEventHandler() { return > std::move(on_initialized_event_handle_); } better since you can't accidentally > grab the ref to the ScopedHandle then destroy this instance, leading to badness. Sounds reasonable. I'll return to that. https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... File chrome/app/chrome_watcher_command_line_win_unittest.cc (right): https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:36: base::FilePath(L"example.exe")); On 2015/12/22 16:31:31, grt (no reviews until Jan 14) wrote: > nit: FILE_PATH_LITERAL("example.exe") here and elsewhere Done. https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:40: // Expect there to be two inherited handles created by the generator. On 2015/12/22 16:31:32, grt (no reviews until Jan 14) wrote: > two -> no? Done. https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:52: EXPECT_FALSE(interpreted.get()); On 2015/12/22 16:31:31, grt (no reviews until Jan 14) wrote: > scoped_ptr can be used directly in boolean expressions, so this can be: > EXPECT_FALSE(interpreted); > please update lines 86 and 130 accordingly Done. https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:56: base::win::ScopedHandle process(::OpenProcess( On 2015/12/22 16:31:31, grt (no reviews until Jan 14) wrote: > can you use base::Process for this and other process handles? > base::Process process(base::Process::OpenWithAccess(base::GetCurrentProcId(), > PROCESS_QUERY_INFORMATION | SYNCHRONIZE)); Done. https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:63: ASSERT_TRUE(event.Get()); On 2015/12/22 16:31:32, grt (no reviews until Jan 14) wrote: > use IsValid() rather than Get() if these assertions are intended to check the > validity of a ScopedHandle Done. https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:74: EXPECT_EQ(2u, handles.size()); On 2015/12/22 16:31:32, grt (no reviews until Jan 14) wrote: > nit: 2U here and line 118 Done. https://codereview.chromium.org/1538923004/diff/60001/chrome/app/chrome_watch... chrome/app/chrome_watcher_command_line_win_unittest.cc:136: on_init.Set(interpreted->on_initialized_event_handle().Take()); On 2015/12/22 16:31:31, grt (no reviews until Jan 14) wrote: > with moving, this can be: > on_init = interpreted->TakeOnInitializedEventHandle(); Done.
The CQ bit was checked by chrisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538923004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538923004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chrisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538923004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538923004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chrisha@chromium.org to run a CQ dry run
siggi: Care to take a last look at this? I'll land it TBR until grt@ is back in town.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538923004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538923004/120001
like it, have a bunch-o-nits. https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... File chrome/app/chrome_watcher_command_line_win.h (right): https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win.h:32: // failure. This can fail if the call to DuplicateHandle fails. Each of this each of these https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win.h:39: base::CommandLine GenerateCommandLine(); Presumably this has to be called after all handles have been added :). https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win.h:49: // the provided |scoped_handle|. Also updated inherited_handles_. inherited_handles_ ? https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... File chrome/app/chrome_watcher_command_line_win_unittest.cc (right): https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win_unittest.cc:46: TestChromeWatcherCommandLineGenerator generator; do you need to use the TEst class here? https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win_unittest.cc:85: // Release the handles from the generator. Ownership will be picked up by the Nit: explain why this is - e.g. in the normal case these should run in different process instances. Maybe best to do this by commenting on teh Release* method. https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win_unittest.cc:107: base::Process process(base::Process::OpenWithAccess( This code repeats a fair bit - move to a fixture? https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win_unittest.cc:146: // Ownership of these handles is passed to the ScopedHandles below via This is a legacy test?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chrisha@chromium.org changed reviewers: + brettw@chromium.org
Thanks for your comments Siggi. Another look? Also, brettw: Care to look as owner of BUILD.gn? https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... File chrome/app/chrome_watcher_command_line_win.h (right): https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win.h:32: // failure. This can fail if the call to DuplicateHandle fails. Each of this On 2016/01/08 16:55:19, Sigurður Ásgeirsson wrote: > each of these Done. https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win.h:39: base::CommandLine GenerateCommandLine(); On 2016/01/08 16:55:19, Sigurður Ásgeirsson wrote: > Presumably this has to be called after all handles have been added :). Done. https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win.h:49: // the provided |scoped_handle|. Also updated inherited_handles_. On 2016/01/08 16:55:19, Sigurður Ásgeirsson wrote: > inherited_handles_ ? Made the comment more verbose. You need to manually specify the list of handles that you explicitly want the child to inherit during process launch, so this tracks duplicated handles explicitly in the vector |inherited_handles_|. The user can then access that list via GetInheritedHandles, for feeding into the process launcher. https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... File chrome/app/chrome_watcher_command_line_win_unittest.cc (right): https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win_unittest.cc:46: TestChromeWatcherCommandLineGenerator generator; On 2016/01/08 16:55:19, Sigurður Ásgeirsson wrote: > do you need to use the TEst class here? Not specifically in this test, but it just makes the code more consistent. https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win_unittest.cc:85: // Release the handles from the generator. Ownership will be picked up by the On 2016/01/08 16:55:19, Sigurður Ásgeirsson wrote: > Nit: explain why this is - e.g. in the normal case these should run in different > process instances. Maybe best to do this by commenting on teh Release* method. Done. https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win_unittest.cc:107: base::Process process(base::Process::OpenWithAccess( On 2016/01/08 16:55:19, Sigurður Ásgeirsson wrote: > This code repeats a fair bit - move to a fixture? Done. https://codereview.chromium.org/1538923004/diff/120001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win_unittest.cc:146: // Ownership of these handles is passed to the ScopedHandles below via On 2016/01/08 16:55:19, Sigurður Ásgeirsson wrote: > This is a legacy test? Yup... commented that fact. It will be removed once we *switch* to using the new classes. In the meantime, the code still uses the old mechanism.
The CQ bit was checked by chrisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538923004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538923004/140001
There's no special ownership or extra review required for the BUILD files. I'm only an owner of that by virtue of being in the root file. Since I already looked at this and you would need a chrome owner otherwise, LGTM for the BUILD and gypi files (didn't look at the rest).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1538923004/diff/140001/chrome/app/chrome_watc... File chrome/app/chrome_watcher_command_line_win.h (right): https://codereview.chromium.org/1538923004/diff/140001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win.h:51: // |inherited_handles_| that will be returned by |GetInheritedHandles|. no such member variable.
Other than the comment, lgtm
Yup, that comment was totally out of date. Care to take a look at the rest of the changes? https://codereview.chromium.org/1538923004/diff/140001/chrome/app/chrome_watc... File chrome/app/chrome_watcher_command_line_win.h (right): https://codereview.chromium.org/1538923004/diff/140001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win.h:51: // |inherited_handles_| that will be returned by |GetInheritedHandles|. On 2016/01/11 16:04:04, Sigurður Ásgeirsson wrote: > no such member variable. Silly me. It used to exist. Removed the comment.
Thanks, committing this TBR for Greg.
Description was changed from ========== [win] Change Chrome Watcher configuration API. This moves away from using bound functions as generators/interpreters, to a pair of classes with setters and getters. This will make it significantly easier to plumb in new parameters in the future. A further CL will revamp the existing code to use this new mechanism. BUG= ========== to ========== [win] Change Chrome Watcher configuration API. This moves away from using bound functions as generators/interpreters, to a pair of classes with setters and getters. This will make it significantly easier to plumb in new parameters in the future. A further CL will revamp the existing code to use this new mechanism. BUG=576299 TBR=grt@chromium.org ==========
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1538923004/#ps160001 (title: "Fix outdated comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538923004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538923004/160001
Message was sent while issue was closed.
Description was changed from ========== [win] Change Chrome Watcher configuration API. This moves away from using bound functions as generators/interpreters, to a pair of classes with setters and getters. This will make it significantly easier to plumb in new parameters in the future. A further CL will revamp the existing code to use this new mechanism. BUG=576299 TBR=grt@chromium.org ========== to ========== [win] Change Chrome Watcher configuration API. This moves away from using bound functions as generators/interpreters, to a pair of classes with setters and getters. This will make it significantly easier to plumb in new parameters in the future. A further CL will revamp the existing code to use this new mechanism. BUG=576299 TBR=grt@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [win] Change Chrome Watcher configuration API. This moves away from using bound functions as generators/interpreters, to a pair of classes with setters and getters. This will make it significantly easier to plumb in new parameters in the future. A further CL will revamp the existing code to use this new mechanism. BUG=576299 TBR=grt@chromium.org ========== to ========== [win] Change Chrome Watcher configuration API. This moves away from using bound functions as generators/interpreters, to a pair of classes with setters and getters. This will make it significantly easier to plumb in new parameters in the future. A further CL will revamp the existing code to use this new mechanism. BUG=576299 TBR=grt@chromium.org Committed: https://crrev.com/5a17ab25f51d52db128dae16a8d3efee441113b8 Cr-Commit-Position: refs/heads/master@{#368609} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/5a17ab25f51d52db128dae16a8d3efee441113b8 Cr-Commit-Position: refs/heads/master@{#368609}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
can you fix:
..\..\chrome/app/chrome_watcher_command_line_win.h(24,1) : error:
[chromium-style] Complex class/struct needs an explicit out-of-line destructor.
class ChromeWatcherCommandLineGenerator {
^
please?
Message was sent while issue was closed.
On 2016/01/11 18:39:02, Nico wrote:
> can you fix:
>
> ..\..\chrome/app/chrome_watcher_command_line_win.h(24,1) : error:
> [chromium-style] Complex class/struct needs an explicit out-of-line
destructor.
> class ChromeWatcherCommandLineGenerator {
> ^
>
> please?
Yup! CL on the way.
Curious why this isn't a presubmit or a CQ check?
Message was sent while issue was closed.
it's the clang/win bots; we don't have enough resources to have these as default-on trybots i'm told :-/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
