193942 - Fix selection in JSObject viewer (left panel) to update right panel

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Alex Vincent [:WeirdAl]]

Two of the major problems I have with the JSObject panel are that it doesn't
show numbered properties of lists (example:  childNodes, stylesheets,
attributes), and that you can't reveal named properties which are not enumerable
(document.defaultView.document, for example).

I'm going to attach three files which in and of themselves make for some very
nice functionality as a standalone XUL applet, and should probably be included
in any rework of DOM Inspector's JSObject panel.

This bug is also intended as a tracking bug for any other issues regarding the
current implementation of the JSObject panel which you'd like fixed. 
Regressions should not be tolerated.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: Object Inspector (part 1 of 3, CSS file)

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: Object Inspector (part 2 of 3, JS file)

Comment 3

[Boris Zbarsky [:bzbarsky]]

hey could we make this panel sort the entries too instead of just listing them
in enumeration order?  Finding things there is HARD.  ;)

Comment 4

[Alex Vincent [:WeirdAl]]

Attachment: Object Inspector (part 3 of 3, XUL file)

Comment 5

[Alex Vincent [:WeirdAl]]

bz:  beat you to it...  see the XUL file.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sweet.  Where do I sign up?  ;)

Comment 7

[Alex Vincent [:WeirdAl]]

Well, with a couple other bugs waiting for feedback from caillon, I've turned my
attention back to this bug.  I now have resorting on-the-fly working now, with
one minor glitch, and I have a few things I want to do here:

* Executing a method of a function (with a possible new operator, if I can make
that work) and have the arguments and returned value appended to the function as
an array
* Filtering tree items by a regular expression
* Giving properties abbreviated variable names which JS can then use (assigning
a returned value the reference document.foo, for example).

I've re-examined the current JSObject panel, and I believe it is not truly
adaptable to the new configuration -- looking at the current panel's source
code, there's a lot of empty JS functions, the context menu event handler is
broken, and the two designs are pretty much incompatible.  In short, this bug
may morph into a complete replacement for the JSObject panel.  It'd just be
easier.  Hence the 1.5alpha milestone.

One thing I wish I could do is to make each column horizontally scrollable; if
someone can show me how to do that, I'd be really happy.

Comment 8

[Alex Vincent [:WeirdAl]]

:) I love being wrong.  The current design is adaptable to the new design.  I
just have to be careful with the scripting.

On another note, chrome://inspector/content/viewers/jsObject/jsObjectView.js is
not used anywhere in the suite; LXR shows it is referenced by filename only in
one place, the jar.mn file.  In other words, it's 5KB of bloat.

If someone could draw up a patch to remove the file from chrome, I'd appreciate
it.  dmose in #mozilla says a cvs remove would be appropriate for losing the
file from the repository as well, but I'd better get caillon's moa= for that.

Comment 9

[timeless]

Attachment: remove file from manifest

Comment 10

[timeless]

Comment on attachment 117149 [details] [diff] [review]
remove file from manifest

this is weirdal's patch, and i agree that the file isn't used

Comment 11

[Alex Vincent [:WeirdAl]]

Thanks for the checkin and r=, timeless.  Thanks to bryner for sr=.

Comment 12

[Alex Vincent [:WeirdAl]]

Attachment: Object Inspector demo v2 (.tar.zip)

Read 'em and hack.

This demo you need to insert into a chrome directory and run from the
appropriate chrome:// url.  I've implemented all the features I mention in
comment 7, and added a feature for defining a new property on-the-fly.

You need it in a chrome:// url because of the three dialog boxes available
(window.openDialog).

I'm aware that the addPropertyDialog closes when you hit Enter or Return in a
XUL multiline textbox.	I'm about to file a bug for that.

Looking for extensive feedback, as 90% of this applet's current features will
be going into the new JSObject panel.

Comment 13

[Alex Vincent [:WeirdAl]]

Filed bug 197568 for the multiline textbox in dialog box issue.

Comment 14

[Alex Vincent [:WeirdAl]]

Attachment: patch, hand-edited

This should work...

Comment 15

[Alex Vincent [:WeirdAl]]

Comment on attachment 118168 [details] [diff] [review]
patch, hand-edited

Stupid me, I forgot to fix the openDialog URLs...

Comment 16

[Alex Vincent [:WeirdAl]]

Attachment: patch, hand-edited (take 2)

Comment 17

[Alex Vincent [:WeirdAl]]

Attachment: jsobject.xul patch

Hand-edited patch 2 is known to be broken for jsObject.xul file.  Here's the
correct patch for that file.  The rest of the patch is good.

