[webkit-reviews] review granted: [Bug 143589] Web Inspector: Better handling for large arrays and collections in Object Trees : [Attachment 379024] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 143589: Web Inspector: Better handling for large arrays and collections in
Object Trees
https://bugs.webkit.org/show_bug.cgi?id=143589

Attachment 379024: Patch

https://bugs.webkit.org/attachment.cgi?id=379024&action=review




--- Comment #32 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 379024
  --> https://bugs.webkit.org/attachment.cgi?id=379024
Patch

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

r=me, with Comments.

If I show 9900 more elements on `new Uint8Array(10000)` then my window hangs
for about 10s, and sometimes is very slow afterwards. So I think we will need
to come up with a better UI. Just outputting all elements is clearly not good
enough.

I tend to think we should do that (a better UI) before landing, given this is
just a poor experience. It is not really worse than what we had before, but it
is a foot gun if clicking the button causes your machine to hang on a large
enough collection.

That said, UI can come later. This gets the required underpinnings. Great
tests.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:111
> +	   console.assert(typeof resolvedValue.size === "number" &&
resolvedValue.size > 0, resolvedValue);

This assertion fires (size > 0) when expanding any prototype, e.g. ( Array
Prototype ) in `dir([])`. Things still behave fine though.

    [Log] Trace
	assert
	addShowMoreIfNeeded (ObjectTreeView.js:111)
	_updateProperties (ObjectTreePropertyTreeElement.js:450)
	(anonymous function) (ObjectTreePropertyTreeElement.js:342)
	_getPropertyDescriptorsResolver (RemoteObject.js:665)
	_getPropertyDescriptorsResolver
	_dispatchResponseToCallback (Connection.js:148)

This assertion could be made to be `size >= 0`. Or,
ObjectTreePropertyTreeElement could avoid calling `addShowMoreIfNeeded` when
inside of a prototype (see comparison like `this.property.name ===
"__proto__"`).

> LayoutTests/inspector/runtime/getDisplayableProperties-expected.txt:19
> +    "0" [writable | enumerable | configurable | isOwn] => "red" (string)
> +    "1" [writable | enumerable | configurable | isOwn] => "green" (string)
> +    "2" [writable | enumerable | configurable | isOwn] => "blue" (string)
> +    "__proto__" [writable | configurable | enumerable | isOwn] => "Array"
(object array)

>From a test output reading perspective it may be easier to
`JSON.stringify(value).padEnd(15, " ")` the property name. Where 15 could be
the longest property length or an assumed value.

This would end up with more readable output:

    Properties:
	"0"	    [writable | enumerable | configurable | isOwn] => "red"
(string)
	"1"	    [writable | enumerable | configurable | isOwn] => "green"
(string)
	"2"	    [writable | enumerable | configurable | isOwn] => "blue"
(string)
	"__proto__" [writable | configurable | enumerable | isOwn] => "Array"
(object array)

Still not perfect but better. Especially when things are almost always the
same.

For instance here it is interesting to note that `configurable` is coming
before `enumerable` on __proto__. Perhaps we should sort, or initialize these
in the same order in the backend.

If we did a sort, I'd probably suggest isOwn at the start since those would
likely all align.

> LayoutTests/inspector/runtime/getDisplayableProperties.html:77
> +    addTestCase({
> +	   name: "Runtime.getDisplayableProperties.BoundConstructorArguments",
> +	   expression: `(class Test { }).bind(null, 1, 2, 3)`,
> +    });

Nice! These tests are great for showing Internal Properties.

I'd suggest adding in a resolved / rejected Promise `Promise.resolve(123)` test
for showing internal Promise state.

> LayoutTests/inspector/runtime/resources/property-descriptor-utilities.js:21
> +    return new Set(makeArray(length).map((item) => item));

This could just be `new Set(makeArray(length))` but it may be fine to keep with
the map to preserve the pattern.

> LayoutTests/inspector/runtime/resources/property-descriptor-utilities.js:33
> +    ProtocolTest.PropertyDescriptorUtilities = {};

Make this a class?

    ProtocolTest.PropertyDescriptorUtilities = class {
	static log({name, value, ...extra}) { ... }
	static stringifyRemoteObject(remoteObject) { ... }
    };

Or would that make searching for `ProtocolTest.PropertyDescriptorUtilities.log`
harder?