Bug 89692 - Web Inspector: show worker started and finished on timeline
Summary: Web Inspector: show worker started and finished on timeline
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 91534
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-21 14:54 PDT by Hanna
Modified: 2016-12-13 15:34 PST (History)
17 users (show)

See Also:


Attachments
Patch (18.21 KB, patch)
2012-07-10 11:45 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (17.73 KB, patch)
2012-07-11 08:01 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (21.55 KB, patch)
2012-07-27 09:46 PDT, Hanna
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-06 (428.31 KB, application/zip)
2012-07-27 10:23 PDT, WebKit Review Bot
no flags Details
Patch (21.66 KB, patch)
2012-07-27 10:49 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (21.86 KB, patch)
2012-07-27 12:05 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (21.86 KB, patch)
2012-07-27 12:11 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (21.73 KB, patch)
2012-07-27 14:30 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (21.99 KB, patch)
2012-07-30 09:13 PDT, Hanna
gustavo: commit-queue-
Details | Formatted Diff | Diff
Patch (21.99 KB, patch)
2012-07-30 10:05 PDT, Hanna
pfeldman: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hanna 2012-06-21 14:54:35 PDT
Maybe add an indicator somewhere on timeline, like add more details to the popup generated by mouse hovering?

Recommendations appreciated, thank you.
Comment 1 Hanna 2012-07-04 07:20:38 PDT
add an event on timeline indicating when a worker started and when it's destroyed instead
Comment 2 Hanna 2012-07-10 11:45:23 PDT
Created attachment 151496 [details]
Patch
Comment 3 Hanna 2012-07-10 11:46:05 PDT
anyone r?
Comment 4 Build Bot 2012-07-10 12:03:17 PDT
Comment on attachment 151496 [details]
Patch

Attachment 151496 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13204055
Comment 5 Pavel Feldman 2012-07-10 12:15:55 PDT
Comment on attachment 151496 [details]
Patch

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.
Comment 6 Hanna 2012-07-10 13:06:24 PDT
Comment on attachment 151496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151496&action=review

fixing now

>> 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.

sure, except the id is the memory address

>> 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.

didDestroyWorker is not called on our platform

>> 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.

turned out that the url passed in is different from context->url().string(), in order to match up with the worker terminated I changed it, but if this messes up other things (ie inspectorAgent->didCreateWorker(id, url, isSharedWorker);) I'll add a new method then pass in context->url().string()

>> 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.

got it

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:586
>> +            case recordTypes.WorkerTerminated:
> 
> ditto. Did you run this change?

it is supposed to fall through, since workercreated has both isSharedWorker and url, but I couldn't pass the isSharedWorker info to workerterminated easily

>> 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.

this method that I used is copied directly from LayoutTests/fast/workers for both sharedWorker and dedicatedWorker, should that be fine?

>> 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.

will change right away
Comment 7 Hanna 2012-07-10 13:48:08 PDT
Comment on attachment 151496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151496&action=review

>>> 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.
> 
> didDestroyWorker is not called on our platform

