Enable overlapping, usage counted SetEnabled()/SetDisabled() for TraceLog

Enable overlapping, usage counted SetEnabled()/SetDisabled() for TraceLog

This allows multiple clients to enable/disable trace events collection independently on each other.

BUG=none
TEST=TraceEventTestFixture.TraceEnableDisable

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177135

[nduca, 2013-01-08 21:27:39]

https://codereview.chromium.org/11802005/diff/1/base/debug/trace_event_unitte...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/11802005/diff/1/base/debug/trace_event_unitte...
base/debug/trace_event_unittest.cc:1346: ManualTestSetUp();
How does enabling work with çategories? I think we should probably delete
SetEnabled(bool) and route everythign throughSetEnabed(included, excluded) and
then fix that one to support nesting?

[caseq, 2013-01-08 22:23:38]

On 2013/01/08 21:27:39, nduca wrote:
>
https://codereview.chromium.org/11802005/diff/1/base/debug/trace_event_unitte...
> File base/debug/trace_event_unittest.cc (right):

> How does enabling work with çategories?

So far we ignore all categories except for the first call (exactly as it used to
be before). A better option would perhaps be to make a union of current sets
with those in SetEnabled()

> I think we should probably delete
> SetEnabled(bool) and route everythign throughSetEnabed(included, excluded) and
> then fix that one to support nesting?

Do you mean we should have counters at the categories level? So how would a
SetDisabled call look like -- should we require explicit sets there as well?

[nduca, 2013-01-08 23:34:05]

I'm fine with anything that is self consistent. It seems to me like
category-level is the right thing, but I haven't quite paged it all in my head.
Matching disabled arguments to those passed into enable seems like a solid
foundation... can we make it work in practice?

[caseq1, 2013-01-09 20:34:35]

On 2013/01/08 23:34:05, nduca wrote:
> I'm fine with anything that is self consistent. It seems to me like
> category-level is the right thing, but I haven't quite paged it all in my
head.
> Matching disabled arguments to those passed into enable seems like a solid
> foundation... can we make it work in practice?

I'm struggling to find a solution that would be both consistent and
comprehensive while keeping the API. I assume we expect subsequent enables to
always broaden set of active categories, and given the existing logic of
treating empty included_categories_ as "allow all except excluded_categories_",
joining categories sets becomes tricky -- SetDisabled() wouldn't be just able to
remove categories added by SetEnabled() (consider we have empty
included_categories and SetEnabled() is called with non-empty one -- we would
have to keep them resulting list empty; if we do SetEnabled() with non-empty
included_categories followed by empty list, we would have to make it empty; to
be consistent we would somehow need to restore original list upon
SetDisabled()).

So my suggestion is to go worse-is-better way here and sacrifice consistency in
favor of simplicity -- don't try to narrow down set of enabled categories upon
all but last SetDisabled() and only extend set of enabled categories upon
SetEnabled().
In other words:
- for first SetEnabled(), copy the provided category lists (present logic)
- for subsequent SetEnabled(), if either current enabled_categories_ or one
passed to SetEnabled is empty, keep it empty; otherwise just merge the lists;
- for all but last SetDisabled(), just decrement enabled_count_;
- for the last SetDisabled(), clear all categories (present logic)

[nduca, 2013-01-09 20:41:45]

That seems fine. Another option, I wonder, is maybe just instead just make
things fail correctly if you setenabled and it was enabled for another purpose.
So, if we had about:tracing started as part of telemetry, and tried to use
inspector timeline (backed by tracing) it would just fail. And vice versa. Same
for the UI...

[caseq1, 2013-01-10 16:49:45]

On 2013/01/09 20:41:45, nduca wrote:
> That seems fine. Another option, I wonder, is maybe just instead just make
> things fail correctly if you setenabled and it was enabled for another
purpose.
> So, if we had about:tracing started as part of telemetry, and tried to use
> inspector timeline (backed by tracing) it would just fail. And vice versa.
Same
> for the UI...

Implemented relaxed categories merge for nested enable. I do like second option
as well (mutually exclusive clients), but there seems to be a lot of mess with
exposing it properly to the user (and quietly discarding failure to enable trace
sounds a bit too harsh).

[nduca, 2013-01-10 19:58:53]

lgtm thank you for giving this so much thought.

[nduca, 2013-01-10 20:00:00]

+jar for owners

[jar (doing other things), 2013-01-12 15:46:59]

Code looks reasoable... but mostly rubber stamp LGTM