Status

defect
P1
blocker
RESOLVED FIXED
9 years ago
Last year

People

(Reporter: ddahl, Assigned: julian.viereck)

Tracking

Trunk
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

(Whiteboard: [kd4b5])

Attachments

(1 attachment, 30 obsolete attachments)

28.07 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
Reporter

Description

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

Updated

9 years ago
Blocks: 568649
Reporter

Updated

9 years ago
Assignee: nobody → jviereck
Reporter

Updated

9 years ago
Summary: Object introspector → JS Object Introspector
Reporter

Updated

9 years ago
Summary: JS Object Introspector → JS/DOM Object Introspector
Reporter

Updated

9 years ago
Blocks: 573103
Reporter

Comment 1

9 years ago
Of course now that I think about this a bit more, perhaps there is code in the "Inspector" for this?
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"?)
Reporter

Comment 3

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

Updated

9 years ago
Priority: -- → P1
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.
Depends on: 561782
(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.
Assignee

Comment 6

9 years ago
Posted patch state of work 1 (obsolete) — Splinter Review
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.
Assignee

Comment 7

9 years ago
(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.
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.
Assignee

Comment 9

9 years ago
Attachment #454496 - Attachment is obsolete: true
Assignee

Comment 12

9 years ago
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
Status: NEW → ASSIGNED
(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!
Assignee

Comment 15

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

Comment 16

9 years ago
> > 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?
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!
updated part3 after import.
Attachment #454847 - Attachment is obsolete: true
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 <jviereck@mozilla.com>
date:        Wed Jun 30 13:49:16 2010 -0300
summary:     Implements a reuseable PropertyPanel.

changeset:   44190:4ea5c1e6320b
user:        Rob Campbell <rcampbell@mozilla.com>
date:        Wed Jun 30 13:49:37 2010 -0300
summary:     # User Julian Viereck <jviereck@mozilla.com>

changeset:   44191:5a554b5d2907
user:        Rob Campbell <rcampbell@mozilla.com>
date:        Wed Jun 30 14:28:03 2010 -0300
summary:     added missing semicolon in clearDOMPanel

changeset:   44192:8c5c71636ee5
user:        Rob Campbell <rcampbell@mozilla.com>
date:        Wed Jun 30 14:35:42 2010 -0300
summary:     # User Julian Viereck <jviereck@mozilla.com>

changeset:   44193:d9c9bb24bf03
tag:         tip
user:        Rob Campbell <rcampbell@mozilla.com>
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.
Assignee

Comment 20

9 years ago
(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.
sounds good.
Reporter

Comment 22

9 years ago
rob:

I have been doing hg qref -u "Julian Viereck <jviereck@mozilla.com>" 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?
Reporter

Comment 23

9 years ago
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.
Assignee

Comment 24

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

Comment 25

9 years ago
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]
Reporter

Comment 26

9 years ago
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>"]
Reporter

Comment 27

9 years ago
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!
(In reply to comment #22)
> rob:
> 
> I have been doing hg qref -u "Julian Viereck <jviereck@mozilla.com>" 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
Reporter

Comment 29

9 years ago
(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
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.
Assignee

Comment 31

9 years ago
exposes 'jsterm' on the HUD object. This patch is necessary to get the jsterm
object when running unit tests (e.g. for tab completion).
Attachment #454845 - Attachment is obsolete: true
Attachment #454846 - Attachment is obsolete: true
Attachment #454956 - Attachment is obsolete: true
Attachment #454959 - Attachment is obsolete: true
Attachment #455176 - Attachment is obsolete: true
Attachment #455415 - Attachment is obsolete: true
Attachment #457401 - Flags: review?(dietrich)
Assignee

Comment 32

9 years ago
Attachment #457403 - Flags: review?(dietrich)
(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.
Depends on: 579073
Assignee

Comment 34

9 years ago
Posted patch Patch 2 (obsolete) — Splinter Review
Quite the same patch as in 457403 but now with JavaDocs all over the place.

Note: depends on 579073
Attachment #457401 - Attachment is obsolete: true
Attachment #457403 - Attachment is obsolete: true
Attachment #457582 - Flags: review?(dietrich)
Attachment #457401 - Flags: review?(dietrich)
Attachment #457403 - Flags: review?(dietrich)
Hardware: x86 → All
Assignee

Updated

9 years ago
Blocks: 579339
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.
Assignee

Comment 36

9 years ago
(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.
Assignee

Comment 37

9 years ago
Posted patch Patch 2 Rebase (obsolete) — Splinter Review
Same as former Patch 2 attachment 457582 [details] [diff] [review] but rebased to apply clean on top of trunk. Also, fix some comment indention.
Attachment #457582 - Attachment is obsolete: true
Attachment #458295 - Flags: review?(dietrich)
Attachment #457582 - Flags: review?(dietrich)
Assignee

Comment 38

9 years ago
Posted patch Patch 2 Rebase 2 (obsolete) — Splinter Review
Rebase with trunk.
Attachment #458295 - Attachment is obsolete: true
Attachment #458356 - Flags: review?(dietrich)
Attachment #458295 - Flags: review?(dietrich)
Assignee

Updated

9 years ago
No longer depends on: 561782
Reporter

Updated

9 years ago
Severity: normal → blocker
blocking2.0: --- → ?
Assignee

Comment 39

9 years ago
Posted patch Patch 2 Rebase 3 (obsolete) — Splinter Review
Attachment #458356 - Attachment is obsolete: true
Attachment #459336 - Flags: review?(dtownsend)
Attachment #458356 - Flags: review?(dietrich)
Assignee

Comment 40

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

Comment 41

9 years ago
The important feature is object inspection and is the engine behind a lot of the discover-ability of our new DevTools.
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?
Assignee

Comment 43

9 years ago
Posted patch Part 1 (obsolete) — Splinter Review
Merged patch part 1: Adding the PropertyPanel.jsm
Attachment #459336 - Attachment is obsolete: true
Attachment #460260 - Flags: review?(robert.bugzilla)
Attachment #459336 - Flags: review?(dtownsend)
Assignee

Comment 44

9 years ago
Posted patch Part 2 (obsolete) — Splinter Review
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.
Attachment #460262 - Flags: review?(robert.bugzilla)
Assignee

Comment 45

9 years ago
(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.
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.
That'd be great. Thanks for the update, Rob!

Updated

9 years ago
Whiteboard: [kd4b4]
Assignee

Updated

9 years ago
Duplicate of this bug: 579339
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?).
Attachment #460260 - Flags: review?(robert.bugzilla) → review-
Assignee

Comment 50

9 years ago
Posted patch Part 1, take 2 (obsolete) — Splinter Review
(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 :)
Attachment #460260 - Attachment is obsolete: true
Attachment #462516 - Flags: review?(gavin.sharp)
Assignee

Comment 51

9 years ago
Posted patch Part2, take 2 (obsolete) — Splinter Review
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.
Attachment #460262 - Attachment is obsolete: true
Attachment #462519 - Flags: review?(gavin.sharp)
Attachment #460262 - Flags: review?(robert.bugzilla)
Assignee

Comment 52

9 years ago
Posted patch Part 1, take 2.1 (obsolete) — Splinter Review
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).
Attachment #462516 - Attachment is obsolete: true
Attachment #462516 - Flags: review?(gavin.sharp)
(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 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 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 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?
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.
Attachment #462519 - Flags: review?(gavin.sharp)
Also forgot to mention that part 2 needs l10n work, but I think you're aware of that.
(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.
> Humph, what did you end up doing in dxr? You had a similar situation I think.

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

Comment 61

9 years ago
Posted patch Combined patch, take 3 (obsolete) — Splinter Review
Attachment #462519 - Attachment is obsolete: true
Attachment #462855 - Attachment is obsolete: true
Attachment #463279 - Flags: review?(gavin.sharp)
Assignee

Comment 62

9 years ago
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...
(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.
(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.
Assignee

Comment 65

9 years ago
Posted patch Combined patch, take 4 (obsolete) — Splinter Review
Updated the patch based on Gavin's latest feedback.
Attachment #463279 - Attachment is obsolete: true
Attachment #463527 - Flags: review?(gavin.sharp)
Attachment #463279 - Flags: review?(gavin.sharp)
Assignee

Comment 66

9 years ago
Posted patch Combined patch, take 4.1 (obsolete) — Splinter Review
Same as patch 463527 but without trailing whitespaces.
Attachment #463527 - Attachment is obsolete: true
Attachment #463527 - Flags: review?(gavin.sharp)
Assignee

Updated

9 years ago
Attachment #463528 - Flags: review?(gavin.sharp)
Comment on attachment 463528 [details] [diff] [review]
Combined patch, take 4.1

>+    case "JavaArray":

Why are you still doing this? See comment 63.
Assignee

Comment 68

9 years ago
Posted patch Combined patch, take 4.2 (obsolete) — Splinter Review
(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.
Attachment #463528 - Attachment is obsolete: true
Attachment #463543 - Flags: review?(gavin.sharp)
Attachment #463528 - Flags: review?(gavin.sharp)

Comment 69

9 years ago
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.
Attachment #463543 - Flags: review?(gavin.sharp) → review?(dietrich)
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!
Attachment #463543 - Flags: review?(dietrich) → review-
(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

9 years ago
>+  // 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'
Assignee

Comment 73

9 years ago
Posted patch Take 5 (obsolete) — Splinter Review
(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!
Attachment #463543 - Attachment is obsolete: true
Attachment #464038 - Flags: review?(gavin.sharp)
Assignee

Comment 74

9 years ago
Posted patch Take 5, rebase 1 (obsolete) — Splinter Review
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.
Attachment #464038 - Attachment is obsolete: true
Attachment #464375 - Flags: review?(gavin.sharp)
Attachment #464038 - Flags: review?(gavin.sharp)
Assignee

Comment 75

9 years ago
Posted patch Take 5, rebase 1 fix 1 (obsolete) — Splinter Review
Missed the call to the unit test function for this bug during rebasing. This patch includes it again.
Attachment #464375 - Attachment is obsolete: true
Attachment #464386 - Flags: review?(gavin.sharp)
Attachment #464375 - Flags: review?(gavin.sharp)

Comment 76

9 years ago
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.
Attachment #464386 - Flags: review?(gavin.sharp) → review?(dolske)
blocking2.0: ? → beta5+
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.
Attachment #464386 - Flags: review?(dolske) → review+
Should also get followups filed for the TODO comments in the patch, and add references to the bug #s.
Assignee

Comment 79

9 years ago
Posted patch Take 6 (obsolete) — Splinter Review
Improved based on Gavin's latest review.
Attachment #464386 - Attachment is obsolete: true
Attachment #464784 - Flags: review?(gavin.sharp)
Comment on attachment 464784 [details] [diff] [review]
Take 6

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

Getting ready for checkin.
Attachment #464784 - Flags: review?(gavin.sharp) → review+
Comment on attachment 464784 [details] [diff] [review]
Take 6

(note that my review was just carrying gavin's forward)
Posted patch [backed-out] Take 7 (obsolete) — Splinter Review
This is ready for checkin...
Attachment #464784 - Attachment is obsolete: true
Attachment #464903 - Flags: review+
Comment on attachment 464903 [details] [diff] [review]
[backed-out] Take 7

http://hg.mozilla.org/mozilla-central/rev/8b3fd544de43
Attachment #464903 - Attachment description: Take 7 → [checked-in] Take 7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Attachment #464903 - Attachment description: [checked-in] Take 7 → [backed-out] Take 7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch [backed-out] Take 8 (obsolete) — Splinter Review
hopefully deleaked and some minor corrections added. Try build is currently running and will post results here.
Attachment #464903 - Attachment is obsolete: true
Attachment #465304 - Flags: review?(dietrich)
I'll review once the try results are in, and confirmed ok.
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 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?
Attachment #465304 - Flags: review?(dietrich) → review+
(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!
Backed out due to incompatibility with the XULification of the console (bug 586514).
Posted patch Proposed patch, version 9. (obsolete) — Splinter Review
Rebased to trunk. Feedback and re-review requested, since this isn't my patch.
Assignee: jviereck → pwalton
Attachment #465304 - Attachment is obsolete: true
Attachment #465802 - Flags: feedback?
Attachment #465802 - Flags: feedback? → feedback?(jviereck)
Comment on attachment 465304 [details] [diff] [review]
[backed-out] Take 8

backed out due to conflict with bug 586514.
Attachment #465304 - Attachment description: Take 8 → [backed-out] Take 8
Assignee

Comment 93

9 years ago
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.
Attachment #465802 - Flags: feedback?(jviereck) → feedback-

Updated

9 years ago
Whiteboard: [kd4b4] → [kd4b5]
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.
Assignee

Updated

9 years ago
No longer blocks: 573103
Assignee

Comment 95

9 years ago
(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.
Assignee

Comment 96

9 years ago
(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.
Assignee

Comment 97

9 years ago
Posted patch patch 9.1Splinter Review
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".
Assignee: pwalton → jviereck
Attachment #465802 - Attachment is obsolete: true
Attachment #466651 - Flags: review?
Assignee

Updated

9 years ago
Attachment #466651 - Flags: review? → review?(gavin.sharp)
Attachment #466651 - Flags: review?(gavin.sharp) → review+
Reporter

Comment 98

9 years ago
http://hg.mozilla.org/mozilla-central/rev/b211364cfa25
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Assignee

Comment 100

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

Updated

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