As described in bug 644596, we have to implement the rest of the de-facto standard methods in the console object. This bug will track the work for dir().

Storing my WIP for safe keeping.

This version almost works, so I could use a little feedback on the direction I've taken, which amounts to reusing as much of the PropertyPanel code as possible. I've broken the update button on the PropertyPanel, I probably need work in pruning these new message nodes and there are no new tests. The most visible issue though is the inner scroll bar in the property tree when viewing something large, like "console.dir(document)". Do we want the interactive inspection of the object a-la PropertyPanel, or should I just output textual nodes instead? Is there a simple way to maintain the interactivity without duplicating the PropertyPanel construction? Is the latter a bad thing?

Comment on attachment 536301 [details] [diff] [review] WIP HUDService.jsm: +Cu.import("resource:///modules/PropertyPanel.jsm"); no longer needed. // childNodes[2] is the xul:description element - if (lastMessage && + // The lastMessage.view check skips object property trees. + if (lastMessage && !lastMessage.view && Seems bit hacky (in an admittedly hacky method). Might be better to add a class you can look for on the message node if it has a tree in it. nit: + * Destroy the PropertyPanel. This closes the poped up panel and removes should be "popped". Here and elsewhere. :) Overall I like the approach. One concern: Are the contents of the tree copyable? I know the PropertyPanel is pretty inert and doesn't allow selection and copying. I'd love to see it possible to get at the contents of the objects, though that might better be done as a follow-up. f+ with a proper unittest.

