573102 - JS/DOM Object Introspector

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[David Dahl :ddahl]

We need an object introspector that can be used by 

1. the console's logging tools (when an object is logged an we want to see what is inside)

and

2. in the JS workspace when initiating tab-completion. 


I think we should be able to share this code in these two use cases.

Comment 1

[David Dahl :ddahl]

Of course now that I think about this a bit more, perhaps there is code in the "Inspector" for this?

Comment 2

[Rob Campbell [:rc] (:robcee)]

there is a DOM object panel in the pipe (see bug 561782), but it's currently a singleton and associated with the currently-inspected node. We might be able to break that out into a more general widget for use with the Console and Workspace.

As long as we don't call it an "Introspector". ;)

(how about JS object "Viewer"?)

Comment 3

[David Dahl :ddahl]

no problem, "JS Object Viewer" is a good name. I was just describing what it does.

Comment 4

[Rob Campbell [:rc] (:robcee)]

I'm going to link in the bug 561782. I think we can reuse this after it lands. This will require splitting the DOM panel code into a separate module and we can do that work here.

Comment 5

[Rob Campbell [:rc] (:robcee)]

(In reply to bug 552982, comment #10)
> As discussed with Rob, I've implemented a basic XUL tree for inspecting a JS
> object.

you should attach a patch of your work here so we can start looking at it.

> Some stuff:
> - some of the objects are wrapped for security reasons. Enumerating over them
> seems to work sometimes, sometimes it fails. Need to dive more into it. Is this
> a know problem with wrapped DOM objects?

They should work just like the underlying object. If not, it's a bug.

> - currently, a function is only displayed as "(function)". What about
> displaying the entire function (function.toString(); means including the
> arguments + name + the body of the function as well)?

We could certainly do it, but I did want to keep the scope of this limited for now. We're not implementing a debugger in the usual sense here. What would happen when a user double-clicked the function? Should we allow editing?

> - the current property inspector tree is inside of a panel. I've got to take a
> closer look at this, but would placing stuff inside of an XUL window be a big
> downside?

It will completely change the behavior of the viewer. When Neil gets his panel titlebars patch ready, the panels will be draggable and labeled with a small titlebar.

Comment 6

[Julian Viereck]

Attachment: state of work 1

This patch is not ready yet to get applied (haven't spent to much time on making the code look nice + some bugs), but it should be enough to get an rough idea.

Comment 7

[Julian Viereck]

(In reply to comment #5)
> (In reply to bug 552982, comment #10)
> > As discussed with Rob, I've implemented a basic XUL tree for inspecting a JS
> > object.
> 
> you should attach a patch of your work here so we can start looking at it.

Uploaded the current state of work as patch. To reproduce the problem, inspect a DOM element with some attributes, then select 'attributes' in the property list, expand it and try to expand one of the array members. For me, this results in an error (copied from the error console):

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOM3Attr.schemaTypeInfo]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: chrome://browser/content/browser.js :: IUI_namesAndValuesOf :: line 1417"  data: no]

> > - currently, a function is only displayed as "(function)". What about
> > displaying the entire function (function.toString(); means including the
> > arguments + name + the body of the function as well)?
> 
> We could certainly do it, but I did want to keep the scope of this limited for
> now. We're not implementing a debugger in the usual sense here. What would
> happen when a user double-clicked the function? Should we allow editing?

Yes, I think we should allow editing but having readOnly as first step is good enough.

> > - the current property inspector tree is inside of a panel. I've got to take a
> > closer look at this, but would placing stuff inside of an XUL window be a big
> > downside?
> 
> It will completely change the behavior of the viewer. When Neil gets his panel
> titlebars patch ready, the panels will be draggable and labeled with a small
> titlebar.

Ahh, I didn't know that. Yes, makes sense. I'll try to figure out how you can open multiple panels.

Comment 8

[Rob Campbell [:rc] (:robcee)]

I realize this is a work-in-progress patch, but I wanted to start some comments early rather than later.

diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
-        <listbox id="inspector-dom-listbox" flex="1"/>
+        <tree id="elementList" flex="1">

how about "object-element-list" to match the nearby naming and give it a slightly more specific name?

-</window>
+</window>
\ No newline at end of file

Make sure you're not stripping newlines off the end of files in your editor.

diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

 ///////////////////////////////////////////////////////////////////////////
 //// InspectorUI

These functions don't really belong in this section anymore.

+function IUI_isWrappedJSObject(aObject) {
+  if (!aObject) {
+    return false;
+  }
...

I'm guessing your first attempt at stripping these functions out is to put them in globals. Probably fine for a working copy, but you can lose the IUI_ prefix on the function name since those are just there to identify the method names inside InspectorUI class.

+var treeView = {
+  // TODO: The complete data doesn't have to be cached here!
+  data: null,

A new file section (rather than appearing in //// InspectorUI would be good for this.

Also, a better name if you're going to make this a top-level object for handling the object viewer's tree view. "PropertiesTreeView" or something.

Comment 9

[Julian Viereck]

Attachment: part1: Implements a reuseable PropertyPanel.

Comment 10

[Julian Viereck]

Attachment: part2: Add support for the PropertyPanel to the console. Clicking on some output should open the panel.

Comment 11

[Julian Viereck]

Attachment: part3: Replace the list-dom-panel with the new PropertyPanel.

Comment 12

[Julian Viereck]

Just uploaded a new version that is split into 3 parts (PropertyPanel itself, changes to console, changes to inspector).

===

One thing fails:

1. Open the inspector and inspect a DOM node with attributes.
2. Select the 'attributes' form the DOM panel
3. expand it and try to expand on of the array members

What happens:

The attribute is not expanded. Taken from the Error Console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOM3Attr.schemaTypeInfo]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: chrome://global/content/PropertyPanel.jsm :: namesAndValuesOf :: line 145"  data: no]

Expected behavior:

The node should expand and show the child elements. Any idea?

===

One thing is strange:

The property panel has to determ if an item represents an object. If it's an object, then that item can get expanded. This is basically done by

    item instanceof Object

However, if you execute 'a = { b: {c: 1} }' in the HUD and click on the output to open an property panel for this,

    item = a.b;
    item instanceof Object === false

Is this a (wrappedObjectSecurityStuff) feature or a bug? I was able to work around this by:

    item.toString().indexOf('[object') == 0

which looks like an ugly hack to me.

===

TODO: write unit tests + future testing

Comment 13

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #12)
> Just uploaded a new version that is split into 3 parts (PropertyPanel itself,
> changes to console, changes to inspector).

sweet. Hopefully get a chance to look at these patches today.

> ===
> 
> One thing fails:
> 
> 1. Open the inspector and inspect a DOM node with attributes.
> 2. Select the 'attributes' form the DOM panel
> 3. expand it and try to expand on of the array members
> 
> What happens:
> 
> The attribute is not expanded. Taken from the Error Console:
> 
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOM3Attr.schemaTypeInfo]"  nsresult:
> "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame ::
> chrome://global/content/PropertyPanel.jsm :: namesAndValuesOf :: line 145" 
> data: no]

I'm guessing here, but I have a feeling something in your TreeView class isn't being implemented. You might want to do some more digging in MDC around TreeViews.

> Expected behavior:
> 
> The node should expand and show the child elements. Any idea?
> 
> ===
> 
> One thing is strange:
> 
> The property panel has to determ if an item represents an object. If it's an
> object, then that item can get expanded. This is basically done by
> 
>     item instanceof Object
> 
> However, if you execute 'a = { b: {c: 1} }' in the HUD and click on the output
> to open an property panel for this,
> 
>     item = a.b;
>     item instanceof Object === false
> 
> Is this a (wrappedObjectSecurityStuff) feature or a bug? I was able to work
> around this by:
> 
>     item.toString().indexOf('[object') == 0
> 
> which looks like an ugly hack to me.

To me too. I'm guessing (again) that wrapped objects don't play well with "instanceof". This seems like a bug to me as the wrapper object should be completely transparent.

I'll ping bz or blake and ask what's up.

> ===
> 
> TODO: write unit tests + future testing

great turnaround on this! Thanks for the help!

Comment 14

[Julian Viereck]

Attachment: part4: Update the inspector#domBox tests to make them pass.

Comment 15

[Julian Viereck]

Attachment: part5: Add "Update" and "Close" button to the console's property panel.

Buttons for the PropertyPanel can be passed to the constructor. Add 'Update' and 'Close' buttons to the console property panel.

Comment 16

[Julian Viereck]

> > One thing fails:
> > 
> > 1. Open the inspector and inspect a DOM node with attributes.
> > 2. Select the 'attributes' form the DOM panel
> > 3. expand it and try to expand on of the array members
> > 
> > What happens:
> > 
> > The attribute is not expanded. Taken from the Error Console:
> > 
> > Error: uncaught exception: [Exception... "Component returned failure code:
> > 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOM3Attr.schemaTypeInfo]"  nsresult:
> > "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame ::
> > chrome://global/content/PropertyPanel.jsm :: namesAndValuesOf :: line 145" 
> > data: no]
> 
> I'm guessing here, but I have a feeling something in your TreeView class isn't
> being implemented. You might want to do some more digging in MDC around
> TreeViews.

Made some deeper research. I took over the sample for TreeView class from MDC and just modified the function bodies, that's why I sure there is nothing missing. Also I could track the problem down a little bit further:

The line above says, that things break in the function namesAndValueOf at line 145, which is this line

    pair.display = name + ': ' + presentableValueFor(aObject[name], name);
    
What I did was surrounding that line with a try catch and do some more tests on it:

    try {
      pair.display = name + ': ' + presentableValueFor(aObject[name], name);      
    }
    catch (e) {
      Services.console.logStringMessage('Fail on:' + name);
      Services.console.logStringMessage(aObject);

      try {
        Services.console.logStringMessage(aObject.wrappedJSObject[name]);        
      }
      catch (e) {
        Services.console.logStringMessage("Wrapped access fails! " + e);
      }

      try {
        Services.console.logStringMessage(aObject[name]);        
      }
      catch (e) {
        Services.console.logStringMessage("Direct access fails! " + e);
      }

      Services.console.logStringMessage('Catched: ' + e);
      continue;
    }

The output in the error console is like this:

    Fail on:schemaTypeInfo
    
    [object XPCNativeWrapper [object Attr]]
    
    Wrapped access fails! [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOM3Attr.schemaTypeInfo]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]
    
    Direct access fails! [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOM3Attr.schemaTypeInfo]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: chrome://global/content/PropertyPanel.jsm :: namesAndValuesOf :: line 161"  data: no]
    
    Catched: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOM3Attr.schemaTypeInfo]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: chrome://global/content/PropertyPanel.jsm :: namesAndValuesOf :: line 147"  data: no]

