Bug 30485 - Web Inspector: Properties on Arrays and NodeLists are not logged correctly
Summary: Web Inspector: Properties on Arrays and NodeLists are not logged correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-17 14:26 PDT by Timothy Hatcher
Modified: 2009-10-18 19:11 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (4.04 KB, patch)
2009-10-18 10:22 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
patch with no changes to InjectedScript (and a test!) (5.62 KB, patch)
2009-10-18 17:11 PDT, Pavel Feldman
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2009-10-17 14:26:05 PDT
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
Comment 1 Keishi Hattori 2009-10-18 10:22:23 PDT
Created attachment 41382 [details]
proposed patch

Adds InjectedScript.getArrayElements to enumerate just the array elements.
Comment 2 Timothy Hatcher 2009-10-18 10:27:54 PDT
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.
Comment 3 Pavel Feldman 2009-10-18 17:10:38 PDT
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?
Comment 4 Pavel Feldman 2009-10-18 17:11:51 PDT
Created attachment 41389 [details]
patch with no changes to InjectedScript (and a test!)
Comment 5 Timothy Hatcher 2009-10-18 18:27:55 PDT
(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.
Comment 6 Timothy Hatcher 2009-10-18 18:37:22 PDT
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 &lt; messages.length; ++i)

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


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

Does this need .replace(/\u200b/g, "")?
Comment 7 Pavel Feldman 2009-10-18 19:05:40 PDT
(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 &lt; 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.
Comment 8 Pavel Feldman 2009-10-18 19:09:49 PDT
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