Bug 146119 – Web Inspector: Layout & Rendering timeline should show paint and layout records in separate rows

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

[Matt Baker]

Created attachment 255125 [details]
[Image] Proposed UI

* SUMMARY
Layout & Rendering timeline should show paint and layout records in separate rows.

The Rendering Frames timeline displays paint events in green to distinguish them from layout events. This needs to be carried over to the Layout & Rendering timeline for consistency. However, using two colors in the same graph presents a problem, since closely spaced "pills" are combined.

This can be solved by creating separate rows for each record type (see attached screenshot).

[Timothy Hatcher]

I know I mentioned separate rows for the two, but perhaps they can overlap. I am not sure we will see overlapping pills. (Basically have two rows on top of each other.)

[Matt Baker]

(In reply to comment #1)
> I know I mentioned separate rows for the two, but perhaps they can overlap.
> I am not sure we will see overlapping pills. (Basically have two rows on top
> of each other.)

As far as I know layout and paint events will never overlap in time, but zooming out far enough could cause the pills to overlap if we overlay the two rows on top of each other. This would look similar to overlapping bars on the Timeline Overview when a resource is collapsed.

[Timothy Hatcher]

(In reply to comment #2)
> (In reply to comment #1)
> > I know I mentioned separate rows for the two, but perhaps they can overlap.
> > I am not sure we will see overlapping pills. (Basically have two rows on top
> > of each other.)
> 
> As far as I know layout and paint events will never overlap in time, but
> zooming out far enough could cause the pills to overlap if we overlay the
> two rows on top of each other. This would look similar to overlapping bars
> on the Timeline Overview when a resource is collapsed.

Might be worth trying both ways to see how they feel.

[Matt Baker]

Created attachment 255270 [details]
[Patch] WIP

[Radar WebKit Bug Importer]

<rdar://problem/21472448>

[Matt Baker]

This WIP patch splits the Layout & Rendering timeline into two rows, with layout records in the top row. Placing layout above paint is meant to exploit the direction the eye travels on the screen (top to bottom) to reinforce the cause-effect relationship between layout and paint events.

[Matt Baker]

Comment on attachment 255270 [details]
[Patch] WIP

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

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:211
> +                this._element.classList.remove("paint");

TimelineRecordBar shouldn't have to query the layout record subtype and add/remove this additional class. This could be avoided by creating a timeline record type for paint records, distinct from TimelineRecord.Type.Layout. This could complicate code elsewhere in Timelines however, as there would no longer be a one-to-one mapping between TimelineRecord.Type and a Timeline object in a recording.

[Matt Baker]

Created attachment 255274 [details]
[Image] Two-row layout

Screenshot from WIP patch

[Timothy Hatcher]

Comment on attachment 255270 [details]
[Patch] WIP

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

Looks good. I'd still be curious to see the bars overlapping. As-is there is a lot of space between the two rows or the rows are too tall. There should be good padding above and below the bars.

> Source/WebInspectorUI/UserInterface/Models/LayoutTimelineRecord.js:109
> +    get isPaint()

This should be a function, if it begins with "is".

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:211
>> +                this._element.classList.remove("paint");
> 
> TimelineRecordBar shouldn't have to query the layout record subtype and add/remove this additional class. This could be avoided by creating a timeline record type for paint records, distinct from TimelineRecord.Type.Layout. This could complicate code elsewhere in Timelines however, as there would no longer be a one-to-one mapping between TimelineRecord.Type and a Timeline object in a recording.

You could just add/remove record.eventType as the class.

[Matt Baker]

Created attachment 255374 [details]
[Image] Two-rows, more padding

Looking much better with extra padding. Layout & Paint overlayed to follow...

[Timothy Hatcher]

That looks good with the padding.

[Matt Baker]

Created attachment 255376 [details]
[Image] One row, overlapping records

I don't think this works quite as well as the two row option.

[Timothy Hatcher]

I agree.

[Matt Baker]

Created attachment 255378 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

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

Clearing flags on attachment: 255378

Committed r185875: <http://trac.webkit.org/changeset/185875>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.