Last Comment Bug 659907 - Expand console object with a dir method that displays an interactive listing of all the properties of an object.
: Expand console object with a dir method that displays an interactive listing ...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on:
Blocks: 644596
  Show dependency treegraph
 
Reported: 2011-05-26 04:13 PDT by Panos Astithas [:past]
Modified: 2011-09-06 07:48 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (10.79 KB, patch)
2011-05-30 09:05 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
WIP (12.55 KB, patch)
2011-05-31 08:00 PDT, Panos Astithas [:past]
rcampbell: feedback+
Details | Diff | Splinter Review
Patch v3 (12.76 KB, patch)
2011-06-06 11:33 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v4 (21.32 KB, patch)
2011-06-08 09:20 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v5 (26.29 KB, patch)
2011-06-09 06:07 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v6 (27.36 KB, patch)
2011-06-13 07:08 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v7 (18.93 KB, patch)
2011-06-13 07:44 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v8 (18.21 KB, patch)
2011-06-17 09:17 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v9 (16.87 KB, patch)
2011-06-22 07:46 PDT, Panos Astithas [:past]
mihai.sucan: review-
Details | Diff | Splinter Review
Patch v10 (17.80 KB, patch)
2011-06-23 03:28 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v11 (18.36 KB, patch)
2011-06-24 04:35 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v12 (12.85 KB, patch)
2011-06-28 07:09 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v13 (12.96 KB, patch)
2011-07-20 03:30 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v14 (12.65 KB, patch)
2011-07-27 09:22 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v15 (12.52 KB, patch)
2011-07-28 09:36 PDT, Panos Astithas [:past]
mihai.sucan: review-
Details | Diff | Splinter Review
Patch v16 (12.58 KB, patch)
2011-07-29 04:27 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v17 (12.62 KB, patch)
2011-07-29 07:17 PDT, Panos Astithas [:past]
mihai.sucan: review+
Details | Diff | Splinter Review
Patch v18 (12.82 KB, patch)
2011-07-30 02:49 PDT, Panos Astithas [:past]
bzbarsky: review+
Details | Diff | Splinter Review
[in-fx-team] Patch v19 (12.97 KB, patch)
2011-08-01 07:42 PDT, Panos Astithas [:past]
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2011-05-26 04:13:51 PDT
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().
Comment 1 Panos Astithas [:past] 2011-05-30 09:05:53 PDT
Created attachment 536110 [details] [diff] [review]
WIP