Comment 18

[Alex Vincent [:WeirdAl]]

Comment on attachment 118169 [details] [diff] [review]
patch, hand-edited (take 2)

requesting r=, sr= as I forgot to do that earlier.  Note this patch does have
the corrupted jsObject.xul diff; the correct diff for that file is in
attachment 118173 [details] [diff] [review].

Reviewers, this is not a patch I'm going to be seeking immediate response on;
I'm looking for 1.5 alpha milestone.  (But I'm betting that if bz checks it in
to his local tree, he'll love it...)

Comment 19

[Alex Vincent [:WeirdAl]]

Comment on attachment 118169 [details] [diff] [review]
patch, hand-edited (take 2)

As nice as this is, there's still a couple wee things to tweak.

(1) I'm really getting sick of telling it to reveal a certain property by name
only to realize I'm not on the right object or value.  I'm going to make that a
dialog.

(2) When the JS Object panel is the left-panel viewer, it should update the
selection.  (This is in conjunction with work I'm doing for bug 201129.)

Comment 20

[Alex Vincent [:WeirdAl]]

Comment on attachment 117149 [details] [diff] [review]
remove file from manifest

Seeking drivers' approval to remove the file, which is unused and adds 5KB of
bloat to the chrome jar.

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 117149 [details] [diff] [review]
remove file from manifest

a=asa (on behalf of drivers) for checkin to 1.4b

Comment 22

[Alex Vincent [:WeirdAl]]

Comment on attachment 117149 [details] [diff] [review]
remove file from manifest

Oops.  Patch was checked in back in mid-March.	Blame Bugzilla for sending me a
late bugspam.  Sorry.

Comment 23

[Alex Vincent [:WeirdAl]]

Re comment 19:

Selection in the left-hand panel now works with the patch I have.  That's the
good news.

Bad news: Concurrent with the patch for bug 156072, I have a new problem. 
Selecting document.defaultView in the left hand panel sets the selection for the
right-hand panel to a DOMWindow interface.  With the changes to
viewer-registry.rdf for bug 156072, that forces the right-hand panel to treat
its subject as the DOMWindow's document property.

Advice, anyone?

Comment 24

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2.2

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 122530 [details] [diff] [review]
patch, v2.2

So should I bother with trying to review this?	Or are you going to attach
another patch and cancel this review request?  (P.S. each time you request sr
you send a spam to [e-mail].  You may want to hold off on
requesting those until your patch has an r, especially if you're going to play
musical patches)

Comment 26

[Alex Vincent [:WeirdAl]]

Chris:  this patch is final, final, final.

Believe me, I know your frustrations with me on that score; when I have to make
major changes to a patch (in this case adding two new files), I don't have a lot
of options.

I'll bear in mind what you say about sr; I'll stop requesting those until r= is
given.

Comment 27

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 122530 [details] [diff] [review]
patch, v2.2

Localize this patch please.  Also a couple comments follow:


>+  sortByAlpha: function(a, b) {
>+    if (a.objectName < b.objectName) return -1;
>+    if (a.objectName > b.objectName) return 1;

Eww yucky syntax.  Please use:

  if (foo) {
    return bar;
  }

>+    return 0;
>+  },
>+
>+  ////////////////////////////////////////////////////////////////////////////
>+  //// tree utility functions
>+
>+  getTreeItem: function(aObject, objectName) {
...
>+
>+    var bool = false;

I really hate for you to call a variable 'bool'.  It sounds too much like a
reserved keyword.  How about 'truth'?  Or better 'hasProperties' or something
since that's what this really denotes.

>+    switch (typeof aObject) {
>+      case "string":
>+      case "undefined":
>+        break;
>+
>+      case "object":
>+        if (aObject === null) {
>+          break;
>+        }

Always include a comment if you fall through intentionally.  |// fall through|
will suffice.  Feel free to explain it if you like.

>+
>+      default:
>+        for (var property in aObject) {
>+          bool = true;
>+          break;
>+        }
>+
>+        if ((!bool)&&(typeof aObject[0] != "undefined")) {
>+          bool = true;
>         }

This seems over-complicated.  Can't you combine the previous two checks
somehow?  Note that |bool| will always be false by the time you get to the if
statement....

>+    }
>+
>+    if (bool) {
>+      treeItem.setAttribute("container", "true");
>+      treeItem.appendChild(document.createElement("treechildren"));
>+    }
>+
>+    treeItem.inspectedObject = aObject;
>+    treeItem.objectName = objectName;
>+    treeItem.listedProps = [];
>+
>+    return treeItem;
>+  },
>+
>+  checkParentObjects: function(newItem) {
>+    var matchCheck = newItem.firstChild.firstChild, matchArray = [], matchTests = 0;

Please declare each variable on it's own line.

>+    while ((typeof newItem.inspectedObject == "object")&&(matchCheck.parentNode.parentNode != this.objectTree)) {
>+      matchCheck = matchCheck.parentNode.parentNode;
>+      matchTests++;

++foo not foo++.

>+      matchArray[matchArray.length] = matchCheck.firstChild.firstChild.getAttribute("label");
>+
>+      if (matchCheck.inspectedObject == newItem.inspectedObject) {
>+        // we have a match, cut off everything before.
>+        matchArray.splice(0, matchArray.length - 1);
>       }
>-    } catch (ex) {
>-      dump("Error in expression.\n");
>-      throw (ex);
>     }
>-  },

I haven't really looked past this.  I'll look later on.  :-)

