<rdar://problem/20491826>

*** Bug 149345 has been marked as a duplicate of this bug. ***

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.

<rdar://problem/16135388>

*** Bug 127182 has been marked as a duplicate of this bug. ***

Created attachment 375830 [details]
[Patch] WIP

This works with Map/Set/WeakMap/WeakSet.

Created attachment 376229 [details]
Patch

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.

Created attachment 376237 [details]
Patch

Created attachment 376635 [details]
Patch

Created attachment 376644 [details]
Patch

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

Created attachment 378939 [details]
Patch

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

Created attachment 379024 [details]
Patch

Created attachment 379025 [details]
[Image] After Patch is applied

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?

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.

We should follow the locale's setting for thousands separators.

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

Created attachment 379097 [details]
Patch

Comment on attachment 379097 [details]
Patch

Clearing flags on attachment: 379097

Committed r250087: <https://trac.webkit.org/changeset/250087>

All reviewed patches have been landed.  Closing bug.