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.

:) 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 on attachment 117149 [details] [diff] [review] remove file from manifest this is weirdal's patch, and i agree that the file isn't used

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

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.

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

This should work...

Comment on attachment 118168 [details] [diff] [review] patch, hand-edited Stupid me, I forgot to fix the openDialog URLs...

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...)

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 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 on attachment 117149 [details] [diff] [review] remove file from manifest a=asa (on behalf of drivers) for checkin to 1.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.

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 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. :-)

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; } };

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

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*

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.

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 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 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.

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.

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"/>

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 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.

(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.

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.

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 ;-)

(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