Storing my WIP for safe keeping.
Comment 2 Panos Astithas [:past] 2011-05-31 08:00:30 PDT
Created attachment 536301 [details] [diff] [review]
WIP

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 3 Rob Campbell [:rc] (:robcee) 2011-05-31 16:14:43 PDT
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.
Comment 4 Kevin Dangoor 2011-06-02 06:42:12 PDT
(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.
Comment 5 Panos Astithas [:past] 2011-06-06 11:33:37 PDT
Created attachment 537600 [details] [diff] [review]
Patch v3

(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.
Comment 6 Panos Astithas [:past] 2011-06-08 09:20:54 PDT
Created attachment 538036 [details] [diff] [review]
Patch v4

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.
Comment 7 Panos Astithas [:past] 2011-06-09 06:07:09 PDT
Created attachment 538242 [details] [diff] [review]
Patch v5

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
Comment 8 Panos Astithas [:past] 2011-06-13 07:08:58 PDT
Created attachment 538891 [details] [diff] [review]
Patch v6

Fix regression in v5.
Comment 9 Panos Astithas [:past] 2011-06-13 07:44:34 PDT
Created attachment 538899 [details] [diff] [review]
Patch v7

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.
Comment 10 Panos Astithas [:past] 2011-06-17 09:17:36 PDT
Created attachment 540066 [details] [diff] [review]
Patch v8

Unbitrotted patch v7.
Comment 11 Panos Astithas [:past] 2011-06-22 07:46:25 PDT
Created attachment 541046 [details] [diff] [review]
Patch v9

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 12 Mihai Sucan [:msucan] 2011-06-22 08:49:23 PDT
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 ...
Comment 13 Panos Astithas [:past] 2011-06-23 03:28:52 PDT
Created attachment 541328 [details] [diff] [review]
Patch v10

(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.
Comment 14 Panos Astithas [:past] 2011-06-24 04:35:55 PDT
Created attachment 541661 [details] [diff] [review]
Patch v11

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.
Comment 15 Panos Astithas [:past] 2011-06-28 07:09:25 PDT
Created attachment 542455 [details] [diff] [review]
Patch v12

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.
Comment 16 Rob Campbell [:rc] (:robcee) 2011-06-28 07:11:50 PDT
This looks like it needs a review request!
Comment 17 Panos Astithas [:past] 2011-06-28 07:13:45 PDT
(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.
Comment 18 Panos Astithas [:past] 2011-06-29 04:17:29 PDT
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...
Comment 19 Rob Campbell [:rc] (:robcee) 2011-06-29 05:36:10 PDT
Sounds like you're caching a DOM node somewhere, either in your unittest or in the code itself.
Comment 20 Rob Campbell [:rc] (:robcee) 2011-06-29 05:39:14 PDT
try clearing the console when you're done your test. Wondering if that tree is managing to hang around.
Comment 21 Panos Astithas [:past] 2011-06-29 06:10:06 PDT
(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.
Comment 22 Panos Astithas [:past] 2011-07-20 03:30:26 PDT
Created attachment 547025 [details] [diff] [review]
Patch v13

Rebased against latest fx-team repo. The leak is still there, but I don't have time at the moment to work on it.
Comment 23 Panos Astithas [:past] 2011-07-27 09:22:36 PDT
Created attachment 548809 [details] [diff] [review]
Patch v14

Rebased due to the creation of the new devtools module. Haven't fixed the leak yet.
Comment 24 Panos Astithas [:past] 2011-07-28 09:36:28 PDT
Created attachment 549145 [details] [diff] [review]
Patch v15

Fixed all broken tests and a memory leak in the console.dir implementation. This is finally ready for review.
Comment 25 Mihai Sucan [:msucan] 2011-07-28 13:25:46 PDT
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.
Comment 26 Panos Astithas [:past] 2011-07-29 04:27:38 PDT
Created attachment 549338 [details] [diff] [review]
Patch v16

(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
Comment 27 Panos Astithas [:past] 2011-07-29 07:17:29 PDT
Created attachment 549362 [details] [diff] [review]
Patch v17

(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 28 Mihai Sucan [:msucan] 2011-07-29 07:47:11 PDT
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 (...) { ... }
...
Comment 29 Panos Astithas [:past] 2011-07-30 02:49:55 PDT
Created attachment 549559 [details] [diff] [review]
Patch v18

(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.
Comment 30 Mihai Sucan [:msucan] 2011-08-01 02:37:45 PDT
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.
Comment 31 Panos Astithas [:past] 2011-08-01 04:14:31 PDT
(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 32 Panos Astithas [:past] 2011-08-01 04:16:33 PDT
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 33 Boris Zbarsky [:bz] 2011-08-01 07:25:23 PDT
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.
Comment 34 Panos Astithas [:past] 2011-08-01 07:42:10 PDT
Created attachment 549793 [details] [diff] [review]
[in-fx-team] Patch v19

(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 35 Boris Zbarsky [:bz] 2011-08-01 08:06:15 PDT
Comment on attachment 549793 [details] [diff] [review]
[in-fx-team] Patch v19

Thanks.
Comment 36 Rob Campbell [:rc] (:robcee) 2011-08-02 07:09:15 PDT
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
Comment 37 Rob Campbell [:rc] (:robcee) 2011-08-02 07:21:30 PDT
oops, disregard that. I didn't rebuild dom/.
Comment 38 Rob Campbell [:rc] (:robcee) 2011-08-02 07:35:07 PDT
Comment on attachment 549793 [details] [diff] [review]
[in-fx-team] Patch v19

http://hg.mozilla.org/integration/fx-team/rev/a8c39fc1b57b
Comment 39 Tim Taubert [:ttaubert] 2011-08-03 07:57:07 PDT
http://hg.mozilla.org/mozilla-central/rev/a8c39fc1b57b
Comment 40 Eric Shepherd [:sheppy] 2011-09-06 07:48:43 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.