Browser watcher part three.

Browser watcher part three.

Implementation of the metrics provider and its tests.
The metrics reporter reports the data captured by the watcher,
communicating through through registry.
See https://codereview.chromium.org/717223002/ for full change.

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

[Sigurður Ásgeirsson, 2014-11-18 20:45:11]

PTAL

[Sigurður Ásgeirsson, 2014-11-18 20:45:39]

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

[Sigurður Ásgeirsson, 2014-11-18 20:45:40]

PTAL

[erikwright (departed), 2014-11-18 21:13:36]

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

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:29: int pid = 0;
this CL and the other one I noticed you are using int to store PIDs. But it
seems there's a risk of overflow since GetProcessId (for example) returns DWORD.

http://msdn.microsoft.com/en-ca/library/windows/desktop/ms683215(v=vs.85).aspx

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:54: // TODO(siggi):
is this an acceptable name?
Seems reasonable to me.

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:68: // user account,
there's a small race that will double-reporti the exit codes
reporti -> report

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:77:
std::vector<base::string16> to_delete;
move inside the if(num!=0) block

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:77:
std::vector<base::string16> to_delete;
<vector>

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:83:
base::SparseHistogram::FactoryGet(kBrowserExitCodeHistogramName,
Is this not available using the macros?

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:94: // Tag the value
for deletion.
Obvious.

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

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:29: virtual
void TearDown() override {
Remove?

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:40: pid =
rand();
include whatever is required for rand()

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:44:
base::win::RegKey key(HKEY_CURRENT_USER, kRegistryPath, KEY_WRITE);
include for RegKey

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:47:
base::string16 key_name(base::StringPrintf(L"%d-%d", pid, rand()));
include string16

[Sigurður Ásgeirsson, 2014-11-18 21:52:37]

thanks - another look?

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

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:29: int pid = 0;
On 2014/11/18 21:13:36, erikwright wrote:
> this CL and the other one I noticed you are using int to store PIDs. But it
> seems there's a risk of overflow since GetProcessId (for example) returns
DWORD.
> 
> http://msdn.microsoft.com/en-ca/library/windows/desktop/ms683215(v=vs.85).aspx

Yeah, in practice the PID space is pretty small though. I've added a compile
assert that the type is at least large enough - are you suggesting some other
change?

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:54: // TODO(siggi):
is this an acceptable name?
On 2014/11/18 21:13:36, erikwright wrote:
> Seems reasonable to me.

Acknowledged.

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:68: // user account,
there's a small race that will double-reporti the exit codes
On 2014/11/18 21:13:36, erikwright wrote:
> reporti -> report

Done.

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:77:
std::vector<base::string16> to_delete;
On 2014/11/18 21:13:36, erikwright wrote:
> move inside the if(num!=0) block

Done.

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:77:
std::vector<base::string16> to_delete;
On 2014/11/18 21:13:36, erikwright wrote:
> move inside the if(num!=0) block

Done.

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:83:
base::SparseHistogram::FactoryGet(kBrowserExitCodeHistogramName,
On 2014/11/18 21:13:36, erikwright wrote:
> Is this not available using the macros?

This particular variant is not present in the macros. Alexei indicated just use
the API.

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win.cc:94: // Tag the value
for deletion.
On 2014/11/18 21:13:36, erikwright wrote:
> Obvious.

Done.

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

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:29: virtual
void TearDown() override {
On 2014/11/18 21:13:36, erikwright wrote:
> Remove?

Done.

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:40: pid =
rand();
On 2014/11/18 21:13:36, erikwright wrote:
> include whatever is required for rand()

Done.

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:44:
base::win::RegKey key(HKEY_CURRENT_USER, kRegistryPath, KEY_WRITE);
On 2014/11/18 21:13:36, erikwright wrote:
> include for RegKey

Done.

https://codereview.chromium.org/742523002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:47:
base::string16 key_name(base::StringPrintf(L"%d-%d", pid, rand()));
On 2014/11/18 21:13:36, erikwright wrote:
> include string16

Done.

[erikwright (departed), 2014-11-18 22:32:48]

LGTM
On 18 Nov 2014 16:52, <siggi@chromium.org> wrote:

> thanks - another look?
>
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win.cc
> File components/browser_watcher/watcher_metrics_provider_win.cc (right):
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win.cc#newcode29
> components/browser_watcher/watcher_metrics_provider_win.cc:29: int pid =
> 0;
> On 2014/11/18 21:13:36, erikwright wrote:
>
>> this CL and the other one I noticed you are using int to store PIDs.
>>
> But it
>
>> seems there's a risk of overflow since GetProcessId (for example)
>>
> returns DWORD.
>
>
> http://msdn.microsoft.com/en-ca/library/windows/desktop/
> ms683215(v=vs.85).aspx
>
> Yeah, in practice the PID space is pretty small though. I've added a
> compile assert that the type is at least large enough - are you
> suggesting some other change?
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win.cc#newcode54
> components/browser_watcher/watcher_metrics_provider_win.cc:54: //
> TODO(siggi): is this an acceptable name?
> On 2014/11/18 21:13:36, erikwright wrote:
>
>> Seems reasonable to me.
>>
>
> Acknowledged.
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win.cc#newcode68
> components/browser_watcher/watcher_metrics_provider_win.cc:68: // user
> account, there's a small race that will double-reporti the exit codes
> On 2014/11/18 21:13:36, erikwright wrote:
>
>> reporti -> report
>>
>
> Done.
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win.cc#newcode77
> components/browser_watcher/watcher_metrics_provider_win.cc:77:
> std::vector<base::string16> to_delete;
> On 2014/11/18 21:13:36, erikwright wrote:
>
>> move inside the if(num!=0) block
>>
>
> Done.
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win.cc#newcode77
> components/browser_watcher/watcher_metrics_provider_win.cc:77:
> std::vector<base::string16> to_delete;
> On 2014/11/18 21:13:36, erikwright wrote:
>
>> move inside the if(num!=0) block
>>
>
> Done.
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win.cc#newcode83
> components/browser_watcher/watcher_metrics_provider_win.cc:83:
> base::SparseHistogram::FactoryGet(kBrowserExitCodeHistogramName,
> On 2014/11/18 21:13:36, erikwright wrote:
>
>> Is this not available using the macros?
>>
>
> This particular variant is not present in the macros. Alexei indicated
> just use the API.
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win.cc#newcode94
> components/browser_watcher/watcher_metrics_provider_win.cc:94: // Tag
> the value for deletion.
> On 2014/11/18 21:13:36, erikwright wrote:
>
>> Obvious.
>>
>
> Done.
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win_unittest.cc
> File components/browser_watcher/watcher_metrics_provider_win_unittest.cc
> (right):
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win_
> unittest.cc#newcode29
> components/browser_watcher/watcher_metrics_provider_win_unittest.cc:29:
> virtual void TearDown() override {
> On 2014/11/18 21:13:36, erikwright wrote:
>
>> Remove?
>>
>
> Done.
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win_
> unittest.cc#newcode40
> components/browser_watcher/watcher_metrics_provider_win_unittest.cc:40:
> pid = rand();
> On 2014/11/18 21:13:36, erikwright wrote:
>
>> include whatever is required for rand()
>>
>
> Done.
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win_
> unittest.cc#newcode44
> components/browser_watcher/watcher_metrics_provider_win_unittest.cc:44:
> base::win::RegKey key(HKEY_CURRENT_USER, kRegistryPath, KEY_WRITE);
> On 2014/11/18 21:13:36, erikwright wrote:
>
>> include for RegKey
>>
>
> Done.
>
> https://codereview.chromium.org/742523002/diff/1/
> components/browser_watcher/watcher_metrics_provider_win_
> unittest.cc#newcode47
> components/browser_watcher/watcher_metrics_provider_win_unittest.cc:47:
> base::string16 key_name(base::StringPrintf(L"%d-%d", pid, rand()));
> On 2014/11/18 21:13:36, erikwright wrote:
>
>> include string16
>>
>
> Done.
>
> https://codereview.chromium.org/742523002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sigurður Ásgeirsson, 2014-11-18 23:17:33]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2014-11-18 23:20:02]

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

[commit-bot: I haz the power, 2014-11-18 23:20:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-18 23:20:06]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Sigurður Ásgeirsson, 2014-11-18 23:45:35]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2014-11-18 23:46:30]

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

[commit-bot: I haz the power, 2014-11-18 23:46:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-18 23:46:36]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[erikwright (departed), 2014-11-19 05:18:57]

On 2014/11/18 23:46:36, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

LGTM

[erikwright (departed), 2014-11-19 05:19:06]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-11-19 05:20:29]

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

[commit-bot: I haz the power, 2014-11-19 05:55:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-19 05:55:34]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

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

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2014-11-19 15:18:35]

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

[commit-bot: I haz the power, 2014-11-19 16:05:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-19 16:05:52]

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