Bug 86406 - Web Inspector: Annotate timeline records with a frame identifier
Summary: Web Inspector: Annotate timeline records with a frame identifier
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Bryan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-14 14:47 PDT by Michael Schneider
Modified: 2012-06-08 06:09 PDT (History)
13 users (show)

See Also:


Attachments
Patch to add frameId to timeline records (60.75 KB, patch)
2012-06-05 20:18 PDT, Bryan
no flags Details | Formatted Diff | Diff
Patch (60.65 KB, patch)
2012-06-05 20:55 PDT, Bryan
no flags Details | Formatted Diff | Diff
Patch with fixes for comments and updated layout tests. (73.26 KB, patch)
2012-06-06 11:26 PDT, Bryan
no flags Details | Formatted Diff | Diff
Patch (94.63 KB, patch)
2012-06-07 12:11 PDT, Bryan
vsevik: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Schneider 2012-05-14 14:47:37 PDT
The timeline events of all frames are interleaved with each other, with no way to know on which frame they originated from.

As the timeline records are a very rich data source and enables to automatically analyze what the browser does, adding a frame id to would greatly simplify tools that analyze the timeline data.
Comment 1 Bryan 2012-06-05 20:18:29 PDT
Created attachment 145920 [details]
Patch to add frameId to timeline records

This adds the ability to track the frame that each timeline event is associated with. Not all events have an associated frame. Only those events with an associated frame will have a frameId.

This patch is based on the suggestions and recommendations provided by caseq and pfeldman. This is a preliminary version of the change, to get some initial feedback. No tests have been written yet.
Comment 2 WebKit Review Bot 2012-06-05 20:25:40 PDT
Attachment 145920 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Bryan 2012-06-05 20:55:26 PDT
Created attachment 145925 [details]
Patch
Comment 4 Andrey Kosyakov 2012-06-06 03:36:59 PDT
Comment on attachment 145925 [details]
Patch

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

A bunch of style nits, but general approach looks good.

> b/Source/WebCore/inspector/InspectorController.cpp:354
> +    m_profilerAgent->disable(&error);

indent

> b/Source/WebCore/inspector/InspectorTimelineAgent.cpp:333
> +      String contextId(m_pageAgent->frameId(frame));
> +      recordRaw->setString("frameId", contextId);

indent

> b/Source/WebCore/inspector/InspectorTimelineAgent.cpp:420
> +      String contextId(m_pageAgent->frameId(frame));
> +      record->setString("frameId", contextId);

indent

> b/Source/WebCore/inspector/front-end/TimelinePresentationModel.js:110
> +    if (!mainFrame || mainFrame.id() != record.frameId) 

!= => !==

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:442
> +      timelineAgent->willLoadXHR(request->url(), frameForScriptExecutionContext(context));

indent

> b/Source/WebCore/inspector/WorkerInspectorController.cpp:105
> +    m_timelineAgent = InspectorTimelineAgent::create(m_instrumentingAgents.get(), NULL, m_state.get(), InspectorTimelineAgent::WorkerInspector, InspectorTimelineAgent::FrameInstrumentationNotSupported);

NULL => 0

> b/Source/WebCore/inspector/InspectorInstrumentation.h:612
> +      return willCallFunctionImpl(instrumentingAgents, scriptName, scriptLine, context);

indent

> b/Source/WebCore/inspector/InspectorInstrumentation.h:651
> +        return willDispatchEventImpl(instrumentingAgents, event, window, node, ancestors, document->frame());

frame looks redundant here, we can use window->frame(). actually, it also looks redundant in the instrumentation interface.

> b/Source/WebCore/inspector/InspectorInstrumentation.h:689
> +        return willDispatchEventOnWindowImpl(instrumentingAgents, event, window, frame);

ditto

> b/Source/WebCore/inspector/InspectorTimelineAgent.h:154
> +        Frame* frame;

Storing a raw pointer to a frame in something that is not a FrameDestructionObserver looks a bit scary. Can we resolve frames to ids early?
Comment 5 Andrey Kosyakov 2012-06-06 03:50:37 PDT
Comment on attachment 145925 [details]
Patch

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

>> b/Source/WebCore/inspector/front-end/TimelinePresentationModel.js:110
>> +    if (!mainFrame || mainFrame.id() != record.frameId) 
> 
> != => !==

