561782 - Create DOM Panel for inspecting DOM nodes and their properties (reticle) Milestone 0.3

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Rob Campbell [:rc] (:robcee)]

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.

Comment 1

[Rob Campbell [:rc] (:robcee)]

Attachment: dom-panel-1

initial wip

Comment 2

[Rob Campbell [:rc] (:robcee)]

Attachment: dom-panel-2

basic dom panel implementation

Comment 3

[Rob Campbell [:rc] (:robcee)]

Attachment: dom-panel-2a

diffed the wrong way, correcting.

Comment 4

[Rob Campbell [:rc] (:robcee)]

Attachment: browser-inspector-domPanel

test file

Comment 5

[Rob Campbell [:rc] (:robcee)]

Attachment: dom-panel-rollup-rebased

dom-panel patches, rebased and folded.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Attachment: dom-panel-rollup-rebased-2

fixed up some merge gunk. working properly now.

Comment 7

[Rob Campbell [:rc] (:robcee)]

Attachment: dom-panel-rollup-rebased-4

More cleanup after fixing up the style-panel patch again.

TODO: change todo->ok in browser_inspector_initialization.js when these land.

Comment 8

[Rob Campbell [:rc] (:robcee)]

Attachment: dom-panel-rebased-20100616

updated version of the patch.

Comment 9

[Rob Campbell [:rc] (:robcee)]

Attachment: dom-panel-refreshed-20100623

Comment 10

[Julian Viereck]

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

Comment 11

[Rob Campbell [:rc] (:robcee)]

let's talk about this in bug 573102.

Comment 12

[Rob Campbell [:rc] (:robcee)]

Attachment: revised dom panel patch

updated after changes to style panel in bug 560692.

Comment 13

[Rob Campbell [:rc] (:robcee)]

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.

Comment 14

[Rob Campbell [:rc] (:robcee)]

Attachment: DOM/JS object property panel

depends on patch in bug 573102.

Comment 15

[Rob Campbell [:rc] (:robcee)]

Attachment: DOM/JS object property panel (folded)

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

Comment 16

[Johnathan Nightingale [:johnath]]

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

Comment 17

[Mihai Sucan [:msucan]]

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 18

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 19

[Rob Campbell [:rc] (:robcee)]

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

Comment 20

[Rob Campbell [:rc] (:robcee)]

Attachment: DOM/JS object panel (post-review)

updated patch addressing review comments.

fixed up a couple of unittests while in there. fallout from removing the _show* variables.

Comment 21

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 463622 [details] [diff] [review]
DOM/JS object panel (post-review)

ignore this. At the wrong level in mq.

Comment 22

[Rob Campbell [:rc] (:robcee)]

Attachment: DOM/JS object panel (post-review)

the real patch!

Comment 23

[:Gavin Sharp [email: [e-mail]]]

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 24

[Axel Hecht [:Pike]]

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.

Comment 25

[Rob Campbell [:rc] (:robcee)]

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

Comment 26

[Rob Campbell [:rc] (:robcee)]

Attachment: [backed-out] DOM/JS object panel (post-review 2)

updates with gavin's comments addressed.

Hopefully better l10n note in inspector.properties.

Comment 27

[Axel Hecht [:Pike]]

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 28

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 464823 [details] [diff] [review]
[backed-out] DOM/JS object panel (post-review 2)

http://hg.mozilla.org/mozilla-central/rev/e77d84f9ebe4

Comment 29

[Rob Campbell [:rc] (:robcee)]

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

Comment 30

[Rob Campbell [:rc] (:robcee)]

Attachment: DOM/JS object panel (post-review 2) deleaked

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 31

[Dietrich Ayala (:dietrich)]

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.

Comment 32

[Rob Campbell [:rc] (:robcee)]

Attachment: DOM/JS object panel (post-review) 2010-08-18

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

Comment 33

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/c40c75011904