Closed
Bug 1096294
Opened 10 years ago
Closed 9 years ago
Display pseudo-arrays like arrays in the console
Categories
(DevTools :: Console, defect)
DevTools
Console
Tracking
(firefox39 fixed)
RESOLVED
FIXED
Firefox 39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: pbro, Assigned: johankj, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [console-papercuts][good first bug][lang=js])
Attachments
(3 files, 2 obsolete files)
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•10 years ago
|
||
It probably doesn't get an ArrayLike previewer for some reason.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [good first bug][lang=js]
Reporter | ||
Comment 2•10 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
Reporter | ||
Comment 3•10 years ago
|
||
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•10 years ago
|
||
Hello, I would like to work on this bug.
Reporter | ||
Comment 5•10 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•10 years ago
|
||
NI? Panos just to check the sanity of the proposal in the previous comment.
Flags: needinfo?(past)
Comment 7•10 years ago
|
||
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•10 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•10 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).
Updated•9 years ago
|
Whiteboard: [good first bug][lang=js] → [console-papercuts]
Updated•9 years ago
|
Whiteboard: [console-papercuts] → [console-papercuts][good first bug][lang=js]
Assignee | ||
Comment 10•9 years ago
|
||
Flags: needinfo?(past)
Assignee | ||
Comment 11•9 years ago
|
||
Comment 12•9 years ago
|
||
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•9 years ago
|
||
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•9 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•9 years ago
|
Assignee: nobody → jj
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Reporter | ||
Comment 15•9 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)
Updated•9 years ago
|
Blocks: firebug-gaps
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8564537 -
Attachment is obsolete: true
Flags: needinfo?(jj) → needinfo?(pbrosset)
Attachment #8567105 -
Flags: review+
Reporter | ||
Comment 17•9 years ago
|
||
New pending TRY push for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b90a224777c
Flags: needinfo?(pbrosset)
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]
Comment 20•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/57e05d6b84a8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•