Bug 150993 – Web Inspector: Batch all scheduled layouts and update the view tree in a single animation frame

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

[Matt Baker]

* SUMMARY
Batch all scheduled updates and layout view tree in a single animation frame.

[Matt Baker]

Created attachment 264990 [details]
[Patch] WIP

[Matt Baker]

Noticed a bug in View._unscheduleLayoutForView. Will fix for the next WIP.

[Matt Baker]

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.

[Joseph Pecoraro]

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.

[Timothy Hatcher]

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.

[Radar WebKit Bug Importer]

<rdar://problem/23454092>

[Matt Baker]

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.

[Matt Baker]

Created attachment 265107 [details]
[Patch] Proposed Fix

[Blaze Burg]

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.'

[Matt Baker]

Created attachment 265227 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

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

Clearing flags on attachment: 265227

Committed r192272: <http://trac.webkit.org/changeset/192272>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.