Bug 190497 – Web Inspector: Canvas Recording loading goes significantly slower when "Frame" tree element is expanded

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

[Joseph Pecoraro]

Canvas Recording loading goes significantly slower when "Frame" tree element is expanded

Steps to Reproduce:
1. Inspect <https://devinrousso.com/demo/DoubleHelixCircle.html>
2. Make Inspector Window very large
3. Capture a single frame recording
  => Notice frame loading is going smoothly
4. Expand "Frame 1" tree element
  => Loading goes significantly slower

[Devin Rousso]

Created attachment 352146 [details]
Patch

[Devin Rousso]

Created attachment 352147 [details]
Patch

Remove the previous system of remembering the index in favor of thottling

[Devin Rousso]

Created attachment 352148 [details]
Patch

[Devin Rousso]

Created attachment 352229 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 352229 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:142
> +Object.defineProperty(Set.prototype, "contains",
> +{
> +    value(other)
> +    {
> +        if (!this.size || !other.size)
> +            return false;

Nit: Should we name this `containsSet` or invert it and name `isSubsetOf`? Right now `contains` sounds just like `has` given the API name in other places...

Shouldn't a non-empty Set contain the empty Set?

    let s = new Set([1]);
    let x = new Set;
    s.contains(x); // expect true

In which case we would just drop this early return.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:737
> +                    return;

This early return seems super sketchy. It looks ike the spacers are being resized below, which I would expect might need to always happen.

> LayoutTests/inspector/unit-tests/set-utilities.html:25
> +            testFalse([], [], "empy sets should not intersect.");

Typo: "empy" => "empty"

> LayoutTests/inspector/unit-tests/set-utilities.html:49
> +            testFalse([], [], "an empy sets should not contain another empty set.");

Typo: "empy" => "empty"

> LayoutTests/inspector/unit-tests/set-utilities.html:53
> +

I'd expect a non-empty set to contain the empty set. Need a test for that since currently the results doesn't match my expectations.

[Devin Rousso]

Comment on attachment 352229 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:737
>> +                    return;
> 
> This early return seems super sketchy. It looks ike the spacers are being resized below, which I would expect might need to always happen.

We don't want to update the spacers unless we are moving DOM nodes around.  If we aren't adding/removing DOM nodes, regular scrolling works as expected.  The only time we want to update spacers is when we are trying to add new elements that are in a different scroll position than what was shown before.

[Devin Rousso]

Created attachment 352529 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 352529 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:134
> +    },

Let's remove the trailing comma on these because we don't ever expect any other properties.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:147
> +    },

Let's remove the trailing comma on these because we don't ever expect any other properties.

[Devin Rousso]

Created attachment 353126 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 353126 [details]
Patch

Clearing flags on attachment: 353126

Committed r237436: <https://trac.webkit.org/changeset/237436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45572257>