Bug 143589 - Web Inspector: Better handling for large arrays and collections in Object Trees
Summary: Web Inspector: Better handling for large arrays and collections in Object Trees
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
: 127182 149345 (view as bug list)
Depends on: 200549
Blocks: 201909 201965 202934
  Show dependency treegraph
 
Reported: 2015-04-09 16:54 PDT by Joseph Pecoraro
Modified: 2019-11-05 17:18 PST (History)
16 users (show)

See Also:


Attachments
[Patch] WIP (19.89 KB, patch)
2019-08-08 11:54 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (128.12 KB, patch)
2019-08-13 19:38 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.20 MB, application/zip)
2019-08-13 20:44 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.08 MB, application/zip)
2019-08-13 21:39 PDT, Build Bot
no flags Details
Patch (128.00 KB, patch)
2019-08-13 21:53 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.22 MB, application/zip)
2019-08-13 22:58 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-highsierra (2.99 MB, application/zip)
2019-08-13 23:41 PDT, Build Bot
no flags Details
Patch (128.69 KB, patch)
2019-08-18 04:57 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (3.22 MB, application/zip)
2019-08-18 06:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.00 MB, application/zip)
2019-08-18 06:47 PDT, Build Bot
no flags Details
Patch (128.74 KB, patch)
2019-08-18 10:57 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (123.44 KB, patch)
2019-09-16 22:16 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (122.42 KB, patch)
2019-09-18 00:56 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (29.96 KB, image/png)
2019-09-18 00:56 PDT, Devin Rousso
no flags Details
Patch (125.35 KB, patch)
2019-09-18 19:45 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 2015-04-09 16:54:45 PDT
* SUMMARY
Better handling for large collections in Object Trees.

  - arrays / maps / sets with lots of entries (100, 1000, 10000, etc)
  - node lists with lots of items
  - objects with lots and lots of properties
Comment 1 Radar WebKit Bug Importer 2015-04-09 16:55:27 PDT
<rdar://problem/20491826>
Comment 2 Joseph Pecoraro 2015-09-18 12:09:17 PDT
*** Bug 149345 has been marked as a duplicate of this bug. ***
Comment 3 Joseph Pecoraro 2015-09-18 12:16:49 PDT
Currently we only show the first 100 values in collections. A good next step would be some pagination UI to show more values.

I'm of the opinion that it pretty much never makes sense to expand and see all the values of a 100+ element collections.

The best case I can think of for myself would be doing a querySelectorAll and scanning the list of potentially many elements to visually see if it contains an element that I did not want. Still, pagination could work okay here if there isn't too much seesawing with scrolling.

I think most cases of looking at 100+ element collections you want to look at slices. For example the first 100, the last 100, some middle section around index 240, etc. A block of 100 elements gives you some context without making the console difficult to use when a large list is expanded.

Set / Map have a well defined order of properties, so pagination works well with them. WeakMap / WeakSet do not, so this could be tricky for those collections.

The protocol already has a means of fetching a range of elements in a Collection (Set/Map). It can be added for Array. What is missing here is a UI for it.
Comment 4 Timothy Hatcher 2016-02-18 09:41:17 PST
<rdar://problem/16135388>
Comment 5 Brian Burg 2018-08-23 13:37:28 PDT
*** Bug 127182 has been marked as a duplicate of this bug. ***
Comment 6 Devin Rousso 2019-08-08 11:54:18 PDT
Created attachment 375830 [details]
[Patch] WIP

This works with Map/Set/WeakMap/WeakSet.
Comment 7 Devin Rousso 2019-08-13 19:38:58 PDT
Created attachment 376229 [details]
Patch
Comment 8 Devin Rousso 2019-08-13 19:39:59 PDT
Comment on attachment 376229 [details]
Patch

I think the UI for this could be improved, but I'm focusing here on getting the protocol working (and in a somewhat future-proofed state), so the UI is more basic.
Comment 9 Build Bot 2019-08-13 19:40:35 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2019-08-13 20:44:08 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2019-08-13 20:44:09 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2019-08-13 21:39:01 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2019-08-13 21:39:03 PDT Comment hidden (obsolete)
Comment 14 Devin Rousso 2019-08-13 21:53:19 PDT
Created attachment 376237 [details]
Patch
Comment 15 Build Bot 2019-08-13 22:58:43 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2019-08-13 22:58:45 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2019-08-13 23:41:41 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2019-08-13 23:41:43 PDT Comment hidden (obsolete)
Comment 19 Devin Rousso 2019-08-18 04:57:30 PDT
Created attachment 376635 [details]
Patch
Comment 20 Build Bot 2019-08-18 06:02:02 PDT Comment hidden (obsolete)
Comment 21 Build Bot 2019-08-18 06:02:04 PDT Comment hidden (obsolete)
Comment 22 Build Bot 2019-08-18 06:47:10 PDT Comment hidden (obsolete)
Comment 23 Build Bot 2019-08-18 06:47:12 PDT Comment hidden (obsolete)
Comment 24 Devin Rousso 2019-08-18 10:57:02 PDT
Created attachment 376644 [details]
Patch
Comment 25 Joseph Pecoraro 2019-09-13 20:04:28 PDT
Comment on attachment 376644 [details]
Patch

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

