Web Inspector: Properties on Arrays and NodeLists are not logged correctly

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Timothy Hatcher]

If you log an Array that has a non-numeric property on it we will output the array with the non-numeric property at the end. But we wont show the property key. And NodeLists have an enumerible "length" property, so we show the length.

We should just skip non-numeric properties and require you to use dir(array) to see everything.

Run this in the console:

a = ["test", "test2"]; a.foo = "bar"; a

[Keishi Hattori]

Created attachment 41382 [details]
proposed patch

Adds InjectedScript.getArrayElements to enumerate just the array elements.

[Timothy Hatcher]

Comment on attachment 41382 [details]
proposed patch

> +    var i;
> +    for (i = 0; i < object.length; i++) {

This should be one line as:

    for (var i = 0; i < object.length; ++i) {


> +    }
> +    return elements;

Please put a new line between these two lines.

I would like Pavel to review this to make sure everything is covered.

[Pavel Feldman]

I've actually started working on this one, but chose different approach: I was filtering indexes on the frontend side by their type (don't think we need a dedicated method for that in the injectedscript).

I did not upload the patch since I wanted to do a test for it first. Now that tests are there I can put it in. Sorry about the confusion. I should have assigned the bug to myself. Keishi, do you think my version makes sense?

[Pavel Feldman]

Created attachment 41389 [details]
patch with no changes to InjectedScript (and a test!)

[Timothy Hatcher]

(In reply to comment #3)
> I've actually started working on this one, but chose different approach: I was
> filtering indexes on the frontend side by their type (don't think we need a
> dedicated method for that in the injectedscript).
> 
> I did not upload the patch since I wanted to do a test for it first. Now that
> tests are there I can put it in. Sorry about the confusion. I should have
> assigned the bug to myself. Keishi, do you think my version makes sense?

This makes sense to me. You also handle sparse arrays better.

[Timothy Hatcher]

Comment on attachment 41389 [details]
patch with no changes to InjectedScript (and a test!)


> +console-format.html:15["test", "test2", undefined, undefined, "test4"]

It would be nice to have a space or ": " seperator here. Like the CONSOLE MESSAGE version.


> -function dumpMessages()
> +window.dumpMessages = function(testController)

These are equivlant. If it is a global function, I prefer the old version.


> +        for (var i = 0; i < messages.length; ++i)

Why &lt; here? I think < would work fine.


> +            result.push(messages[i].toMessageElement().textContent);

Does this need .replace(/\u200b/g, "")?

[Pavel Feldman]

(In reply to comment #6)
> (From update of attachment 41389 [details])
> 
> > +console-format.html:15["test", "test2", undefined, undefined, "test4"]
> 
> It would be nice to have a space or ": " seperator here. Like the CONSOLE
> MESSAGE version.
>

I'll need to add some universal dump code for console messages in another change.

> 
> > -function dumpMessages()
> > +window.dumpMessages = function(testController)
> 
> These are equivlant. If it is a global function, I prefer the old version.

Done.

> 
> 
> > +        for (var i = 0; i < messages.length; ++i)
> 
> Why &lt; here? I think < would work fine.
> 

Done.

> 
> > +            result.push(messages[i].toMessageElement().textContent);
> 
> Does this need .replace(/\u200b/g, "")?

No, since it does render any expandable nodes.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/inspector/console-dir.html
	M	LayoutTests/inspector/console-dirxml.html
	M	LayoutTests/inspector/console-format-expected.txt
	M	LayoutTests/inspector/console-format.html
	M	LayoutTests/inspector/console-tests.html
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorDOMAgent.cpp (to trigger WebCore build)
	M	WebCore/inspector/front-end/ConsoleView.js
Committed r49762