I tried to figure this out by reading docs on the MDC, but couldn't find something (I haven't understand the entire mechanism of wrapping objects to be honest...).

@Rob: Could you ask/ping some Firefox expert for this problem?

Comment 17

[Rob Campbell [:rc] (:robcee)]

checked this out this afternoon. I had to update part 3 to import cleanly against my repo (mozilla-inspector-3), but it works well.

I'm making another minor correction in part 3 and will update the version in this patch and will check it into the inspector repo.

Another item: Items that don't have children probably shouldn't have twisties. e.g., functions.

otherwise this is pretty great. Thanks!

Comment 18

[Rob Campbell [:rc] (:robcee)]

Attachment: part3: Replace the list-dom-panel with the new PropertyPanel - updated

updated part3 after import.

Comment 19

[Rob Campbell [:rc] (:robcee)]

DOM panel tests pass with patch 4 applied. Pushed to hg.mozilla.org/users/rcampbell_mozilla.com/mozilla-inspector-3

comparing with ssh://hg.mozilla.org/users/rcampbell_mozilla.com/mozilla-inspector-3
searching for changes
changeset:   44189:5d778e57f5d6
user:        Julian Viereck <[e-mail]>
date:        Wed Jun 30 13:49:16 2010 -0300
summary:     Implements a reuseable PropertyPanel.

changeset:   44190:4ea5c1e6320b
user:        Rob Campbell <[e-mail]>
date:        Wed Jun 30 13:49:37 2010 -0300
summary:     # User Julian Viereck <[e-mail]>

changeset:   44191:5a554b5d2907
user:        Rob Campbell <[e-mail]>
date:        Wed Jun 30 14:28:03 2010 -0300
summary:     added missing semicolon in clearDOMPanel

changeset:   44192:8c5c71636ee5
user:        Rob Campbell <[e-mail]>
date:        Wed Jun 30 14:35:42 2010 -0300
summary:     # User Julian Viereck <[e-mail]>

changeset:   44193:d9c9bb24bf03
tag:         tip
user:        Rob Campbell <[e-mail]>
date:        Wed Jun 30 14:36:12 2010 -0300
summary:     Buttons for the PropertyPanel can be passed to the constructor. Add 'Update' and 'Close' buttons to the console property panel.

Comment 20

[Julian Viereck]

(In reply to comment #17)
> Another item: Items that don't have children probably shouldn't have twisties.
> e.g., functions.

Functions have some properties as well, e.g. `name`. The problem is, that I can't get them as long as the Object.getOwnPropertyNames function isn't implemented (see bug 518663). As soon as it is, I have to improve the lookup algo a little bit and then functions will have children.

Comment 21

[Rob Campbell [:rc] (:robcee)]

sounds good.

Comment 22

[David Dahl :ddahl]

rob:

I have been doing hg qref -u "Julian Viereck <[e-mail]>" to get proper blame. the commit message is not set-able though. I thought you could but cannot find the docs for that. maybe edit the patch's header?

Comment 23

[David Dahl :ddahl]

Julian:

Can you update your heads-up-display repo and create a folded patch for the 3 patches that belong in console/hudservice ?

I just tried, but there were a bunch of failures:

 ~/code/moz/user_repos/HUD/mozilla % hg qpush
applying JSPropertyPanel-5.diff
patching file toolkit/components/console/hudservice/HUDService.jsm
Hunk #1 FAILED at 2319
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/console/hudservice/HUDService.jsm.rej
unable to find 'toolkit/components/console/hudservice/PropertyPanel.jsm' for patching
3 out of 3 hunks FAILED -- saving rejects to file toolkit/components/console/hudservice/PropertyPanel.jsm.rej
patch failed, unable to continue (try -v)
toolkit/components/console/hudservice/PropertyPanel.jsm: No such file or directory
patch failed, rejects left in working dir
errors during apply, please fix and refresh JSPropertyPanel-5.diff

I'm sure you can fix this quicker than me.

Comment 24

[Julian Viereck]

Attachment: Folded patch for DDahl's repo (applies on 732bb261eb4b)

This is a folded patch that includes attachment 454845 [details] [diff] [review], 454846 and 454959 and applies on top of changset 732bb261eb4b.

Comment 25

[David Dahl :ddahl]

not to self:

I entered "document" into the console and clicked on the resulting logged item an d I get this erorr:
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOM3Document.domConfig]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]

