Bug 143533 – Web Inspector: create content view and details sidebar for Frames timeline

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Create a ContentView for the Frames timeline displaying frames and their child records. Parent frame rows should include a breakdown of time spent in the frame (layout, script, other), and child rows for layout and script events should show initiator/location information and event duration.

A details sidebar can display the aggregate time spent for all frames in the current timeline selection. We may want to add additional information later, such as the longest frame time, average frame time, and standard deviation.

[Radar WebKit Bug Importer]

<rdar://problem/20470631>

[Matt Baker]

Created attachment 250387 [details]
[PATCH] Proposed Fix

[Matt Baker]

Created attachment 250447 [details]
UI screenshot

[Joseph Pecoraro]

Comment on attachment 250387 [details]
[PATCH] Proposed Fix

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

I just did a quick once over, haven't reviewed this in full.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:54
> +    // Static
> +    static shouldShowViewForTimeline(timeline)

Style: Newline

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:270
> +                return null;
>              return new WebInspector.RunLoopTimelineRecord(startTime, endTime, children);

Style: Newline

> Source/WebInspectorUI/UserInterface/Models/RunLoopTimelineRecord.js:84
> +        // Time spent in layout events which were synchronously triggered from Javascript must be deducted from the

Typo: "Javascript" => "JavaScript"

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.css:46
> + }

Style: Extra whitespace.

> Source/WebInspectorUI/UserInterface/Views/RunLoopTimelineDataGridNode.js:35
> +// FIXME: Move to a WebInspector.Object subclass and we can remove this.
> +WebInspector.Object.deprecatedAddConstructorFunctions(WebInspector.RunLoopTimelineDataGridNode);

I don't think this is needed. I don't see anyone doing "WebInspector.RunLoopTimelineDataGridNode.addEventListener" etc.

> Source/WebInspectorUI/UserInterface/Views/RunLoopTimelineView.js:36
> +    columns.location.title = WebInspector.UIString("Initiator/Location");

I wonder if this should be "Initiator / Location".

[Timothy Hatcher]

Comment on attachment 250387 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:270
>> +                return null;
>>              return new WebInspector.RunLoopTimelineRecord(startTime, endTime, children);
> 
> Style: Newline

In this case I this it is fine to not have a newline because of the early return the line before the real return.

> Source/WebInspectorUI/UserInterface/Models/Timeline.js:-74
> -        if (this._type === WebInspector.TimelineRecord.Type.Layout)
> -            return WebInspector.UIString("Layout & Rendering");

I think the new timeline should be called "Rendering Frames" instead of just "Frames".

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:133
> +        drawSlice(centerX, centerY + window.devicePixelRatio, 0, 2.0 * Math.PI, this._shadowColor);

You should use 2D canvas shadow support instead. 

context.save();
context.shadowBlur = 3 * window.devicePixelRatio;
context.shadowOffsetY = 3 * window.devicePixelRatio;
context.shadowColor = "rgba(0, 0, 0, 0.5)";
drawSlice(centerX, centerY, 0, 2.0 * Math.PI, "transparent"); // might not work as transparent though.
context.restore();

Then draw the slices.

> Source/WebInspectorUI/UserInterface/Views/RunLoopDetailsSidebarPanel.js:122
> +        this._timeRangeRow.value = WebInspector.UIString("%s - %s").format(Number.secondsToString(firstRecord.startTime), Number.secondsToString(lastRecord.endTime));

Nit: Use a en dash for the range and not just a normal hyphen. http://www.thepunctuationguide.com/en-dash.html

>> Source/WebInspectorUI/UserInterface/Views/RunLoopTimelineDataGridNode.js:35
>> +WebInspector.Object.deprecatedAddConstructorFunctions(WebInspector.RunLoopTimelineDataGridNode);
> 
> I don't think this is needed. I don't see anyone doing "WebInspector.RunLoopTimelineDataGridNode.addEventListener" etc.

Dosen't hurt though. But we should remove it since it isn't needed.

>> Source/WebInspectorUI/UserInterface/Views/RunLoopTimelineView.js:36
>> +    columns.location.title = WebInspector.UIString("Initiator/Location");
> 
> I wonder if this should be "Initiator / Location".

I think just Location is fine.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordTreeElement.js:104
> +            title = WebInspector.UIString("Frame %d").format(timelineRecord.frameNumber);

I think the Frame tree elements should have a colored square icon. I can help make that for you.

[Matt Baker]

Created attachment 250547 [details]
[PATCH Proposed Fix]

[Timothy Hatcher]

Comment on attachment 250547 [details]
[PATCH Proposed Fix]

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

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:154
> +            context.fillStyle = "#444";

Nit: We try to use rgb() or hsl().

[Matt Baker]

Created attachment 250570 [details]
[PATCH] Proposed Fix

[Build Bot]

Comment on attachment 250570 [details]
[PATCH] Proposed Fix

Attachment 250570 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5198299975909376

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/console/console-api.html
inspector/event-listener.html
inspector/protocol-promise-result.html
inspector/test-harness-trivially-works.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html

[Build Bot]

Created attachment 250571 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 250570 [details]
[PATCH] Proposed Fix

Attachment 250570 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4854343962460160

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/console/console-api.html
inspector/event-listener.html
inspector/protocol-promise-result.html
inspector/test-harness-trivially-works.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html

[Build Bot]

Created attachment 250572 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Matt Baker]

Created attachment 250573 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 250573 [details]
[PATCH] Proposed Fix

Rejecting attachment 250573 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Commit Bot]

Comment on attachment 250573 [details]
[PATCH] Proposed Fix

Rejecting attachment 250573 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Timothy Hatcher]

If you just want to commit queue a reviewed patch, post it as cq+ and leave r blank.

[Timothy Hatcher]

Oh, but you need commiter for that. Never mind for now.

[WebKit Commit Bot]

Comment on attachment 250573 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 250573

Committed r182660: <http://trac.webkit.org/changeset/182660>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 250573 [details]
[PATCH] Proposed Fix

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

Cool stuff! Also very nice and clean patch.

> Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineOverviewGraph.js:42
> +WebInspector.RenderingFrameTimelineOverviewGraph.StyleClassName = "rendering-frame";

Nit: we are inlining these StyleClassName things now if it is only used in one place.