Also, ResourceTreeFrame.id is a getter, not a function. If it doesn't break tests, we're in trouble :)
Comment 6 Bryan 2012-06-06 11:26:34 PDT
Created attachment 146070 [details]
Patch with fixes for comments and updated layout tests.
Comment 7 Bryan 2012-06-06 11:39:43 PDT
Comment on attachment 146070 [details]
Patch with fixes for comments and updated layout tests.

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

A couple comments about whether certain objects can be null, and in turn whether I should protect against a null pointer dereference in those locations. Thanks!

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:595
> +        timelineAgent->willSendResourceRequest(identifier, request, loader->frame());

Is it possible that the loader may be null here? If so I'll add code to prevent a null pointer dereference.

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:702
> +        timelineAgent->didFinishLoadingResource(identifier, false, finishTime, loader->frame());

Same question.

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:710
> +        timelineAgent->didFinishLoadingResource(identifier, true, 0, loader->frame());

Same

> b/Source/WebCore/inspector/InspectorInstrumentation.h:590
> +        didScheduleResourceRequestImpl(instrumentingAgents, url, document->frame());

Is it possible for document to be null here?

> b/Source/WebCore/inspector/InspectorInstrumentation.h:853
> +        return willRecalculateStyleImpl(instrumentingAgents, document->frame());

Null issue here?

> b/Source/WebCore/inspector/InspectorInstrumentation.h:1177
> +        return willWriteHTMLImpl(instrumentingAgents, length, startLine, document->frame());

Same here? (And other locations as well)
Comment 8 WebKit Review Bot 2012-06-06 11:46:51 PDT
Attachment 146070 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Andrey Kosyakov 2012-06-07 06:22:41 PDT
Comment on attachment 146070 [details]
Patch with fixes for comments and updated layout tests.

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

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:321
> +        timelineAgent->willDispatchEvent(event, window ? window->frame() : 0);

On a closer look at the call site of InspectorInstrumentation::willDispatchEvent(), I think that window, as passed to us, is not the best way to get frame. Check out WindowEventContext. I'd rather get the frame from the document.

> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:349
> +        timelineAgent->willDispatchEvent(event, window ? window->frame() : 0);

This call, on the other hand, should never receive a null window.

>> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:595
>> +        timelineAgent->willSendResourceRequest(identifier, request, loader->frame());
> 
> Is it possible that the loader may be null here? If so I'll add code to prevent a null pointer dereference.

I don't think so -- if loader were null, we would crash in the resource agent anyway.

>> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:702
>> +        timelineAgent->didFinishLoadingResource(identifier, false, finishTime, loader->frame());
> 
> Same question.

From what I can see, we have documentLoader while any resource notifications are dispatched.

>> b/Source/WebCore/inspector/InspectorInstrumentation.cpp:710
>> +        timelineAgent->didFinishLoadingResource(identifier, true, 0, loader->frame());
> 
> Same

As above.

>> b/Source/WebCore/inspector/InspectorInstrumentation.h:590
>> +        didScheduleResourceRequestImpl(instrumentingAgents, url, document->frame());
> 
> Is it possible for document to be null here?

instrumentingAgentsForDocument would crash if document were null.

>> b/Source/WebCore/inspector/InspectorInstrumentation.h:853
>> +        return willRecalculateStyleImpl(instrumentingAgents, document->frame());
> 
> Null issue here?

ditto

>> b/Source/WebCore/inspector/InspectorInstrumentation.h:1177
>> +        return willWriteHTMLImpl(instrumentingAgents, length, startLine, document->frame());
> 
> Same here? (And other locations as well)

as above.
Comment 10 Andrey Kosyakov 2012-06-07 06:28:51 PDT
Comment on attachment 146070 [details]
Patch with fixes for comments and updated layout tests.

Also, please provide ChangeLog entries. Tools/Scripts/prepare-ChangeLog will do that for you (also normally invoked as part of Tools/Scripts/webkit-patch upload)
Comment 11 Bryan 2012-06-07 12:11:33 PDT
Created attachment 146357 [details]
Patch
Comment 12 Andrey Kosyakov 2012-06-08 05:04:01 PDT
Comment on attachment 146357 [details]
Patch

LGTM. Vsevolod, could you please take a look?
Comment 13 Vsevolod Vlasov 2012-06-08 05:22:34 PDT
Comment on attachment 146357 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by Andrey Kosyakov

Please fix this before landing.
Comment 14 Andrey Kosyakov 2012-06-08 06:09:02 PDT
Committed r119826: <http://trac.webkit.org/changeset/119826>