Comment 26

[David Dahl :ddahl]

more fun errors when clicking on the window itself:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303E8: file /home/ddahl/code/moz/user_repos/HUD/mozilla/dom/base/nsDOMClassInfo.cpp, line 8262
JavaScript error: , line 0: uncaught exception: [Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"  location: "<unknown>"]

Comment 27

[David Dahl :ddahl]

Julian: in the console patch we really should attach the panel over to the right since it covers up the console output. I'm going to push this, we can file followup bugs.

86da36043d1d 2010-07-01 15:45 -0700	Julian Viereck - Folded patch that adds the property panel to the HUD.

Pushed!

Comment 28

[Johnathan Nightingale [:johnath]]

(In reply to comment #22)
> rob:
> 
> I have been doing hg qref -u "Julian Viereck <[e-mail]>" to get
> proper blame. the commit message is not set-able though. I thought you could
> but cannot find the docs for that. maybe edit the patch's header?

qref -e to open a text editor for commit message, or qref -m "Commit Message" to do it on the command line

Comment 29

[David Dahl :ddahl]

(In reply to comment #28)
> (In reply to comment #22)
> > rob:

> qref -e to open a text editor for commit message, or qref -m "Commit Message"
> to do it on the command line

Thanks, I tried to do something like that but it failed with a worthless error message

Comment 30

[Rob Campbell [:rc] (:robcee)]

a couple of quick observations:

1) this will need some basic tests before landing. I didn't see any in the folded patch, although something similar to the browser_inspector-domPanel.js tests in /browser/base/content/test should work.

2) make sure you add me to the PropertyPanel.jsm license section under Contributors. Some of that's my code.

I'll follow up with more in-depth review notes shortly.

Comment 31

[Julian Viereck]

Attachment: Required patch from ddahls repo

exposes 'jsterm' on the HUD object. This patch is necessary to get the jsterm
object when running unit tests (e.g. for tab completion).

Comment 32

[Julian Viereck]

Attachment: Implements DOM/JS Object Inspector

Comment 33

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #32)
> Created attachment 457403 [details] [diff] [review]
> Implements DOM/JS Object Inspector

This is looking pretty good, but you'll need to use JavaDoc style comments in your PropertyTreeView class. When that's done we can request review.

Comment 34

[Julian Viereck]

Attachment: Patch 2

Quite the same patch as in 457403 but now with JavaDocs all over the place.

Note: depends on 579073

Comment 35

[Dietrich Ayala (:dietrich)]

Comment on attachment 457582 [details] [diff] [review]
Patch 2

how do i test this? i applied the patch, but there's nothing clickable in the console output that i can see.

Comment 36

[Julian Viereck]

(In reply to comment #35)
> Comment on attachment 457582 [details] [diff] [review]
> Patch 2
> 
> how do i test this? i applied the patch, but there's nothing clickable in the
> console output that i can see.

The UX for this is really bad at the moment. This is the next things on my plate.

Execute something on the command line, that returns an object (e.g. `a = { b: 1 }`). You will see something like `[object Object]` in the output. When you move your mouse over this output, the cursor should become a pointer and when you click on it, the panel should show up.

Note: The version of the PropertyPanel in this patch is not able to inspect the 'window' or 'document' object, but after applying the patch from bug 579339, this works. You might want to review this patch together with the patch from bug 579339 (fold them), as I changed some things in 579339, that are introduced in this bug.

Comment 37

[Julian Viereck]

Attachment: Patch 2 Rebase

Same as former Patch 2 attachment 457582 [details] [diff] [review] but rebased to apply clean on top of trunk. Also, fix some comment indention.

Comment 38

[Julian Viereck]

Attachment: Patch 2 Rebase 2

Rebase with trunk.

Comment 39

[Julian Viereck]

Attachment: Patch 2 Rebase 3

Comment 40

[Julian Viereck]

I'm requesting blocking status for this bug as it implements an important feature that is required by the WebConsole (aka Heads Up Display) and the new DOM Inspector.

Comment 41

[David Dahl :ddahl]

The important feature is object inspection and is the engine behind a lot of the discover-ability of our new DevTools.

Comment 42

[Johnathan Nightingale [:johnath]]

I'd like to block once on either this bug or bug 579339. The latter was filed as a follow up to this one, but this hasn't landed yet - is merging them into one bug a bad idea for some reason?

Comment 43

[Julian Viereck]

Attachment: Part 1

Merged patch part 1: Adding the PropertyPanel.jsm

Comment 44

[Julian Viereck]

Attachment: Part 2

Merged Patch Part 2: Implement interaction between JS output and PropertyPanel + unit tests. After executing something in the WebConsole, you can inspect the resulting object by clicking on the output string. This opens a PropertyPanel.

Comment 45

[Julian Viereck]

(In reply to comment #42)
> I'd like to block once on either this bug or bug 579339. The latter was filed
> as a follow up to this one, but this hasn't landed yet - is merging them into
> one bug a bad idea for some reason?

I've merged the patches from this bug and bug 579339. The final patch was split up into two smaller patches, that are hopefully easier for review.

Comment 46

[Robert Strong (they/them - no direct email)]

I'll try to get to this review within the week... I have a couple of blocker bugs and a crashkill bug that have priority.

Comment 47

[Rob Campbell [:rc] (:robcee)]

That'd be great. Thanks for the update, Rob!

Comment 49

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 460260 [details] [diff] [review]
Part 1

>diff --git a/toolkit/components/console/jar.mn b/toolkit/components/console/jar.mn

>+*+ content/global/PropertyPanel.jsm                     (hudservice/PropertyPanel.jsm)

>diff --git a/toolkit/components/console/hudservice/Makefile.in b/toolkit/components/console/hudservice/Makefile.in

> EXTRA_JS_MODULES = HUDService.jsm \
>+        PropertyPanel.jsm \

Why add it both to the JAR and as a module? jar.mn addition seems unnecessary.

>diff --git a/toolkit/components/console/hudservice/PropertyPanel.jsm b/toolkit/components/console/hudservice/PropertyPanel.jsm

>+function presentableValueFor(aObject)

>+  // (aObject instanceof RegExp) is false for RegExp.

Because aObject's global is different from the module's global object, presumably. aObject.constructor.name may be a more reliable regexp test. Might also work for functions.

>+//// PropertyTreeView.

>+function createElement(aDocument, aTag, aAttributes)

>+  for (var attr in aAttributes) {
>+    node.setAttribute(attr, aAttributes[attr]);
>+  }

nit: could use aAttributes.forEach().

>+function PropertyPanel(aParent, aDocument, aTitle, aObject, aButtons)

>+  let panel = this.panel = createElement(aDocument, "panel", {

nit: getting rid of "panel" and just using "this.panel" consistently would make things a bit clearer, IMO (I missed the second assignment initially).

>+    "class": "dimmable",

I think we got rid of this in later revisions of the style panel patch?

>+    noautohide: "true",
>+    level: "top",
>+    "aria-labelledby": "inspectDOMPanelTitle"

"inspectDOMPanelTitle" doesn't exist. https://developer.mozilla.org/en/XUL/panel also claims that specifying noautohide means that "level" is ignored. Should probably get Enn to audit these attributes on all inspector-related panels once they all exist.

>+  let lable = aDocument.createElement("label");

label? :)

>+  appendChild(aDocument, treecols, "treecol", {
>+    label: "Element",

needs l10n

>+      // QUESTION: How to make this international?

l10n should be handled by the caller, presumably?

>+  // Set the treeView object on the tree view. This has to be done *after*
>+  // the panel is shown.

Because the tree binding must be attached, presumably. Would be good to note that in the comment. An alternative would be to force a style flush (and therefore the binding attachment), but I guess that isn't necessary (unless only drawing the tree contents after the panel initially shows causes flickering?).

Comment 50

[Julian Viereck]

Attachment: Part 1, take 2

(In reply to comment #49)
> Comment on attachment 460260 [details] [diff] [review]
> Part 1
> 
> >diff --git a/toolkit/components/console/jar.mn b/toolkit/components/console/jar.mn
> 
> >+*+ content/global/PropertyPanel.jsm                     (hudservice/PropertyPanel.jsm)
> 
> >diff --git a/toolkit/components/console/hudservice/Makefile.in b/toolkit/components/console/hudservice/Makefile.in
> 
> > EXTRA_JS_MODULES = HUDService.jsm \
> >+        PropertyPanel.jsm \
> 
> Why add it both to the JAR and as a module? jar.mn addition seems unnecessary.

The PropertyPanel.jsm needs to be importable from the /browser as well (the DOM Inspector uses the PropertyPanel). That's why I've added this line in jar.mn. (I'm still don't understand how Firefox is working completely so I could be completely wrong and there might be an easier way to do this.)

> 
> >diff --git a/toolkit/components/console/hudservice/PropertyPanel.jsm b/toolkit/components/console/hudservice/PropertyPanel.jsm
> 
> >+function presentableValueFor(aObject)
> 
> >+  // (aObject instanceof RegExp) is false for RegExp.
> 
> Because aObject's global is different from the module's global object,
> presumably. aObject.constructor.name may be a more reliable regexp test. Might
> also work for functions.

aObject.constructor.name, that sounds good. I use this within a switch(aObject.constructor.name) { case ...: } now. It makes the code look much cleaner ;)

> 
> >+//// PropertyTreeView.
> 
> >+function createElement(aDocument, aTag, aAttributes)
> 
> >+  for (var attr in aAttributes) {
> >+    node.setAttribute(attr, aAttributes[attr]);
> >+  }
> 
> nit: could use aAttributes.forEach().

aAttributes is an object, not an array. forEach() doesn't work on objects.

> 
> >+function PropertyPanel(aParent, aDocument, aTitle, aObject, aButtons)
> 
> >+  let panel = this.panel = createElement(aDocument, "panel", {
> 
> nit: getting rid of "panel" and just using "this.panel" consistently would make
> things a bit clearer, IMO (I missed the second assignment initially).

Done.

> 
> >+    "class": "dimmable",
> 
> I think we got rid of this in later revisions of the style panel patch?

Removed.

> 
> >+    noautohide: "true",
> >+    level: "top",
> >+    "aria-labelledby": "inspectDOMPanelTitle"
> 
> "inspectDOMPanelTitle" doesn't exist.
> https://developer.mozilla.org/en/XUL/panel also claims that specifying
> noautohide means that "level" is ignored. Should probably get Enn to audit
> these attributes on all inspector-related panels once they all exist.
> 
> >+  let lable = aDocument.createElement("label");
> 
> label? :)

I removed the label + the toolbar. Instead, the attribute "titlebar": "normal" to put the panel inside of an small window (a new feature Neil implemented). The title is now set as a "label" attribute on the panel.

Note: There is currently a problem with selecting rows within an XUL tree that lives inside of on of these "new" panels, see bug 583255.

> 
> >+  appendChild(aDocument, treecols, "treecol", {
> >+    label: "Element",
> 
> needs l10n

I removed the "Element" string. I wanted to hide the treecol entirely, because there is no need for it but that didn't worked out. Might fill a follow bug.

> 
> >+      // QUESTION: How to make this international?
> 
> l10n should be handled by the caller, presumably?

Right.

> 
> >+  // Set the treeView object on the tree view. This has to be done *after*
> >+  // the panel is shown.
> 
> Because the tree binding must be attached, presumably. Would be good to note
> that in the comment. An alternative would be to force a style flush (and
> therefore the binding attachment), but I guess that isn't necessary (unless
> only drawing the tree contents after the panel initially shows causes
> flickering?).

Expanded the comment. There is no flickering, so this way of doing it is all fine :)

Comment 51

[Julian Viereck]

Attachment: Part2, take 2

This is an updated version of the patch that applies clean on trunk.

Note: This patch is missing i10n support. I need to add a reference to the parent HUD on the JSTerm object to call the HUD.getStr() function that is used to lookup an i10n string. Changing might require some more refactoring, so I don't want it to go with this bug as this one introduces enough new stuff already.

Comment 52

[Julian Viereck]

Attachment: Part 1, take 2.1

This is based on patch "Part 1, take 2" (attachment 462516 [details] [diff] [review]) with the change that the panel's "level" attribute is now set to "floating" (see bug 584344).

Comment 53

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #50)
> The PropertyPanel.jsm needs to be importable from the /browser as well (the DOM
> Inspector uses the PropertyPanel). That's why I've added this line in jar.mn.
> (I'm still don't understand how Firefox is working completely so I could be
> completely wrong and there might be an easier way to do this.)

Its inclusion in EXTRA_JS_MODULES adds it to the GRE modules directory, which makes it import()able by everyone. There's no need to add it to the JAR and give it a chrome:// URI, which is what the jar.mn addition does, so you can remove that change.

> > nit: getting rid of "panel" and just using "this.panel" consistently would make
> > things a bit clearer, IMO (I missed the second assignment initially).
> 
> Done.

This doesn't seem to be included in the updated patch.

Comment 54

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 462855 [details] [diff] [review]
Part 1, take 2.1

>diff --git a/toolkit/components/console/hudservice/PropertyPanel.jsm b/toolkit/components/console/hudservice/PropertyPanel.jsm

>+ * The Initial Developer of the Original Code is
>+ *   Mozilla Foundation

nit: "the Mozilla Foundation" (without the space at the front).

>+function presentableValueFor(aObject)

>+  let presentable;
>+  switch (aObject.constructor.name) {

You should probably guard against constructor being null (e.g. ({constructor: null})) (falling back to the simple "Object" case, I guess?).

>+    case "JavaArray":

o_O Where does this come from?

>+PropertyTreeView.prototype = {

>+  getChildItems: function(aItem, aRootElement)
>+  {
>+    // If item.children is an array, then the children has already been
>+    // computed and can get returned directly.
>+    if (aItem && aItem.children instanceof Array) {

Won't this fail if I try to inspect ({children:[1,2,3]})? Could reduce the likelihood of there being problems by using .__introspectorChildren or some other obscure property name, but perhaps we should consider keeping the data separately from the inspected object (in a mirrored object?).

>+  /** nsITreeView interface implementation **/

Looks like you're missing: canDrop, drop, isSelectable, setCellValue, setCellText, performActionOnRow.

>+  treeBox: null,

treeBox is actually "private" and not part of nsITreeView, right? Add a prefix and move it above the comment?

>+  isContainer: function(idx)         { return this.rows[idx].children; },

use !! for clarity?

>+  getParentIndex: function(idx)

Please put return statements on their own line. This function also doesn't always necessarily return a value (which triggers a strict warning - add a |return -1| at the end).

>+  hasNextSibling: function(idx, after)

>+    for (var t = after + 1; t < this.visibleData.length; t++) {

I don't see visibleData defined anywhere.

>+  toggleOpenState: function(idx)

>+      var thisLevel = item.level;
>+      var deleteCount = 0;
>+      for (var t = idx + 1; t < this.rows.length; t++) {
>+        if (this.getLevel(t) > thisLevel) deleteCount++;
>+        else break;
>+      }

I find:
var t = idx + 1, deletecount = 0;
while (t < this.rows.length && this.getLevel(t++) > thisLevel)
  deleteCount++;

clearer, but it's up to you. If you keep it as-is, add some newlines after the if and else.

>+function PropertyPanel(aParent, aDocument, aTitle, aObject, aButtons)

>+  this.panel = createElement(aDocument, "panel", {

>+    level: "floating",
>+    noautohide: "true"

You're still specifying both of these attributes. Is the MDC <panel> page wrong about the latter overriding the former?

>+  // The footer can have butttons.
>+  for each (var button in aButtons) {
>+    let buttonNode = appendChild(aDocument, footer, "button", {
>+      label: button.label
>+    });

probably want accesskeys as well?

>+    buttonNode.onclick = button.onclick;

Hmm, does this work?

>+PropertyPanel.prototype.destroy = function PP_destory()
>+{
>+  this.panel.hidePopup();
>+  this.panel.parentNode.removeChild(this.panel);

Perhaps this should null out treeView explicitly, just to be safe...

Comment 55

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 462855 [details] [diff] [review]
Part 1, take 2.1

>diff --git a/toolkit/components/console/hudservice/PropertyPanel.jsm b/toolkit/components/console/hudservice/PropertyPanel.jsm

>+PropertyTreeView.prototype = {

>+  set data(aObject) {
>+    if (this.treeBox && this.rows.length != 0) {
>+      this.treeBox.rowCountChanged(0, this.rows.length * -1)
>+    }
>+    this.rows = this.getChildItems(aObject, true);
>+    if (this.treeBox) {
>+      this.treeBox.rowCountChanged(0, this.rows.length);
>+    }

How about:
let oldLen = this.rows.length;
this.rows = this.getChildItems(aObject, true);
if (this.treeBox) {
  this.treeBox.beginUpdateBatch();
  if (oldLen)
    this.treeBox.rowCountChanged(0, -oldLen);
  this.treeBox.rowCountChanged(0, this.rows.length);
  this.treeBox.endUpdateBatch();
}

Comment 56

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 462519 [details] [diff] [review]
Part2, take 2

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>   execute: function JST_execute(aExecuteString)

>       if (result || result === false || result === " ") {

unrelated, but that last check is redundant (" " is true-y)

>+  openPropertyPanel: function JST_openPropertyPanel(aEvalString, aOutputObject,
>+                                                    aAnchor)

>+    try {
>+      if (!this.PropertyTreeView) {
>+        Cu.import("chrome://global/content/PropertyPanel.jsm", this);
>+      }
>+    } catch (err) {
>+      dump("ERROR" + err + "\n");

Use Cu.reportError, not dump(). This could be a global lazy getter, though (as in e.g. http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#282 )

>+          catch (ex) {
>+            if (ex) {
>+              self.console.error(ex);
>+            }

ex can never be null, I hope!

>+  writeOutputJS: function JST_writeOutputJS(aEvalString, aOutputObject)

>+    var link = this.elementFactory("span");
>+    link.setAttribute("class", "hud-clickable");
>+    link.onclick = function() {
>+      self.openPropertyPanel(aEvalString, aOutputObject, link);
>+    }

Can we just use an <a> rather than faking with a span+style? Might want some ARIA on this too - ask davidb?

Comment 57

[:Gavin Sharp [email: [e-mail]]]

Can I get a rollup that includes both patches as well as the changes to address these comments? Should be quick to r+ once that's here.

Comment 58

[:Gavin Sharp [email: [e-mail]]]

Also forgot to mention that part 2 needs l10n work, but I think you're aware of that.

Comment 59

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #56)
> Comment on attachment 462519 [details] [diff] [review]
> >+    var link = this.elementFactory("span");
> >+    link.setAttribute("class", "hud-clickable");
> >+    link.onclick = function() {
> >+      self.openPropertyPanel(aEvalString, aOutputObject, link);
> >+    }
> 
> Can we just use an <a> rather than faking with a span+style? Might want some
> ARIA on this too - ask davidb?

Julian found me on IRC thanks!

Julian you could go with a regular <a> as Gavin suggests, and or use ARIA to describe the element. For example for could add:
link.setAttribute("role", "link");
link.setAttribute("aria-haspopup", "true"); // OR
link.setAttribute("aria-controls", "[id of display panel]");

I'm cc'ing Marco so he can test try builds and we can file follow ups if we need them.

There isn't a good keyboard idiom for dealing with vast amounts of actionable text - is that the case here? Putting them in the tab order can hinder more than help, but we should probably consider it (perhaps on another bug - not sure).

Humph, what did you end up doing in dxr? You had a similar situation I think.

Comment 60

[David Humphrey (:humph)]

> Humph, what did you end up doing in dxr? You had a similar situation I think.

I use aria-haspopup="true" on its own.

Comment 61

[Julian Viereck]

Attachment: Combined patch, take 3

Comment 62

[Julian Viereck]

Folded comments to new attachment: 

> >+    case "JavaArray":
>
> o_O Where does this come from?

See:
https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Objects/Object/toString

There is a toString() method mentioned for JavaArray. Don't think this is common, but seems to be there.

> Won't this fail if I try to inspect ({children:[1,2,3]})? Could reduce the
> likelihood of there being problems by using .__introspectorChildren or some
> other obscure property name, but perhaps we should consider keeping the data
> separately from the inspected object (in a mirrored object?).

Yeah, it would. I've added a !rootElement to the if (...). Also added comments there.

> >+    level: "floating",
> >+    noautohide: "true"

> You're still specifying both of these attributes. Is the MDC <panel> page wrong
> about the latter overriding the former?

Seems like noautohide="true" is necessary to get the new panel-toolbar working. Not sure if that's a bug or a feature...

> >+    buttonNode.onclick = button.onclick;
> 
> Hmm, does this work?

Why shouldn't it? I can click on the buttons and it works. So I'm quite sure the answer is "yes".

> >       if (result || result === false || result === " ") {
>
>  unrelated, but that last check is redundant (" " is true-y)

" " == false << it's not true! Therefor, this extra check is necessary.

> >+    var link = this.elementFactory("span");
> >+    link.setAttribute("class", "hud-clickable");
> >+    link.onclick = function() {
> >+      self.openPropertyPanel(aEvalString, aOutputObject, link);
> >+    }
>
> Can we just use an <a> rather than faking with a span+style? Might want some
> ARIA on this too - ask davidb?

It was an <a> once. I've added aria-haspopup="true" as suggested by David. 

> Also forgot to mention that part 2 needs l10n work, but I think you're aware of
> that.

Will open a follow up bug...

Comment 63

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #62)
> Folded comments to new attachment: 
> 
> > >+    case "JavaArray":
> >
> > o_O Where does this come from?

LiveConnect is gone (bug 442399). Additionally, you didn't special-case any of the other LiveConnect types, so you shouldn't here. (Especially if you're fine with leaving out real JS types like Error and its various flavors.)

> See:
> https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Objects/Object/toString

These docs need updating in light of the whole "No more LiveConnect" thing.

Comment 64

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #62)
> > >       if (result || result === false || result === " ") {

> " " == false << it's not true! Therefor, this extra check is necessary.

I don't understand. If |result| is " ", |if (result)| will be true, so the last check is redundant.
 
> Will open a follow up bug...

Why followup bug? Getting these strings from a stringbundle is not difficult, we should do it right before landing.

Comment 65

[Julian Viereck]

Attachment: Combined patch, take 4

Updated the patch based on Gavin's latest feedback.

Comment 66

[Julian Viereck]

Attachment: Combined patch, take 4.1

Same as patch 463527 but without trailing whitespaces.

Comment 67

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 463528 [details] [diff] [review]
Combined patch, take 4.1

>+    case "JavaArray":

Why are you still doing this? See comment 63.

Comment 68

[Julian Viereck]

Attachment: Combined patch, take 4.2

(In reply to comment #67)
> Comment on attachment 463528 [details] [diff] [review]
> Combined patch, take 4.1
> 
> >+    case "JavaArray":
> 
> Why are you still doing this? See comment 63.

Sorry, forgotten about this one. Fixed in the new patch.

Comment 69

[Kevin Dangoor]

Comment on attachment 463543 [details] [diff] [review]
Combined patch, take 4.2

Gavin's out for a few days now. Hoping we can finish this up with a new reviewer.

Comment 70

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 463543 [details] [diff] [review]
Combined patch, take 4.2

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  openPropertyPanel: function JST_openPropertyPanel(aEvalString, aOutputObject,
>+                                                    aAnchor)

>+    } catch (err) {
>+      Cu.reportError("Error" + err + "\n");
>+    }

nit: space after "Error" (or actually just omit it, it's redundant - "\n" also not necessary)

>+    // 2. `Close`: destroyes the panel.

nit: destroys

>+    let doc = self.parentNode.ownerDocument;

(unrelated: The fact that JSTerm (and HeadsUpDisplay) instances have non-DOM "parentNode" properties confused me for a bit! Maybe we should that property for clarity's sake.)

>+    let parent = doc.getElementById("placesContext").parentNode;

Why not doc.getElementById("mainPopupSet")?

>+    let title = (aEvalString
>+        ? HUDService.getStr("jsPropertyInspectTitle") + ": " + aEvalString

This kind of string concatenation is typically not very l10n friendly. Use getFormatStr instead.

>diff --git a/toolkit/components/console/hudservice/PropertyPanel.jsm b/toolkit/components/console/hudservice/PropertyPanel.jsm

>+function presentableValueFor(aObject)

I wonder whether "Array" and "Object" as generic descriptors might be usefully localizable. File a followup perhaps?

>+  switch (aObject.constructor.name) {

Need to null check aObject.constructor (can just make this |switch (aObject.constructor && aObject.constructor.name)| ).

>+function namesAndValuesOf(aObject)

>+    pair.display = propName + ": " + presentable.display;

Maybe include this in the followup to evaluate potential l10nization?

>+PropertyTreeView.prototype = {

>+  getChildItems: function(aItem, aRootElement)
>+  {
>+    // If item.children is an array, then the children has already been
>+    // computed and can get returned directly.
>+    // Skip this checking if aRootElement is true. It could happen, that aItem
>+    // is passed as ({children:[1,2,3]}) which would be true, although these
>+    // "kind" of children has no value/type etc. data as needed to display in
>+    // the tree.
>+    if (!aRootElement && aItem && aItem.children instanceof Array) {

Actually, now that I think of it, the instanceof check would probably catch the example mentioned here, since the passed-in object's .children would be instanceof itsWindow.Array, rather than this module's global Array. In other words, the bug that makes presentableValueFor more complicated might actually be useful in this case... Probably best to keep the !aRootElement check, but re-word the comment accordingly.

>+  /** nsITreeView interface implementation **/

>+  hasNextSibling: function(idx, after)

var thisLevel = this.getLevel(idx);
return this._rows.slice(after + 1).some(function (r) r.level == thisLevel);

(other methods may be able to make use of similar tricks)
  
>+  toggleOpenState: function(idx)

An updateBatch here wouldn't hurt.

>+function PropertyPanel(aParent, aDocument, aTitle, aObject, aButtons)

>+  // Create the underlaying panel
>+  this.panel = createElement(aDocument, "panel", {
>+    label: aTitle,
>+    level: "floating",
>+    orient: "vertical",
>+    titlebar: "normal",
>+    ignorekeys: "true",
>+    noautofocus: "true",
>+    noautohide: "true"
>+  });

Think I mentioned this earlier, but Neil Deakin should take a look at all the devtool-related panel usage (attributes used, etc.) once things settle down - maybe file a followup?

>diff --git a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties

>+buttonClose=Close
>+accesskeyClose=C
>+buttonUpdate=Update
>+accesskeyUpdate=U

Use [update|close].[button|accesskey] as string names for better l10n tool support.

>diff --git a/toolkit/themes/pinstripe/global/headsUpDisplay.css b/toolkit/themes/pinstripe/global/headsUpDisplay.css
>diff --git a/toolkit/themes/winstripe/global/headsUpDisplay.css b/toolkit/themes/winstripe/global/headsUpDisplay.css

>+.hud-output-node .hud-clickable {
>+    cursor: pointer;
>+}

Was using an actual <a> element not feasible? Would be nicer than having to do this styling manually...

r- because I'd like to see an updated patch, but I think this is pretty close!

Comment 71

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #70)
> I wonder whether "Array" and "Object" as generic descriptors might be usefully
> localizable. File a followup perhaps?

> Maybe include this in the followup to evaluate potential l10nization?

Could maybe even just roll these in to bug 585030.

Comment 72

[Neil Deakin]

>+  // Create the underlaying panel
>+  this.panel = createElement(aDocument, "panel", {
>+    label: aTitle,
>+    level: "floating",
>+    orient: "vertical",
>+    titlebar: "normal",
>+    ignorekeys: "true",
>+    noautofocus: "true",
>+    noautohide: "true"
>+  });

orient: "vertical" isn't needed as this is the default for popups and panels.

level: "floating" isn't needed as this is default for panels with titlebars.

ignorekeys: "true" also isn't needed as it shouldn't have any effect on noautohide panels.

Also, 'underlaying' -> 'underlying'

Comment 73

[Julian Viereck]

Attachment: Take 5

(In reply to comment #70)
> Comment on attachment 463543 [details] [diff] [review]
> Combined patch, take 4.2

> >+function presentableValueFor(aObject)
> 
> I wonder whether "Array" and "Object" as generic descriptors might be usefully
> localizable. File a followup perhaps?
> 

> >+function namesAndValuesOf(aObject)
> 
> >+    pair.display = propName + ": " + presentable.display;
> 
> Maybe include this in the followup to evaluate potential l10nization?

Added a comment + screenshot in bug 585030.

> >+function PropertyPanel(aParent, aDocument, aTitle, aObject, aButtons)
> 
> >+  // Create the underlaying panel
> >+  this.panel = createElement(aDocument, "panel", {
> >+    label: aTitle,
> >+    level: "floating",
> >+    orient: "vertical",
> >+    titlebar: "normal",
> >+    ignorekeys: "true",
> >+    noautofocus: "true",
> >+    noautohide: "true"
> >+  });
> 
> Think I mentioned this earlier, but Neil Deakin should take a look at all the
> devtool-related panel usage (attributes used, etc.) once things settle down -
> maybe file a followup?

Filled bug 585583. Neil commented already on this bug and this patch has his feedback included. Thanks Neil.

> Was using an actual <a> element not feasible? Would be nicer than having to do
> this styling manually...

Changed to an <a> tag. The only downside with this is, that the user sees a "javascript:" in the status bar when he hovers over that <a> tag.
 
> r- because I'd like to see an updated patch, but I think this is pretty close!

Gavin, your doing a great review job. I appreciate that!

Comment 74

[Julian Viereck]

Attachment: Take 5, rebase 1

Rebased patch. This was necessary as some of Patrick's work has landed which required some small modifications to the JST_writeOutputJS function. Instead of adding the new nodes directly to the JSTerm outputNode, they are now added to an group node.

Comment 75

[Julian Viereck]

Attachment: Take 5, rebase 1 fix 1

Missed the call to the unit test function for this bug during rebasing. This patch includes it again.

Comment 76

[Kevin Dangoor]

Comment on attachment 464386 [details] [diff] [review]
Take 5, rebase 1 fix 1

Though Gavin has nearly finished with this one, I don't want to wait until he gets back because I'm worried that we'll miss beta 4. We need this object inspector for both the Console and the Inspector and bug 561782 has already completed review and is waiting on this bug.

Comment 77

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 464386 [details] [diff] [review]
Take 5, rebase 1 fix 1

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  openPropertyPanel: function JST_openPropertyPanel(aEvalString, aOutputObject,
>+                                                    aAnchor)

>+    try {
>+      if (!this.PropertyTreeView) {
>+        Cu.import("resource://gre/modules/PropertyPanel.jsm", this);
>+      }
>+    } catch (err) {
>+      Cu.reportError(err);
>+    }

Could make this a global lazy getter, e.g. http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#282 .

>diff --git a/toolkit/components/console/hudservice/PropertyPanel.jsm b/toolkit/components/console/hudservice/PropertyPanel.jsm

>+function presentableValueFor(aObject)

>+    case null:
>+    case undefined:

No need to list these explicitly, is there?

>+function PropertyPanel(aParent, aDocument, aTitle, aObject, aButtons)

>+  for each (var button in aButtons) {
>+    let buttonNode = appendChild(aDocument, footer, "button", {
>+      label: button.label,
>+      accesskey: button.accesskey || ""
>+    });
>+    buttonNode.onclick = button.onclick;

This should actually be oncommand, I suppose. If .oncommand = function (){} doesn't work, you can use addEventListener with a closure...

>diff --git a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties

>+jsPropertyTitle=Object Inspector
>+jsPropertyInspectTitle=Inspect: %S

These could probably use some LOCALIZATION NOTES to explain how they're used.

Comment 78

[:Gavin Sharp [email: [e-mail]]]

Should also get followups filed for the TODO comments in the patch, and add references to the bug #s.

Comment 79

[Julian Viereck]

Attachment: Take 6

Improved based on Gavin's latest review.

Comment 80

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 464784 [details] [diff] [review]
Take 6

removed additional line-spacing between comment @param and @returns.

Getting ready for checkin.

Comment 81

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 464784 [details] [diff] [review]
Take 6

(note that my review was just carrying gavin's forward)

Comment 82

[Rob Campbell [:rc] (:robcee)]

Attachment: [backed-out] Take 7

This is ready for checkin...

Comment 83

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 464903 [details] [diff] [review]
[backed-out] Take 7

http://hg.mozilla.org/mozilla-central/rev/8b3fd544de43

Comment 84

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 464903 [details] [diff] [review]
[backed-out] Take 7

Backed out as part of http://hg.mozilla.org/mozilla-central/rev/5f699108f3cf.

We need to investigate the leak logs here:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281575546.1281577015.2705.gz

I believe this patch may be the cause of the problem. Check that the PropertyPanels are being destroyed properly and references to them are being removed from the chrome window on destruction.

Comment 85

[Rob Campbell [:rc] (:robcee)]

Attachment: [backed-out] Take 8

hopefully deleaked and some minor corrections added. Try build is currently running and will post results here.

Comment 86

[Dietrich Ayala (:dietrich)]

I'll review once the try results are in, and confirmed ok.

Comment 87

[Rob Campbell [:rc] (:robcee)]

Try build seemed successful. I say "seemed" because it's impossible to actually find all of the logs associated with it. The warnings I did receive and were able to locate on debug seemed to be crashes or other spurious errors. I think this is fixed.

Comment 88

[Dietrich Ayala (:dietrich)]

Comment on attachment 465304 [details] [diff] [review]
[backed-out] Take 8

>+    this.panel.addEventListener("popuphidden", function onPopupHide()
>+    {
>+      self.panel.removeEventListener("popuphidden", onPopupHide, false);
>+      aButtons.forEach(function(button) {
>+        button.dom.removeEventListener("command", button.oncommand, false);
>+      });
>+    }, false);
>+  }

nit: if you didn't want to declare the self local var, you could do something like:

this.panel.addEventListener("popuphidden", function onPopupHide(e) {
  e.target.removeEventListener("popuphidden", arguments.callee, false);
  ...
});

same same though.

if you keep self, move the declaration down by this usage, if it's not used elsewhere.

>+PropertyPanel.prototype.destroy = function PP_destroy()
>+{
>+  this.panel.hidePopup();
>+  this.panel.parentNode.removeChild(this.panel);
>+  this.treeView = null;
>+  this.panel = null;
>+  this.tree = null;
>+}

uber-nit: group the panel lines and tree lines.

>+  let treeRows = propPanel.treeView._rows;
>+  is (treeRows[0].display, "0.01: 1", "1. element is okay");
>+  is (treeRows[1].display, "0.02: 0", "2. element is okay");
>+  is (treeRows[2].display, "1: 3",    "3. element is okay");
>+  is (treeRows[3].display, "1.1: 6",  "4. element is okay");
>+  is (treeRows[4].display, "1.2: 5",  "5. element is okay");
>+  is (treeRows[5].display, "02: 2",   "6. element is okay");
>+  is (treeRows[6].display, "11: 4",   "7. element is okay");
>+  is (treeRows[7].display, "bar: 8",  "8. element is okay");
>+  is (treeRows[8].display, "foo: 7",  "9. element is okay");
>+  propPanel.destroy();
>+}

why destroying propPanel manually here? what calls it's destroy method normally?

Comment 89

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #88)
> Comment on attachment 465304 [details] [diff] [review]
> Take 8
> 
> >+    this.panel.addEventListener("popuphidden", function onPopupHide()
> >+    {
> >+      self.panel.removeEventListener("popuphidden", onPopupHide, false);
> >+      aButtons.forEach(function(button) {
> >+        button.dom.removeEventListener("command", button.oncommand, false);
> >+      });
> >+    }, false);
> >+  }
> 
> nit: if you didn't want to declare the self local var, you could do something
> like:
> 
> this.panel.addEventListener("popuphidden", function onPopupHide(e) {
>   e.target.removeEventListener("popuphidden", arguments.callee, false);
>   ...
> });
> 
> same same though.
> 
> if you keep self, move the declaration down by this usage, if it's not used
> elsewhere.

I like e.target and arguments.callee. I'll switch to cut down on declarative solipsism.

> >+PropertyPanel.prototype.destroy = function PP_destroy()
> >+{
> >+  this.panel.hidePopup();
> >+  this.panel.parentNode.removeChild(this.panel);
> >+  this.treeView = null;
> >+  this.panel = null;
> >+  this.tree = null;
> >+}
> 
> uber-nit: group the panel lines and tree lines.

Wow!!!

> >+  let treeRows = propPanel.treeView._rows;
> >+  is (treeRows[0].display, "0.01: 1", "1. element is okay");
> >+  is (treeRows[1].display, "0.02: 0", "2. element is okay");
> >+  is (treeRows[2].display, "1: 3",    "3. element is okay");
> >+  is (treeRows[3].display, "1.1: 6",  "4. element is okay");
> >+  is (treeRows[4].display, "1.2: 5",  "5. element is okay");
> >+  is (treeRows[5].display, "02: 2",   "6. element is okay");
> >+  is (treeRows[6].display, "11: 4",   "7. element is okay");
> >+  is (treeRows[7].display, "bar: 8",  "8. element is okay");
> >+  is (treeRows[8].display, "foo: 7",  "9. element is okay");
> >+  propPanel.destroy();
> >+}
> 
> why destroying propPanel manually here? what calls it's destroy method
> normally?

I believe it has to be called explicitly. Ideally, we might want add a popuphidden callback that sends this message, but this is what we have for now.

Thanks for the review!

Comment 90

[Patrick Walton (:pcwalton)]

Backed out due to incompatibility with the XULification of the console (bug 586514).

Comment 91

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 9.

Rebased to trunk. Feedback and re-review requested, since this isn't my patch.

Comment 92

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 465304 [details] [diff] [review]
[backed-out] Take 8

backed out due to conflict with bug 586514.

Comment 93

[Julian Viereck]

Comment on attachment 465802 [details] [diff] [review]
Proposed patch, version 9.

Most of the patch is okay, but the printed JS object doesn't look like "clickable". I discussed this with Patrick and he wants to tune the CSS a little bit to make this more visible.

Comment 94

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 465802 [details] [diff] [review]
Proposed patch, version 9.

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  writeOutputJS: function JST_writeOutputJS(aEvalString, aOutputObject)

>+    node.onclick = function() {
>+      self.openPropertyPanel(aEvalString, aOutputObject, node);
>+    }

This should probably actually be keyboard accessible (i.e. tabbable and responsive to "Enter"). I don't think onclick on xul:labels handles that. Followup bug?

>diff --git a/toolkit/components/console/hudservice/PropertyPanel.jsm b/toolkit/components/console/hudservice/PropertyPanel.jsm

>+function PropertyPanel(aParent, aDocument, aTitle, aObject, aButtons)

>+    this.panel.addEventListener("popuphidden", function onPopupHide()
>+    {
>+      self.panel.removeEventListener("popuphidden", onPopupHide, false);
>+      aButtons.forEach(function(button) {
>+        button.dom.removeEventListener("command", button.oncommand, false);
>+      });
>+    }, false);

You don't need to do this, given that destroy() always gets called (and if it doesn't, there are bigger problems to worry about). Means you don't need to set button.dom, either.

>diff --git a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties

>+jsPropertyInspectTitle=Inspect: %S

Might have been mentioned elsewhere, but the long l10n note doesn't make it clear what "%S" is here.

Comment 95

[Julian Viereck]

(In reply to comment #94)
> Comment on attachment 465802 [details] [diff] [review]
> Proposed patch, version 9.
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >+  writeOutputJS: function JST_writeOutputJS(aEvalString, aOutputObject)
> 
> >+    node.onclick = function() {
> >+      self.openPropertyPanel(aEvalString, aOutputObject, node);
> >+    }
> 
> This should probably actually be keyboard accessible (i.e. tabbable and
> responsive to "Enter"). I don't think onclick on xul:labels handles that.
> Followup bug?

I don't think that this should be keyboard accessible. There will be a lot of output in the WebConsole (also and I don't think that someone wants to tab through all of that.

Comment 96

[Julian Viereck]

(In reply to comment #95)
> (In reply to comment #94)
> > Comment on attachment 465802 [details] [diff] [review] [details]
> > Proposed patch, version 9.
> > 
> > >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> > 
> > >+  writeOutputJS: function JST_writeOutputJS(aEvalString, aOutputObject)
> > 
> > >+    node.onclick = function() {
> > >+      self.openPropertyPanel(aEvalString, aOutputObject, node);
> > >+    }
> > 
> > This should probably actually be keyboard accessible (i.e. tabbable and
> > responsive to "Enter"). I don't think onclick on xul:labels handles that.
> > Followup bug?
> 
> I don't think that this should be keyboard accessible. There will be a lot of
> output in the WebConsole (also and I don't think that someone wants to tab
> through all of that.

Rob pointed out that there are may elements on a page as well and you can tab through most of them. Opened bug 588010.

Comment 97

[Julian Viereck]

Attachment: patch 9.1

Improved patch based Patrick's latest work + Gavin's comments.

Note: I've opened a follow up bug 588014 that is about making the JS output look "clickable".

Comment 98

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/b211364cfa25

Comment 99

[Axel Hecht [:Pike]]

Can you land a follow up to make the localization notes follow the markup on  https://developer.mozilla.org/en/Localization_notes, i.e.

# LOCALIZATION NOTE (jsPropertyTitle,jsPropertyInspectTitle):...

Most l10n tools don't offer a source view, but there are l10n tools that actually parse that format, and understand when to offer the comment.

Comment 100

[Julian Viereck]

(In reply to comment #99)
> Can you land a follow up to make the localization notes follow the markup on 
> https://developer.mozilla.org/en/Localization_notes, i.e.
> 
> # LOCALIZATION NOTE (jsPropertyTitle,jsPropertyInspectTitle):...
> 
> Most l10n tools don't offer a source view, but there are l10n tools that
> actually parse that format, and understand when to offer the comment.

Thanks a lot for that hint! I've just opened bug 588345 as a followup and will upload a patch soon.