DevTools: Connect worker cpu profiles to worker tracing threads.

DevTools: Connect worker cpu profiles to worker tracing threads.

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

[alph, 2014-11-13 17:25:26]

alph@chromium.org changed reviewers:
+ caseq@chromium.org, loislo@chromium.org, yurys@chromium.org

[yurys, 2014-11-14 13:25:27]

https://codereview.chromium.org/724013003/diff/1/LayoutTests/inspector/tracin...
File LayoutTests/inspector/tracing/worker-js-profile.html (right):

https://codereview.chromium.org/724013003/diff/1/LayoutTests/inspector/tracin...
LayoutTests/inspector/tracing/worker-js-profile.html:74: Tests js cpu profile in
timeline.
... for worker?

https://codereview.chromium.org/724013003/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorWorkerAgent.cpp (right):

https://codereview.chromium.org/724013003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorWorkerAgent.cpp:210:
it->value->proxy()->writeTimelineStartedEvent(it->key, sessionId);
This will not write events for workers without connected front-end.

https://codereview.chromium.org/724013003/diff/1/Source/devtools/front_end/ti...
File Source/devtools/front_end/timeline/TimelineModel.js (right):

https://codereview.chromium.org/724013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TimelineModel.js:553: if
(categories.indexOf("devtools.js.profile") >= 0)
Let's pass it as explicit enableJSSampling parameter instead of introducing a
new category.

https://codereview.chromium.org/724013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TimelineModel.js:804: target =
WebInspector.workerTargetManager &&
WebInspector.workerTargetManager.targetByWorkerId(workerId);
When can WebInspector.workerTargetManager be null?

[alph, 2014-11-14 15:01:59]

ptal

https://codereview.chromium.org/724013003/diff/1/LayoutTests/inspector/tracin...
File LayoutTests/inspector/tracing/worker-js-profile.html (right):

https://codereview.chromium.org/724013003/diff/1/LayoutTests/inspector/tracin...
LayoutTests/inspector/tracing/worker-js-profile.html:74: Tests js cpu profile in
timeline.
On 2014/11/14 13:25:26, yurys wrote:
> ... for worker?

for worker after FIXME is addressed.

https://codereview.chromium.org/724013003/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorWorkerAgent.cpp (right):

https://codereview.chromium.org/724013003/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorWorkerAgent.cpp:210:
it->value->proxy()->writeTimelineStartedEvent(it->key, sessionId);
On 2014/11/14 13:25:26, yurys wrote:
> This will not write events for workers without connected front-end.

Done.

https://codereview.chromium.org/724013003/diff/1/Source/devtools/front_end/ti...
File Source/devtools/front_end/timeline/TimelineModel.js (right):

https://codereview.chromium.org/724013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TimelineModel.js:553: if
(categories.indexOf("devtools.js.profile") >= 0)
On 2014/11/14 13:25:26, yurys wrote:
> Let's pass it as explicit enableJSSampling parameter instead of introducing a
> new category.

Done.

https://codereview.chromium.org/724013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TimelineModel.js:804: target =
WebInspector.workerTargetManager &&
WebInspector.workerTargetManager.targetByWorkerId(workerId);
On 2014/11/14 13:25:26, yurys wrote:
> When can WebInspector.workerTargetManager be null?

before the main target is created. It is obviously not a case here. Fixed.

[yurys, 2014-11-17 12:51:36]

lgtm

https://codereview.chromium.org/724013003/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorWorkerAgent.h (right):

https://codereview.chromium.org/724013003/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorWorkerAgent.h:80: class WorkerInfo {
Please move definition of the class into the .cpp file and leave only forward
declaration here.

https://codereview.chromium.org/724013003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/WorkerTargetManager.js (right):

https://codereview.chromium.org/724013003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/WorkerTargetManager.js:16:
mainTarget.workerManager.addEventListener(WebInspector.WorkerManager.Events.WorkerAdded,
this._onWorkerAdded, this);
Please make sure there is 1-1 relationship between WorkerManager and main target
and WorkerManager doesn't rely on the worker id being unique per render process.

[alph, 2014-11-17 13:36:12]

https://codereview.chromium.org/724013003/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorWorkerAgent.h (right):

https://codereview.chromium.org/724013003/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorWorkerAgent.h:80: class WorkerInfo {
On 2014/11/17 12:51:36, yurys wrote:
> Please move definition of the class into the .cpp file and leave only forward
> declaration here.

I can't. HashMap below has a by-value use.

https://codereview.chromium.org/724013003/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/WorkerTargetManager.js (right):

https://codereview.chromium.org/724013003/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/WorkerTargetManager.js:16:
mainTarget.workerManager.addEventListener(WebInspector.WorkerManager.Events.WorkerAdded,
this._onWorkerAdded, this);
On 2014/11/17 12:51:36, yurys wrote:
> Please make sure there is 1-1 relationship between WorkerManager and main
target
> and WorkerManager doesn't rely on the worker id being unique per render
process.

Acknowledged.

[alph, 2014-11-17 13:36:16]

The CQ bit was checked by alph@chromium.org

[commit-bot: I haz the power, 2014-11-17 13:36:38]

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

[commit-bot: I haz the power, 2014-11-17 15:09:29]

Committed patchset #2 (id:20001) as 185446

[pdr., 2014-11-19 07:19:11]

This test is currently timing out on trunk:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=vir...