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.
<rdar://problem/20470631>
Created attachment 250387 [details] [PATCH] Proposed Fix
Created attachment 250447 [details] UI screenshot
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 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.
Created attachment 250547 [details] [PATCH Proposed Fix]
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().
Created attachment 250570 [details] [PATCH] Proposed Fix
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
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 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
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
Created attachment 250573 [details] [PATCH] Proposed Fix
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 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.
If you just want to commit queue a reviewed patch, post it as cq+ and leave r blank.
Oh, but you need commiter for that. Never mind for now.
Comment on attachment 250573 [details] [PATCH] Proposed Fix Clearing flags on attachment: 250573 Committed r182660: <http://trac.webkit.org/changeset/182660>
All reviewed patches have been landed. Closing bug.
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.