Bug 188960 – Web Inspector: when scrolling a virtualized TreeOutline, only update the DOM periodically

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

[Devin Rousso]

Currently, we add/remove all TreeElement from the DOM every time the TreeOutline is scrolled.  This is wasteful, especially if the user is just scrolling a few pixels and the TreeOutline has a lot of children.  We should be smarter about when we want to recalculate the needed DOM elements when scrolling.

[Devin Rousso]

Created attachment 348116 [details]
Patch

[Blaze Burg]

Comment on attachment 348116 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:8
> +        * UserInterface/Views/TreeOutline.js:

Needs explanation.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:58
> +        this._vritualizedPreviousFirstItem = NaN;

Typo. Also I don't like this name, is there a way to better convey that this is the last point from which we selected rows and over-rendered them? For all intents, this is the "current" virtualized first item until updateVirtualizedElements() is called again, after which we don't care about the previous value.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:713
> +            shouldScroll = (index < firstItem + extraRows) || (index > lastItem - extraRows);

Okay, so this means we are over-rendering by 2*extraRows, right? Above and below? That's what I would expect this code to do, but you haven't written a changelog so I could be reading the code wrong.

I don't remember who wrote this code in the first place. Have you looked to see how quickly we can render the total number of rows? It might need to decrease in order to fit the frame budget.

In any case, this improvement should be much much faster than the previous which seemed to not do any over-rendering at all.

[Radar WebKit Bug Importer]

<rdar://problem/43766257>

[Devin Rousso]

Comment on attachment 348116 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:713
>> +            shouldScroll = (index < firstItem + extraRows) || (index > lastItem - extraRows);
> 
> Okay, so this means we are over-rendering by 2*extraRows, right? Above and below? That's what I would expect this code to do, but you haven't written a changelog so I could be reading the code wrong.
> 
> I don't remember who wrote this code in the first place. Have you looked to see how quickly we can render the total number of rows? It might need to decrease in order to fit the frame budget.
> 
> In any case, this improvement should be much much faster than the previous which seemed to not do any over-rendering at all.

I actually was the one who wrote it 😅.  Your expectation is correct.  We render `extraRows` on either "side" (above and below) in order to give us a bit of a buffer with regards to scrolling.  There hasn't been any issues as far as the frame budget in the past, and we used to regenerate the entire `WI.TreeOutline` DOM on every scroll event, so this should be MUCH better.

[Devin Rousso]

Created attachment 348193 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 348193 [details]
Patch

Clearing flags on attachment: 348193

Committed r235401: <https://trac.webkit.org/changeset/235401>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.