Bug 192648 – Web Inspector: Debugger: virtualize the list of variables in the Scope sidebar

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

On pages with lots of variables per-scope (e.g. most minified scripts), WebInspector can slow down significantly as we try to create/render a `WI.ObjectTreeView` per-variable.

[Radar WebKit Bug Importer]

<rdar://problem/46800949>

[Devin Rousso]

Created attachment 358179 [details]
Patch

[EWS Watchlist]

Comment on attachment 358179 [details]
Patch

Attachment 358179 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10603406

New failing tests:
imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html

[Joseph Pecoraro]

Comment on attachment 358179 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:713
> +        const observerHandler = (entries) => {
> +            console.assert(entries.length === 1);
> +            if (entries[0].isIntersecting)
> +                this.updateVirtualizedElements({shouldRefresh: true});
> +        };
> +        const observerOptions = {
> +            root: this._virtualizedScrollContainer,
> +            threshold: [0],
> +        };
> +        let observer = new IntersectionObserver(observerHandler, observerOptions);
> +        observer.observe(this.element);

What is this doing that the scroll handler is not doing? Is it supposed to be handling the first display of the element?

I did notice this assert firing quite a bit in local testing.

            console.assert(entries.length === 1);

I also noticed this handler firing a lot during scrolling, which seems like it would maybe eliminate the need for the throttler?

I also noticed an IntersectionObserver crash, which I've filed appropriately.

[Devin Rousso]

Comment on attachment 358179 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:713
>> +        observer.observe(this.element);
> 
> What is this doing that the scroll handler is not doing? Is it supposed to be handling the first display of the element?
> 
> I did notice this assert firing quite a bit in local testing.
> 
>             console.assert(entries.length === 1);
> 
> I also noticed this handler firing a lot during scrolling, which seems like it would maybe eliminate the need for the throttler?
> 
> I also noticed an IntersectionObserver crash, which I've filed appropriately.

Yeah, this is supposed to handle the case where you start out with it being visible, but I think the `updateVirtualizedElements` call below handles that just as well.  I'll remove this unless there is some other case I'm forgetting (or didn't know about) that needs this.

As far as the assertions, my guess is that in the process of adding/resizing the top/bottom spacers, we cause the element to shift suddenly.  I'll investigate a bit more.

[Joseph Pecoraro]

Comment on attachment 358179 [details]
Patch

r- for now given the assertions I was seeing imply something amiss. If you can't reproduce let me know.

[Devin Rousso]

Created attachment 364437 [details]
Patch

[EWS Watchlist]

Comment on attachment 364437 [details]
Patch

Attachment 364437 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11482702

New failing tests:
http/tests/cache/memory-cache-pruning.html

[EWS Watchlist]

Created attachment 364485 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[Joseph Pecoraro]

Comment on attachment 364437 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:588
> +                return true;

Nice. Should update the existing Array.prototype.remove tests in:
LayoutTests/inspector/unit-tests/array-utilities.html

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:452
> +        if (this.virtualized)
> +            this._virtualizedDebouncer.delayForFrame();

Previously we would only delayForFrame if the element was not known. Do we want to do this always now (probably fine).

[Devin Rousso]

Comment on attachment 364437 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:452
>> +            this._virtualizedDebouncer.delayForFrame();
> 
> Previously we would only delayForFrame if the element was not known. Do we want to do this always now (probably fine).

I don't think there's a difference.  It should be an error for us to re-add the same element twice without removing it in between.  Always doing it also gives us an "assurance" that each time we try to add an element, we should try to show it.

[Devin Rousso]

Created attachment 365395 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365395 [details]
Patch

Clearing flags on attachment: 365395

Committed r243242: <https://trac.webkit.org/changeset/243242>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.