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 Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer

Priority:

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

Joseph Pecoraro

Reported 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

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
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Devin Rousso

Comment 1 2018-10-12 00:02:47 PDT

Created attachment 352146 [details] Patch

Devin Rousso

Comment 2 2018-10-12 00:21:11 PDT

Created attachment 352147 [details] Patch Remove the previous system of remembering the index in favor of thottling

Devin Rousso

Comment 3 2018-10-12 00:29:10 PDT

Created attachment 352148 [details] Patch

Devin Rousso

Comment 4 2018-10-12 16:10:38 PDT

Created attachment 352229 [details] Patch

Joseph Pecoraro

Comment 5 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.

Devin Rousso

Comment 6 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.

Devin Rousso

Comment 7 2018-10-16 18:01:27 PDT

Created attachment 352529 [details] Patch

Joseph Pecoraro

Comment 8 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.

Devin Rousso

Comment 9 2018-10-25 16:22:10 PDT

Created attachment 353126 [details] Patch

WebKit Commit Bot

Comment 10 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>

WebKit Commit Bot

Comment 11 2018-10-25 16:59:53 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 12 2018-10-25 17:01:04 PDT

<rdar://problem/45572257>

Note You need to log in before you can comment on or make changes to this bug.