[webkit-reviews] review granted: [Bug 188960] Web Inspector: when scrolling a virtualized TreeOutline, only update the DOM periodically : [Attachment 348116] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 188960: Web Inspector: when scrolling a virtualized TreeOutline, only
update the DOM periodically
https://bugs.webkit.org/show_bug.cgi?id=188960

Attachment 348116: Patch

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




--- Comment #2 from Brian Burg <bburg at apple.com> ---
Comment on attachment 348116
  --> https://bugs.webkit.org/attachment.cgi?id=348116
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.