1096294 - Display pseudo-arrays like arrays in the console

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Patrick Brosset <:pbro>]

Attachment: Screenshot 2014-11-10 15.03.49.png

STR:

- Go to http://jquery.com/
- Open the web console
- Type in the following input: $("div")
- Hit enter

This returns an object that has number-indexed properties and a length property, just like an array, however, it is being displayed as an object (see the screenshot).

It would help reduce the amount of screen real estate occupied and make the output more readable if the object was being previewed like an array.

The screenshot also contains the result for the following input: [...document.querySelectorAll("div")] which is what $("div") should look like.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

It probably doesn't get an ArrayLike previewer for some reason.

Comment 2

[Patrick Brosset <:pbro>]

I don't know this code *a lot*, but I do have some experience with it, so I'll set myself as a mentor if anyone's interested in contributing. I think this should be a relatively easy bug.

Comment 3

[Patrick Brosset <:pbro>]

Oh by the way, forgot to mention, this comes from this uservoice feedback: https://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/6263603-concise-object-representations-in-the-console

Comment 4

[didjereedooPlayer]

Hello, I would like to work on this bug.

Comment 5

[Patrick Brosset <:pbro>]

(In reply to didjereedooPlayer from comment #4)
> Hello, I would like to work on this bug.
Sure, that'd be great. Do you have the dev environment ready? https://wiki.mozilla.org/DevTools/Hacking if not.
If you have dev environment issues, you can get in help on IRC on #introduction or #devtools for devtools-specific problems.

Once in place, you can start looking at the code:
- /toolkit/devtools/server/actors/script.js
  This is the server-side code that has a reference to the object that will be previewed in the console and that creates a preview for it
- /browser/devtools/webconsole/console-output.js
  This contains the client-side code that actually renders the preview on to the console.

I think this bug should be solvable by changing the server-side code only.
As Panos said in comment 1, we are probably just not creating the right preview for pseudo-arrays. We need to find a way to detect these types of objects, and create a preview just like we do for Arrays.

In /toolkit/devtools/server/actors/script.js, search for DebuggerServer.ObjectActorPreviewers.Object, you'll see a list of functions that the previewer mechanism uses, one by one, until it finds one that actually can preview the object.
In there, there's one function called ArrayLike, which is the one we use to preview things like String lists, attribute maps, css rules, etc...

My take on this bug is that we add a new function right after this one, called PseudoArray or something.
This function would check if the object passed has a 'length' property that is a positive, finite, integer, and would also check if the object has positive, finite, integer properties from 0 to length-1.
If the object passed in validate these conditions, then this function should add the "ArrayLike" preview kind and length and items to the grip object just like the ArrayLike function does.

Comment 6

[Patrick Brosset <:pbro>]

NI? Panos just to check the sanity of the proposal in the previous comment.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

That could get tricky in some corner cases I guess, but it does sound like the best path forward. For example, maybe we should allow sparse array-like objects, by not demanding every integer property from 0 to length-1 to be present?

I would also put this new function right before GenericObject, so we can be sure it's the last thing we try before a plain object preview.

Comment 8

[Tim Nguyen :ntim]

I would consider this bug as wontfix since the object returned by Jquery has a few more properties :
If you check the end of the object, you should see something like this :
prevObject: Object, context: HTMLDocument → jquery.com, selector: "div"}
It'd be kinda bad to ignore those.

But, I agree that :
{0 : "foo", 1: "bar"} should be displayed as ["foo","bar"], since there are no other properties in the object.

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Tim Nguyen [:ntim] from comment #8)
> I would consider this bug as wontfix since the object returned by Jquery has
> a few more properties :
> If you check the end of the object, you should see something like this :
> prevObject: Object, context: HTMLDocument → jquery.com, selector: "div"}
> It'd be kinda bad to ignore those.
Totally agree, we should not be ignoring these properties in the console output. jQuery in comment 0 was just an example, but probably not the best in fact. Maybe, as you said, as soon as there are other properties, we should just format the output as an object (what we do today).

Comment 10

[Johan K. Jensen :johankj]

Attachment: First draft. Show objects with incrementing integer properties as pseudo-arrays.

Comment 11

[Johan K. Jensen :johankj]

Attachment: rev-1.png

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Looks good from a quick skim! Now you need to add tests that cover at least the cases in your screenshot and it would be ready for review.

Comment 13

[Johan K. Jensen :johankj]

Attachment: Rev2 including tests

Added mochitests for the cases described earlier and fixed a bug in previous patch where empty objects would be matched.

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8564537 [details] [diff] [review]
Rev2 including tests

Review of attachment 8564537 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
I only have a few minor comments about formatting and commenting which won't affect the patch, so I've pushed it to TRY to see if the tests pass fine: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12e3623d87b4
Also, the commit message is: Bug 1096294 - "Display pseudo-arrays like arrays in the console" []
You'll need to add the reviewer's nick to (and you can get rid of the quotes and []):
Bug 1096294 - Display pseudo-arrays like arrays in the console; r=pbrosset

::: browser/devtools/webconsole/test/browser_webconsole_output_02.js
@@ +146,5 @@
>      printOutput: "[object Map]",
>      inspectable: true,
>      variablesViewLabel: "Map[3]",
>    },
> +

Can you explain why you chose to add your test cases to browser_webconsole_output_02.js and not 06 for instance (which seems to be dealing with array output).

::: toolkit/devtools/server/actors/script.js
@@ +4330,5 @@
>      return true;
>    },
>  
> +  function PseudoArray({obj, threadActor}, aGrip, aRawObj) {
> +      let length = 0;

nit: the indentation in this function is incorrect, the body is indented with 4 spaces instead of 2.

@@ +4335,5 @@
> +      let keys = obj.getOwnPropertyNames();
> +      if (keys.length == 0) {
> +        return false;
> +      }
> +      for (let key of keys) {

Could you add a comment above this for?
// Making sure all keys are numbers from 0 to length-1

@@ +4346,5 @@
> +        kind: "ArrayLike",
> +        length: length,
> +      };
> +
> +      if (threadActor._gripDepth > 1) {

I think it also helps to add this comment above this line:
// Avoid recursive object grips.

Comment 15

[Patrick Brosset <:pbro>]

That try push shows that (so far at least) one test is failing because of your changes:
browser_dbg_variables-view-large-array-buffer.js
Can you fix this? I don't believe an extra review is required once done, so you can R+ the new patch yourself once done.

You should probably run all the webconsole tests locally before we push to try again, there may be other tests that need fixing:
./mach mochitest-devtools browser/devtools/webconsole/test

Comment 16

[Johan K. Jensen :johankj]

Attachment: Rev3 - Moved testcases and minor changes per the review.

Comment 17

[Patrick Brosset <:pbro>]

New pending TRY push for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b90a224777c

Comment 18

[Patrick Brosset <:pbro>]

Try push is all green. Let's land this.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/57e05d6b84a8

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57e05d6b84a8