Browser watcher first installment.

Browser watcher first installment.

This CL carves out the component directory and the process watcher itself.
See https://codereview.chromium.org/717223002/ for the full change.

BUG=412384
Committed: https://crrev.com/c0d0a0e65cad9079d4c0878f1f3766aa59a9cf29
Cr-Commit-Position: refs/heads/master@{#304516}

[Sigurður Ásgeirsson, 2014-11-17 16:04:11]

siggi@chromium.org changed reviewers:
+ erikwright@chromium.org

[Sigurður Ásgeirsson, 2014-11-17 16:04:11]

PTAL!

[erikwright (departed), 2014-11-17 18:12:31]

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
File components/browser_watcher/watcher_win.cc (right):

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.cc:10: #include
"components/browser_watcher/watcher_win.h"
must be the first file included.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.cc:27:
process_(base::kNullProcessHandle),
I would assume this is unnecessary for this non-POD member.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.cc:36:
cmd_line.GetSwitchValueASCII("parent-handle");
this string represents part of the API of this class and should be exposed as a
public static constant.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.cc:70: base::win::RegKey
key(HKEY_CURRENT_USER,
I'm in the habit of factoring out the writing of state into little helper
classes or free functions. This gives a few advantages:

(1) I can quickly test the reading and writing code against itself without
having to trigger all of the logic that causes a given state to be written.
(2) It makes the business logic more readable. Instead of a bunch of
domain-specific code interrupted by 9 lines of registry access (and 7 more in
WaitForExit()), I can read a single "state_->WriteStillActive()" or similar.
(3) Your unit tests, which verify the functionality of this test by reading the
state it has written, currently also reproduce the registry access logic. They
could be simplified by using your data access helpers.

In summary, your current design will have four classes with registry access code
(client, server, and a unit-test for each). By factoring it out into symmetrical
helpers you can reduce this to two focused classes and leave your business logic
more readable.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.cc:77: LOG(ERROR) << "Unable to write to
registry, error " << result;
#include "base/logging.h"

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
File components/browser_watcher/watcher_win.h (right):

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.h:32: // hosting the instance of this
class.
Once you have exposed the flag as a public static constant update this comment
to refer to it.

Alternatively, create a symmetrical pair of free functions to parse and generate
the command lines that can be used in the client and server, and update this
class to take a HANDLE rather than parsing it out itself.

As things currently stand, this class cannot be used/tested without digging into
its implementation and duplicating a constant hidden there, which is
problematic.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
File components/browser_watcher/watcher_win_unittest.cc (right):

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win_unittest.cc:36:
process_(base::kNullProcessHandle), is_killed_(false) {
process_id_ should be initialized

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win_unittest.cc:47:
ASSERT_EQ(base::kNullProcessHandle, process_);
I'm personally opposed to using "ASSERT" in functions/methods besides the main
test body. The reasoning being that it is a hidden return, but doesn't actually
have the "expected" effect of terminating the test.

I would use EXPECT in such cases. I don't insist on it, though.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win_unittest.cc:205:
sleeper.set_process(base::kNullProcessHandle);
Maybe replace with a nullary "Release()" (or ReleaseHandle,
ReleaseProcessHandle, etc.?).

Also, this ought to go after line 194 (ParseArguments()) as a failure in the
asserts on 195 or 01 would cause double-closes.

Maybe Kill should be a free function taking a HANDLE, in that case.

[Sigurður Ásgeirsson, 2014-11-17 21:27:12]

Thanks - please take another look.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
File components/browser_watcher/watcher_win.cc (right):

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.cc:10: #include
"components/browser_watcher/watcher_win.h"
On 2014/11/17 18:12:30, erikwright wrote:
> must be the first file included.

Done.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.cc:27:
process_(base::kNullProcessHandle),
On 2014/11/17 18:12:30, erikwright wrote:
> I would assume this is unnecessary for this non-POD member.

Oops, leftover...

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.cc:36:
cmd_line.GetSwitchValueASCII("parent-handle");
On 2014/11/17 18:12:30, erikwright wrote:
> this string represents part of the API of this class and should be exposed as
a
> public static constant.

Done.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.cc:70: base::win::RegKey
key(HKEY_CURRENT_USER,
On 2014/11/17 18:12:30, erikwright wrote:
> I'm in the habit of factoring out the writing of state into little helper
> classes or free functions. This gives a few advantages:
> 
> (1) I can quickly test the reading and writing code against itself without
> having to trigger all of the logic that causes a given state to be written.
> (2) It makes the business logic more readable. Instead of a bunch of
> domain-specific code interrupted by 9 lines of registry access (and 7 more in
> WaitForExit()), I can read a single "state_->WriteStillActive()" or similar.
> (3) Your unit tests, which verify the functionality of this test by reading
the
> state it has written, currently also reproduce the registry access logic. They
> could be simplified by using your data access helpers.
> 
> In summary, your current design will have four classes with registry access
code
> (client, server, and a unit-test for each). By factoring it out into
symmetrical
> helpers you can reduce this to two focused classes and leave your business
logic
> more readable.

IMHO there isn't enough code here to warrant a whole lot of abstraction, that
being said, I've refactored the two writes here into a common function.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.cc:77: LOG(ERROR) << "Unable to write to
registry, error " << result;
On 2014/11/17 18:12:30, erikwright wrote:
> #include "base/logging.h"

Done.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
File components/browser_watcher/watcher_win.h (right):

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.h:32: // hosting the instance of this
class.
On 2014/11/17 18:12:30, erikwright wrote:
> Once you have exposed the flag as a public static constant update this comment
> to refer to it.
> 
> Alternatively, create a symmetrical pair of free functions to parse and
generate
> the command lines that can be used in the client and server, and update this
> class to take a HANDLE rather than parsing it out itself.
> 
> As things currently stand, this class cannot be used/tested without digging
into
> its implementation and duplicating a constant hidden there, which is
> problematic.

Done.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
File components/browser_watcher/watcher_win_unittest.cc (right):

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win_unittest.cc:36:
process_(base::kNullProcessHandle), is_killed_(false) {
On 2014/11/17 18:12:30, erikwright wrote:
> process_id_ should be initialized

Done.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win_unittest.cc:47:
ASSERT_EQ(base::kNullProcessHandle, process_);
On 2014/11/17 18:12:31, erikwright wrote:
> I'm personally opposed to using "ASSERT" in functions/methods besides the main
> test body. The reasoning being that it is a hidden return, but doesn't
actually
> have the "expected" effect of terminating the test.
> 
> I would use EXPECT in such cases. I don't insist on it, though.

Hmmm - there's an implicit return value in such test functions, see
ASSERT_NO_FATAL_FAILURE.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win_unittest.cc:205:
sleeper.set_process(base::kNullProcessHandle);
On 2014/11/17 18:12:30, erikwright wrote:
> Maybe replace with a nullary "Release()" (or ReleaseHandle,
> ReleaseProcessHandle, etc.?).
> 
> Also, this ought to go after line 194 (ParseArguments()) as a failure in the
> asserts on 195 or 01 would cause double-closes.
> 
> Maybe Kill should be a free function taking a HANDLE, in that case.

Went a slightly different way...

[erikwright (departed), 2014-11-17 22:07:04]

LGTM.

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
File components/browser_watcher/watcher_win.cc (right):

https://codereview.chromium.org/729363004/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_win.cc:70: base::win::RegKey
key(HKEY_CURRENT_USER,
On 2014/11/17 21:27:12, Sigurður Ásgeirsson wrote:
> On 2014/11/17 18:12:30, erikwright wrote:
> > I'm in the habit of factoring out the writing of state into little helper
> > classes or free functions. This gives a few advantages:
> > 
> > (1) I can quickly test the reading and writing code against itself without
> > having to trigger all of the logic that causes a given state to be written.
> > (2) It makes the business logic more readable. Instead of a bunch of
> > domain-specific code interrupted by 9 lines of registry access (and 7 more
in
> > WaitForExit()), I can read a single "state_->WriteStillActive()" or similar.
> > (3) Your unit tests, which verify the functionality of this test by reading
> the
> > state it has written, currently also reproduce the registry access logic.
They
> > could be simplified by using your data access helpers.
> > 
> > In summary, your current design will have four classes with registry access
> code
> > (client, server, and a unit-test for each). By factoring it out into
> symmetrical
> > helpers you can reduce this to two focused classes and leave your business
> logic
> > more readable.
> 
> IMHO there isn't enough code here to warrant a whole lot of abstraction, that
> being said, I've refactored the two writes here into a common function.

It's not abstraction (no indirection is introduced, no virtual methods, the
helper is not injected) but factoring out.

I don't mind not doing it for now, but I'd like to make sure that there is not
duplication (with risk of divergence) between the write code here and any code
that may also write the same data in the same place and same format from the
unit test of the corresponding client (and likewise the read code in the unit
test of this class and the implementation of the client).

Since the client and its unit test don't exist yet it's a moot point.

https://codereview.chromium.org/729363004/diff/40001/components/browser_watch...
File components/browser_watcher/watcher_win.cc (right):

https://codereview.chromium.org/729363004/diff/40001/components/browser_watch...
components/browser_watcher/watcher_win.cc:89: // Start by writing the value
STILL_ACTIVE to registry, to allow detection
This comment is a copy-paste artifact.

https://codereview.chromium.org/729363004/diff/40001/components/browser_watch...
File components/browser_watcher/watcher_win_unittest.cc (right):

https://codereview.chromium.org/729363004/diff/40001/components/browser_watch...
components/browser_watcher/watcher_win_unittest.cc:213:
EXPECT_TRUE(watcher.ParseArguments(cmd_line_));
If this returns false sleeper_handle is leaked. I don't really care that much as
this is a unit test. I leave it to your judgement whether tidying that up is
worth the extra conditionals.

[Sigurður Ásgeirsson, 2014-11-17 22:30:16]

Thanks!

https://codereview.chromium.org/729363004/diff/40001/components/browser_watch...
File components/browser_watcher/watcher_win.cc (right):

https://codereview.chromium.org/729363004/diff/40001/components/browser_watch...
components/browser_watcher/watcher_win.cc:89: // Start by writing the value
STILL_ACTIVE to registry, to allow detection
On 2014/11/17 22:07:04, erikwright wrote:
> This comment is a copy-paste artifact.

Ugh, so it is - thanks!

https://codereview.chromium.org/729363004/diff/40001/components/browser_watch...
File components/browser_watcher/watcher_win_unittest.cc (right):

https://codereview.chromium.org/729363004/diff/40001/components/browser_watch...
components/browser_watcher/watcher_win_unittest.cc:213:
EXPECT_TRUE(watcher.ParseArguments(cmd_line_));
On 2014/11/17 22:07:04, erikwright wrote:
> If this returns false sleeper_handle is leaked. I don't really care that much
as
> this is a unit test. I leave it to your judgement whether tidying that up is
> worth the extra conditionals.

Yeah, if the test fails, there's bigger problems - there's a comment to this
effect above.

[Sigurður Ásgeirsson, 2014-11-17 22:30:23]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2014-11-17 22:31:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/729363004/60001

[commit-bot: I haz the power, 2014-11-17 23:29:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-17 23:30:58]

Patchset 4 (id:??) landed as
https://crrev.com/c0d0a0e65cad9079d4c0878f1f3766aa59a9cf29
Cr-Commit-Position: refs/heads/master@{#304516}