Create a panel to view the contents of a DOM node and all its properties. The panel should display the contents of the currently highlighted node selected in the tree panel.

initial wip

basic dom panel implementation

diffed the wrong way, correcting.

test file

dom-panel patches, rebased and folded.

fixed up some merge gunk. working properly now.

More cleanup after fixing up the style-panel patch again. TODO: change todo->ok in browser_inspector_initialization.js when these land.

updated version of the patch.

As discussed with Rob, I've implemented a basic XUL tree for inspecting a JS object. 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? - 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)? - 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? Julian

let's talk about this in bug 573102.

updated after changes to style panel in bug 560692.

Comment on attachment 457607 [details] [diff] [review] revised dom panel patch canceling review. I'm going to base this on bug 573102 instead. Same code, different place.

depends on patch in bug 573102.

folded version of DOM/JS property panel patch. Relies on patch in bug 573102.

Need this. blocking betaN+ but let's get it in ASAP so we have time for feedback and iteration.

There's an error in the patch, in the Makefile.in: browser_inspector_stylePanel.js \ + browser_inspector_domPanel.js browser_pageInfo.js \ It should have a \ at the end. Other than this, this patch needs a minimal rebase to apply cleanly (again, due to the Makefile).

Comment on attachment 459850 [details] [diff] [review] DOM/JS object property panel (folded) >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul >+ <toolbarbutton id="inspector-dom-toolbutton" >+ label="DOM" >+ accesskey="D" l10n >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js >- highlightColor: "#EEEE66", >- highlightThickness: 4, >- highlightOpacity: 0.4, You don't seem to be removing the other reference to these (in initializeHighlighter)... >+ toggleDOMPanel: function IUI_toggleDOMPanel() >+ { >+ if (this._showDOMPanel) { Seems to me like this should use isDOMPanelOpen (especially given that _showDOMPanel defaults to true?). I know I've asked this before, but I forget the answer - can we remove the _show*Panel variables? > openTreePanel: function IUI_openTreePanel() >- this.treePanel.openPopup(bar, "overlap", 120, -120, false, false); >+ this.treePanel.openPopupAtScreen(this.win.screenX + 80, >+ this.win.outerHeight + this.win.screenY); Why this change? > if (this._showDOMPanel) { >+ if (!this.PropertyTreeView) { >+ Cu.import("chrome://global/content/PropertyPanel.jsm", this); >+ } Can be a global lazy getter, as in http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#282 . >+ // If there is no domPanel yet, then create a new one. >+ if (!this.domPanel) { Could also add a lazyGetter for this. >+ let propPanel = new (this.PropertyPanel)(parent, document, "DOM", {}); Hrm, not sure whether "DOM" as a string is usefully localizable, but it can't really hurt to treat it as such. > if (this.isStylePanelOpen) { > this.stylePanel.hidePopup(); > } >+ if (this.isStylePanelOpen) { >+ this.stylePanel.hidePopup(); >+ } mismerge? >+ addDOMItem: function IUI_addDOMItem(aPair) >+ item.setAttribute("label", aPair.name + ": " + aPair.value); triggers my l10n-spidey-senses again... Is there an "l10n review" bug for inspector that we can add this potential concern to, perhaps? >diff --git a/browser/base/content/test/browser_inspector_domPanel.js b/browser/base/content/test/browser_inspector_domPanel.js >+function createDocument() >+ setTimeout(setupDOMTests, 0); Shouldn't be needed, right? In fact that method should be inlined here, I think. >+function performTestComparisons(evt) >+ ok(InspectorUI.treeView.selectedNode, "selection"); >+ ok(InspectorUI._showDOMPanel, "_showDOMPanel"); >+ is(InspectorUI.isDOMPanelOpen, InspectorUI._showDOMPanel, "DOM panel matches _showDOMPanel?"); >+ ok(InspectorUI.highlighter.isHighlighting, "panel is highlighting"); >+ ok(InspectorUI.domTreeView.rowCount > 0, "domBox has items"); Better comparisons (actual checks of valid values for the elements being inspected) would be good here (maybe roll into that other bug you have about improving the tests?) >diff --git a/browser/base/content/test/browser_inspector_initialization.js b/browser/base/content/test/browser_inspector_initialization.js > function runInspectorTests(evt) > { >- if (evt.target.id != "inspector-panel") >+ if (evt.target.id != "inspector-dom-panel") Are you changing this because dom-panel is the last panel to be opened? >diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css >diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css >diff --git a/browser/themes/winstripe/browser/browser.css b/browser/themes/winstripe/browser/browser.css > #highlighter-panel { >+ background: -moz-linear-gradient(top -1deg, #ffdd88, #ffeeaa); >+ border: none; >+ opacity: 0.35; Are these changes unrelated?

(In reply to comment #18) > Comment on attachment 459850 [details] [diff] [review] > DOM/JS object property panel (folded) > > >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul > > >+ <toolbarbutton id="inspector-dom-toolbutton" > >+ label="DOM" > >+ accesskey="D" > > l10n whups. missed that. > > >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js > > >- highlightColor: "#EEEE66", > >- highlightThickness: 4, > >- highlightOpacity: 0.4, > > You don't seem to be removing the other reference to these (in > initializeHighlighter)... also whups. Removed. > >+ toggleDOMPanel: function IUI_toggleDOMPanel() > >+ { > >+ if (this._showDOMPanel) { > > Seems to me like this should use isDOMPanelOpen (especially given that > _showDOMPanel defaults to true?). I know I've asked this before, but I forget > the answer - can we remove the _show*Panel variables? I originally wanted them as placeholders for preferences. The idea being if someone turned one of the panels off in one session, it wouldn't open up again in another one. That's why they're hooked up the way they are. I guess I can either get rid of them now and file follow-up bugs to preferencerize (what? it's totally a word) them later or leave them in and do the preferences stuff now. Given that this is a review comment and you asked me to do one of those two things, I think I'll do what you asked for. excised. > > openTreePanel: function IUI_openTreePanel() > > >- this.treePanel.openPopup(bar, "overlap", 120, -120, false, false); > >+ this.treePanel.openPopupAtScreen(this.win.screenX + 80, > >+ this.win.outerHeight + this.win.screenY); > > Why this change? I was having (and am still having) some placement issues with the treepanel. Positioning over the status bar isn't going to work if the status bar is hidden. Also, in overlap mode, if there wasn't enough room to display the tree, the tree would "jump" to the top of the browser window obscuring the toolbar. Neither of these were good. This alleviates that problem somewhat with absolute positioning. I'm going to follow another follow-up bug to do some smarter panel positioning. > > if (this._showDOMPanel) { > >+ if (!this.PropertyTreeView) { > >+ Cu.import("chrome://global/content/PropertyPanel.jsm", this); > >+ } > > Can be a global lazy getter, as in > http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#282 neat! I moved a few other variables into lazy getters from this method as well. It gets rid of a bunch of pointless checks. InspectorUI.style for the style module. InspectorUI.PropertyPanel and PropertyTreeView for the DOM panel pieces. InspectorUI.strings for the string bundle. Renamed this.inspectorBundle to this.strings > >+ // If there is no domPanel yet, then create a new one. > >+ if (!this.domPanel) { > > Could also add a lazyGetter for this. added. > > >+ let propPanel = new (this.PropertyPanel)(parent, document, "DOM", {}); > > Hrm, not sure whether "DOM" as a string is usefully localizable, but it can't > really hurt to treat it as such. ok, added to inspector.properties. > > > if (this.isStylePanelOpen) { > > this.stylePanel.hidePopup(); > > } > >+ if (this.isStylePanelOpen) { > >+ this.stylePanel.hidePopup(); > >+ } > > mismerge? ugh. Looks like. > > >+ addDOMItem: function IUI_addDOMItem(aPair) > > >+ item.setAttribute("label", aPair.name + ": " + aPair.value); > > triggers my l10n-spidey-senses again... Is there an "l10n review" bug for > inspector that we can add this potential concern to, perhaps? There isn't. Creating... bug 585030. > >diff --git a/browser/base/content/test/browser_inspector_domPanel.js b/browser/base/content/test/browser_inspector_domPanel.js > > >+function createDocument() > > >+ setTimeout(setupDOMTests, 0); > > Shouldn't be needed, right? In fact that method should be inlined here, I > think. yes, excellent! > >+function performTestComparisons(evt) > > >+ ok(InspectorUI.treeView.selectedNode, "selection"); > >+ ok(InspectorUI._showDOMPanel, "_showDOMPanel"); > >+ is(InspectorUI.isDOMPanelOpen, InspectorUI._showDOMPanel, "DOM panel matches _showDOMPanel?"); > >+ ok(InspectorUI.highlighter.isHighlighting, "panel is highlighting"); > >+ ok(InspectorUI.domTreeView.rowCount > 0, "domBox has items"); > > Better comparisons (actual checks of valid values for the elements being > inspected) would be good here (maybe roll into that other bug you have about > improving the tests?) Sounds like a plan. Filed bug 585043 to capture it. > >diff --git a/browser/base/content/test/browser_inspector_initialization.js b/browser/base/content/test/browser_inspector_initialization.js > > > function runInspectorTests(evt) > > { > >- if (evt.target.id != "inspector-panel") > >+ if (evt.target.id != "inspector-dom-panel") > > Are you changing this because dom-panel is the last panel to be opened? yes. I add a notifier and observer to these tests with the new tree panel code so we can stop looking for specific panels to popup and hide, but that's in a separate bug. > >diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css > >diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css > >diff --git a/browser/themes/winstripe/browser/browser.css b/browser/themes/winstripe/browser/browser.css > > > #highlighter-panel { > > >+ background: -moz-linear-gradient(top -1deg, #ffdd88, #ffeeaa); > >+ border: none; > >+ opacity: 0.35; > > Are these changes unrelated? they are. I moved the highlighter styling into CSS to make it look a bit nicer. I didn't like the border on the existing one. I can break those pieces out if they're not needed or wanted.

updated patch addressing review comments. fixed up a couple of unittests while in there. fallout from removing the _show* variables.

Comment on attachment 463622 [details] [diff] [review] DOM/JS object panel (post-review) ignore this. At the wrong level in mq.

the real patch!

Comment on attachment 463629 [details] [diff] [review] DOM/JS object panel (post-review) >diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc > disabled="true"/> > <command id="Inspector:Style" > oncommand="InspectorUI.toggleStylePanel();"/> >+ <command id="Inspector:Dom" >+ oncommand="InspectorUI.toggleDOMPanel();"/> > </commandset> I should have noticed this before, but since these commands aren't ever disabled and don't have effects on other nodes, you don't really need the <command> at all, do you? Just have the buttons use oncommand="toggle*Panel()" directly? >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js >+ get stylePanel() >+ { >+ return document.getElementById("inspector-style-panel"); why change this? Old way (lazy init and remember in openStylePanel) seems better. Or add a getter for it next to inspectCmd. >+ this.openTreePanel(); >+ this.styleBox = document.getElementById("inspector-style-listbox"); >+ >+ this.clearStylePanel(); >+ this.openStylePanel(); nit: fix grouping (keep style/dom/tree panel stuff together) > /** >+ * Add a new item to the DOM panel listbox >+ * >+ * @param aPair >+ * an object with name and value properties. >+ */ >+ addDOMItem: function IUI_addDOMItem(aPair) This looks unused?

Comment on attachment 463629 [details] [diff] [review] DOM/JS object panel (post-review) Do we expect more panels? I'm a bit surprised to see the DOM panel title in a .properties, but I didn't dig into the code to figure out why. Also, the two strings "DOM" are commented inconsistently, they should a) cross reference b) have a similar comment, and probably expand the acronym in there.

