Bug 190497

Summary: Web Inspector: Canvas Recording loading goes significantly slower when "Frame" tree element is expanded
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=190484
https://bugs.webkit.org/show_bug.cgi?id=190483
Bug Depends on:    
Bug Blocks: 175485    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
joepeck: review+
Patch none

Description Joseph Pecoraro 2018-10-11 15:35:25 PDT
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
Comment 1 Devin Rousso 2018-10-12 00:02:47 PDT
Created attachment 352146 [details]
Patch
Comment 2 Devin Rousso 2018-10-12 00:21:11 PDT
Created attachment 352147 [details]
Patch

Remove the previous system of remembering the index in favor of thottling
Comment 3 Devin Rousso 2018-10-12 00:29:10 PDT
Created attachment 352148 [details]
Patch
Comment 4 Devin Rousso 2018-10-12 16:10:38 PDT
Created attachment 352229 [details]
Patch
Comment 5 Joseph Pecoraro 2018-10-16 17:07:03 PDT
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.
Comment 6 Devin Rousso 2018-10-16 18:00:04 PDT
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.
Comment 7 Devin Rousso 2018-10-16 18:01:27 PDT
Created attachment 352529 [details]
Patch
Comment 8 Joseph Pecoraro 2018-10-25 13:57:39 PDT
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.
Comment 9 Devin Rousso 2018-10-25 16:22:10 PDT
Created attachment 353126 [details]
Patch
Comment 10 WebKit Commit Bot 2018-10-25 16:59:51 PDT
Comment on attachment 353126 [details]
Patch

Clearing flags on attachment: 353126

Committed r237436: <https://trac.webkit.org/changeset/237436>
Comment 11 WebKit Commit Bot 2018-10-25 16:59:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-10-25 17:01:04 PDT
<rdar://problem/45572257>