Nice! Lets rebase this and I'll give the patch a closer look as soon as possible. The backend bits looked good. I really like the consistency of converging on `fetchStart` and `fetchCount` for all types.

Unless I missed something I think this will need to better handle super-large collections though.

> Source/WebInspectorUI/ChangeLog:53
> +2019-08-08  Devin Rousso  <drousso@apple.com>

Nit: Double ChangeLog

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:-743
> -        // FIXME: <https://webkit.org/b/143589> Web Inspector: Better handling for large collections in Object Trees
> -        // For array types with a large length we attempt to skip getOwnPropertyNames and instead just sublist of indexes.

I think `isArrayLike` worked around the fact that `Object.getOwnPropertyNames(o)` for a TypedArray of 1000000 returns a massive array. Hence this comment!

For example:

    console.time();
    console.log(Object.getOwnPropertyNames(new Uint8Array(1000000)).length);
    console.timeEnd();

Outputs on my very fast iMacPro:

    [Log] 1000000
    [Debug] default: 312.283ms

Not to mention the temporary memory spike of a million strings of varying length.

So I think we will still want to keep the isArrayLike path:

   1. For performance
   2. To avoid running out of memory

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:475
> +    auto weakMapArg = exec->uncheckedArgument(0);
> +    auto* weakMap = jsDynamicCast<JSWeakMap*>(vm, weakMapArg);

I think the auto works well for the jsDynamicCast lines because it is clear what the type is. But it is not as clear for the uncheckedArgument calls. I'd rather the uncheckedArgument line stay JSValue so the type is clear.

In these cases I suppose the uncheckedArgument call could just be inlined into the cast.

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:255
> +    if (!start)

Lets add the same comment as __proto__ that we only get the internal properties with the early fetch. I see the comment in the protocol JSON but it would be helpful here too.

> Source/JavaScriptCore/inspector/protocol/Runtime.json:277
> +                { "name": "fetchStart", "optional": true, "type": "integer", "description": "If provided skip to this value before collecting values. Otherwise, start at the beginning. Has no effect when the <code>objectId</code> is for a <code>iterator</code>/<code>WeakMap</code>/<code>WeakSet</code> object." },

Aside: I'm thinking at this point that we consider using Markdown in the descriptions. it is easier to read and we don't use the HTML anywhere. So instead of <code>Foo</code> we could `Foo` and it would be much easier to read.

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:158
> +    static get fetchCount() { return 100; }

Seems weird to put this here. RemoteObject never uses it, and our UI uses it. Maybe RuntimeManager would be a better place for this?

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:607
> +        // COMPATIBILITY (iOS 13): `result` was renamed to `properties` (but kept in the same position).

Is this comment valuable? This invoke is only the parameters, not the returned values, so the callback would be what is affected and nothing changes there.

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:617
> +    _getDisplayableProperties(callback, options = {})

Style: What would you think about destructuring here?

    getDisplayableProperties(callbacks, {fetchStart, fetchCount, generatePreview} = {})

That will make code search a bit better when I find a signature here. I find destructuring as a param works best when it is options. We end up with `options = {}` self documenting `{optionA, optionB} = {}`.

> LayoutTests/inspector/model/remote-object-expected.txt:1190
> -EXPRESSION: var buffer = new ArrayBuffer(10000000); var int8View = new Int8Array(buffer); int8View
> +EXPRESSION: var buffer = new ArrayBuffer(100); var int8View = new Int8Array(buffer); int8View

The large size is probably very valuable to test! We should still have a large array test, if not in this file than another.
Comment 26 Devin Rousso 2019-09-16 21:49:13 PDT
Comment on attachment 376644 [details]
Patch

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

