* 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!
<rdar://problem/22280383>
Created attachment 258962 [details] [Patch] Proposed Fix
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.
(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.
(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. :)
We should flip the default for that property. Make it true explicitly for the debugger sidebar.
Created attachment 258968 [details] [Patch] Proposed Fix
(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.
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?
Created attachment 259037 [details] [Patch] Proposed Fix
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'
Comment on attachment 259037 [details] [Patch] Proposed Fix r=me
Created attachment 259041 [details] [Patch] Proposed Fix
Comment on attachment 259041 [details] [Patch] Proposed Fix Clearing flags on attachment: 259041 Committed r188494: <http://trac.webkit.org/changeset/188494>
All reviewed patches have been landed. Closing bug.