Comment 28

[Alex Vincent [:WeirdAl]]

caillon and I were having a debate about revealing numbered but unenumerated
properties of NodeList, StyleSheetList, etc. objects.

>+        if ((!bool)&&(typeof aObject[0] != "undefined")) {
>+          bool = true;
>         }

My objection is I can't rely on the length property of aObject, because aObject
may be like this:

lineSegment = {length: 3}

caillon suggested sniffing with something like this:

Object.prototype.__lookupGetter__('length').call(lineSegment);

Personally, I've never seen the __lookupGetter__ bit, so I would assume it
returns a Function-type value.

rginda:  can you advise on what is the best way to determine if an object has
unenumerated whole-number properties?

Comment 29

[Alex Vincent [:WeirdAl]]

http://weblogs.mozillazine.org/weirdal/archives/bug193942_FULL.diff

Localized, but I still haven't figured out what to do about unenumerated
numerable properties.  caillon, could you take a look at it, get back to me with
other review comments?

Re comment 28, 
var lineSegment = {
  get length() {
    return 3;
  }
};

Comment 30

[Alex Vincent [:WeirdAl]]

Attachment: patch, v3

Localized patch.  Bug 206353 will enumerate the numbered properties of DOM
lists, so I took that part of the code out.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Issues:

1)  Dialog that comes up for "execute function" is all cut off on the right
2)  In the tree view, the newlines in the serialization of the functions look like
    little boxes; probably want to replace them with spaces or something.

Comment 32

[Alex Vincent [:WeirdAl]]

Comment on attachment 127854 [details] [diff] [review]
patch, v3

Testing shows a couple of functionality bugs in this patch anyway.

(1) I somehow broke adding properties dialog, probably because of my
integrating it with the showPropertyDialog.js file for code reuse.

(2) Certain properties of nodes, when you select them in JSObject (left panel),
do not update the right-side panel's subject.  This most often happens with the
all-caps constants (like Node.ELEMENT_NODE).  Unknown why, but I suspect
inspector.xml and viewer-registry.js.

*sigh*

Comment 33

[Alex Vincent [:WeirdAl]]

I have painstakingly confirmed that the bustage under comment 32 point 2 is not
my fault.  It appears to be a JavaScript Engine failure.  Note bug 219221 for
details.

Thanks to timeless for suggesting Venkman; I'd've never figured that out without it.

Comment 34

[Alex Vincent [:WeirdAl]]

Attachment: maintenance patch (fixing a couple small bugs, no new features)

Comment 35

[Alex Vincent [:WeirdAl]]

Attachment: maintenance patch, without nullviewer.xul

I did some tinkering, and found out I didn't need nullviewer.xul anyway.  This
patch also now handles DOM Inspector going from one document to another.

Comment 36

[Alex Vincent [:WeirdAl]]

Comment on attachment 134872 [details] [diff] [review]
maintenance patch, without nullviewer.xul

All this patch does is enable selection in the left-hand panel to become the
subject in the right-hand panel.  This is in accordance with the other two
left-hand viewers (DOM Nodes, Stylesheets).

Comment 37

[Alex Vincent [:WeirdAl]]

Comment on attachment 134872 [details] [diff] [review]
maintenance patch, without nullviewer.xul

Sorry, I forgot one of the other performance fixes I came up with.  New patch
in a few seconds.

Comment 38

[Alex Vincent [:WeirdAl]]

Attachment: maintenance patch

Allows JSObject viewer in left-hand panel to set selection, and stops
right-hand panel from searching for viewers again if its subject doesn't
change.  (This matters because the left hand viewer may change but its
selection may remain the inspected document.)

