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).
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.)
(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.
(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.
Created attachment 255270 [details] [Patch] WIP
<rdar://problem/21472448>
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.
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.
Created attachment 255274 [details] [Image] Two-row layout Screenshot from WIP patch
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.
Created attachment 255374 [details] [Image] Two-rows, more padding Looking much better with extra padding. Layout & Paint overlayed to follow...
That looks good with the padding.
Created attachment 255376 [details] [Image] One row, overlapping records I don't think this works quite as well as the two row option.
I agree.
Created attachment 255378 [details] [Patch] Proposed Fix
Comment on attachment 255378 [details] [Patch] Proposed Fix Clearing flags on attachment: 255378 Committed r185875: <http://trac.webkit.org/changeset/185875>
All reviewed patches have been landed. Closing bug.