(In reply to comment #24) > Comment on attachment 463629 [details] [diff] [review] > DOM/JS object panel (post-review) > > Do we expect more panels? > > I'm a bit surprised to see the DOM panel title in a .properties, but I didn't > dig into the code to figure out why. Because the panel's built programmatically, we need to pass it a string through JS to give it a title. Hence, the .properties file. > Also, the two strings "DOM" are commented inconsistently, they should > a) cross reference > b) have a similar comment, and probably expand the acronym in there. the labels for the button and the panel title? I found a comment with a spelling error and have to address gavin's final comment as well. I'll correct these and resubmit. thanks for the review!

updates with gavin's comments addressed. Hopefully better l10n note in inspector.properties.

Comment on attachment 464823 [details] [diff] [review] [backed-out] DOM/JS object panel (post-review 2) Looks good to me, I'd add a localization note to browser.dtd, too, so that you have references both ways.

Comment on attachment 464823 [details] [diff] [review] [backed-out] DOM/JS object panel (post-review 2) http://hg.mozilla.org/mozilla-central/rev/e77d84f9ebe4

Backed out as part of http://hg.mozilla.org/mozilla-central/rev/5f699108f3cf. Need to investigate leaks from: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281575546.1281577015.2705.gz

Made use of the PropertyPanel.destroy() method on shutdown to clean up after the PropertyPanel. Leaks should be addressed now. Running a try build to verify.

Comment on attachment 465306 [details] [diff] [review] DOM/JS object panel (post-review 2) deleaked changes between this patch and previous look fine, r=me.

updated so browser/base/content/test/Makefile.in applies cleanly. Ready to land.