Comment 39

[Alex Vincent [:WeirdAl]]

Comment on attachment 134873 [details] [diff] [review]
maintenance patch

Also fixes a small typo in inspector.xml.

Comment 40

[Alex Vincent [:WeirdAl]]

Comment on attachment 134873 [details] [diff] [review]
maintenance patch

>+      catch (e) {

      catch (e if (e.result == Components.results.NS_ERROR_ILLEGAL_VALUE)) {

per irc w/ caillon.

Comment 41

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 134873 [details] [diff] [review]
maintenance patch

>@@ -174,16 +175,29 @@ JSObjectViewer.prototype = 
>   
>   cmdInspectInNewView: function()
>   {
>     var sel = getSelectedItem();
>     if (sel)
>       inspectObject(sel.__JSValue__);
>   },
>   
>+  onItemSelected: function() {

Follow prevailing bracing style!


>+    if (this.pane.id == "bxDocPanel") {
>+      try {
>+        var selectedItem = this.mTree.view.getItemAtIndex(this.mTree.currentIndex);
>+        this.mSelection = selectedItem.__JSValue__;
>+      }
>+      catch (e) {


Please catch just the exception which you expect , and let others propogate
through.



>+        // This happens when we're loading a new document.
>+        this.mSelection = this.mSubject;
>+      }
>+      this.mObsMan.dispatchEvent("selectionChange", { selection: this.mSelection } );
>+    }
>+  },
>   ////////////////////////////////////////////////////////////////////////////
>   //// tree construction
> 
>   emptyTree: function(aTreeKids)
>   {
>     var kids = aTreeKids.childNodes;
>     for (var i = 0; i < kids.length; ++i) {
>       aTreeKids.removeChild(kids[i]);
>Index: extensions/inspector/resources/content/viewers/jsObject/jsObject.xul
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/inspector/resources/content/viewers/jsObject/jsObject.xul,v
>retrieving revision 1.11
>diff -p -u -8 -d -r1.11 jsObject.xul
>--- extensions/inspector/resources/content/viewers/jsObject/jsObject.xul	18 May 2003 12:15:13 -0000	1.11
>+++ extensions/inspector/resources/content/viewers/jsObject/jsObject.xul	6 Nov 2003 02:40:28 -0000
>@@ -27,17 +27,17 @@
>     <popup id="popupContext" onpopupshowing="return viewer.onpopupshowingContext(this)">
>       <menuitem label="&inspectNewWindow.label;" observes="cmdInspectInNewView"/>
>       <menuseparator/>
>       <menuitem label="&jsCopyValue.label;" observes="cmdCopyValue"/>
>       <menuitem label="&jsEval.label;" observes="cmdEvalExpr"/>
>     </popup>
>   </popupset>
>   
>-  <tree id="treeJSObject" flex="1" context="popupContext">
>+  <tree id="treeJSObject" flex="1" context="popupContext" onselect="viewer.onItemSelected()">
>     <treecols>
>       <treecol id="colProp" flex="1" primary="true" label="&jsProperty.label;"/>
>       <splitter class="tree-splitter"/>
>       <treecol id="colVal" flex="1" label="&jsValue.label;"/>
>     </treecols>
> 
>     <treechildren id="trchJSObject"/>
>   </tree>
>Index: extensions/inspector/resources/content/inspector.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/inspector/resources/content/inspector.xml,v
>retrieving revision 1.15
>diff -p -u -8 -d -r1.15 inspector.xml
>--- extensions/inspector/resources/content/inspector.xml	22 Jun 2003 07:13:27 -0000	1.15
>+++ extensions/inspector/resources/content/inspector.xml	6 Nov 2003 02:40:29 -0000
>@@ -558,16 +558,19 @@
>       // them, and load the default viewer for the node.
>       //
>       // @param Object aObject - the object to begin viewing
>       //////////////////////////////////////////////////////////////////////// -->
> 
>       <method name="setSubject">
>         <parameter name="aObject"/>
>         <body><![CDATA[
>+          if (this.mSubject == aObject)
>+            return; // we don't want to redraw if the left-hand panel changes and the right-hand panel's subject does not

Don't put comments on the same line as code.  Also use proper sentence case,
and change the last "and" to a "but".

That said, I don't really see how this comment pertains to this code, I kind of
do, but I think your comment confuses the casual reader more than it helps him.
 Something which says (more or less) that the subject hasn't changed so we can
bail early would be a better comment here, but I question its usefulness since
that seems quite obvious.

>+        
>           this.mSubject = aObject;
>           this.mParams = null;
>           
>           // get the list of viewers which match the node
>           var entries = this.registry.findViewersForObject(aObject, this.id);
>           this.rebuildViewerList(entries);
> 
>           if (entries.length == 0) {
>@@ -698,17 +701,17 @@
>            this.mContextMenu = pp;
>           } else {
>             this.mMenuEl.setAttribute("disabled", "true");
>           }
>         ]]></body>
>       </method>
> 
>   <!-- ////////////////////////////////////////////////////////////////////////////
>-      // Check to see if an entry exists in an arry of entries
>+      // Check to see if an entry exists in an array of entries
>       //
>       // @param nsIRDFResource aEntry - the entry being searched for
>       // @param Array aList - array of entries
>       //////////////////////////////////////////////////////////////////////// -->
> 
>       <method name="entryInList">
>         <parameter name="aEntry"/>
>         <parameter name="aList"/>

Comment 42

[Alex Vincent [:WeirdAl]]

Attachment: maintenance patch, answering caillon's comments

Comment 43

[Alex Vincent [:WeirdAl]]

This bug is getting out of hand, so on caillon's advice, I'm going to file a new
bug for the replacement of the JavaScript Object viewer.  We basically want to
start with a fresh slate, instead of a million patches.  Blame me for poor bug
management.

Comment 44

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 134881 [details] [diff] [review]
maintenance patch, answering caillon's comments

>@@ -174,16 +175,31 @@ JSObjectViewer.prototype = 
>   
>   cmdInspectInNewView: function()
>   {
>     var sel = getSelectedItem();
>     if (sel)
>       inspectObject(sel.__JSValue__);
>   },
>   
>+  onItemSelected: function()
>+  {
>+    if (this.pane.id == "bxDocPanel") {
>+      try {
>+        var selectedItem = this.mTree.view.getItemAtIndex(this.mTree.currentIndex);
>+        this.mSelection = selectedItem.__JSValue__;
>+      }
>+      catch (ex if ex.result == Components.results.NS_ERROR_ILLEGAL_VALUE) {
>+        // This happens when we're loading a new document.
>+        this.mSelection = this.mSubject;


Actually, on looking at this again, why do we need to catch an illegal value? 
Is that thrown in getItemAtIndex()?  I suspect so...  Is this something we can
trap before we call it?



>+      }
>+
>+      this.mObsMan.dispatchEvent("selectionChange", { selection: this.mSelection } );
>+    }
>+  },
>   ////////////////////////////////////////////////////////////////////////////
>   //// tree construction


There was a new line before the above comment, and I think you should keep one
there.

Comment 45

[Alex Vincent [:WeirdAl]]

(In reply to comment #43)
> This bug is getting out of hand, so on caillon's advice, I'm going to file a new
> bug for the replacement of the JavaScript Object viewer.

Bug 272906.

Comment 46

[Alex Vincent [:WeirdAl]]

Reassigning DOM-I bugs which have stagnated in my buglist back to default owner.  Hopefully someone will pick up some of these bugs and work on them.  I'll continue to follow them.

Comment 47

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

Attachment: Starting dispatching selection

Comment 49

[[e-mail]]

Comment on attachment 539334 [details] [diff] [review]
Starting dispatching selection

>+        InsUtil.getNearestIndex(currentIndex, view.getSelectedIndexes());
JavaScript error: chrome://inspector/content/viewers/jsObject/jsObjectViewer.js,  line 246: view.getSelectedIndexes is not a function

Rest works fine.

Comment 50

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

Attachment: getSelectedIndices

(In reply to comment #49)
> Comment on attachment 539334 [details] [diff] [review] [review]
> Starting dispatching selection
> 
> >+        InsUtil.getNearestIndex(currentIndex, view.getSelectedIndexes());
> JavaScript error:
> chrome://inspector/content/viewers/jsObject/jsObjectViewer.js,  line 246:
> view.getSelectedIndexes is not a function
> 
> Rest works fine.

Oops.  I guess I didn't spin up a new diff.

Comment 51

[[e-mail]]

Comment on attachment 539505 [details] [diff] [review]
getSelectedIndices

Just happened to notice an unrelated "bug" in the viewer - try selecting a string in the left-hand viewer ;-)

Comment 52

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

(In reply to comment #51)
> Comment on attachment 539505 [details] [diff] [review] [review]
> getSelectedIndices
> 
> Just happened to notice an unrelated "bug" in the viewer - try selecting a
> string in the left-hand viewer ;-)

Bug 664184

Pushed: http://hg.mozilla.org/dom-inspector/rev/5f5593349f9e