Browser watcher end-end-to-end .

Not for submission.

BUG=

[erikwright (departed), 2014-11-14 19:46:08]

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

[erikwright (departed), 2014-11-14 19:46:10]

First high-level pass. I'll reserve detailed review for the individual CLs.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_client_win.h (right):

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_client_win.h:9: #include
"base/files/file_path.h"
not used

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_main_win.cc (right):

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_main_win.cc:53: static void CompileAsserts()
{
should this go in an anonymous namespace?

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_main_win.h (right):

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_main_win.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
I would rename this file to watcher_main_api_win or watcher_main_entrypoint_win
or similar. And create a corresponding .cc file that contains the constant
definitions.

Instead of having them live in some strange, somewhat unrelated file.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_metrics_provider_win.h (right):

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.h:8: #include
<windows.h>
not used

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.h:20: 
An explicit destructor should be supplied.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.h:21: virtual void
ProvideStabilityMetrics(
// metrics::MetricsProvider implementation

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.h:24: static const char
kBrowserExitCodeHistogramName[];
According to
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
constants go before methods (including constructor).

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.h:29:
DISALLOW_COPY_AND_ASSIGN(WatcherMetricsProviderWin);
IWYU base/macros.h

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_win.h (right):

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_win.h:7: #include <windows.h>
not used

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_win.h:26: explicit ExitCodeWatcher(const
wchar_t* registry_path);
It seems relatively unusual to pass around char buffer pointers in Chromium.
While technically better for performance, I would use a base::string16 for
clarity, consistency, and safety.

At a minimum, we never use wchar_t, only base::char16

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_win.h:27: 
an explicit out-of-line destructor is required.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_win.h:46: // Handle to the process we wait
for and it's creation time.
it's -> its

But this comment is unnerving in its almost, but not quite, complete description
of this list of members.

I would say, "Handle, ID, and creation time of the watched process.".

[Sigurður Ásgeirsson, 2014-11-17 15:21:48]

Patchset #5 (id:80001) has been deleted

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

Patchset #5 (id:100001) has been deleted

[Sigurður Ásgeirsson, 2014-11-17 15:22:35]

Patchset #6 (id:140001) has been deleted

[Sigurður Ásgeirsson, 2014-11-17 15:26:59]

Thanks - I'm breaking this up for proper review and landing now, please do not
comment further on this CL.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_client_win.h (right):

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_client_win.h:9: #include
"base/files/file_path.h"
On 2014/11/14 19:46:09, erikwright wrote:
> not used

Done.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_main_win.cc (right):

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_main_win.cc:53: static void CompileAsserts()
{
On 2014/11/14 19:46:09, erikwright wrote:
> should this go in an anonymous namespace?

Done.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_main_win.h (right):

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_main_win.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/11/14 19:46:09, erikwright wrote:
> I would rename this file to watcher_main_api_win or
watcher_main_entrypoint_win
> or similar. And create a corresponding .cc file that contains the constant
> definitions.
> 
> Instead of having them live in some strange, somewhat unrelated file.

Done.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_metrics_provider_win.h (right):

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.h:8: #include
<windows.h>
On 2014/11/14 19:46:10, erikwright wrote:
> not used

Done.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.h:20: 
On 2014/11/14 19:46:09, erikwright wrote:
> An explicit destructor should be supplied.

Sure, why?

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.h:21: virtual void
ProvideStabilityMetrics(
On 2014/11/14 19:46:10, erikwright wrote:
> // metrics::MetricsProvider implementation

Done.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.h:24: static const char
kBrowserExitCodeHistogramName[];
On 2014/11/14 19:46:10, erikwright wrote:
> According to
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
> constants go before methods (including constructor).

Done.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.h:29:
DISALLOW_COPY_AND_ASSIGN(WatcherMetricsProviderWin);
On 2014/11/14 19:46:10, erikwright wrote:
> IWYU base/macros.h

Done.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_win.h (right):

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_win.h:7: #include <windows.h>
On 2014/11/14 19:46:10, erikwright wrote:
> not used

Done.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_win.h:26: explicit ExitCodeWatcher(const
wchar_t* registry_path);
On 2014/11/14 19:46:10, erikwright wrote:
> It seems relatively unusual to pass around char buffer pointers in Chromium.
> While technically better for performance, I would use a base::string16 for
> clarity, consistency, and safety.
> 
> At a minimum, we never use wchar_t, only base::char16

Done.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_win.h:27: 
On 2014/11/14 19:46:10, erikwright wrote:
> an explicit out-of-line destructor is required.

Sure, why?

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_win.h:46: // Handle to the process we wait
for and it's creation time.
On 2014/11/14 19:46:10, erikwright wrote:
> it's -> its
> 
> But this comment is unnerving in its almost, but not quite, complete
description
> of this list of members.
> 
> I would say, "Handle, ID, and creation time of the watched process.".

:)

[erikwright (departed), 2014-11-17 15:35:17]

No new comments, but a response to your question.

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_win.h (right):

https://codereview.chromium.org/717223002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_win.h:27: 
On 2014/11/17 15:26:58, Sigurður Ásgeirsson wrote:
> On 2014/11/14 19:46:10, erikwright wrote:
> > an explicit out-of-line destructor is required.
> 
> Sure, why?

The default destructor will be inlined, which violates Chromium coding style and
is contra-indicated (though not explicitly banned) by Google style guide.

http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inline_Functions

There is a Clang rule that enforces this, but of course it won't run on your
Windows-only code.

The gist of the Clang rule is that it calculates a "complexity" score for the
destructor and prevents inlining if it is too high. Pretty much any non-POD
member pushes the complexity score over the threshold.

[Sigurður Ásgeirsson, 2014-11-17 15:37:59]

Patchset #6 (id:160001) has been deleted