Bug 145177 - Web Inspector: Improve Preview for NodeList / array like collections
Summary: Web Inspector: Improve Preview for NodeList / array like collections
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: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-05-19 13:28 PDT by Joseph Pecoraro
Modified: 2015-05-19 17:36 PDT (History)
9 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (7.49 KB, patch)
2015-05-19 13:35 PDT, Joseph Pecoraro
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-05-19 13:28:45 PDT
* 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.
Comment 1 Joseph Pecoraro 2015-05-19 13:29:00 PDT
<rdar://problem/20537340>
Comment 2 Joseph Pecoraro 2015-05-19 13:35:07 PDT
Created attachment 253393 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2015-05-19 13:38:39 PDT
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.
Comment 4 Joseph Pecoraro 2015-05-19 15:51:37 PDT
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.
Comment 5 Timothy Hatcher 2015-05-19 16:47:14 PDT
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.
Comment 6 WebKit Commit Bot 2015-05-19 17:36:00 PDT
Comment on attachment 253393 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 253393

Committed r184602: <http://trac.webkit.org/changeset/184602>
Comment 7 WebKit Commit Bot 2015-05-19 17:36:05 PDT
All reviewed patches have been landed.  Closing bug.