Bug 143533 - Web Inspector: create content view and details sidebar for Frames timeline
Summary: Web Inspector: create content view and details sidebar for Frames timeline
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-04-08 12:52 PDT by Matt Baker
Modified: 2015-04-11 19:20 PDT (History)
11 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (82.48 KB, patch)
2015-04-08 14:25 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
UI screenshot (352.38 KB, image/png)
2015-04-09 11:03 PDT, Matt Baker
no flags Details
[PATCH Proposed Fix] (118.76 KB, patch)
2015-04-10 18:25 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (127.02 KB, patch)
2015-04-11 00:38 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (702.53 KB, application/zip)
2015-04-11 01:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-mavericks (593.96 KB, application/zip)
2015-04-11 01:12 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (119.79 KB, patch)
2015-04-11 10:30 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2015-04-08 12:52:39 PDT
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.
Comment 1 Radar WebKit Bug Importer 2015-04-08 12:53:00 PDT
<rdar://problem/20470631>
Comment 2 Matt Baker 2015-04-08 14:25:05 PDT
Created attachment 250387 [details]
[PATCH] Proposed Fix
Comment 3 Matt Baker 2015-04-09 11:03:12 PDT
Created attachment 250447 [details]
UI screenshot
Comment 4 Joseph Pecoraro 2015-04-09 11:08:47 PDT
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".
Comment 5 Timothy Hatcher 2015-04-09 11:34:52 PDT
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.
Comment 6 Matt Baker 2015-04-10 18:25:24 PDT
Created attachment 250547 [details]
[PATCH Proposed Fix]
Comment 7 Timothy Hatcher 2015-04-10 19:26:09 PDT
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().
Comment 8 Matt Baker 2015-04-11 00:38:22 PDT
Created attachment 250570 [details]
[PATCH] Proposed Fix
Comment 9 Build Bot 2015-04-11 01:06:59 PDT
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
Comment 10 Build Bot 2015-04-11 01:07:02 PDT
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
Comment 11 Build Bot 2015-04-11 01:12:14 PDT
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
Comment 12 Build Bot 2015-04-11 01:12:19 PDT
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
Comment 13 Matt Baker 2015-04-11 10:30:58 PDT
Created attachment 250573 [details]
[PATCH] Proposed Fix
Comment 14 WebKit Commit Bot 2015-04-11 11:15:07 PDT
Comment on attachment 250573 [details]
[PATCH] Proposed Fix

Rejecting attachment 250573 [details] from review queue.

mattbaker@apple.com 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.
Comment 15 WebKit Commit Bot 2015-04-11 11:15:49 PDT
Comment on attachment 250573 [details]
[PATCH] Proposed Fix

Rejecting attachment 250573 [details] from commit-queue.

mattbaker@apple.com 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.
Comment 16 Timothy Hatcher 2015-04-11 12:33:57 PDT
If you just want to commit queue a reviewed patch, post it as cq+ and leave r blank.
Comment 17 Timothy Hatcher 2015-04-11 12:34:40 PDT
Oh, but you need commiter for that. Never mind for now.
Comment 18 WebKit Commit Bot 2015-04-11 12:37:38 PDT
Comment on attachment 250573 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 250573

Committed r182660: <http://trac.webkit.org/changeset/182660>
Comment 19 WebKit Commit Bot 2015-04-11 12:37:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Joseph Pecoraro 2015-04-11 19:20:17 PDT
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.