>> Source/JavaScriptCore/inspector/InjectedScriptSource.js:-743
>> -        // For array types with a large length we attempt to skip getOwnPropertyNames and instead just sublist of indexes.
> 
> I think `isArrayLike` worked around the fact that `Object.getOwnPropertyNames(o)` for a TypedArray of 1000000 returns a massive array. Hence this comment!
> 
> For example:
> 
>     console.time();
>     console.log(Object.getOwnPropertyNames(new Uint8Array(1000000)).length);
>     console.timeEnd();
> 
> Outputs on my very fast iMacPro:
> 
>     [Log] 1000000
>     [Debug] default: 312.283ms
> 
> Not to mention the temporary memory spike of a million strings of varying length.
> 
> So I think we will still want to keep the isArrayLike path:
> 
>    1. For performance
>    2. To avoid running out of memory

One kinda "sucky" part of this is that we don't show autocompletion for any custom values on array-like objects.  But given that that's what we have now, I'm inclined to leave this as is and have it as a followup.

>> Source/JavaScriptCore/inspector/protocol/Runtime.json:277
>> +                { "name": "fetchStart", "optional": true, "type": "integer", "description": "If provided skip to this value before collecting values. Otherwise, start at the beginning. Has no effect when the <code>objectId</code> is for a <code>iterator</code>/<code>WeakMap</code>/<code>WeakSet</code> object." },
> 
> Aside: I'm thinking at this point that we consider using Markdown in the descriptions. it is easier to read and we don't use the HTML anywhere. So instead of <code>Foo</code> we could `Foo` and it would be much easier to read.

I like it!

>> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:607
>> +        // COMPATIBILITY (iOS 13): `result` was renamed to `properties` (but kept in the same position).
> 
> Is this comment valuable? This invoke is only the parameters, not the returned values, so the callback would be what is affected and nothing changes there.

This is here in case we change from a callback to a promise, where the name of the parameter _does_ matter.  It's to prevent a potential future foot-gun.
Comment 27 Devin Rousso 2019-09-16 22:16:34 PDT
Created attachment 378939 [details]
Patch
Comment 28 Joseph Pecoraro 2019-09-17 19:07:13 PDT
Comment on attachment 378939 [details]
Patch

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

I can end up seeing multiple Object Prototype buttons.

    Issue:
    1. js> dir([])
    2. Expand Array Prototype button
      => I see two "Object Prototype" buttons

I'd expect the Show More buttons to be side by side. Currently this patch shows:

    js> new Uint8Array(1000);
    0: 0
    1: 0
    ...
    99: 0
    ( Show 100 More )
    ( Show All (900 More) )

I'd have expected:

    js> new Uint8Array(1000);
    0: 0
    1: 0
    ...
    99: 0
    ( Show 100 More ) ( Show All (900 More) )

We can iterate on the UI later but it is always worth a screenshot attached to the bug if you're adding / non trivially changing UI.

I have a few more things to review (tests), but r- for now given the issues.

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProvider.js:258
>              if (propertyNames && typeof propertyNames.length === "number") {
> -                var max = Math.min(propertyNames.length, 1000);
> -                for (var i = 0; i < max; ++i)
> +                for (let i = 0; i < propertyNames.length; ++i)
>                      propertyNames[i] = true;
>              }

Does this mean we create a completion list of 1000000 entries if the object is large? We probably shouldn't do that. It would be better for the completion controller generate numbers to handle a number range, instead of an object with millions of keys.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreePropertyTreeElement.js:36
> +        this._fetchEnd = WI.ObjectTreeView.showMoreFetchCount;

`WI.ObjectTreeView.showMoreFetchCount` is not a getter, so every time this happens this._fetchEnd is a function.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreePropertyTreeElement.js:435
> +                hadProto = true;
> +                this.appendChild(new WI.ObjectTreePropertyTreeElement(propertyDescriptor, propertyPath, mode, prototypeName));

Either we should `continue` here or also check `hadProto` around line 442 to avoid creating a second __proto__ button.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:106
> +    static showMoreFetchCount() { return 100; }

This should be a getter, otherwise everything below is totally broken.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:114
> +        console.assert(typeof handleShowMoreClicked === "function", handleShowMoreClicked);

Might as well assert for `handleShowAllClicked`. Perhaps also assert `typeof resolvedValue.size === "number"` since that is used below; we're assuming resolvedValue is a collection / array type with a size.

> LayoutTests/inspector/model/remote-object-get-properties-expected.txt:44
> +    length

I suspect these changes need to be reverted.
Comment 29 Devin Rousso 2019-09-18 00:51:25 PDT
Comment on attachment 378939 [details]
Patch

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

Apologies about the screenshot.  I could've sworn I'd uploaded one 🤔

>> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProvider.js:258
>>              }
> 
> Does this mean we create a completion list of 1000000 entries if the object is large? We probably shouldn't do that. It would be better for the completion controller generate numbers to handle a number range, instead of an object with millions of keys.

That's true.  We don't want to fill the autocomplete list with a list item for each index.  Frankly, I'm not sure how useful an autocomplete of index keys really is, but given that it exists now, I won't remove it.  I'll revert this part and create a followup bug to do something sane for this case.

>> Source/WebInspectorUI/UserInterface/Views/ObjectTreePropertyTreeElement.js:435
>> +                this.appendChild(new WI.ObjectTreePropertyTreeElement(propertyDescriptor, propertyPath, mode, prototypeName));
> 
> Either we should `continue` here or also check `hadProto` around line 442 to avoid creating a second __proto__ button.

Yup!  `continue`

>> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:106
>> +    static showMoreFetchCount() { return 100; }
> 
> This should be a getter, otherwise everything below is totally broken.

Oops.  Nice catch!

>> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:114
>> +        console.assert(typeof handleShowMoreClicked === "function", handleShowMoreClicked);
> 
> Might as well assert for `handleShowAllClicked`. Perhaps also assert `typeof resolvedValue.size === "number"` since that is used below; we're assuming resolvedValue is a collection / array type with a size.

I like it!

>> LayoutTests/inspector/model/remote-object-get-properties-expected.txt:44
>> +    length
> 
> I suspect these changes need to be reverted.

Yup.
Comment 30 Devin Rousso 2019-09-18 00:56:12 PDT
Created attachment 379024 [details]
Patch
Comment 31 Devin Rousso 2019-09-18 00:56:19 PDT
Created attachment 379025 [details]
[Image] After Patch is applied
Comment 32 Joseph Pecoraro 2019-09-18 12:31:40 PDT
Comment on attachment 379024 [details]
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?
Comment 33 Joseph Pecoraro 2019-09-18 12:51:02 PDT
Also I kind of think we should avoid the commas. "1000" versus "1,000". I tend to prefer the former. Would like to know what others think.
Comment 34 Greg Marriott 2019-09-18 12:54:14 PDT
We should follow the locale's setting for thousands separators.
Comment 35 Joseph Pecoraro 2019-09-18 13:30:35 PDT
(In reply to Greg Marriott from comment #34)
> We should follow the locale's setting for thousands separators.

Yep, we already do.

But I find thousands separators weird in many places. For example in time measurements I think we would normally say "1234ms" or "1.234s" instead of "1,234ms". Likewise for the 1234th index into an array (though in that case the key is literally 1234 or "1234").
Comment 36 Devin Rousso 2019-09-18 19:36:08 PDT
Comment on attachment 379024 [details]
Patch

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

> 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.
Yes, and this performance gets even worse the larger the array/collection gets.

> 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.
I agree.  My goal with this was to get a reasonable and robust protocol implemented with some non-crazy UI, and then work on a better UI that doesn't need as many backend changes.

<https://webkit.org/b/201965> Web Inspector: allow examining items in the middle of an array/collection without having to show all items before

> That said, UI can come later. This gets the required underpinnings. Great tests.
Agreed.  Thanks!

>> 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__"`).

Ah yeah, we should gate/assert that we only call this for arrays or collections, as objects don't support this type of "segmentation".

>> LayoutTests/inspector/runtime/getDisplayableProperties-expected.txt:19
>> +    "__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.

I actually realized that what I had here isn't correct.  We should only log items that are truthy, as `Object.getOwnPropertyDescriptor` gives us an object that has keys with `false` too.

>> LayoutTests/inspector/runtime/getDisplayableProperties.html:77
>> +    });
> 
> 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.

Oops.  Missed that.  Good catch!

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

😂 I have no idea why I did this.  I'll remove it.

>> 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?

We normally keep these as objects.  I think it's slightly better that way for the reason you described, as well as the interplay with functions that shouldn't be referenceable by the test including the utility file (e.g. a local function).  If we made this a class, then we'd likely have some local non-class functions that are used by static functions, and that seems a bit weird.
Comment 37 Devin Rousso 2019-09-18 19:45:55 PDT
Created attachment 379097 [details]
Patch
Comment 38 WebKit Commit Bot 2019-09-18 22:58:34 PDT
Comment on attachment 379097 [details]
Patch

Clearing flags on attachment: 379097

Committed r250087: <https://trac.webkit.org/changeset/250087>
Comment 39 WebKit Commit Bot 2019-09-18 22:58:36 PDT
All reviewed patches have been landed.  Closing bug.