[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 #154987|1                           |0
        is obsolete|                            |




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

> Source/WebCore/ChangeLog:8
> +        Test: inspector/timeline/timeline-web-workers.html

Please explain what happens and why.

I'd also be interested in learning more about the use cases. Is this a user request? There is a lot happening in the browser, not everything deserves to be on the timeline. Sorry for bringing it up so late, but I am sure you have some good answers to this. We do need to justify the code we are adding.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:105
> +static const char WorkerFinished[] = "WorkerFinished";

"WorkerTerminated" ?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:449
> +void InspectorTimelineAgent::didFinishWorker(int workerId)

::didTerminateWorker

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:451
> +    appendRecord(TimelineRecordFactory::createAnimationFrameData(workerId), TimelineRecordType::WorkerFinished, false, 0);

Why createAnimationFrameData ?

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:59
> +        , m_id(id)

Please roll this back.

>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:86
>> +    static int s_nextId;
> 
> I don't like the variable being public.  I'd rather see a static getter and static setter.

Please don't make random fields public.

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:196
> +        createWorkerFrontendChannel(workerContextProxy, url.string(), WorkerFrontendChannel::s_nextId);

Wouldn't it be more elegant for createWorkerFrontendChannel to return channel's id?

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:202
> +    int id = 0;

Remove this code.

>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:-206
>> -            return;
> 
> why did you remove the early return?  You should "return id;" here as well.

should be return it->second->id()

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:214
> +    return id;

return 0; (or -1 depending on the value you use to identify non-existing worker)

> Source/WebCore/inspector/InspectorWorkerAgent.h:82
> +    typedef HashMap<WorkerContextProxy*, std::pair<int, String> > DedicatedWorkers;

You don't need to change this map, do you?

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:102
> +    recordStyles[recordTypes.WorkerFinished] = { title: WebInspector.UIString("Worker Finished"), category: categories["scripting"] };

Worker Terminated

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