Collect an initial set of stability activities

Collect an initial set of stability activities

BUG=620813
Committed: https://crrev.com/3fbe4d26c6c6920babf2a83b3741b4e94ce689ba
Cr-Commit-Position: refs/heads/master@{#430938}

[manzagop (departed), 2016-11-01 21:48:12]

manzagop@chromium.org changed reviewers:
+ bcwhite@chromium.org

[manzagop (departed), 2016-11-01 21:48:12]

Hi Brian!
This adds collection for the existing data. Please have a look!
Thanks!
Pierre

[bcwhite, 2016-11-02 16:52:38]

https://codereview.chromium.org/2467103002/diff/1/base/debug/activity_analyzer.h
File base/debug/activity_analyzer.h (right):

https://codereview.chromium.org/2467103002/diff/1/base/debug/activity_analyze...
base/debug/activity_analyzer.h:77: const ActivitySnapshot& activity_snapshot() {
return activity_snapshot_; }
I had originally planned to *not* expose the snapshot to the caller...  but I
guess that's probably the simplest way.

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
File components/browser_watcher/postmortem_report_collector_unittest.cc (right):

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
components/browser_watcher/postmortem_report_collector_unittest.cc:364:
tracker->PushActivity(reinterpret_cast<void*>(kTaskOrigin),
This is far away from where it's tested.  How about moving it down into that
method?

Also, you'd be better off using the ScopedFoo objects if you can since they
should be constant interfaces while FooActivity methods are subject to change as
the implementation evolves.

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
File components/browser_watcher/stability_report.proto (right):

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
components/browser_watcher/stability_report.proto:83: optional uint64
event_address = 6;
What about the (optional) call_stack on ASAN builds?

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
components/browser_watcher/stability_report.proto:103: optional int32
activity_cnt = 3;
Is it typical to use abbreviated "cnt" for a count?

[manzagop (departed), 2016-11-02 20:15:24]

Addressed comments! Please have another look.

https://codereview.chromium.org/2467103002/diff/1/base/debug/activity_analyzer.h
File base/debug/activity_analyzer.h (right):

https://codereview.chromium.org/2467103002/diff/1/base/debug/activity_analyze...
base/debug/activity_analyzer.h:77: const ActivitySnapshot& activity_snapshot() {
return activity_snapshot_; }
On 2016/11/02 16:52:37, bcwhite wrote:
> I had originally planned to *not* expose the snapshot to the caller...  but I
> guess that's probably the simplest way.

Acknowledged.

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
File components/browser_watcher/postmortem_report_collector_unittest.cc (right):

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
components/browser_watcher/postmortem_report_collector_unittest.cc:364:
tracker->PushActivity(reinterpret_cast<void*>(kTaskOrigin),
On 2016/11/02 16:52:37, bcwhite wrote:
> This is far away from where it's tested.  How about moving it down into that
> method?

Done.

> Also, you'd be better off using the ScopedFoo objects if you can since they
> should be constant interfaces while FooActivity methods are subject to change
as
> the implementation evolves.

I'm leaning towards keeping as is if you're ok with it. Using the scoped
versions would require creating a GlobalActivityTracker and then setting up
actual tasks / locks.

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
File components/browser_watcher/stability_report.proto (right):

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
components/browser_watcher/stability_report.proto:83: optional uint64
event_address = 6;
On 2016/11/02 16:52:37, bcwhite wrote:
> What about the (optional) call_stack on ASAN builds?

I'm keeping it for a subsequent CL as current focus is on the postmortem
reports.

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
components/browser_watcher/stability_report.proto:103: optional int32
activity_cnt = 3;
On 2016/11/02 16:52:37, bcwhite wrote:
> Is it typical to use abbreviated "cnt" for a count?

A rapid chromium code search seems to say no. Done.

[bcwhite, 2016-11-03 17:56:25]

lgtm

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
File components/browser_watcher/postmortem_report_collector_unittest.cc (right):

https://codereview.chromium.org/2467103002/diff/1/components/browser_watcher/...
components/browser_watcher/postmortem_report_collector_unittest.cc:364:
tracker->PushActivity(reinterpret_cast<void*>(kTaskOrigin),
On 2016/11/02 20:15:23, manzagop wrote:
> On 2016/11/02 16:52:37, bcwhite wrote:
> > This is far away from where it's tested.  How about moving it down into that
> > method?
> 
> Done.
> 
> > Also, you'd be better off using the ScopedFoo objects if you can since they
> > should be constant interfaces while FooActivity methods are subject to
change
> as
> > the implementation evolves.
> 
> I'm leaning towards keeping as is if you're ok with it. Using the scoped
> versions would require creating a GlobalActivityTracker and then setting up
> actual tasks / locks.

Acknowledged.

[manzagop (departed), 2016-11-03 18:32:30]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 18:33:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 20:03:52]

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

[commit-bot: I haz the power, 2016-11-03 20:03:53]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[manzagop (departed), 2016-11-08 21:35:46]

The CQ bit was checked by manzagop@chromium.org

[manzagop (departed), 2016-11-08 21:35:47]

The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org
Link to the patchset: https://codereview.chromium.org/2467103002/#ps40001
(title: "Address clang warnings")

[commit-bot: I haz the power, 2016-11-08 21:36:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 23:43:41]

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

[commit-bot: I haz the power, 2016-11-08 23:43:42]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[manzagop (departed), 2016-11-09 14:29:57]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2016-11-09 14:30:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 15:44:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-09 15:48:10]

Description was changed from

==========
Collect an initial set of stability activities

BUG=620813
==========

to

==========
Collect an initial set of stability activities

BUG=620813

Committed: https://crrev.com/3fbe4d26c6c6920babf2a83b3741b4e94ce689ba
Cr-Commit-Position: refs/heads/master@{#430938}
==========

[commit-bot: I haz the power, 2016-11-09 15:48:11]

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