Independently enable recording and event callback

Independently enable recording and event callback

Previously we count TraceLog::SetEnabled() and SetDisabled() to allow
nested calls from about:tracing and Timeline panel of the DevTools.
Actually DevTools only needs the event callbacks, so we can allow the
two features (recording for about:tracing, event callback for DevTools)
to be independently enabled.

Also don't count TraceLog::SetEnabled() and SetDisabled() to ease
trace-startup and simplify the API semantics.

BUG=none
TEST=TraceEventCallbackTest.TraceEventCallbackAndRecording*, TraceEventTestFixture.*
R=caseq@chromium.org, nduca@chromium.org, pfeldman@chromium.org, tony@chromium.org
TBR=sky (for trivial call site change in content/browser/browser_shutdown_profile_dumper.cc)

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

[Xianzhu, 2013-11-12 16:39:57]

ping...

[nduca, 2013-11-15 23:11:07]

The reason for nestign is because devtools needs to be able to enable tracing
when its off, but when its on, install an observer.

We need to make sure that case still works --- with impl side painting mode
turned on, devtools timeline should still work.

[Xianzhu, 2013-11-16 00:50:36]

Considering 4 cases:
1. DevTools start; about:tracing start; DevTools stop; about:tracing stop
2. DevTools start; about:tracing start; about:tracing stop; DevTools stop
3. about:tracing start; DevTools start; DevTools stop; about:tracing stop
4. about:tracing start; DevTools start; about:tracing stop; DevTools stop

A) We expect it to work: in all cases, both DevTools and about:tracing can get
all events from their own starts to their own stops.

B) Before this change:
1 and 3 are as expected; For 2 and 4 about:tracing won't get trace because
TraceLog::Flush() only works when tracing is disabled (due to thread-local
buffers).

C) After this change (Patch Set 2):
In all cases, events after the first stop will be lost for who stops later.

D) If we let DevTools disable tracing if tracing was enabled by it:
3 will work as expected; 1, 2 and 4 will still work like C). However, this is
better than B) because about:tracing will always get traces.

E) If we enable tracing when either EventCallback is set or SetEnabled is
called, it would work just like as if we count SetEnabled/SetDisabled (B).

I'd prefer C) because the rule is simple if there were no better solutions.

Is running DevTools timeline and about:tracing at the same time a real useful
scenario? How about some special events to let the user know what happened, e.g.
TRACE_EVENT_INSTANT0("devtools", "Tracing Stopped by DevTools") in
DevToolsAgent?

[nduca, 2013-11-16 01:26:53]

There are valid scenarios where we want both running at once, without
information lost, I'm afraid. They are telemetry related. In the long term I'd
like to pursuade devtools to use about:tracing for the timeline all the time,
but that is a very large project.

A key case is 3, whee about:tracing starts without devtools category enabled.
Enabling devtools needs to enable the tracing category.

I think we can achieve compatiblity with all these use cases:

TraceLog::SetEnabled(CategoryFilter)/SetDisabled  used by regular tracing
systems
TraceLog::SetDevToolsEnabled(CategoryFilter)/SetDevToolsDisabled  used by
devtools

Do you envision implementaiton problems from such an interface as relates to
thread-local trace buffers?

[Xianzhu, 2013-11-18 17:10:36]

On 2013/11/16 01:26:53, nduca wrote:
> There are valid scenarios where we want both running at once, without
> information lost, I'm afraid. They are telemetry related. In the long term I'd
> like to pursuade devtools to use about:tracing for the timeline all the time,
> but that is a very large project.
> 
> A key case is 3, whee about:tracing starts without devtools category enabled.
> Enabling devtools needs to enable the tracing category.
> 
> I think we can achieve compatiblity with all these use cases:
> 
> TraceLog::SetEnabled(CategoryFilter)/SetDisabled  used by regular tracing
> systems
> TraceLog::SetDevToolsEnabled(CategoryFilter)/SetDevToolsDisabled  used by
> devtools
> 
> Do you envision implementaiton problems from such an interface as relates to
> thread-local trace buffers?

My comments in #5 is based on the current thread-local buffer impl. To support
flush during tracing (which has been broken since we used thread-local buffers),
the async flush procedure needs to be modified. Doing this.

[Xianzhu, 2013-11-18 17:10:56]

On 2013/11/18 17:10:36, Xianzhu wrote:
> On 2013/11/16 01:26:53, nduca wrote:
> > There are valid scenarios where we want both running at once, without
> > information lost, I'm afraid. They are telemetry related. In the long term
I'd
> > like to pursuade devtools to use about:tracing for the timeline all the
time,
> > but that is a very large project.
> > 
> > A key case is 3, whee about:tracing starts without devtools category
enabled.
> > Enabling devtools needs to enable the tracing category.
> > 
> > I think we can achieve compatiblity with all these use cases:
> > 
> > TraceLog::SetEnabled(CategoryFilter)/SetDisabled  used by regular tracing
> > systems
> > TraceLog::SetDevToolsEnabled(CategoryFilter)/SetDevToolsDisabled  used by
> > devtools
> > 
> > Do you envision implementaiton problems from such an interface as relates to
> > thread-local trace buffers?
> 
> My comments in #5 is based on the current thread-local buffer impl. To support
> flush during tracing (which has been broken since we used thread-local
buffers),
> the async flush procedure needs to be modified. Doing this.

s/#5/#4/

[Xianzhu, 2013-11-18 20:30:36]

PTAL. Now allow EventCallback and recording to be enabled independently. Please
also see the updated CL description.

[Xianzhu, 2013-11-19 17:02:01]

+pfeldman@: for content/renderer/devtools
+tony: for webkit/child

