[Webkit-unassigned] [Bug 89692] Web Inspector: show worker started and finished on timeline

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=89692


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #151496|review?                     |review-
               Flag|                            |




--- Comment #5 from Pavel Feldman <pfeldman at chromium.org>  2012-07-10 12:15:55 PST ---
(From update of attachment 151496)
View in context: https://bugs.webkit.org/attachment.cgi?id=151496&action=review

> Source/WebCore/inspector/InspectorInstrumentation.cpp:982
> +        timelineAgent->didCreateWorker(url, isSharedWorker);

You should pass the worker id as well so that we could navigate between worker records.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1009
> +    if (InspectorTimelineAgent* timelineAgent = instrumentingAgents->inspectorTimelineAgent())

Why aren't you using didDestroyWorker? It belongs to the legacy workers inspection (fake workers), but should still work.

> Source/WebCore/inspector/InspectorInstrumentation.h:1267
> +        didCreateWorkerImpl(instrumentingAgents, id, context->url().string(), isSharedWorker);

Why did this change? We don't make any calls from these inline public methods, all processing should be in impl.

> Source/WebCore/inspector/TimelineRecordFactory.cpp:201
> +    data->setBoolean("isSharedWorker", isSharedWorker);

"shared"

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:523
> +        if (this.type !== recordTypes.WorkerCreated && this.type !== recordTypes.WorkerTerminated) {

We already have atomic events with 0 duration that we report. Fixing them altogether should be handled in a separate patch.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:584
> +            case recordTypes.WorkerCreated:

Missing break; above.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:586
> +            case recordTypes.WorkerTerminated:

ditto. Did you run this change?

> LayoutTests/inspector/timeline/timeline-web-workers.html:25
> +    var sharedWorker = createSharedWorker();

Shared workers instantiation is browser-specific. Please make sure tests pass at least on Mac ports and for Chromium.

> LayoutTests/inspector/timeline/timeline-web-workers.html:29
> +    dedicatedWorker.postMessage("close");

Worker message processing is asynchronous to the inspector message interchange. By the time you leave this method and stop timeline, the worker is not necessarily stopped.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.