Bug 145177 – Web Inspector: Improve Preview for NodeList / array like collections

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
Improve Preview for NodeList / array like collections

* ACTUAL

   js> document.querySelectorAll("nothing")
   <- ▶︎ [item: function] (0) = $1

* EXPECTED

   js> document.querySelectorAll("nothing")
   <- NodeList [] (0) = $1

* NOTES
- Show just indices. Previously we only skipped "length" and "constructor". Now for any "array" subtype, just get indices.
- I think we should show the Array type name (e.g. NodeList) next to the array, to show this is not an actual "Array" Array
- For simplicity, I think we should aim for making these lossless even if there are non-index properties. I don't have a good counter-example yet. The closest I've seen in Int8Array and other TypedArrays where you might want to allow expansion to see the arrayBuffer, etc. Worst case, console.dir() will allow expanding in that case, or a large list.

[Joseph Pecoraro]

<rdar://problem/20537340>

[Joseph Pecoraro]

Created attachment 253393 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 253393 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1058
> +            // For arrays, only allow indexes.
> +            if (this.subtype === "array" && !isUInt32(name))
>                  continue;

This is what we may want to change to be something like:

    if (this.subtype === "array" && !isUInt32(name)) {
        if (name !== "length" && "value" in descriptor && typeof descriptor.value !== "function")
            preview.lossless = false;
        continue;
    }

Which would mean "if there is an array type and it has a value property that is not a function allow it to be expanded".

I'm on the fence about this.

[Joseph Pecoraro]

Comment on attachment 253393 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1058
>>                  continue;
> 
> This is what we may want to change to be something like:
> 
>     if (this.subtype === "array" && !isUInt32(name)) {
>         if (name !== "length" && "value" in descriptor && typeof descriptor.value !== "function")
>             preview.lossless = false;
>         continue;
>     }
> 
> Which would mean "if there is an array type and it has a value property that is not a function allow it to be expanded".
> 
> I'm on the fence about this.

Given we can't even see non-index properties in ObjectTreeViews at all I'm no longer on the fence.

We should address non-index property handling in bug 145190. I've expanded on this in that description.

[Timothy Hatcher]

Comment on attachment 253393 [details]
[PATCH] Proposed Fix

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

>>> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1058
>>>                  continue;
>> 
>> This is what we may want to change to be something like:
>> 
>>     if (this.subtype === "array" && !isUInt32(name)) {
>>         if (name !== "length" && "value" in descriptor && typeof descriptor.value !== "function")
>>             preview.lossless = false;
>>         continue;
>>     }
>> 
>> Which would mean "if there is an array type and it has a value property that is not a function allow it to be expanded".
>> 
>> I'm on the fence about this.
> 
> Given we can't even see non-index properties in ObjectTreeViews at all I'm no longer on the fence.
> 
> We should address non-index property handling in bug 145190. I've expanded on this in that description.

Makes sense to me. I think we should have a way to see these properties on expand, which bug 145190 covers.

[WebKit Commit Bot]

Comment on attachment 253393 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 253393

Committed r184602: <http://trac.webkit.org/changeset/184602>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.