Display pseudo-arrays like arrays in the console

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: pbro, Assigned: jj, Mentored)

Tracking

(Blocks 1 bug)

unspecified
Firefox 39
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [console-papercuts][good first bug][lang=js])

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
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.
It probably doesn't get an ArrayLike previewer for some reason.
Reporter

Updated

5 years ago
Whiteboard: [good first bug][lang=js]
Reporter

Comment 2

5 years ago
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.
Mentor: pbrosset

Comment 4

5 years ago
Hello, I would like to work on this bug.
Reporter

Comment 5

5 years ago
(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.
Reporter

Comment 6

5 years ago
NI? Panos just to check the sanity of the proposal in the previous comment.
Flags: needinfo?(past)
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.
Flags: needinfo?(past)

Comment 8

5 years ago
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.
Reporter

Comment 9

5 years ago
(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).
Whiteboard: [good first bug][lang=js] → [console-papercuts]

Updated

5 years ago
Whiteboard: [console-papercuts] → [console-papercuts][good first bug][lang=js]
Assignee

Comment 11

4 years ago
Posted image rev-1.png
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.
Flags: needinfo?(past)
Assignee

Comment 13

4 years ago
Posted patch Rev2 including tests (obsolete) — Splinter Review
Added mochitests for the cases described earlier and fixed a bug in previous patch where empty objects would be matched.
Attachment #8559109 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Reporter

Comment 14

4 years ago
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.
Attachment #8564537 - Flags: review+
Reporter

Updated

4 years ago
Assignee: nobody → jj
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Reporter

Comment 15

4 years ago
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
Flags: needinfo?(jj)
Assignee

Comment 16

4 years ago
Attachment #8564537 - Attachment is obsolete: true
Flags: needinfo?(jj) → needinfo?(pbrosset)
Attachment #8567105 - Flags: review+
Reporter

Comment 17

4 years ago
New pending TRY push for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b90a224777c
Flags: needinfo?(pbrosset)
Reporter

Comment 18

4 years ago
Try push is all green. Let's land this.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/57e05d6b84a8
Keywords: checkin-needed
Whiteboard: [console-papercuts][good first bug][lang=js] → [console-papercuts][good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/57e05d6b84a8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [console-papercuts][good first bug][lang=js][fixed-in-fx-team] → [console-papercuts][good first bug][lang=js]
Target Milestone: --- → Firefox 39
Depends on: 1295729

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.