Bug 190497 - Web Inspector: Canvas Recording loading goes significantly slower when "Frame" tree element is expanded
Summary: Web Inspector: Canvas Recording loading goes significantly slower when "Frame...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: WebInspectorCanvasTab
  Show dependency treegraph
 
Reported: 2018-10-11 15:35 PDT by Joseph Pecoraro
Modified: 2018-10-25 18:34 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.92 KB, patch)
2018-10-12 00:02 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (10.60 KB, patch)
2018-10-12 00:21 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (10.58 KB, patch)
2018-10-12 00:29 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (13.05 KB, patch)
2018-10-12 16:10 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (13.93 KB, patch)
2018-10-16 18:01 PDT, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
Patch (13.93 KB, patch)
2018-10-25 16:22 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>