(In reply to comment #2) > This version almost works, so I could use a little feedback on the direction > I've taken, which amounts to reusing as much of the PropertyPanel code as > possible. I've broken the update button on the PropertyPanel, I probably > need work in pruning these new message nodes and there are no new tests. The > most visible issue though is the inner scroll bar in the property tree when > viewing something large, like "console.dir(document)". Do we want the > interactive inspection of the object a-la PropertyPanel, or should I just > output textual nodes instead? Panos and I spoke a bit about this on the phone yesterday. I thought I'd document our discussion here... At this point, console.dir in the other consoles produces interactive output that allows you to drill into the object that you're viewing. Firebug's does a nice job of copy/paste for this output as well. I think that's ideal, but we can certainly (as Rob points out) land a decent console.dir to start and expand it from there in a followup.

(In reply to comment #3) > Comment on attachment 536301 [details] [diff] [review] [review] > WIP > > HUDService.jsm: > +Cu.import("resource:///modules/PropertyPanel.jsm"); > > no longer needed. I need this because I create a PropertyViewer to display the properties. > // childNodes[2] is the xul:description element > - if (lastMessage && > + // The lastMessage.view check skips object property trees. > + if (lastMessage && !lastMessage.view && > > Seems bit hacky (in an admittedly hacky method). Might be better to add a > class you can look for on the message node if it has a tree in it. Done. > nit: > + * Destroy the PropertyPanel. This closes the poped up panel and removes > > should be "popped". Here and elsewhere. :) Done. > Overall I like the approach. One concern: Are the contents of the tree > copyable? I know the PropertyPanel is pretty inert and doesn't allow > selection and copying. I'd love to see it possible to get at the contents of > the objects, though that might better be done as a follow-up. Copying is not yet supported, same as in the property panel. I agree that handling it in a follow-up bug would be the best course of action. > f+ with a proper unittest. I haven't created a unittest yet, and I also have some more work to do to fix the widget embedding in the console output. I fixed the update button though.

Added a new test, fixed broken ones and fixed the tree cleanup when closing the console. Fixing the tree appearance is the last remaining item.

More tests, fixed the tree placement, but not the clipping, alas. Try run: http://tbpl.mozilla.org/?tree=Try&rev=55b4354404de Builds and logs: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/pastithas@mozilla.com-55b4354404de

Fix regression in v5.

A different take from v6, that disentangles the implementation of console.dir from the property panel. This will hopefully help pinpoint the small rectangle problem.

Unbitrotted patch v7.

This is a simplified console.dir that does not support expanding object property nodes. The plan we discussed with Kevin is to file a followup bug for adding this functionality on top of this patch, in order to increase the likelihood of console.dir shipping with FF7.

Comment on attachment 541046 [details] [diff] [review] Patch v9 Review of attachment 541046 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks fine, but there are 37 tests that fail now, in HUDService. Please make sure that all the tests pass. Another concern is the UI which kinda ugly. :) But I believe that's beyond the purpose of this bug. Maybe you can add a max-height, to make the richlistbox scrollable when there are too many elements. Giving the patch r- for now. ::: toolkit/components/console/hudservice/HUDService.jsm @@ +5615,5 @@ > > + node.appendChild(timestampNode); > + node.appendChild(iconContainer); > + // Display the object tree after the message node. > + if (aLevel == "dir") { I think you should put the UI of console.dir() in a separate method that you invoke here. To keep things clearer. ::: toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_659907_console_dir.js @@ +17,5 @@ > + > + openConsole(); > + let hudId = HUDService.getHudIdByWindow(content); > + let hud = HUDService.hudReferences[hudId]; > + outputNode = hud.outputNode; Please do let outputNode ...

(In reply to comment #12) > Comment on attachment 541046 [details] [diff] [review] [review] > Patch v9 > > Review of attachment 541046 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > Patch looks fine, but there are 37 tests that fail now, in HUDService. > Please make sure that all the tests pass. Checkpointing my work for now, I'll update the patch when I have identified what is causing these failures. > Another concern is the UI which kinda ugly. :) But I believe that's beyond > the purpose of this bug. Maybe you can add a max-height, to make the > richlistbox scrollable when there are too many elements. How much should max-height be? One of my main goals has been to not add an extra scrollbar besides the console output's. This is the main difference as I see it between inspect() and console.dir(). The latter produces traditional console output by appending lines, whereas the former allows for flexible UI manipulation. Copying the console output for pasting into a bug has been one of the most frequent ways I use the console. Note that console.dir output isn't copyable yet, but that is definitely a goal. > Giving the patch r- for now. > > ::: toolkit/components/console/hudservice/HUDService.jsm > @@ +5615,5 @@ > > > > + node.appendChild(timestampNode); > > + node.appendChild(iconContainer); > > + // Display the object tree after the message node. > > + if (aLevel == "dir") { > > I think you should put the UI of console.dir() in a separate method that you > invoke here. To keep things clearer. I planned to create a separate method while implementing the interactive behavior, but I just went ahead and did it as you suggested. > ::: > toolkit/components/console/hudservice/tests/browser/ > browser_webconsole_bug_659907_console_dir.js > @@ +17,5 @@ > > + > > + openConsole(); > > + let hudId = HUDService.getHudIdByWindow(content); > > + let hud = HUDService.hudReferences[hudId]; > > + outputNode = hud.outputNode; > > Please do let outputNode ... Unfortunately, outputNode has to be in global scope in order for findLogEntry to work. In bug 610953 we will do some cleanup there at some point.

This is a new version that uses a tree widget again. Neil Deakin pointed out to me the one line in attachment 540066 [details] [diff] [review] that caused the problem with displaying the tree contents and I've also fixed the clipping problem. The only thing that remains is to fix the cleanup sequence, because I've created some leaks somewhere, that break lots of tests.

Rebased patch, removed hard-coded reference to the number of window properties in the test, and made sure the test failures are not caused by this test.

This looks like it needs a review request!

(In reply to comment #16) > This looks like it needs a review request! I haven't fixed the leak yet, only made sure it's caused somewhere in HUDService and not in the new test.

I don't see how this patch can break unrelated tests, but the breakage starts already at the first hudservice mochitest, browser_warn_user_about_replaced_api.js. There is this function: function testWarningPresent() { // Then test that the warning does appear on a page that replaces the API browser.addEventListener("load", function() { browser.removeEventListener("load", arguments.callee, true); testOpenWebConsole(true); finishTest(); }, true); browser.contentWindow.location = TEST_REPLACED_API_URI; } With the patch applied, after the location is changed and before the load event listener is entered, the following errors appear in the output: ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "linkNode is null" {file: "resource://gre/modules/HUDService.jsm" line: 2438}]' when calling method: [nsIHttpActivityObserver::observeActivity]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "<unknown>" data: yes] ************************************************************ before 598016, after 585728, break 109600000 WARNING: NS_ENSURE_TRUE(mState == STATE_TRANSFERRING) failed: file /Users/past/src/devtools/netwerk/base/src/nsSocketTransport2.cpp, line 1887 WARNING: NS_ENSURE_TRUE(mState == STATE_TRANSFERRING) failed: file /Users/past/src/devtools/netwerk/base/src/nsSocketTransport2.cpp, line 1887 TEST-INFO | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_warn_user_about_replaced_api.js | Console message: [JavaScript Error: "linkNode is null" {file: "resource://gre/modules/HUDService.jsm" line: 2438}] ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "tee is null" {file: "resource://gre/modules/HUDService.jsm" line: 2529}]' when calling method: [nsIHttpActivityObserver::observeActivity]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "<unknown>" data: yes] ************************************************************ ++DOMWINDOW == 18 (0x1272bdcc8) [serial = 18] [outer = 0x12730dc10] ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "originalListener is null" {file: "resource://gre/modules/HUDService.jsm" line: 2560}]' when calling method: [nsIHttpActivityObserver::observeActivity]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "<unknown>" data: yes] ************************************************************ TEST-INFO | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_warn_user_about_replaced_api.js | Console message: [JavaScript Error: "tee is null" {file: "resource://gre/modules/HUDService.jsm" line: 2529}] TEST-INFO | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_warn_user_about_replaced_api.js | Console message: [JavaScript Error: "originalListener is null" {file: "resource://gre/modules/HUDService.jsm" line: 2560}] I can't see how this patch could be messing with this...

