Create a new TimelineOverview for the Rendering Frames timeline, and add UI to the Timelines sidebar for switching between the two overview modes. By moving the frames graph to a separate overview, we no longer have to graph frames on a time axis. This allows us to use the same width for all frame elements and eliminates the need to combine frames during zoom and represent idle time in the graph. The Rendering Frames overview should use a unitless TimelineRuler showing the frame index.
<rdar://problem/20703766>
Created attachment 251723 [details] [Patch] Proposed Fix
Patch looks good at a glance! I'll take a closer look soon. Screenshots?
Comment on attachment 251723 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=251723&action=review Very nice and clean! > Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineOverview.js:35 > + selectionDuration: 100 So this is 100 frames? > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:79 > + this._element.style.width = (1 / graphDataSource.timelineOverview.secondsPerPixel) + "px"; Nit: Might want to Math.round or Math.ceil. With sub pixel reddening, this might lead to a blurry edge on the bar. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:276 > + var endIndex = Math.min(Math.floor(endTime), timeline.records.length - 1); Should this do Math.ceil for endTime? > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:536 > + const timelineHeight = 36; > + const renderingFramesTimelineHeight = 108; > + const rulerHeight = 29; Nit: These should be computed using offsetHeight, but that is likely tricky. > Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:576 > - var eventTitleBarOffset = 51; > - var contentElementOffset = 74; > + var eventTitleBarOffset = 58; > + var contentElementOffset = 81; Ugh, I see what you meant. Would be nice to compute these.
(In reply to comment #4) > Comment on attachment 251723 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251723&action=review > > Very nice and clean! > > > Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineOverview.js:35 > > + selectionDuration: 100 > > So this is 100 frames? Correct. > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:79 > > + this._element.style.width = (1 / graphDataSource.timelineOverview.secondsPerPixel) + "px"; > > Nit: Might want to Math.round or Math.ceil. With sub pixel reddening, this > might lead to a blurry edge on the bar. How can I verify that this is an issue? > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:276 > > + var endIndex = Math.min(Math.floor(endTime), timeline.records.length - 1); > > Should this do Math.ceil for endTime? It's a little counterintuitive but using Math.ceil will result in an off-by-one error, since frames are positioned with their left edge at the tick mark corresponding to their index. For example, a selection end time at 0.5 will be positioned in the center of the frame with index 0. Taking Math.ceil gives us 1, causing the first frame to the right of the selection's end to be included. > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:536 > > + const timelineHeight = 36; > > + const renderingFramesTimelineHeight = 108; > > + const rulerHeight = 29; > > Nit: These should be computed using offsetHeight, but that is likely tricky. > > > Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:576 > > - var eventTitleBarOffset = 51; > > - var contentElementOffset = 74; > > + var eventTitleBarOffset = 58; > > + var contentElementOffset = 81; > > Ugh, I see what you meant. Would be nice to compute these. I think we should address this separately.
Created attachment 251817 [details] [Patch] Proposed Fix Fixed cookie and selection bugs. TimelineSidebar and TimelineRecordingContentView could probably use a quick glance :)
Comment on attachment 251817 [details] [Patch] Proposed Fix Rejecting attachment 251817 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 251817, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: fs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 183451 = 39ae02a659082662886a2568bbac0a59a064e040 r183452 = 10df131d7c680e83eff0965b2011dee2c136754f r183453 = 0582af436b62eec0a3580324885a43385704019c r183454 = c1a9da5dabd9dede6f6d85b7aa2500b851503ba8 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://webkit-queues.appspot.com/results/6267201908637696
Created attachment 251832 [details] [Patch] Proposed Fix
Comment on attachment 251832 [details] [Patch] Proposed Fix Clearing flags on attachment: 251832 Committed r183469: <http://trac.webkit.org/changeset/183469>
All reviewed patches have been landed. Closing bug.