Summary: | Web Inspector: Batch all scheduled layouts and update the view tree in a single animation frame | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, simon.fraser, timothy, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 151058 | ||||||||||
Attachments: |
|
Description
Matt Baker
2015-11-06 19:01:06 PST
Created attachment 264990 [details]
[Patch] WIP
Noticed a bug in View._unscheduleLayoutForView. Will fix for the next WIP. While trying out this change I noticed that View._visitViewTreeForLayout is called *very* rarely. Almost every scheduled animation frame is canceled by a forced layout (a call to View.updateLayout). I suspect we're performing synchronous layouts when scheduled layouts would suffice. AFAIK, there are only two cases where updateLayout must be used instead of needsLayout: 1) An element's position/size must be read after updating styles, etc, in order to lay out a related part of the UI. NavigationBar is an example of this. 2) Responsiveness of layouts in quick succession is needed, such as when resizing a sidebar or other "split" view. Comment on attachment 264990 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=264990&action=review Nifty. > Source/WebInspectorUI/UserInterface/Views/View.js:174 > + this._dirty = false; This should probably also set this._dirtyDescendants = 0, if you are keeping that value up to date. > Source/WebInspectorUI/UserInterface/Views/View.js:220 > + let viewStack = [[WebInspector.View._rootView]]; > + while (viewStack.length) { An array of arrays of views sounds unnecessarily complicated. How about just an array of views?: let views = [View._rootView]; while (views.length) { let view = views.shift(); if (view.dirty) view._layoutSubtree(); else { views = views.concat(view.subviews); view._dirtyDescendants = 0; } } Note: - this takes into account _layoutSubtree re-setting dirtyDescendants. - no need to create a copy of the subviews lists if you don't modify those lists directly, here we just add the values to the main list - JoePeck Fun Fact: "WebInspector.View" can just be "View" in this case, because we are inside "class View {...}" scope! Or just use the recursive solution, since the depth of our view tree should not be large at all. Comment on attachment 264990 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=264990&action=review > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:212 > - if (!this._endTimePinned && this.layoutPending) > + if (!this._endTimePinned && this.dirty) I like the layoutPending name better, at least as a public API. Comment on attachment 264990 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=264990&action=review >> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:212 >> + if (!this._endTimePinned && this.dirty) > > I like the layoutPending name better, at least as a public API. It does look better. We might want to keep the private variable named this._dirty, since it's consistent with this._dirtyDescendants. >> Source/WebInspectorUI/UserInterface/Views/View.js:220 >> + while (viewStack.length) { > > An array of arrays of views sounds unnecessarily complicated. How about just an array of views?: > > let views = [View._rootView]; > while (views.length) { > let view = views.shift(); > if (view.dirty) > view._layoutSubtree(); > else { > views = views.concat(view.subviews); > view._dirtyDescendants = 0; > } > } > > Note: > - this takes into account _layoutSubtree re-setting dirtyDescendants. > - no need to create a copy of the subviews lists if you don't modify those lists directly, here we just add the values to the main list > - JoePeck Fun Fact: "WebInspector.View" can just be "View" in this case, because we are inside "class View {...}" scope! > > Or just use the recursive solution, since the depth of our view tree should not be large at all. I was on autopilot and wrote it as a depth-first traversal. Definitely not the behavior we want. Nice catch. Created attachment 265107 [details]
[Patch] Proposed Fix
Comment on attachment 265107 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=265107&action=review r=me, nice work > Source/WebInspectorUI/UserInterface/Views/View.js:37 > + this._dirtyDescendants = 0; I would prefer `this._dirtyDescendantsCount` to emphasize it's a counter, not a list of descendants. > Source/WebInspectorUI/UserInterface/Views/View.js:212 > + let unscheduledLayouts = view._dirtyDescendants; This variable name took me a while to grok. Maybe `forcedLayoutsCount` or something? > Source/WebInspectorUI/UserInterface/Views/View.js:226 > + cancelAnimationFrame(WebInspector.View._scheduledLayoutUpdateIdentifier); Comment: 'No views need layout, so cancel the pending requestAnimationFrame.' Created attachment 265227 [details]
[Patch] Proposed Fix
Comment on attachment 265227 [details] [Patch] Proposed Fix Clearing flags on attachment: 265227 Committed r192272: <http://trac.webkit.org/changeset/192272> All reviewed patches have been landed. Closing bug. |