Web Inspector: Long delay when row selection changes in timeline data grids

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* SUMMARY
Long delay when row selection changes in timeline data grids. The delay is noticeable in all timeline data grids, and is especially slow in the Rendering Frames view.

* STEPS TO REPRODUCE
1. Open Timeilnes -> Rendering Frames
2. Record a long timeline (> 100 frames)
3. Select a row
4. Click another row, or use keyboard up/down and attempt to quickly cycle through rows
  => 0.5 s - 1 s delay while row selection changes

* NOTES
Inspector^2 shows that tree element mouse down is taking 400-800 ms on average, of which most of the time is spent in NavigationBar.updateLayout, which calls realOffsetWidth for the bar element and for each navigation item element (each time forcing a page reflow). In the Timelines grid, NavigationBar.updateLayout is called 6 times when the row selection changes. In the Rendering Frames grid it's called 18 times!

[Radar WebKit Bug Importer]

<rdar://problem/22280383>

[Matt Baker]

Created attachment 258962 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

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

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

> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:466
> +        this._navigationBar.updateLayoutSoon();

I worry this will cause a disconnect between the DOM and what the layout becomes later. So you might see a flash of content then the layout happens later to adjust. A two step update instead of one. If updateLayoutSoon is using requestAnimationFrame, we should be fine.

> Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineView.js:35
> +        this.navigationSidebarTreeOutline.allowsRepeatSelection = false;

We can do this for any sidebar that does not cause a view change. So all Timeline sidebar outlines now.

[Matt Baker]

(In reply to comment #3)
> Comment on attachment 258962 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258962&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:466
> > +        this._navigationBar.updateLayoutSoon();
> 
> I worry this will cause a disconnect between the DOM and what the layout
> becomes later. So you might see a flash of content then the layout happens
> later to adjust. A two step update instead of one. If updateLayoutSoon is
> using requestAnimationFrame, we should be fine.
> 
> > Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineView.js:35
> > +        this.navigationSidebarTreeOutline.allowsRepeatSelection = false;
> 
> We can do this for any sidebar that does not cause a view change. So all
> Timeline sidebar outlines now.

Except for the Debugger sidebar, which highlights breakpoint lines in the content view when the breakpoint's tree element is clicked.

[Matt Baker]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 258962 [details]
> > [Patch] Proposed Fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=258962&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:466
> > > +        this._navigationBar.updateLayoutSoon();
> > 
> > I worry this will cause a disconnect between the DOM and what the layout
> > becomes later. So you might see a flash of content then the layout happens
> > later to adjust. A two step update instead of one. If updateLayoutSoon is
> > using requestAnimationFrame, we should be fine.
> > 
> > > Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineView.js:35
> > > +        this.navigationSidebarTreeOutline.allowsRepeatSelection = false;
> > 
> > We can do this for any sidebar that does not cause a view change. So all
> > Timeline sidebar outlines now.
> 
> Except for the Debugger sidebar, which highlights breakpoint lines in the
> content view when the breakpoint's tree element is clicked.

All *timeline* sidebars. Right. :)

[Timothy Hatcher]

We should flip the default for that property. Make it true explicitly for the debugger sidebar.

[Matt Baker]

Created attachment 258968 [details]
[Patch] Proposed Fix

[Matt Baker]

(In reply to comment #3)
> Comment on attachment 258962 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258962&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:466
> > +        this._navigationBar.updateLayoutSoon();
> 
> I worry this will cause a disconnect between the DOM and what the layout
> becomes later. So you might see a flash of content then the layout happens
> later to adjust. A two step update instead of one. If updateLayoutSoon is
> using requestAnimationFrame, we should be fine.

Filed a follow up: https://bugs.webkit.org/show_bug.cgi?id=148010

> > Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineView.js:35
> > +        this.navigationSidebarTreeOutline.allowsRepeatSelection = false;
> 
> We can do this for any sidebar that does not cause a view change. So all
> Timeline sidebar outlines now.

Updated to set allowsRepeatSelection = false for all TimelineSidebarPanel tree outlines.

[Timothy Hatcher]

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

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

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:232
> +        treeOutline.allowsRepeatSelection = false;

Actually this will break the sidebar when a non-timeline content view is showing, since we still jump to locations on select in that case. Was there any other reason to change this?

[Matt Baker]

Created attachment 259037 [details]
[Patch] Proposed Fix

[BJ Burg]

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

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

> Source/WebInspectorUI/ChangeLog:14
> +        Call updateLayoutSoon instead of updateLayout to compress layout requests.

'coalesce'

[BJ Burg]

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

r=me

[Matt Baker]

Created attachment 259041 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

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

Clearing flags on attachment: 259041

Committed r188494: <http://trac.webkit.org/changeset/188494>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.