DevTools: add Tracing agent on back-end

DevTools: add Tracing agent on back-end

We used to have tracing agent as a browser target only, this adds handling
of start command to the back-end and moves the session id logic that was
initialliy implemented in https://codereview.chromium.org/247513002/ to
the tracing agent.

BUG=361045
R=yurys@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172623

[caseq, 2014-04-24 11:55:10]

Please note that the test included with this CL should be landed separately, as
it requires https://codereview.chromium.org/256473004

[yurys, 2014-04-24 14:08:09]

https://codereview.chromium.org/254613002/diff/30001/Source/core/inspector/In...
File Source/core/inspector/InspectorTracingAgent.cpp (right):

https://codereview.chromium.org/254613002/diff/30001/Source/core/inspector/In...
Source/core/inspector/InspectorTracingAgent.cpp:91:
m_frontend->pageTracingStarted(sessionId);
I believe we should use separate events to notify front-end that tracing
started/stopped by the user code. Using the same event for
console.profile/profileEnd and front-end initiated profiles proved to be
difficult to maintain and we eventually introduced separate events for
console.profile/profileEnd.

https://codereview.chromium.org/254613002/diff/30001/Source/core/inspector/In...
File Source/core/inspector/InspectorTracingAgent.h (right):

https://codereview.chromium.org/254613002/diff/30001/Source/core/inspector/In...
Source/core/inspector/InspectorTracingAgent.h:2: * Copyright 2014 The Chromium
Authors. All rights reserved.
We normally use line comments (//) instead of block ones for the short license
header.

https://codereview.chromium.org/254613002/diff/30001/Source/core/inspector/In...
Source/core/inspector/InspectorTracingAgent.h:47:
InspectorTracingAgent(InspectorClient*);
explicit

https://codereview.chromium.org/254613002/diff/30001/Source/devtools/front_en...
File Source/devtools/front_end/TracingModel.js (right):

https://codereview.chromium.org/254613002/diff/30001/Source/devtools/front_en...
Source/devtools/front_end/TracingModel.js:460: pageTracingStarted:
function(sessionId)
Annotate please.

https://codereview.chromium.org/254613002/diff/30001/Source/devtools/protocol...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/254613002/diff/30001/Source/devtools/protocol...
Source/devtools/protocol.json:4037: { "name": "categories", "type": "string",
"description": "Category/tag filter used when tracing is enabled by the
inspected page." }
We already pass categories in Tracing.start why repeat it here?

https://codereview.chromium.org/254613002/diff/30001/Source/devtools/protocol...
Source/devtools/protocol.json:4049: { "name": "sessionId", "type": "string",
"description": "A system-unique identifier of the tracing session that allows
associating of some of the trace events with the inspected page" }
Can we send it in response to Tracing.start instead of adding new event?

[caseq, 2014-04-24 16:12:52]

On 2014/04/24 14:08:09, yurys wrote:
>
https://codereview.chromium.org/254613002/diff/30001/Source/core/inspector/In...
> File Source/core/inspector/InspectorTracingAgent.cpp (right):
> 
>
https://codereview.chromium.org/254613002/diff/30001/Source/core/inspector/In...
> Source/core/inspector/InspectorTracingAgent.cpp:91:
> m_frontend->pageTracingStarted(sessionId);
> I believe we should use separate events to notify front-end that tracing
> started/stopped by the user code. Using the same event for
> console.profile/profileEnd and front-end initiated profiles proved to be
> difficult to maintain and we eventually introduced separate events for
> console.profile/profileEnd.
> 

Done (we now only emit startedFromPage; when tracing is started by the
front-end, there's no event, just the callback).

>
https://codereview.chromium.org/254613002/diff/30001/Source/core/inspector/In...
> File Source/core/inspector/InspectorTracingAgent.h (right):
> 
>
https://codereview.chromium.org/254613002/diff/30001/Source/core/inspector/In...
> Source/core/inspector/InspectorTracingAgent.h:2: * Copyright 2014 The Chromium
> Authors. All rights reserved.
> We normally use line comments (//) instead of block ones for the short license
> header.

Done.

> 
>
https://codereview.chromium.org/254613002/diff/30001/Source/core/inspector/In...
> Source/core/inspector/InspectorTracingAgent.h:47:
> InspectorTracingAgent(InspectorClient*);
> explicit

Done.

> 
>
https://codereview.chromium.org/254613002/diff/30001/Source/devtools/front_en...
> File Source/devtools/front_end/TracingModel.js (right):
> 
>
https://codereview.chromium.org/254613002/diff/30001/Source/devtools/front_en...
> Source/devtools/front_end/TracingModel.js:460: pageTracingStarted:
> function(sessionId)
> Annotate please.

Done.

>
https://codereview.chromium.org/254613002/diff/30001/Source/devtools/protocol...
> File Source/devtools/protocol.json (right):
> 
>
https://codereview.chromium.org/254613002/diff/30001/Source/devtools/protocol...
> Source/devtools/protocol.json:4037: { "name": "categories", "type": "string",
> "description": "Category/tag filter used when tracing is enabled by the
> inspected page." }
> We already pass categories in Tracing.start why repeat it here?

This sets the categories that will be used when tracing is started by the page.

> 
>
https://codereview.chromium.org/254613002/diff/30001/Source/devtools/protocol...
> Source/devtools/protocol.json:4049: { "name": "sessionId", "type": "string",
> "description": "A system-unique identifier of the tracing session that allows
> associating of some of the trace events with the inspected page" }
> Can we send it in response to Tracing.start instead of adding new event?

As discussed, it appears that we need both.

[yurys, 2014-04-25 09:12:59]

lgtm

> This sets the categories that will be used when tracing is started by the
page.

Maybe remove them from Tracing.start then? It is confusing to have them in two
different places.

https://codereview.chromium.org/254613002/diff/50001/Source/core/inspector/In...
File Source/core/inspector/InspectorTracingAgent.cpp (right):

https://codereview.chromium.org/254613002/diff/50001/Source/core/inspector/In...
Source/core/inspector/InspectorTracingAgent.cpp:100: const char
devtoolsMetadataEventCategory[] =
TRACE_DISABLED_BY_DEFAULT("devtools.timeline");
Please move this into anonymous namespace at the top of the file or just inline.

[caseq, 2014-04-25 11:26:22]

On 2014/04/25 09:12:59, yurys wrote:
> lgtm
> 
> > This sets the categories that will be used when tracing is started by the
> page.
> 
> Maybe remove them from Tracing.start then? It is confusing to have them in two
> different places.

As discussed offline, since it's not evident at the moment if the the benefits
of
console.timeline()/console.timelineEnd() justify the amount of complexity
required
for supporting backend-initiated tracing, I'm currently removing support for
starting
tracing from back-end. Let's come back to this in a separate patch when we
really
need it.

>
https://codereview.chromium.org/254613002/diff/50001/Source/core/inspector/In...
> File Source/core/inspector/InspectorTracingAgent.cpp (right):
> 
>
https://codereview.chromium.org/254613002/diff/50001/Source/core/inspector/In...
> Source/core/inspector/InspectorTracingAgent.cpp:100: const char
> devtoolsMetadataEventCategory[] =
> TRACE_DISABLED_BY_DEFAULT("devtools.timeline");
> Please move this into anonymous namespace at the top of the file or just
inline.

Done.

[caseq, 2014-04-25 11:26:30]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2014-04-25 11:26:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caseq@chromium.org/254613002/70001

[caseq, 2014-04-25 11:28:32]

The CQ bit was unchecked by caseq@chromium.org

[caseq, 2014-04-25 11:28:41]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2014-04-25 11:29:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caseq@chromium.org/254613002/70001

[caseq, 2014-04-25 11:30:22]

The CQ bit was unchecked by caseq@chromium.org

[caseq, 2014-04-25 11:38:14]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2014-04-25 11:45:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caseq@chromium.org/254613002/90001

[commit-bot: I haz the power, 2014-04-25 12:15:56]

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

[commit-bot: I haz the power, 2014-04-25 12:15:56]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[caseq, 2014-04-25 12:16:50]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2014-04-25 12:17:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caseq@chromium.org/254613002/90001

[commit-bot: I haz the power, 2014-04-25 12:20:06]

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

[commit-bot: I haz the power, 2014-04-25 12:20:07]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[caseq, 2014-04-25 12:55:45]

Committed patchset #5 manually as r172623 (presubmit successful).