Bug 188960

Created attachment 348116 [details] Patch

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.

<rdar://problem/43766257>

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.

Created attachment 348193 [details] Patch

Comment on attachment 348193 [details] Patch Clearing flags on attachment: 348193 Committed r235401: <https://trac.webkit.org/changeset/235401>

All reviewed patches have been landed. Closing bug.