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

RESOLVED FIXED

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: rc, Assigned: rc)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [kd4b5])

Attachments

(1 attachment, 16 obsolete attachments)

32.47 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
(Assignee)

Comment 1

7 years ago
Created attachment 441800 [details] [diff] [review]
dom-panel-1

initial wip
(Assignee)

Comment 2

7 years ago
Created attachment 442477 [details] [diff] [review]
dom-panel-2

basic dom panel implementation
Attachment #442477 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Attachment #441800 - Flags: review?(gavin.sharp)
(Assignee)

Comment 3

7 years ago
Created attachment 442480 [details] [diff] [review]
dom-panel-2a

diffed the wrong way, correcting.
Attachment #442477 - Attachment is obsolete: true
Attachment #442480 - Flags: review?(gavin.sharp)
Attachment #442477 - Flags: review?(gavin.sharp)
(Assignee)

Comment 4

7 years ago
Created attachment 442788 [details] [diff] [review]
browser-inspector-domPanel

test file
Attachment #442788 - Flags: review?(gavin.sharp)
(Assignee)

Comment 5

7 years ago
Created attachment 445381 [details] [diff] [review]
dom-panel-rollup-rebased

dom-panel patches, rebased and folded.
Attachment #441800 - Attachment is obsolete: true
Attachment #442480 - Attachment is obsolete: true
Attachment #442788 - Attachment is obsolete: true
Attachment #445381 - Flags: review?(gavin.sharp)
Attachment #441800 - Flags: review?(gavin.sharp)
Attachment #442480 - Flags: review?(gavin.sharp)
Attachment #442788 - Flags: review?(gavin.sharp)
(Assignee)

Comment 6

7 years ago
Created attachment 445408 [details] [diff] [review]
dom-panel-rollup-rebased-2

fixed up some merge gunk. working properly now.
Attachment #445381 - Attachment is obsolete: true
Attachment #445408 - Flags: review?(gavin.sharp)
Attachment #445381 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

7 years ago
Created attachment 445420 [details] [diff] [review]
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.
Attachment #445408 - Attachment is obsolete: true
Attachment #445420 - Flags: review?(gavin.sharp)
Attachment #445408 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Blocks: 566096
(Assignee)

Updated

7 years ago
Blocks: 572038
(Assignee)

Comment 8

7 years ago
Created attachment 451637 [details] [diff] [review]
dom-panel-rebased-20100616

updated version of the patch.
Attachment #445420 - Attachment is obsolete: true
Attachment #445420 - Flags: review?(gavin.sharp)
(Assignee)

Comment 9

7 years ago
Created attachment 453431 [details] [diff] [review]
dom-panel-refreshed-20100623
Attachment #451637 - Attachment is obsolete: true
Attachment #453431 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Blocks: 573102

Comment 10

7 years ago
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
(Assignee)

Comment 11

7 years ago
let's talk about this in bug 573102.
(Assignee)

Comment 12

7 years ago
Created attachment 457607 [details] [diff] [review]
revised dom panel patch

updated after changes to style panel in bug 560692.
Attachment #453431 - Attachment is obsolete: true
Attachment #457607 - Flags: review?(gavin.sharp)
Attachment #453431 - Flags: review?(gavin.sharp)

Updated

7 years ago
No longer blocks: 573102
(Assignee)

Comment 13

7 years ago
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.
Attachment #457607 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Depends on: 573102
(Assignee)

Comment 14

7 years ago
Created attachment 459843 [details] [diff] [review]
DOM/JS object property panel

depends on patch in bug 573102.
Attachment #457607 - Attachment is obsolete: true
Attachment #459843 - Flags: review?(gavin.sharp)
(Assignee)

Comment 15

7 years ago
Created attachment 459850 [details] [diff] [review]
DOM/JS object property panel (folded)

folded version of DOM/JS property panel patch. Relies on patch in bug 573102.
Attachment #459843 - Attachment is obsolete: true
Attachment #459850 - Flags: review?(gavin.sharp)
Attachment #459843 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
blocking2.0: --- → ?
Need this. blocking betaN+ but let's get it in ASAP so we have time for feedback and iteration.
blocking2.0: ? → betaN+

Updated

7 years ago
Whiteboard: [kd4b4]
(Assignee)

Updated

7 years ago
Priority: -- → P1
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?
Attachment #459850 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 19

7 years ago
(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.
(Assignee)

Comment 20

7 years ago
Created attachment 463622 [details] [diff] [review]
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.
Attachment #459850 - Attachment is obsolete: true
Attachment #463622 - Flags: review?(gavin.sharp)
(Assignee)

Comment 21

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

ignore this. At the wrong level in mq.
Attachment #463622 - Flags: review?(gavin.sharp)
(Assignee)

Comment 22

7 years ago
Created attachment 463629 [details] [diff] [review]
DOM/JS object panel (post-review)

the real patch!
Attachment #463622 - Attachment is obsolete: true
Attachment #463629 - Flags: review?(gavin.sharp)
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?
Attachment #463629 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

7 years ago
Attachment #463629 - Flags: review?(l10n)

Comment 24

7 years ago
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.
Attachment #463629 - Flags: review?(l10n) → review-
(Assignee)

Comment 25

7 years ago
(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!
(Assignee)

Comment 26

7 years ago
Created attachment 464823 [details] [diff] [review]
[backed-out] DOM/JS object panel (post-review 2)

updates with gavin's comments addressed.

Hopefully better l10n note in inspector.properties.
Attachment #463629 - Attachment is obsolete: true
Attachment #464823 - Flags: review?

Comment 27

7 years ago
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.
Attachment #464823 - Flags: review? → review+
(Assignee)

Comment 28

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

http://hg.mozilla.org/mozilla-central/rev/e77d84f9ebe4
Attachment #464823 - Attachment description: DOM/JS object panel (post-review 2) → [checked-in] DOM/JS object panel (post-review 2)
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 29

7 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

7 years ago
Attachment #464823 - Attachment description: [checked-in] DOM/JS object panel (post-review 2) → [backed-out] DOM/JS object panel (post-review 2)
(Assignee)

Comment 30

7 years ago
Created attachment 465306 [details] [diff] [review]
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.
Attachment #464823 - Attachment is obsolete: true
Attachment #465306 - Flags: review?
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.
Attachment #465306 - Flags: review? → review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed

Updated

7 years ago
Whiteboard: [kd4b4] → [kd4b5]
(Assignee)

Comment 32

7 years ago
Created attachment 467005 [details] [diff] [review]
DOM/JS object panel (post-review) 2010-08-18

updated so browser/base/content/test/Makefile.in applies cleanly. Ready to land.
Attachment #465306 - Attachment is obsolete: true
Attachment #467005 - Flags: review+

Comment 33

7 years ago
http://hg.mozilla.org/mozilla-central/rev/c40c75011904
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.