Closed Bug 193942 Opened 17 years ago Closed 8 years ago

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

Categories

(Other Applications :: DOM Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: WeirdAl, Assigned: crussell)

References

()

Details

Attachments

(1 file, 15 obsolete files)

9.60 KB, patch
neil
: review+
Details | Diff | Splinter Review
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.
hey could we make this panel sort the entries too instead of just listing them
in enumeration order?  Finding things there is HARD.  ;)
bz:  beat you to it...  see the XUL file.
Sweet.  Where do I sign up?  ;)
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.
Target Milestone: --- → mozilla1.5alpha
:) 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.
Attached patch remove file from manifest (obsolete) — Splinter Review
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
Attachment #117149 - Flags: review+
Attachment #117149 - Flags: superreview?(roc+moz)
Attachment #117149 - Flags: superreview?(roc+moz) → superreview+
Thanks for the checkin and r=, timeless.  Thanks to bryner for sr=.
Attached file Object Inspector demo v2 (.tar.zip) (obsolete) —
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.
Attachment #114827 - Attachment is obsolete: true
Attachment #114828 - Attachment is obsolete: true
Attachment #114829 - Attachment is obsolete: true
Filed bug 197568 for the multiline textbox in dialog box issue.
Attached patch patch, hand-edited (obsolete) — Splinter Review
This should work...
Comment on attachment 118168 [details] [diff] [review]
patch, hand-edited

Stupid me, I forgot to fix the openDialog URLs...
Attachment #118168 - Attachment is obsolete: true
Attached patch patch, hand-edited (take 2) (obsolete) — Splinter Review
Attached patch jsobject.xul patch (obsolete) — Splinter Review
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 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...)
Attachment #118169 - Flags: superreview?(bzbarsky)
Attachment #118169 - Flags: review?(bugmail)
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.)
Attachment #118169 - Attachment is obsolete: true
Attachment #118169 - Flags: superreview?(bzbarsky)
Attachment #118169 - Flags: review?(bugmail)
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.
Attachment #117149 - Flags: approval1.4b?
Comment on attachment 117149 [details] [diff] [review]
remove file from manifest

a=asa (on behalf of drivers) for checkin to 1.4b
Attachment #117149 - Flags: approval1.4b? → approval1.4b+
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.
Attachment #117149 - Attachment is obsolete: true
Attachment #117149 - Flags: approval1.4b+
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?
Depends on: 156072
No longer depends on: 156072
Attached patch patch, v2.2 (obsolete) — Splinter Review
Attachment #118173 - Attachment is obsolete: true
Attachment #122530 - Flags: superreview?(bz-bugspam)
Attachment #122530 - Flags: review?(caillon)
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 reviewers@mozilla.org.  You may want to hold off on
requesting those until your patch has an r, especially if you're going to play
musical patches)
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 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.  :-)
Attachment #122530 - Flags: superreview?(bz-bugspam)
Attachment #122530 - Flags: review?(caillon)
Attachment #122530 - Flags: review-
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?
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;
  }
};
Target Milestone: mozilla1.5alpha → mozilla1.5beta
Attached patch patch, v3 (obsolete) — Splinter Review
Localized patch.  Bug 206353 will enumerate the numbered properties of DOM
lists, so I took that part of the code out.
Attachment #122530 - Attachment is obsolete: true
Attachment #127854 - Flags: superreview?(bzbarsky)
Attachment #127854 - Flags: review?(caillon)
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 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*
Attachment #127854 - Attachment is obsolete: true
Attachment #127854 - Flags: superreview?(bzbarsky)
Attachment #127854 - Flags: review?(caillon)
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.
Depends on: 219221
OS: Windows 98 → All
Hardware: PC → All
Target Milestone: mozilla1.5beta → mozilla1.6alpha
No longer depends on: 219221
Depends on: 219221
No longer depends on: 219221
Attachment #133555 - Flags: review?(caillon)
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.
Attachment #133555 - Attachment is obsolete: true
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).
Attachment #134872 - Flags: review?(caillon)
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.
Attachment #134872 - Attachment is obsolete: true
Attachment #134872 - Flags: review?(caillon)
Attached patch maintenance patch (obsolete) — Splinter Review
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 on attachment 134873 [details] [diff] [review]
maintenance patch

Also fixes a small typo in inspector.xml.
Attachment #134873 - Flags: review?(caillon)
Attachment #133555 - Flags: review?(caillon)
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 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"/>
Attachment #134873 - Flags: review?(caillon) → review-
Attachment #134873 - Attachment is obsolete: true
Attachment #134881 - Flags: review?(caillon)
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.

Summary: JavaScript object panel needs more features → Fix selection in JSObject viewer (left panel) to update right panel
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.
Attachment #134881 - Flags: review?(caillon)
Product: Core → Other Applications
(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.
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.
Assignee: ajvincent → dom-inspector
Target Milestone: mozilla1.6alpha → ---
Assignee: dom-inspector → nobody
QA Contact: timeless → dom-inspector
Assignee: nobody → Sevenspade
Attached patch Starting dispatching selection (obsolete) — Splinter Review
Attachment #117327 - Attachment is obsolete: true
Attachment #134881 - Attachment is obsolete: true
Attachment #539334 - Flags: review?(neil)
Duplicate of this bug: 314446
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.
(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.
Attachment #539334 - Attachment is obsolete: true
Attachment #539505 - Flags: review?(neil)
Attachment #539334 - Flags: review?(neil)
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 ;-)
Attachment #539505 - Flags: review?(neil) → review+
(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
Blocks: DOMi2.0.10
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.