[webkit-reviews] review denied: [Bug 89692] Web Inspector: show worker started and finished on timeline : [Attachment 151710] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Hanna <hanma at rim.com>'s
request for review:
Bug 89692: Web Inspector: show worker started and finished on timeline
https://bugs.webkit.org/show_bug.cgi?id=89692

Attachment 151710: Patch
https://bugs.webkit.org/attachment.cgi?id=151710&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=151710&action=review


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

Passing two urls is confusing, we need to figure out why they differ.

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

It would be nice if didDestroyWorker was called when it should be.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:586
> +		   contentHelper._appendTextRow(WebInspector.UIString("URL"),
this.data["url"]);

I think Worker ID applies to the terminated info. Loading multiple workers from
the same URL could be used for parallel data processing, so it happens more
often than you might think.

> LayoutTests/inspector/timeline/timeline-web-workers.html:31
> +    InspectorTest.startTimeline(function() {

We always use named functions with { on the next line.

> LayoutTests/inspector/timeline/timeline-web-workers.html:37
> +	   setTimeout(function(){InspectorTest.stopTimeline(step3);}, 5000);

Tests should not have timeouts. You should signal worker termination from the
page to the front-end via logging something to the console and sniffing for the
console message.