Sounds like you're caching a DOM node somewhere, either in your unittest or in the code itself.

try clearing the console when you're done your test. Wondering if that tree is managing to hang around.

(In reply to comment #19) > Sounds like you're caching a DOM node somewhere, either in your unittest or > in the code itself. That's my guess as well, and I've already ruled out the unittest. (In reply to comment #20) > try clearing the console when you're done your test. Wondering if that tree > is managing to hang around. I'd already tried invoking HUDService.clearDisplay in various places, but it didn't appear to make a difference. Note that it's a leak that doesn't directly involve console.dir calls (it manifests itself even with no dir test), so a tree node would have never been created. Using strict mode in the modified functions didn't turn up anything either.

Rebased against latest fx-team repo. The leak is still there, but I don't have time at the moment to work on it.

Rebased due to the creation of the new devtools module. Haven't fixed the leak yet.

Fixed all broken tests and a memory leak in the console.dir implementation. This is finally ready for review.

Comment on attachment 549145 [details] [diff] [review] Patch v15 Review of attachment 549145 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks fine. I have only a few comments, it's very close to landing. Please note that the new test fails: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_659907_console_dir.js | found document.body - Got body: HTMLBodyElement, expected body: Object Once you update the patch, please send it to the try server. Thank you! ::: browser/devtools/webconsole/HUDService.jsm @@ +1201,5 @@ > + * aParam nsIDOMNode > + * The message node that contains the property inspector from a > + * console.dir call. > + */ > +function destroyInspector(aInspector) I would like this function as a method in the HUD object. @@ +1208,5 @@ > + let tree = aInspector.childNodes[2].childNodes[1]; > + tree.parentNode.removeChild(tree); > + aInspector.view = null; > + tree.view = null; > + tree = null; It took me some time to untangle this bit of code. :) I would suggest something less confusing: - aInspector should be the aMessageNode. - aInspector.childNodes[2].childNodes[1] is prone to errors and hard to read. Please make it aMessageNode.querySelector("tree"). - aMessageNode.view should be aMessageNode.propertyTreeView so we don't confuse it with tree.view. @@ +1223,5 @@ > + */ > +function initInspector(aInspector) > +{ > + let tree = aInspector.childNodes[2].childNodes[1]; > + tree.view = aInspector.view; Same suggestion for naming variables and properties as for destroyInspector(). @@ +1269,5 @@ > } > delete hudRef.cssNodes[desc + location]; > } > + else if (messageNodes[i].classList.contains("webconsole-msg-inspector")) { > + destroyInspector(messageNodes[i]); You have a hudRef that you can use here, hence the suggestion to put destroyInspector() in the HUD object. Also, destroyInspector() should be renamed to, perhaps, pruneConsoleDirNode() or something like that. @@ +2061,5 @@ > > ConsoleUtils.outputMessageNode(node, aHUDId); > + > + if (level == "dir") { > + initInspector(node); initInspector() is used only once, that's here. I would suggest you inline that code here. ::: browser/devtools/webconsole/test/browser/browser_webconsole_bug_659907_console_dir.js @@ +28,5 @@ > + for (let prop in content.document) { > + docProps++; > + } > + is(view.rowCount, docProps, "document object has the correct number of elements"); > + is(view._rows[30].display, "body: Object", "found document.body"); Both checks are prone to errors. For example we might decide to exclude some props/methods in the tree view. 1. Check a couple of properties you know will be included in the _rows array. 2. Never use hard coded array indexes. Search the entire array for the row you want.

(In reply to comment #25) > Patch looks fine. I have only a few comments, it's very close to landing. Patch updated according to your comments, thanks. > Please note that the new test fails: > > TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/ > browser_webconsole_bug_659907_console_dir.js | found document.body - Got > body: HTMLBodyElement, expected body: Object > > Once you update the patch, please send it to the try server. Thank you! I couldn't reproduce this and still can't. Try run is at: http://tbpl.mozilla.org/?tree=Try&rev=beab3584dc98

(In reply to comment #26) > (In reply to comment #25) > > Please note that the new test fails: > > > > TEST-UNEXPECTED-FAIL | > > chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/ > > browser_webconsole_bug_659907_console_dir.js | found document.body - Got > > body: HTMLBodyElement, expected body: Object > > > > Once you update the patch, please send it to the try server. Thank you! > > I couldn't reproduce this and still can't. Try run is at: > > http://tbpl.mozilla.org/?tree=Try&rev=beab3584dc98 So document.body is different between debug and opt builds. Who knew?

Comment on attachment 549362 [details] [diff] [review] Patch v17 Review of attachment 549362 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the test fix. Patch looks fine, I have only a couple of nits. Additionally, I think the console.dir() tree should have a limited height. Currently it takes the height of the element being inspected which can be a LOT. Please limit it, perhaps, to 80% of the height of the web console at the time of evaluation, or something. r+ with the nits addressed! Thank you! ::: browser/devtools/webconsole/HUDService.jsm @@ +4824,1 @@ > } Nit: probably the loop would look better if it would be while (let node = hud.outputNode.firstChild) { ... }. Also note that classList is not available on all nodes, so please prepend the if condition with node.classList && ... ::: browser/devtools/webconsole/test/browser/browser_webconsole_bug_659907_console_dir.js @@ +25,5 @@ > + is(msg.length, 1, "one message node displayed"); > + let rows = msg[0].propertyTreeView._rows; > + for (let i = 0; i < rows.length; i++) { > + if (rows[i].display == "querySelectorAll: function querySelectorAll()") { > + var foundBody = true; Nit: Please do not define the vars inside the loop, put these vars outside of the loop. let rows = ...; let found... = false; for (...) { ... } ...

(In reply to comment #28) > Patch looks fine, I have only a couple of nits. All nits addressed, thanks. > Additionally, I think the console.dir() tree should have a limited height. > Currently it takes the height of the element being inspected which can be a > LOT. Please limit it, perhaps, to 80% of the height of the web console at > the time of evaluation, or something. I've tried this and the user experience isn't good. First of all 80% seems rather arbitrary and then we would have to have another arbitrary size threshold above which limiting should take place (it wouldn't make sense to limit a 4-property object to 80%). The arbitrariness partly stems from the fact that it should be relative to the output size, which isn't fixed. And associating the limit to the height of the console at the time of evaluation, would lead to a collection of different sizes of trees every time the user resizes the console output. Then there is the visual clutter of the dual vertical scrollbars, one for the console output and another for the console.dir output. Furthermore, scrolling using touchpad gestures or the mouse scrollwheel is problematic in such cases, because the scrolling of the console output at some point brings the tree under the cursor and then scroll events are captured by the tree, which is usually not what the user intended. In this case scrolling to the top of the output, or past a tree requires placing the cursor in a suitable position to avoid the tree. Finally, we should bear in mind that console.dir shall be used mostly for inspecting user objects from content code, which should not be that large, compared to say displaying window or document in an exploratory console session. Not to mention that there is also inspect() et al. to pop up a property panel for inspecting such large objects without overwhelming the console output (assuming that is a problem in the first place). Would you like me to post to the devtools list or d-a-f for a broader perspective on this? > r+ with the nits addressed! Thank you! > > ::: browser/devtools/webconsole/HUDService.jsm > @@ +4824,1 @@ > > } > > Nit: probably the loop would look better if it would be while (let node = > hud.outputNode.firstChild) { ... }. > > Also note that classList is not available on all nodes, so please prepend > the if condition with node.classList && ... Done, with a minor change in what you proposed, since let declarations apparently cannot be defined in a while expression. > ::: > browser/devtools/webconsole/test/browser/ > browser_webconsole_bug_659907_console_dir.js > @@ +25,5 @@ > > + is(msg.length, 1, "one message node displayed"); > > + let rows = msg[0].propertyTreeView._rows; > > + for (let i = 0; i < rows.length; i++) { > > + if (rows[i].display == "querySelectorAll: function querySelectorAll()") { > > + var foundBody = true; > > Nit: Please do not define the vars inside the loop, put these vars outside > of the loop. > > let rows = ...; > let found... = false; > for (...) { ... } > ... Oh well, I was just trying to take advantage of variable hoisting to reduce verbosity. Done.

Thanks for the updated patch! (In reply to comment #29) > Would you like me to post to the devtools list or d-a-f for a broader > perspective on this? Good arguments, but I think the UI can be improved. However, this doesn't need to stop this patch from landing. So this patch is done. ;) A thread on a mailing list ... I am not sure if this is needed. I think it would better be suited to talk to shorlander from the UX team and with robcee. In the future, we will want to improve the console.dir() output UI and the Property Panel UI. Anyhow, this is not urgent.

(In reply to comment #30) > Thanks for the updated patch! > > (In reply to comment #29) > > Would you like me to post to the devtools list or d-a-f for a broader > > perspective on this? > > Good arguments, but I think the UI can be improved. However, this doesn't > need to stop this patch from landing. So this patch is done. ;) > > A thread on a mailing list ... I am not sure if this is needed. I think it > would better be suited to talk to shorlander from the UX team and with > robcee. In the future, we will want to improve the console.dir() output UI > and the Property Panel UI. Anyhow, this is not urgent. OK, understood. We can talk about it in one of our meetings and plan accordingly.

Comment on attachment 549559 [details] [diff] [review] Patch v18 We also need a review of the dom/ bits and a superreview while we're at it.

Comment on attachment 549559 [details] [diff] [review] Patch v18 r=me on the DOM bits, though I recommend putting somewhere (the bug, the checkin comment, code comments, _somewhere_; checkin comment and code comments probably preferred) just what it is that dir() is supposed to do. I'd certainly sr- without that if you asked me for the sr.

(In reply to comment #33) > Comment on attachment 549559 [details] [diff] [review] [diff] [details] [review] > Patch v18 > > r=me on the DOM bits, though I recommend putting somewhere (the bug, the > checkin comment, code comments, _somewhere_; checkin comment and code > comments probably preferred) just what it is that dir() is supposed to do. > I'd certainly sr- without that if you asked me for the sr. Fair point, I put it in all those places, bug, code comment and checkin comment.

Comment on attachment 549793 [details] [diff] [review] [in-fx-team] Patch v19 Thanks.

ran this locally, had a test failure or four. INFO TEST-START | Shutdown Browser Chrome Test Summary Passed: 19937 Failed: 4 Todo: 7 *** End BrowserChrome Test Results *** NOTE: child process received `Goodbye', closing down INFO | automation.py | Application ran for: 0:13:08.669438 INFO | automation.py | Reading PID log: /var/folders/Bv/BvxnJzazGzeR6Yy4QRUEFU+++TI/-Tmp-/tmpBUkjBUpidlog WARNING | automationutils.processLeakLog() | refcount logging is off, so leaks can't be detected! INFO | runtests.py | Running tests: end. mochitest-browser-chrome failed: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_659907_console_dir.js | found [object HTMLDocument - Got false, expected true TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_659907_console_dir.js | one message node displayed - Got 0, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_659907_console_dir.js | Test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleAPITests.js | console.dir is here make: *** [mochitest-browser-chrome] Error 1

oops, disregard that. I didn't rebuild dom/.

Comment on attachment 549793 [details] [diff] [review] [in-fx-team] Patch v19 http://hg.mozilla.org/integration/fx-team/rev/a8c39fc1b57b

Added the dir() method to the list of methods supported on the console object: https://developer.mozilla.org/en/Using_the_Web_Console#The_console_object Also updated Firefox 8 for developers.