my bad, it might be called on the worker thread, but I tried didDestroyWorker it doesn't show up on the timeline of the mainthread (which is the purpose of this PR)
Comment 8 Hanna 2012-07-11 08:01:49 PDT
Created attachment 151710 [details]
Patch
Comment 9 Hanna 2012-07-11 09:10:42 PDT
Note:
1. the worker ID is the address
2. on termination, worker ID and isSharedWorker is hard to know (since didDestroyWorker doesn't work in this case), it might be possible to include those things in ScriptExecutionContext, however I do not know if this is desired
3. for testing, I added setTimeout to StopTimeline, I hope the workers will be done in 5 seconds
Comment 10 Hanna 2012-07-12 08:25:57 PDT
r?
Comment 11 Pavel Feldman 2012-07-12 08:43:09 PDT
Comment on attachment 151496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151496&action=review

>>>> 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.
>> 
>> didDestroyWorker is not called on our platform
> 
> my bad, it might be called on the worker thread, but I tried didDestroyWorker it doesn't show up on the timeline of the mainthread (which is the purpose of this PR)

Hm. That would be a serios bug. We should never get here on the worker thread. Could you look into what is wrong here? This change is about reliably instrumenting creation and destruction of the worker on the main thread and you should not use workarounds if underlying code is behaving unexpectedly.

>>> 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.
> 
> turned out that the url passed in is different from context->url().string(), in order to match up with the worker terminated I changed it, but if this messes up other things (ie inspectorAgent->didCreateWorker(id, url, isSharedWorker);) I'll add a new method then pass in context->url().string()

What is the difference between the url and context's url?
Comment 12 Pavel Feldman 2012-07-12 08:48:17 PDT
Comment on attachment 151710 [details]
Patch

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.
Comment 13 Hanna 2012-07-12 10:01:20 PDT
Comment on attachment 151496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151496&action=review

>>>>> 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.
>>> 
>>> didDestroyWorker is not called on our platform
>> 
>> my bad, it might be called on the worker thread, but I tried didDestroyWorker it doesn't show up on the timeline of the mainthread (which is the purpose of this PR)
> 
> Hm. That would be a serios bug. We should never get here on the worker thread. Could you look into what is wrong here? This change is about reliably instrumenting creation and destruction of the worker on the main thread and you should not use workarounds if underlying code is behaving unexpectedly.

Sorry if it sounds confusing, I was making speculations because as of now the inspector I have only shows the timeline events on the mainthread (I was trying to say I do not know if diddestroyworker would show up on the inspector window for the worker which I do not have) and unfortunately diddestroyworker is never called either way, so I used didterminatecontext to approximate when the worker is destroyed since didterminatecontext is called after the workerthread (and everything relating to it) is de-allocated which should also be valid

>>>> 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.
>> 
>> turned out that the url passed in is different from context->url().string(), in order to match up with the worker terminated I changed it, but if this messes up other things (ie inspectorAgent->didCreateWorker(id, url, isSharedWorker);) I'll add a new method then pass in context->url().string()
> 
> What is the difference between the url and context's url?

the url being passed in is more specific, the context's url is cut half, ie less specific, but the first part of the urls are identical
Comment 14 Pavel Feldman 2012-07-13 00:57:51 PDT
> > Hm. That would be a serios bug. We should never get here on the worker thread. Could you look into what is wrong here? This change is about reliably instrumenting creation and destruction of the worker on the main thread and you should not use workarounds if underlying code is behaving unexpectedly.
> 
> Sorry if it sounds confusing, I was making speculations because as of now the inspector I have only shows the timeline events on the mainthread (I was trying to say I do not know if diddestroyworker would show up on the inspector window for the worker which I do not have) and unfortunately diddestroyworker is never called either way, so I used didterminatecontext to approximate when the worker is destroyed since didterminatecontext is called after the workerthread (and everything relating to it) is de-allocated which should also be valid

I do realize that it will work this way, I just want to make sure that we fix things along the way and if the one that should work is not working, we fix it instead of looking for the workaround.

> the url being passed in is more specific, the context's url is cut half, ie less specific, but the first part of the urls are identical

Why would one want the truncated url? Can we use the full url for everything?
Comment 15 Hanna 2012-07-18 12:53:46 PDT
(In reply to comment #14)
> 
> I do realize that it will work this way, I just want to make sure that we fix things along the way and if the one that should work is not working, we fix it instead of looking for the workaround.
> 

So I looked into it a little. The current inspectorinstrumentation::diddestroyworker is called in ~abstrackworker, which sharedworker inherits, though it wasn't even called on sharedworker's destruction

What I am thinking is to move the instrumentation of diddestroyworker to injectedscripthost, which is called via injectedfakeworker.js,and the instrumentation should be called on fakeworker destruction. Please let me know if this is be valid as I am not familiar with fakeworker, thank you
Comment 16 Pavel Feldman 2012-07-20 08:58:00 PDT
I'd like to remove fake workers inspection to avoid the confusion (https://bugs.webkit.org/show_bug.cgi?id=91867). Once that instrumentation is removed, you should be able to add missing instrumentation to show workers in the timeline.
Comment 17 Hanna 2012-07-20 09:17:26 PDT
(In reply to comment #16)
> I'd like to remove fake workers inspection to avoid the confusion (https://bugs.webkit.org/show_bug.cgi?id=91867). Once that instrumentation is removed, you should be able to add missing instrumentation to show workers in the timeline.

I think it might also be a javascript parsing issue that diddestroyworker is not called (in workersidebarpane, on http://pmav.eu/stuff/javascript-webworkers/, if you run the workers multiple times, they get piled up and is never terminated in the workersidebarpane, nevertheless this does not happen in the latest build of chromium, but happens in the official release of chromium on ubuntu and in everything else, ie safari, bb, qt), so I wonder if this has anything to do with how javascript core and v8 handles worker termination, nevertheless I am still investigating and will use diddestroyworker to make the instrumentation to timeline once I get it working
Comment 18 Hanna 2012-07-27 09:46:13 PDT
Created attachment 154976 [details]
Patch
Comment 19 Hanna 2012-07-27 09:47:46 PDT
note, the layout test for workerFinished only has id but not url since url for workerFinished is added in the frontend when the id of workerFinished matches with its parent record (workerStarted)
Comment 20 WebKit Review Bot 2012-07-27 10:23:16 PDT
Comment on attachment 154976 [details]
Patch

Attachment 154976 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13380326

New failing tests:
inspector/timeline/timeline-web-workers.html
Comment 21 WebKit Review Bot 2012-07-27 10:23:21 PDT
Created attachment 154982 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 22 Hanna 2012-07-27 10:49:18 PDT
Created attachment 154987 [details]
Patch
Comment 23 Konrad Piascik 2012-07-27 11:12:53 PDT
Comment on attachment 154987 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154987&action=review

You have workerId, url and workerId, url used repeatedly in different places.  Choose an order for the two parameters for consistency.  I prefer workerId, url but I leave it to you

> Source/WebCore/inspector/InspectorInstrumentation.cpp:972
> +    int id = 0;

can a worker ever have id 0?
it's better to set it to -1

> Source/WebCore/inspector/InspectorInstrumentation.cpp:977
> +    if (id && timelineAgent)

check for id > 0 if you set id = -1 as initial value.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:996
> +    int id = 0;

ditto

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1001
> +    if (id && timelineAgent)

ditto

> 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.

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:-206
> -            return;

why did you remove the early return?  You should "return id;" here as well.
Comment 24 Hanna 2012-07-27 12:05:30 PDT
Created attachment 155008 [details]
Patch
Comment 25 Pavel Feldman 2012-07-27 12:07:36 PDT
Comment on attachment 154987 [details]
Patch

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
Comment 26 Pavel Feldman 2012-07-27 12:08:13 PDT
Comment on attachment 155008 [details]
Patch

r- as per my earlier review.
Comment 27 Hanna 2012-07-27 12:11:44 PDT
Created attachment 155012 [details]
Patch
Comment 28 Hanna 2012-07-27 12:21:29 PDT
Comment on attachment 154987 [details]
Patch

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.

It is a user request

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

there will be instances where the browser don't have workerfrontend

>> 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?

the problem is not every browser has a worker frontend (as a matter of fact only chromium has it), idealy this thing has to work with all the browser

>> Source/WebCore/inspector/InspectorWorkerAgent.h:82
>> +    typedef HashMap<WorkerContextProxy*, std::pair<int, String> > DedicatedWorkers;
> 
> You don't need to change this map, do you?

I have to store id somewhere
Comment 29 Konrad Piascik 2012-07-27 12:40:22 PDT
Comment on attachment 154987 [details]
Patch

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.
> 
> It is a user request

Even though the worker's javascript is processed on a separate thread it is still useful to know when a worker is created/terminated.  This is especially important on devices with limited system resources.  This is meant to be a surface level overview and for further details about how things run Web Workers can be inspected in their own timeline.
Comment 30 Pavel Feldman 2012-07-27 12:42:30 PDT
Comment on attachment 154987 [details]
Patch

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.
> 
> It is a user request

I'd like to challenge it. Is is mentioned in any public forum or a bug tracker?

>>> Source/WebCore/inspector/InspectorWorkerAgent.cpp:59
>>> +        , m_id(id)
>> 
>> Please roll this back.
> 
> there will be instances where the browser don't have workerfrontend

I see what you mean now.

>>> 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.

I would move it into the InspectorWorkerAgent then.

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:194
> +    m_dedicatedWorkers.set(workerContextProxy, std::make_pair(++WorkerFrontendChannel::s_nextId, url.string()));

It is cleaner to generate an id first and then to use it in two methods below.
Comment 31 Pavel Feldman 2012-07-27 12:43:41 PDT
Comment on attachment 155012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155012&action=review

r- per review comments from previous patches.

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:86
> +    static int nextId()

No, i don't think we need getters, as I mention, I'd just move it into the InspectorWorkerAgent.
Comment 32 Pavel Feldman 2012-07-27 13:19:00 PDT
> Even though the worker's javascript is processed on a separate thread it is still useful to know when a worker is created/terminated.  This is especially important on devices with limited system resources.  This is meant to be a surface level overview and for further details about how things run Web Workers can be inspected in their own timeline.

There are multiple timeline operation modes we are providing:

1. Performance angle. Workers should not be a big deal here. They operate elsewhere, while worker script loading is reflected in the page's timeline
2. Async causation analysis - we glue setTimeout with timer fire events in that mode. Also does not make much sense to the workers
3. Memory view (no correlation)
4. Frame mode (no correlation)
5. Getting to know what the program is doing when one has no clue

Workers fall to under (5), but many, many other things do too. I don't think we should add workers before we add most of the events there. That's why I am a bit reluctant to this change.
Comment 33 Hanna 2012-07-27 14:30:34 PDT
Created attachment 155049 [details]
Patch
Comment 34 Rob Buis 2012-07-30 07:10:54 PDT
Comment on attachment 155049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155049&action=review

Looks good in general.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:978
> +        timelineAgent->didStartWorker(id, url);

How about more early return style here?
InspectorWorkerAgent* workerAgent = instrumentingAgents->inspectorWorkerAgent();
if (!workerAgent)
    return;

int id = workerAgent->didStartWorkerContext(workerContextProxy, url);
if (!id)
    return;

if (InspectorTimelineAgent* timelineAgent = instrumentingAgents->inspectorTimelineAgent())
    timelineAgent->didStartWorker(id, url);

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1002
> +        timelineAgent->didTerminateWorker(id);

Ditto.
Comment 35 Hanna 2012-07-30 09:13:29 PDT
Created attachment 155299 [details]
Patch
Comment 36 Gustavo Noronha (kov) 2012-07-30 09:26:54 PDT
Comment on attachment 155299 [details]
Patch

Attachment 155299 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13389378
Comment 37 Build Bot 2012-07-30 09:27:39 PDT
Comment on attachment 155299 [details]
Patch

Attachment 155299 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13381825
Comment 38 Early Warning System Bot 2012-07-30 09:29:00 PDT
Comment on attachment 155299 [details]
Patch

Attachment 155299 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13390429
Comment 39 Build Bot 2012-07-30 09:41:43 PDT
Comment on attachment 155299 [details]
Patch

Attachment 155299 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13387438
Comment 40 Hanna 2012-07-30 10:05:10 PDT
Created attachment 155311 [details]
Patch
Comment 41 Pavel Feldman 2012-08-02 11:47:03 PDT
Comment on attachment 155311 [details]
Patch

Is there a way I get a pointer to a request for this feature? It sounds like 1) there are not so many scenarios that involve it and 2) worker context destruction time does not really map to what developer might expect well.
Comment 42 Hanna 2012-08-02 12:13:27 PDT
(In reply to comment #41)
> (From update of attachment 155311 [details])
> Is there a way I get a pointer to a request for this feature? It sounds like 1) there are not so many scenarios that involve it and 2) worker context destruction time does not really map to what developer might expect well.


The current worker panel uses instrumentation to worker context start/finished to add and remove workers in the panel, so it makes sense to use them here (only for dedicated workers)

The original request was for the timeline to show all the events in the main and the worker process instead of having them in 2 seperate frontends (they want to see the events happening all together in one place and having a tag indicating it is from which process), and this is just a start. I would appreciate your opinions/thoughts on this project/request.

It also makes sense if the user wants to see when the worker starts and how long it's been active relative to everything else
Comment 43 Pavel Feldman 2012-08-02 12:17:18 PDT
> The current worker panel uses instrumentation to worker context start/finished to add and remove workers in the panel, so it makes sense to use them here (only for dedicated workers)

It does not. Sources panel does not imply the timing is accurate, while timeline does.

> The original request was for the timeline to show all the events in the main and the worker process instead of having them in 2 seperate frontends (they want to see the events happening all together in one place and having a tag indicating it is from which process), and this is just a start. I would appreciate your opinions/thoughts on this project/request.

Worker front-end has its own Timeline, we can't show workers and main page on the same timeline and I wonder why one would need that.

> 
> It also makes sense if the user wants to see when the worker starts and how long it's been active relative to everything else

Given that this is a fairly rare debugging scenario, it seems like user could put console.timeStamp() marks in his code to track the worker lifetime.
Comment 44 Radar WebKit Bug Importer 2014-12-09 10:48:11 PST
<rdar://problem/19192124>