[pfeldman, 2013-11-19 17:26:33]

devtools lgtm

[nduca, 2013-11-19 17:52:37]

lgtm i think. lets have @caseq double check. Thank you for making this change.

[tony, 2013-11-19 18:18:56]

webkit/child LGTM

[caseq, 2013-11-19 18:32:29]

https://codereview.chromium.org/66193005/diff/180001/base/debug/trace_event_i...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/66193005/diff/180001/base/debug/trace_event_i...
base/debug/trace_event_impl.h:468: // For TRACE_EVENT_PHASE_COMPLETE events,
the/ client will still receive pairs
nit: the/ client => the client

https://codereview.chromium.org/66193005/diff/180001/content/renderer/devtool...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/66193005/diff/180001/content/renderer/devtool...
content/renderer/devtools/devtools_agent.cc:144:
trace_log->SetEventCallbackEnabled(base::debug::CategoryFilter(""),
Why did we stop using the default filter here? I think we rather want to
eventually narrow down the category filter here, expanding it to include
categories disable by default may slow things down.

[dsinclair, 2013-11-19 18:39:10]

https://codereview.chromium.org/66193005/diff/180001/content/renderer/devtool...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/66193005/diff/180001/content/renderer/devtool...
content/renderer/devtools/devtools_agent.cc:144:
trace_log->SetEventCallbackEnabled(base::debug::CategoryFilter(""),
On 2013/11/19 18:32:29, caseq wrote:
> Why did we stop using the default filter here? I think we rather want to
> eventually narrow down the category filter here, expanding it to include
> categories disable by default may slow things down.


This won't enable disabled-by-default-. Those will only be enabled if you
explicitly put them into the category string. This will enable the Debug and
Test categories that are removed as part of the default category filter.

[Xianzhu, 2013-11-19 18:39:21]

https://codereview.chromium.org/66193005/diff/180001/base/debug/trace_event_i...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/66193005/diff/180001/base/debug/trace_event_i...
base/debug/trace_event_impl.h:468: // For TRACE_EVENT_PHASE_COMPLETE events,
the/ client will still receive pairs
On 2013/11/19 18:32:29, caseq wrote:
> nit: the/ client => the client

Done.

https://codereview.chromium.org/66193005/diff/180001/content/renderer/devtool...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/66193005/diff/180001/content/renderer/devtool...
content/renderer/devtools/devtools_agent.cc:144:
trace_log->SetEventCallbackEnabled(base::debug::CategoryFilter(""),
On 2013/11/19 18:32:29, caseq wrote:
> Why did we stop using the default filter here? I think we rather want to
> eventually narrow down the category filter here, expanding it to include
> categories disable by default may slow things down.

Actually CategoryFilter("") will create a default filter. However I changed back
to the original code which seems clearer.

[caseq, 2013-11-19 19:02:46]

On 2013/11/19 18:39:21, Xianzhu wrote:
>
https://codereview.chromium.org/66193005/diff/180001/base/debug/trace_event_i...
> File base/debug/trace_event_impl.h (right):
> 
>
https://codereview.chromium.org/66193005/diff/180001/base/debug/trace_event_i...
> base/debug/trace_event_impl.h:468: // For TRACE_EVENT_PHASE_COMPLETE events,
> the/ client will still receive pairs
> On 2013/11/19 18:32:29, caseq wrote:
> > nit: the/ client => the client
> 
> Done.
> 
>
https://codereview.chromium.org/66193005/diff/180001/content/renderer/devtool...
> File content/renderer/devtools/devtools_agent.cc (right):
> 
>
https://codereview.chromium.org/66193005/diff/180001/content/renderer/devtool...
> content/renderer/devtools/devtools_agent.cc:144:
> trace_log->SetEventCallbackEnabled(base::debug::CategoryFilter(""),
> On 2013/11/19 18:32:29, caseq wrote:
> > Why did we stop using the default filter here? I think we rather want to
> > eventually narrow down the category filter here, expanding it to include
> > categories disable by default may slow things down.
> 
> Actually CategoryFilter("") will create a default filter. However I changed
back
> to the original code which seems clearer.

Ah, indeed! I can see it does now, but I was originally misled by the comment
here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/trace_e...
lgtm.

[Xianzhu, 2013-11-19 19:11:49]

On 2013/11/19 19:02:46, caseq wrote:
> 
> Ah, indeed! I can see it does now, but I was originally misled by the comment
> here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/trace_e...
> lgtm.

The comment should be updated according to the current behavior. Will do in a
separate change.

[commit-bot: I haz the power, 2013-11-19 19:26:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/66193005/270001

[commit-bot: I haz the power, 2013-11-19 20:10:53]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-19 20:11:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/66193005/450002

[tony, 2013-11-19 20:14:29]

The presubmit error is:

Missing LGTM from an OWNER for these files:
    content/browser/browser_shutdown_profile_dumper.cc

I think you still need this approval.

[commit-bot: I haz the power, 2013-11-19 20:38:05]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Xianzhu, 2013-11-19 20:39:28]

On 2013/11/19 20:14:29, tony wrote:
> The presubmit error is:
> 
> Missing LGTM from an OWNER for these files:
>     content/browser/browser_shutdown_profile_dumper.cc
> 
> I think you still need this approval.

Thanks for the info. There was also a test failure which made me overlook the
presubmit error. Now CQ seems to run some of other try bots before
chromium_presubmit.

TBR content/browser/browser_shutdown_profile_dumper.cc because there is only a
trivial change due to change of TraceLog API.

[commit-bot: I haz the power, 2013-11-19 20:44:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/66193005/450002

[Xianzhu, 2013-11-20 00:40:55]

Committed patchset #6 manually as r236078 (presubmit successful).