Bug 144245 – Web Inspector: add a separate overview for the Rendering Frames timeline

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

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.

[Radar WebKit Bug Importer]

<rdar://problem/20703766>

[Matt Baker]

Created attachment 251723 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Patch looks good at a glance! I'll take a closer look soon.

Screenshots?

[Timothy Hatcher]

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.

[Matt Baker]

(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.

[Matt Baker]

Created attachment 251817 [details]
[Patch] Proposed Fix

Fixed cookie and selection bugs. TimelineSidebar and TimelineRecordingContentView could probably use a quick glance :)

[WebKit Commit Bot]

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

[Matt Baker]

Created attachment 251832 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 251832 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 251832

Committed r183469: <http://trac.webkit.org/changeset/183469>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.