547453 - (reticle) Create web page inspector in Firefox

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

This is a meta bug to capture the creation of a web page inspector feature in Firefox. The web page inspector should incorporate the following design goals.

* fast '''and''' pretty
* Graphical highlighter
* Rulers & guides
* Properties, palettes?
* Canvas/Layer/ini-flasher? 
* unobtrusive to inspected document

See https://wiki.mozilla.org/Firefox/Projects/Inspector

Comment 1

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

Attachment: initial inspector patch

first pass patch. Some bugginess. No toolbar buttons for windows or linux (and the mac one needs work).

Dynamic inspection partially working. Need to implement tree selection on highlight. Highlighting from tree selection is working.

Very preliminary. No review requested, though review comments and suggestions welcome.

Comment 2

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

Attachment: diff-based initial patch

possibly easier-to-read (but harder to apply?) patch generated against yesterday's mozilla-central.

Comment 3

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

(ignore the microsummaries diffs in above)

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 434325 [details] [diff] [review]
initial inspector patch

I'm assuming this chunk is unrelated to your code:
https://bugzilla.mozilla.org/attachment.cgi?id=434325&action=diff#a/browser/base/content/browser.xul_sec3

In fact, it looks like they all go bye-bye here:
https://bugzilla.mozilla.org/attachment.cgi?id=434325&action=diff#a/browser/base/content/browser.xul_sec1

It looks like you just concatenated your patch queue maybe?  This made this pass over a bit difficult FWIW.  Not doing that would also have made your diff much smaller.

It would be nice to get PanelHighlighter morphed into the new inIFlasher.  You'll fix firebug and dom inspector + a few other add-ons that I've randomly seen use the API.

I think your approach looks fine though.

Comment 5

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

(In reply to comment #4)
> (From update of attachment 434325 [details] [diff] [review])
> I'm assuming this chunk is unrelated to your code:
> https://bugzilla.mozilla.org/attachment.cgi?id=434325&action=diff#a/browser/base/content/browser.xul_sec3
> 
> In fact, it looks like they all go bye-bye here:
> https://bugzilla.mozilla.org/attachment.cgi?id=434325&action=diff#a/browser/base/content/browser.xul_sec1

yeah, unrelated. I think what happened was I did some white-space cleanup that conflicted with a later patch in moz-central.

> It looks like you just concatenated your patch queue maybe?  This made this
> pass over a bit difficult FWIW.  Not doing that would also have made your diff
> much smaller.

I did. My second attachment is just a straight diff of browser/ against mozilla-central + a patch or two with some microsummaries changes.

> It would be nice to get PanelHighlighter morphed into the new inIFlasher. 
> You'll fix firebug and dom inspector + a few other add-ons that I've randomly
> seen use the API.

yeah, for sure, and I may do that yet. At this stage, it's easier to just keep it close by.

fwiw, Firebug doesn't use the inIFlasher. They have their own css-based highlighting mechanism in Firebug itself.

> I think your approach looks fine though.

good to know. Thanks for the first pass!

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> fwiw, Firebug doesn't use the inIFlasher. They have their own css-based
> highlighting mechanism in Firebug itself.
Ah - it used to!  I bet they worked around the issue too :/

Comment 7

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

Attachment: 0.1

first patch. 0.1. Includes basic test verifying opening and closing.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 436401 [details] [diff] [review]
0.1

For more context with each comment, please see http://reviews.visophyte.org/r/436401/.

on file: browser/base/content/browser-inspector.js line 1
> /*
> #ifdef 0

Any reason why this isn't inside the ifdef?

You also have not added the proper modelines here:
https://developer.mozilla.org/En/Developer_Guide/Coding_Style#section_23


on file: browser/base/content/browser-inspector.js line 18
>  * The Initial Developer of the Original Code is Mozilla.

Mozilla Foundation, and it's supposed to go on a new line.  Make sure you copy
this from http://www.mozilla.org/MPL/boilerplate-1.1/ and not another file.


on file: browser/base/content/browser-inspector.js line 41
> /*
>  * Highlighter

nit: /**
      * note the two stars on the first line


on file: browser/base/content/browser-inspector.js line 58
>   "isindex": true

nit: trailing comma so you don't mess up blame when you add something


on file: browser/base/content/browser-inspector.js line 61
> function childWindowsDo(aWindow, aFunction) {

java-doc style comments here would be appreciated
nit: opening brace on new line


on file: browser/base/content/browser-inspector.js line 75
> function Highlighter(aColor, aThickness, isInverted) {

java-doc style comments here would be appreciated
nit: opening brace on new line


on file: browser/base/content/browser-inspector.js line 84
>   getElement: function() { return this.element; },
>   setElement: function(node) {
>     if (node && node.nodeType == Node.ELEMENT_NODE) {
>       this.node = node;
>       this.shell.scrollElementIntoView(node);
>     } else
>       throw "Invalid node type.";
>   },
> 
>   getColor: function() { return this.shell.color; },
>   setColor: function(value) { return this.shell.color = value; },
> 
>   getThickness: function() { return this.shell.thickness; },
>   setThickness: function(thickness) { this.shell.thickness = thickness; },

any reason these aren't proper JS getters and setters?


on file: browser/base/content/browser-inspector.js line 100
>   highlight: function Highlighter_highlight() {

java-doc style comments here would be appreciated
nit: opening brace on new line


on file: browser/base/content/browser-inspector.js line 107
>   highlightNode: function Highlighter_highlightNode(element) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 113
>   unhighlight: function Highlighter_unhighlight() {

java-doc style comments here would be appreciated
nit: opening brace on new line



on file: browser/base/content/browser-inspector.js line 115
>     if (this.node)
>       this.shell.repaintElement(this.node);

nit: one line ifs are supposed to be braced


on file: browser/base/content/browser-inspector.js line 117
>   }

nit: add trailing comma


on file: browser/base/content/browser-inspector.js line 125
> function PanelHighlighter(browser, bgColor, borderSize, opacity) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 136
>   getColor: function() { return this.color; },
>   setColor: function(aColor) {
>     this.backgroundColor = aColor;
>     this.highlighterPanel.style.backgroundColor = aColor;
>   },
> 
>   getBorder: function() { return this.border; },
>   setBorder: function(aThickness) {
>     this.border = aThickness;
>     this.highlighterPanel.style.border = "solid blue " + aThickness + "px";
>   },
> 
>   getOpacity: function() { return this.opacity; },
>   setOpacity: function(anAlpha) {
>     this.opacity = anAlpha;
>     this.highlighterPanel.style.opacity = anAlpha;
>   },
> 
>   getNode: function() { return this.node; },

why aren't these also not proper JS getters and setters?


on file: browser/base/content/browser-inspector.js line 158
>   updatePanelStyles: function PanelHighlighter_updatePanelStyles() {

java-doc style comments here would be appreciated
nit: opening brace on new line


on file: browser/base/content/browser-inspector.js line 166
>   highlight: function PanelHighlighter_highlight() {

java-doc style comments here would be appreciated
nit: opening brace on new line


on file: browser/base/content/browser-inspector.js line 182
>   highlightAndScroll: function PanelHighlighter_highlightAndScroll() {

java-doc style comments here would be appreciated
nit: opening brace on new line


on file: browser/base/content/browser-inspector.js line 189
>   highlightNode: function Highlighter_highlightNode(element) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 195
>   highlightAndScrollNode: function Highlighter_highlightNode(element) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 201
>   unhighlight: function PanelHighlighter_unhighlight() {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 208
>   isHighlighting: function PanelHighlighter_isHighlighting() {
>     return this.highlighterPanel.state == "open";
>   },

this feels like it should be a JS getter


on file: browser/base/content/browser-inspector.js line 216
>   rectForElement: function PanelHighlighter_rectForElement(element) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 225
>       height: correct(rect.height)

nit: add trailing comma


on file: browser/base/content/browser-inspector.js line 230
>   scrollIfOutOfBounds: function PanelHighlighter_scrollIfOutOfBounds(node, rect) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 236
>   viewContainsRect: function PanelHighlighter_viewContainsRect(rect) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 243
>     if ((scrollX <= rect.left) && (scrollWidth >= rect.right) && 
>       (scrollY <= rect.top) && (scrollHeight >= rect.bottom)) {

how you indent here is odd.  You should line up the second line to be inside
the ( for the if, and the body of the if statement should only be two indents


on file: browser/base/content/browser-inspector.js line 249
>   }

nit: add trailing comma


on file: browser/base/content/browser-inspector.js line 252
> /*

nit: two *s please


on file: browser/base/content/browser-inspector.js line 258
> function InspectorTreeView() {

java-doc style comments here would be appreciated
nit: opening brace on new line


on file: browser/base/content/browser-inspector.js line 270
>   getEditable: function() { return false; },
>   getSelection: function() { return this.view.selection; },

These should be proper JS getters


on file: browser/base/content/browser-inspector.js line 273
>   initialize: function ITV_init(win) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 276
>     // this.setAnonContent(false); // PrefUtils.getPref("inspector.dom.showAnon")
>     // this.setProcessingInstructions(false); // PrefUtils.getPref("inspector.dom.showProcessingInstructions")
>     // this.setAccessibleNodes(false); // PrefUtils.getPref("inspector.dom.showAccessibleNodes")
>     // this.setWhitespaceNodes(false); // PrefUtils.getPref("inspector.dom.showWhitespaceNodes")
> 
>     // aPane.notifyViewerReady(this);

?


on file: browser/base/content/browser-inspector.js line 286
>   destroy: function ITV_destroy() {

java-doc style comments here would be appreciated
nit: opening brace on new line


on file: browser/base/content/browser-inspector.js line 290
>   getCellText: function ITV_getCellText(row, col) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 298
>   getSelectedNode: function ITV_getSelectedNode() {
>     let rowIndex = this.getSelectionIndex();
>     return this.view.getNodeFromRowIndex(rowIndex);
>   },
> 
>   setSelectedRow: function ITV_setSelectedRow(index) {
>     this.view.selection.select(index);
>     this.tree.treeBoxObject.ensureRowIsVisible(index);
>   },

These should probably be JS getters and setters


on file: browser/base/content/browser-inspector.js line 308
>   setSelectedNode: function ITV_setSelectedNode(node) {
>     let rowIndex = this.view.getRowIndexFromNode(node);
>     InspectorUI._log("setSelectedNode: " + node + " row: " + rowIndex);
>     if (rowIndex > -1) {
>       this.setSelectedRow(rowIndex);
>     } else {
>       this.selectElementInTree(node);
>     }

JS setter I think too


on file: browser/base/content/browser-inspector.js line 318
>   selectElementInTree: function ITV_selectElementInTree(node) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 327
>     let domUtils = Cc["@mozilla.org/inspector/dom-utils;1"]
>                    .getService(Ci.inIDOMUtils);

nit: pretty sure we put the dot at the end of the first line


on file: browser/base/content/browser-inspector.js line 346
>     // we've got all the ancestors, now open them 
>     // one-by-one from the top on down

I see this in lots of places, but I'm only going to comment once.  I really
prefer actual sentences with capitalization and punctuation and stuff in
comments please!


on file: browser/base/content/browser-inspector.js line 368
>   }

nit: add trailing comma


on file: browser/base/content/browser-inspector.js line 371
> /*

nit: two *s


on file: browser/base/content/browser-inspector.js line 395
>   destroy:
>   function InspectorUI_destroy() {

java-doc style comments here would be appreciated
nit: opening brace on new line


on file: browser/base/content/browser-inspector.js line 396
>   function InspectorUI_destroy() {
>     this.detachPageListeners();
>   },

You've suddenly changed style here.  Please be consistent.  If it's because
the named functions are too long, just change them to IUI_*


on file: browser/base/content/browser-inspector.js line 400
>   onToolbarButtonCommand:
>   function InspectorUI_onToolbarButtonCommand(evt) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 406
>   toggleInspectorUI:
>   function InspectorUI_toggleInspectorUI(event) {

java-doc style comments here would be appreciated
nit: opening brace on new line
nit: argument doesn't follow are argument naming conventions


on file: browser/base/content/browser-inspector.js line 416
>   isInspecting:
>   function InspectorUI_isInspecting() {
>     return this.inspecting;
>   },

is this even needed?  callers can just do |InspectorUI.inspecting|


on file: browser/base/content/browser-inspector.js line 421
>   isInspectorOn:
>   function InspectorUI_isInspectorOn() {
>     return this.isPanelOpen();
>   },

should be a JS getter


on file: browser/base/content/browser-inspector.js line 426
>   isPanelOpen:
>   function InspectorUI_isPanelOpen() {
>     return this.treePanel && this.treePanel.state == "open";
>   },

should be a JS getter


on file: browser/base/content/browser-inspector.js line 431
>   openTreePanel:
>   function InspectorUI_openTreePanel() {

java-doc style comments here would be appreciated
nit: opening brace on new line

I'm actually going to stop commenting on this now.


on file: browser/base/content/browser-inspector.js line 442
>       this.treePanel.sizeTo(720, 240);

magic numbers!  :(


on file: browser/base/content/browser-inspector.js line 519
>   /* model creation methods */

much bigger fan of this style for dividing sections (it's more obvious):
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManagerUI.js#58


on file: browser/base/content/browser-inspector.js line 548
>       case "mousedown":

I think you actually want mouseup, but I also think that click handles it.


on file: browser/base/content/browser-inspector.js line 570
>     this.highlighter.unhighlight();
>     this.highlighter.highlightNode(node);

Seems like highlighting a new node should automatically unhighlight the last
one.  Not making the caller have to worry about that would be good I think.


on file: browser/base/content/browser-sets.inc line None

LOL


on file: browser/base/content/browser.xul line 228
>     <panel id="inspector-panel"
>            orient="vertical"
>            ignorekeys="true"
>            noautofocus="true"
>            noautohide="true"
>            left="100"
>            top="600"
>            level="top"
>            onpopupshown="InspectorUI.panelShown(event);"
>            aria-labelledby="inspectPagePanelTitle">
>       <tree id="inspector-tree" class="plain" seltype="single" treelines="true" 

You aren't consistent in how you do multiple attributes.  Check with a browser
peer on what the right thing to do here is.


on file: browser/base/content/test/browser_inspector_initialization.js line 1
> let doc;

No license block?  Please add it per
http://www.mozilla.org/MPL/license-policy.html (Should be Creative Commons
Public Domain Dedication)


on file: browser/themes/pinstripe/browser/browser.css line 655
> /* ----- DEFAULT INSPECT BUTTON ----- */	

don't forget about winstripe and gnomestripe!

Comment 9

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

thanks for the detailed review! Just checked in a first-pass cleanup. I'll keep tidying, commenting and fixing over the weekend.

Comment 10

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

Attachment: inspector 0.1b

I think I got all of the nits and issues you mentioned. Pretty significant changes here as there were a number of different event handlers added.

Added some button-styling stubs for winstripe and gnomestripe to borrow the Stop icon for the toolbar button. I need some graphics for that as well as a proper button for pinstripe.

Comment 11

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

In the interests of helping you through the review, I'm going to comment on the specific issues you found. I believe I dealt with the nits around comment and variables sufficiently that they don't need individual comment.

(In reply to comment #8)
> (From update of attachment 436401 [details] [diff] [review])
> For more context with each comment, please see
> http://reviews.visophyte.org/r/436401/.
> 
> on file: browser/base/content/browser-inspector.js line 1
> > /*
> > #ifdef 0
> 
> Any reason why this isn't inside the ifdef?

fixed

> You also have not added the proper modelines here:
> https://developer.mozilla.org/En/Developer_Guide/Coding_Style#section_23

fixed.

> on file: browser/base/content/browser-inspector.js line 18
> >  * The Initial Developer of the Original Code is Mozilla.
> 
> Mozilla Foundation, and it's supposed to go on a new line.  Make sure you copy
> this from http://www.mozilla.org/MPL/boilerplate-1.1/ and not another file.

I should have my licensing blocks corrected now. Feel free to give them a once-over for any extra issues.

> on file: browser/base/content/browser-inspector.js line 84
> >   getElement: function() { return this.element; },
> >   setElement: function(node) {
> >     if (node && node.nodeType == Node.ELEMENT_NODE) {
> >       this.node = node;
> >       this.shell.scrollElementIntoView(node);
> >     } else
> >       throw "Invalid node type.";
> >   },
> > 
> >   getColor: function() { return this.shell.color; },
> >   setColor: function(value) { return this.shell.color = value; },
> > 
> >   getThickness: function() { return this.shell.thickness; },
> >   setThickness: function(thickness) { this.shell.thickness = thickness; },
> 
> any reason these aren't proper JS getters and setters?

done.

> on file: browser/base/content/browser-inspector.js line 136
> >   getColor: function() { return this.color; },
> >   setColor: function(aColor) {
> >     this.backgroundColor = aColor;
> >     this.highlighterPanel.style.backgroundColor = aColor;
> >   },
> > 
> >   getBorder: function() { return this.border; },
> >   setBorder: function(aThickness) {
> >     this.border = aThickness;
> >     this.highlighterPanel.style.border = "solid blue " + aThickness + "px";
> >   },
> > 
> >   getOpacity: function() { return this.opacity; },
> >   setOpacity: function(anAlpha) {
> >     this.opacity = anAlpha;
> >     this.highlighterPanel.style.opacity = anAlpha;
> >   },
> > 
> >   getNode: function() { return this.node; },
> 
> why aren't these also not proper JS getters and setters?

I ditched these altogether. Unnecessary.

> on file: browser/base/content/browser-inspector.js line 208
> >   isHighlighting: function PanelHighlighter_isHighlighting() {
> >     return this.highlighterPanel.state == "open";
> >   },
> 
> this feels like it should be a JS getter

'tis.

> on file: browser/base/content/browser-inspector.js line 243
> >     if ((scrollX <= rect.left) && (scrollWidth >= rect.right) && 
> >       (scrollY <= rect.top) && (scrollHeight >= rect.bottom)) {
> 
> how you indent here is odd.  You should line up the second line to be inside
> the ( for the if, and the body of the if statement should only be two indents

Yeah, I think I've got this now. Looks like:

+    if ((0 <= aRect.left) && (aRect.right <= visibleWidth) && 
+        (0 <= aRect.top) && (aRect.bottom <= visibleHeight)) {

(code changed a little too, you'll notice)

> on file: browser/base/content/browser-inspector.js line 270
> >   getEditable: function() { return false; },
> >   getSelection: function() { return this.view.selection; },
> 
> These should be proper JS getters

Done.

> on file: browser/base/content/browser-inspector.js line 276
> >     // this.setAnonContent(false); // PrefUtils.getPref("inspector.dom.showAnon")
> >     // this.setProcessingInstructions(false); // PrefUtils.getPref("inspector.dom.showProcessingInstructions")
> >     // this.setAccessibleNodes(false); // PrefUtils.getPref("inspector.dom.showAccessibleNodes")
> >     // this.setWhitespaceNodes(false); // PrefUtils.getPref("inspector.dom.showWhitespaceNodes")
> > 
> >     // aPane.notifyViewerReady(this);
> 
> ?

gone. Pasted from DOMInspector code for reference. Unneeded.

> on file: browser/base/content/browser-inspector.js line 298
> >   getSelectedNode: function ITV_getSelectedNode() {
> >     let rowIndex = this.getSelectionIndex();
> >     return this.view.getNodeFromRowIndex(rowIndex);
> >   },
> > 
> >   setSelectedRow: function ITV_setSelectedRow(index) {
> >     this.view.selection.select(index);
> >     this.tree.treeBoxObject.ensureRowIsVisible(index);
> >   },
> 
> These should probably be JS getters and setters

And they are now.

> on file: browser/base/content/browser-inspector.js line 308
> >   setSelectedNode: function ITV_setSelectedNode(node) {
> >     let rowIndex = this.view.getRowIndexFromNode(node);
> >     InspectorUI._log("setSelectedNode: " + node + " row: " + rowIndex);
> >     if (rowIndex > -1) {
> >       this.setSelectedRow(rowIndex);
> >     } else {
> >       this.selectElementInTree(node);
> >     }
> 
> JS setter I think too

I agree. Done.

> on file: browser/base/content/browser-inspector.js line 327
> >     let domUtils = Cc["@mozilla.org/inspector/dom-utils;1"]
> >                    .getService(Ci.inIDOMUtils);
> 
> nit: pretty sure we put the dot at the end of the first line

OK, fixed.

> on file: browser/base/content/browser-inspector.js line 346
> >     // we've got all the ancestors, now open them 
> >     // one-by-one from the top on down
> 
> I see this in lots of places, but I'm only going to comment once.  I really
> prefer actual sentences with capitalization and punctuation and stuff in
> comments please!

I believe I got most of them. A lot of one-line comments are just phrases though and don't stand on their own as full sentences. The capitalization and punctuation is unnecessary, imo.

> on file: browser/base/content/browser-inspector.js line 416
> >   isInspecting:
> >   function InspectorUI_isInspecting() {
> >     return this.inspecting;
> >   },
> 
> is this even needed?  callers can just do |InspectorUI.inspecting|

You're right. Removed it.

> on file: browser/base/content/browser-inspector.js line 421
> >   isInspectorOn:
> >   function InspectorUI_isInspectorOn() {
> >     return this.isPanelOpen();
> >   },
> 
> should be a JS getter

quite.

> on file: browser/base/content/browser-inspector.js line 426
> >   isPanelOpen:
> >   function InspectorUI_isPanelOpen() {
> >     return this.treePanel && this.treePanel.state == "open";
> >   },
> 
> should be a JS getter

true. Done.

> java-doc style comments here would be appreciated
> nit: opening brace on new line
> 
> I'm actually going to stop commenting on this now.

Really, one of them would've sufficed, but I appreciate the tenacity. :)

> on file: browser/base/content/browser-inspector.js line 442
> >       this.treePanel.sizeTo(720, 240);
> 
> magic numbers!  :(

I've changed this to:

+      this.treePanel.openPopup(bar, "overlap", 120, -120, false, false);
+      this.treePanel.sizeTo(this.win.outerWidth / 8 * 7, this.win.outerHeight / 5);

Deciding to go for ratios of window dimensions instead of hard values. A couple of milestones down the road we'll get proper resizers on these things store window sizes as preferences so these will likely be a one-time thing.

> on file: browser/base/content/browser-inspector.js line 519
> >   /* model creation methods */
> 
> much bigger fan of this style for dividing sections (it's more obvious):
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManagerUI.js#58

yeah, that's nicer. Converted.

> on file: browser/base/content/browser-inspector.js line 548
> >       case "mousedown":
> 
> I think you actually want mouseup, but I also think that click handles it.

Removed. Click is sufficient.

> on file: browser/base/content/browser-inspector.js line 570
> >     this.highlighter.unhighlight();
> >     this.highlighter.highlightNode(node);
> 
> Seems like highlighting a new node should automatically unhighlight the last
> one.  Not making the caller have to worry about that would be good I think.

agreed. Done.

There is a case where if I'm moving around in the document, I may want to rehighlight differently, but for now unhighlighting within the highlight method is good.

> on file: browser/base/content/browser-sets.inc line None
> 
> LOL

Yeah, I dunno.

> on file: browser/base/content/browser.xul line 228
> >     <panel id="inspector-panel"
> >            orient="vertical"
> >            ignorekeys="true"
> >            noautofocus="true"
> >            noautohide="true"
> >            left="100"
> >            top="600"
> >            level="top"
> >            onpopupshown="InspectorUI.panelShown(event);"
> >            aria-labelledby="inspectPagePanelTitle">
> >       <tree id="inspector-tree" class="plain" seltype="single" treelines="true" 
> 
> You aren't consistent in how you do multiple attributes.  Check with a browser
> peer on what the right thing to do here is.

I talked to Gavin and he said this was OK. browser.xul is pretty inconsistent. This felt reasonable to me, though we can change it if you think they'd be better off on separate lines.

> on file: browser/base/content/test/browser_inspector_initialization.js line 1
> > let doc;
> 
> No license block?  Please add it per
> http://www.mozilla.org/MPL/license-policy.html (Should be Creative Commons
> Public Domain Dedication)

done.

> on file: browser/themes/pinstripe/browser/browser.css line 655
> > /* ----- DEFAULT INSPECT BUTTON ----- */	
> 
> don't forget about winstripe and gnomestripe!

Reused stop buttons for now.

Hope this helps speed up your review!

Comment 12

[Shawn Wilsher :sdwilsh]

I should get to the review on Monday.

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 439414 [details] [diff] [review]
inspector 0.1b

For review comments with context, see http://reviews.visophyte.org/r/439414/

on file: .hgtags line 40
> b95dedf41142fa51d80a5f00faf01284ff675b70 pre-alpha

make sure this doesn't get pushed when you push this


on file: browser/base/content/browser-inspector.js line 61
>  * @param   aWindow
>  *          top-level window object
>  *
>  * @param   aFunction
>  *          function to execute on the window object
>  *
>  * @return  null

we don't usually put spaces between each @param and @returns.  Also, the two
or three spaces after looks odd.  I don't care enough to make you change it,
but wanted to make you aware of it.  Use your judgment.

Same goes with the line between the comment and the method implementation. 
Don't think I've seen that in our code before either.

Note that you do want to use @returns and not @return.  mak can fill you in as
to why :)


on file: browser/base/content/browser-inspector.js line 131
>   updatePanelStyles: function PanelHighlighter_updatePanelStyles()
>   {
>     let style = this.panel.style;
>     style.backgroundColor = this.backgroundColor;
>     style.border = "solid blue " + this.border + "px";
>     style.MozBorderRadius = "4px";
>     style.opacity = this.opacity;
>   },

is it going to be a problem that this isn't themable?


on file: browser/base/content/browser-inspector.js line 170
>       this.panel.sizeTo(rect.width, rect.height); // # todo

what is todo about this?


on file: browser/base/content/browser-inspector.js line 227
>     if (aRect.top > visibleHeight || aRect.left > visibleWidth ||
>         aRect.bottom < 0 || aRect.right < 0) {
>       return false; // totally off-view
>     }

A more detailed comment about this if and what it does would be appreciated.


on file: browser/base/content/browser-inspector.js line 232
>     offsetX = aRect.left < 0 ? Math.abs(aRect.left) : 0;
>     offsetY = aRect.top < 0 ? Math.abs(aRect.top) : 0;
>     width = aRect.right > visibleWidth ? visibleWidth - aRect.left : aRect.width;
>     width -= offsetX;
>     height = aRect.bottom > visibleHeight ? visibleHeight - aRect.top :
>       aRect.height;
>     height -= offsetY;

I'd like a comment on all this math too.  I could sit and figure out what you
are trying to do, or I could read a comment and then just check that that is
what it does.  Future hackers will appreciate the comment.


on file: browser/base/content/browser-inspector.js line 243
>     if (width > 0 && height > 0) {

a comment here would also likely be useful.


on file: browser/base/content/browser-inspector.js line 369
>     if ((0 <= aRect.left) && (aRect.right <= visibleWidth) && 
>         (0 <= aRect.top) && (aRect.bottom <= visibleHeight)) {
>       return true;
>     }
> 
>     return false;

would it not be simpler to do return (0 <=...?


on file: browser/base/content/browser-inspector.js line 380
>   /**
>    * Attach keypress, mousemove and scroll events to panels.
>    */
> 
>   attachListeners: function PanelHighlighter_attachListeners()

comment does not match implementation.


on file: browser/base/content/browser-inspector.js line 392
>   /**
>    * Detach keypress, mousemove and scroll events to panels.
>    */
> 
>   detachListeners: function PanelHighlighter_detachListeners()

comment does not match implementation


on file: browser/base/content/browser-inspector.js line 423
>           InspectorUI.inspectNode(element); // # todo

what is todo about this?


on file: browser/base/content/browser-inspector.js line 877
>       case "browser.inspector.enabled":
>         let inspectorMenuItem = document.getElementById("menu_inspector");
>         inspectorMenuItem.hidden = !gPrefService.getBoolPref("browser.inspector.enabled");
>         break;

you will want to brace for this case statement to make sure your let is only
valid for this case.


on file: browser/base/content/browser-inspector.js line 881
>       default: break;

nit: break on new line (or just omit default)


on file: browser/base/content/browser-inspector.js line 895
>     if (this.selectEventsSuppressed)
>       return false;

nit: you brace one line ifs everywhere else in this file


on file: browser/base/content/browser-inspector.js line 901
>     this.highlighter.highlightNode(node); // # todo scrolling causes issues

what kind of issues?  Can it be fixed in a follow-up bug?


on file: browser/base/content/browser-inspector.js line 907
>    * attach event listeners to content window and child windows to enable 
>    * highlighting and click to stop inspection

nit: I'd prefer actual sentences with punctuation and stuff in java-doc style
comments.


on file: browser/base/content/browser.xul line 226
>            onclick="InspectorUI.stopInspecting();" />

nit: we don't seem to do spaces before the closing slash in this file (at
least in this area)


on file: browser/base/content/browser.xul line 238
>       <tree id="inspector-tree" class="plain" seltype="single" treelines="true"
>         onselect="InspectorUI.onTreeSelected()" flex="1">

nit: onselect should line up with id per style of rest of file


on file: browser/base/content/browser.xul line 241
>           <treecol id="colNodeName" label="nodeName" primary="true"
>             persist="width,hidden,ordinal" flex="1"/>

nit: persists should line up with id (going to stop commenting on this issue)


r- only because you have a bunch of todo's still.  ddahl should look over your tests, and I think I've been about as much help as I can be.  Your next step should be to finish up the todos, and then ask a browser peer to review this.

Comment 14

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

(In reply to comment #13)
> nit: we don't seem to do spaces before the closing slash in this file (at
> least in this area)

I think this qualifies as an uber-nit.

Comment 15

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

(In reply to comment #13)
> (From update of attachment 439414 [details] [diff] [review])
> For review comments with context, see http://reviews.visophyte.org/r/439414/
> 
> on file: .hgtags line 40
> > b95dedf41142fa51d80a5f00faf01284ff675b70 pre-alpha
> 
> make sure this doesn't get pushed when you push this

good catch. Thanks for mentioning it.

> on file: browser/base/content/browser-inspector.js line 61
> >  * @param   aWindow
> >  *          top-level window object
> >  *
> >  * @param   aFunction
> >  *          function to execute on the window object
> >  *
> >  * @return  null
> 
> we don't usually put spaces between each @param and @returns.  Also, the two
> or three spaces after looks odd.  I don't care enough to make you change it,
> but wanted to make you aware of it.  Use your judgment.

I see. Examples in browser.js that use javadoc style comments don't do the extra indenting which I did just to line up the parameter and return types. The extra space between lines might've been overkill.

> Same goes with the line between the comment and the method implementation. 
> Don't think I've seen that in our code before either.

I like vertical whitespace. Makes things look less cramped. Personal taste, I guess. I can take it out if it's inconsistent.

> Note that you do want to use @returns and not @return.  mak can fill you in as
> to why :)

I'll as him!

> on file: browser/base/content/browser-inspector.js line 131
> >   updatePanelStyles: function PanelHighlighter_updatePanelStyles()
> >   {
> >     let style = this.panel.style;
> >     style.backgroundColor = this.backgroundColor;
> >     style.border = "solid blue " + this.border + "px";
> >     style.MozBorderRadius = "4px";
> >     style.opacity = this.opacity;
> >   },
> 
> is it going to be a problem that this isn't themable?

this might be something we want to do down the road. For an alpha-release, I think we can use some hard-coded rules, but that's my opinion.

This is really a first-pass prototype and ultimate, the highlighter is going to look quite a bit different.

See https://wiki.mozilla.org/Firefox/Projects/Inspector#Designs for what I'm moving towards for the highlighter.

> on file: browser/base/content/browser-inspector.js line 170
> >       this.panel.sizeTo(rect.width, rect.height); // # todo
> 
> what is todo about this?

That's a left-over. Removed.

> on file: browser/base/content/browser-inspector.js line 227
> >     if (aRect.top > visibleHeight || aRect.left > visibleWidth ||
> >         aRect.bottom < 0 || aRect.right < 0) {
> >       return false; // totally off-view
> >     }
> 
> A more detailed comment about this if and what it does would be appreciated.

added:
    // If any of these edges are out-of-bounds, the node's rectangle is
    // completely out-of-view and we can return

> on file: browser/base/content/browser-inspector.js line 232
> >     offsetX = aRect.left < 0 ? Math.abs(aRect.left) : 0;
> >     offsetY = aRect.top < 0 ? Math.abs(aRect.top) : 0;
> >     width = aRect.right > visibleWidth ? visibleWidth - aRect.left : aRect.width;
> >     width -= offsetX;
> >     height = aRect.bottom > visibleHeight ? visibleHeight - aRect.top :
> >       aRect.height;
> >     height -= offsetY;
> 
> I'd like a comment on all this math too.  I could sit and figure out what you
> are trying to do, or I could read a comment and then just check that that is
> what it does.  Future hackers will appreciate the comment.

This is one of the trickiest part of the highlighter code. It seems to work pretty well except for when I'm highlighting during mousemove from the highlighter panel. I could use a consult on that part of it.

I'll break it up and walk through what I'm trying to do in the comments.

> on file: browser/base/content/browser-inspector.js line 243
> >     if (width > 0 && height > 0) {
> 
> a comment here would also likely be useful.
> 
> 
> on file: browser/base/content/browser-inspector.js line 369
> >     if ((0 <= aRect.left) && (aRect.right <= visibleWidth) && 
> >         (0 <= aRect.top) && (aRect.bottom <= visibleHeight)) {
> >       return true;
> >     }
> > 
> >     return false;
> 
> would it not be simpler to do return (0 <=...?

yes, yes it would. :)

> on file: browser/base/content/browser-inspector.js line 380
> >   /**
> >    * Attach keypress, mousemove and scroll events to panels.
> >    */
> > 
> >   attachListeners: function PanelHighlighter_attachListeners()
> 
> comment does not match implementation.

not entirely finished. Planned on adding a keypress event at some point, still working on DOMMouseScroll logic in the event handler, but forgot to update the comment. Fixed comment to match for now.

> on file: browser/base/content/browser-inspector.js line 392
> >   /**
> >    * Detach keypress, mousemove and scroll events to panels.
> >    */
> > 
> >   detachListeners: function PanelHighlighter_detachListeners()
> 
> comment does not match implementation

fixed.

> on file: browser/base/content/browser-inspector.js line 423
> >           InspectorUI.inspectNode(element); // # todo
> 
> what is todo about this?

rather than use the inspectNode() logic to position the panel, I want a separate method that is a little smarter about the way we highlight. Implementing a highlighter method to move and resize the panel would reduce flickering. As an alpha version, I think the current implementation is adequate though.

> on file: browser/base/content/browser-inspector.js line 877
> >       case "browser.inspector.enabled":
> >         let inspectorMenuItem = document.getElementById("menu_inspector");
> >         inspectorMenuItem.hidden = !gPrefService.getBoolPref("browser.inspector.enabled");
> >         break;
> 
> you will want to brace for this case statement to make sure your let is only
> valid for this case.

good call.

> on file: browser/base/content/browser-inspector.js line 881
> >       default: break;
> 
> nit: break on new line (or just omit default)

yanked.

> on file: browser/base/content/browser-inspector.js line 895
> >     if (this.selectEventsSuppressed)
> >       return false;
> 
> nit: you brace one line ifs everywhere else in this file

I've been seeing more unbraced single-line conditionals scattered around the code-base. Nevertheless, I think I just missed this one.

> on file: browser/base/content/browser-inspector.js line 901
> >     this.highlighter.highlightNode(node); // # todo scrolling causes issues
> 
> what kind of issues?  Can it be fixed in a follow-up bug?

Yep. Similar to the #todo in the mousemove method above, I'm seeing some flashing as the node is unhighlighted and rehighlighted. I need a new mechanism to move and resize the panel along with document scroll events.

> on file: browser/base/content/browser-inspector.js line 907
> >    * attach event listeners to content window and child windows to enable 
> >    * highlighting and click to stop inspection
> 
> nit: I'd prefer actual sentences with punctuation and stuff in java-doc style
> comments.

stuff added.

> on file: browser/base/content/browser.xul line 226
> >            onclick="InspectorUI.stopInspecting();" />
> 
> nit: we don't seem to do spaces before the closing slash in this file (at
> least in this area)

OK.

> on file: browser/base/content/browser.xul line 238
> >       <tree id="inspector-tree" class="plain" seltype="single" treelines="true"
> >         onselect="InspectorUI.onTreeSelected()" flex="1">
> 
> nit: onselect should line up with id per style of rest of file

K.

> on file: browser/base/content/browser.xul line 241
> >           <treecol id="colNodeName" label="nodeName" primary="true"
> >             persist="width,hidden,ordinal" flex="1"/>
> 
> nit: persists should line up with id (going to stop commenting on this issue)

gotcha.

> r- only because you have a bunch of todo's still.  ddahl should look over your
> tests, and I think I've been about as much help as I can be.  Your next step
> should be to finish up the todos, and then ask a browser peer to review this.

Well, hopefully I've addressed the todo's. This is a work-in-progress and there's definitely still work to do, but I'm hoping that can be resolved in separate bugs.

I'll post my revised patch after making sure this still compiles.

Thanks again for the reviews, Shawn!

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> this might be something we want to do down the road. For an alpha-release, I
> think we can use some hard-coded rules, but that's my opinion.
> 
> This is really a first-pass prototype and ultimate, the highlighter is going to
> look quite a bit different.
Then you should put in a TODO comment that references a bug number where this will be fixed, and it should be marked as blocking beta.

> added:
>     // If any of these edges are out-of-bounds, the node's rectangle is
>     // completely out-of-view and we can return
Hopefully with punctuation at the end ;)

> rather than use the inspectNode() logic to position the panel, I want a
> separate method that is a little smarter about the way we highlight.
> Implementing a highlighter method to move and resize the panel would reduce
> flickering. As an alpha version, I think the current implementation is adequate
> though.
For other things like this, please make sure you add TODO comments that reference a bug number then.

> I've been seeing more unbraced single-line conditionals scattered around the
> code-base. Nevertheless, I think I just missed this one.
That's because we used to not care, but then six or so months ago the code style guide was updated, and new code is supposed to follow it.  In old code, you should just try to be consistent (unless the module owner says otherwise).

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> I think this qualifies as an uber-nit.
See also: OCD

Comment 18

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

(In reply to comment #16)
> (In reply to comment #15)
> > this might be something we want to do down the road. For an alpha-release, I
> > think we can use some hard-coded rules, but that's my opinion.
> > 
> > This is really a first-pass prototype and ultimate, the highlighter is going to
> > look quite a bit different.
> Then you should put in a TODO comment that references a bug number where this
> will be fixed, and it should be marked as blocking beta.

Bit of a chicken-and-egg problem since this isn't checked in yet, it doesn't make a lot of sense to file bugs against it.

I think we might be able to do that by capturing the milestones doc from the project page and creating top-level bugs for each one of them. That's going to take a little while though.

How about a // TODO bugXXXXXX until those are created and then I can update?

> > added:
> >     // If any of these edges are out-of-bounds, the node's rectangle is
> >     // completely out-of-view and we can return
> Hopefully with punctuation at the end ;)

"Augh," imo.

> > rather than use the inspectNode() logic to position the panel, I want a
> > separate method that is a little smarter about the way we highlight.
> > Implementing a highlighter method to move and resize the panel would reduce
> > flickering. As an alpha version, I think the current implementation is adequate
> > though.
> For other things like this, please make sure you add TODO comments that
> reference a bug number then.

see above.

> > I've been seeing more unbraced single-line conditionals scattered around the
> > code-base. Nevertheless, I think I just missed this one.
> That's because we used to not care, but then six or so months ago the code
> style guide was updated, and new code is supposed to follow it.  In old code,
> you should just try to be consistent (unless the module owner says otherwise).

OK, will comply.

Comment 19

[Shawn Wilsher :sdwilsh]

(In reply to comment #18)
> Bit of a chicken-and-egg problem since this isn't checked in yet, it doesn't
> make a lot of sense to file bugs against it.
It is going to land though, right?  It's usually a prereq for checking stuff in that follow-up bugs are filed before hand (otherwise, people might forget/get distracted/etc).  Bugs are cheap, so I don't think it hurts to file a new bug for the issues (and mark them as dependent on this bug).  But, your browser peer will have the final say.

Comment 20

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

Attachment: 0.1e

Nits hopefully addressed, added better commentary for TODO items (additional bugs will follow to capture those as well as future milestone work).

Moving review to Gavin for some browser-peer consideration.

Comment 21

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

(In reply to comment #19)
> (In reply to comment #18)
> > Bit of a chicken-and-egg problem since this isn't checked in yet, it doesn't
> > make a lot of sense to file bugs against it.
> It is going to land though, right?  It's usually a prereq for checking stuff in
> that follow-up bugs are filed before hand (otherwise, people might forget/get
> distracted/etc).  Bugs are cheap, so I don't think it hurts to file a new bug
> for the issues (and mark them as dependent on this bug).  But, your browser
> peer will have the final say.

I'm hopeful we can land this. I'm in the process of filing these bugs now and will update the comments when they're done.

Comment 22

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

added:
bug 560829,
bug 560830,
bug 560831

Comment 23

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

Comment on attachment 440330 [details] [diff] [review]
0.1e

A lot of the _log() calls seem not so useful now that this is mostly running, so you should probably clean those up a bit.

>diff --git a/browser/base/content/browser-inspector.js b/browser/base/content/browser-inspector.js

Can we put the object definitions in a JavaScript module instead? We'd still need to have object instances per-window (as lazy getters, perhaps?), and have them keep references to the per-window elements they refer to, which would require some refactoring, but it seems like that shouldn't be too hard to manage?

Something like:
XPCOMUtils.defineLazyGetter(this, "InspectorUI", function () {
  let temp = {};
  Cu.import("resource:///modules/Inspector.jsm", temp);
  return new temp.InspectorUI(panelElement, treeElement, tabbrowser, <etc.>);
});

>+const INSPECTOR_INVISIBLE_ELEMENTS = {

In general, these kind of checks are best done using |instanceof HTML*Element| rather than localName comparisons, but that's more trouble and I suppose precision isn't particularly important here, so I guess this is fine.

>+PanelHighlighter.prototype = {

>+  highlightNode: function PanelHighlighter_highlightNode(anElement)

>+  highlightAndScrollNode: function PanelHighlighter_highlightAndScrollNode(aNode)

Can't highlightNode just take an optional aScroll parameter? Or even an optional "extra params" object parameter with a "scroll" property, to allow potentially adding more arguments in the future? e.g. highlightNode(node, {scroll: true});

>+   * Highlight the visible region of the node, if any.

>+  highlightVisibleRegion: function PanelHighlighter_highlightVisibleRegion(aRect)

"of the node" is a bit confusing here - it doesn't actually use this.node, right? Seems like it should just either always use this.rectForElement(this.node), or the description should be more generic ("highlights the region described by rect").

>+  rehighlight: function PanelHighlighter_rehighlight()
>+  {
>+    if (this.node) {
>+      this.unhighlight();
>+      this.highlight(false);

highlight() already calls unhighlight(), so this method seems redundant - can't onscroll just call highlight(false)?

>+  rectForElement: function PanelHighlighter_rectForElement(anElement)
>+  {
>+    let correct = function(a) { return Math.round(a); };
>+    let correctedRect = {

roundedRect and round() seem like better names (and you could just do "round = Math.round;", or even just use Math.round directly). Do you really want Math.round, though? Seems like it'd actually be best to floor() left/top, and ceil() right/bottom/width/height.

>+   * Scroll the given node bounded by the given rect into view if out-of-view.

>+  scrollIfOutOfBounds: function PanelHighlighter_scrollIfOutOfBounds(node, rect)

This is a bit of an odd signature. This function doesn't appear to be used in this patch; assuming it's needed, is there any reason not to just have it take the node, and use rectForElement() to get the rect? Or is there actually a usecase that involves using different rects?

>+  handleEvent: function PanelHighlighter_handleEvent(anEvent)

>+    switch (anEvent.type) {
>+      case "keypress": break;

unused?

>+          // TODO bugXXXXXX, highlighter panel jumping around on mousemove.

>+        // TODO need to implement this behavior, see bugXXXXXX.

don't forget to update these comments

>+      default: break;

just omit this?

>+var InspectorUI = {

>+  onToolbarButtonCommand: function InspectorUI_onToolbarButtonCommand(anEvent)

Is there any use in having this? Seems like the button could just call toggleInspectorUI directly (like the menuitem)?

>+  get isInspectorOn()

>+  get isPanelOpen()

Are these expected to be different at some later point? Can we just add the extra method when they are, rather than now?

>+  openTreePanel: function InspectorUI_openTreePanel()

>+    if (!this.isPanelOpen) {
>+      let bar = document.getElementById("status-bar");
>+      this._log("have panel, not open");
>+      this.treePanel.openPopup(bar, "overlap", 120, -120, false, false);
>+      this.treePanel.sizeTo(this.win.outerWidth / 8 * 7, this.win.outerHeight / 5);

Some constants here would probably be best (e.g. outerWidth * widthFactor, outerHeight * heightFactor)

>+  openInspectorUI: function InspectorUI_openInspectorUI()

>+    this.browser = gBrowser.selectedTab.linkedBrowser;

gBrowser.selectedBrowser

>+  startInspecting: function InspectorUI_startInspecting()

>+    this.attachPageListeners();
>+    this.highlighter.attachListeners();

Why does the highlighter have its own mousemove listener? I guess because it's possible to mouseover the highlighter panel to select a new node? 

>+  observe: function InspectorUI_observe(aSubject, aTopic, aDatum)

Does this pref really need to be live? Doesn't seem to me like the observer is worth the trouble.

>+  attachPageListeners: function InspectorUI_attachPageListeners()

>+    childWindowsDo(this.win, function(subWin) {
>+      self._log("childWindow: " + subWin);
>+      subWin.document.addEventListener("mouseover", self, true);
>+      subWin.document.addEventListener("click", self, true);
>+    });

Is it really necessary to add handlers to all windows? Does a capturing listener on the root window not work?

>+  detachPageListeners: function InspectorUI_detachPageListeners()

>+    childWindowsDo(this.win, function(subWin) {

Can't the set of windows have changed since the call to attachPageListeners? Seems like this would throw, if so. Do you actually need to remove the listeners?

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+          <treecol id="col@id" label="id" persist="width,hidden,ordinal"

is "@" valid in IDs? Seems kind of odd even if it is, is there a reason for that?

>diff --git a/browser/base/content/test/browser_inspector_treeSelection.js b/browser/base/content/test/browser_inspector_treeSelection.js

>+function createDocument()

>+  doc.body.addEventListener("DOMSubtreeModified", setupSelectionTests, false);
>+  doc.body.appendChild(div);

Why not just call setupSelectionTests() directly rather than relying on a mutation event listener?

These tests are a good start, but need to get rid of the timeouts before they land to avoid random oranges.

Looks like all the themeing stuff is also incomplete at this point - if you're looking to land this now, should probably just remove the button and do that in a followup?

Comment 24

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

(In reply to comment #23)
> Can we put the object definitions in a JavaScript module instead? We'd still
> need to have object instances per-window (as lazy getters, perhaps?), and have
> them keep references to the per-window elements they refer to, which would
> require some refactoring, but it seems like that shouldn't be too hard to
> manage?
> 
> Something like:
> XPCOMUtils.defineLazyGetter(this, "InspectorUI", function () {
>   let temp = {};
>   Cu.import("resource:///modules/Inspector.jsm", temp);
>   return new temp.InspectorUI(panelElement, treeElement, tabbrowser, <etc.>);
> });

since we were talking about this in IRC, it looks like we can consider this section optional. I think everybody thinks this is a good idea, we're just not sure how much time this is going to take. In light of that, I'm filing a follow-up bug to track this part.

filed bug 562168 for this issue.

Comment 25

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

(In reply to comment #23)
> (From update of attachment 440330 [details] [diff] [review])
> A lot of the _log() calls seem not so useful now that this is mostly running,
> so you should probably clean those up a bit.

Yeah, they're actually causing more problems than they're solving right now. Too much log chatter. I'll ditch 'em!

[module bit removed and addressed above]

> >+const INSPECTOR_INVISIBLE_ELEMENTS = {
> 
> In general, these kind of checks are best done using |instanceof HTML*Element|
> rather than localName comparisons, but that's more trouble and I suppose
> precision isn't particularly important here, so I guess this is fine.

This seemed easy and got the job done.

> >+PanelHighlighter.prototype = {
> 
> >+  highlightNode: function PanelHighlighter_highlightNode(anElement)
> 
> >+  highlightAndScrollNode: function PanelHighlighter_highlightAndScrollNode(aNode)
> 
> Can't highlightNode just take an optional aScroll parameter? Or even an
> optional "extra params" object parameter with a "scroll" property, to allow
> potentially adding more arguments in the future? e.g. highlightNode(node,
> {scroll: true});

Not sure what future options we'll need, but I've added a "params" object to the parameters.

I'd removed senders of highlightAndScroll after encountering some confusion with it. I think I'll add a specific action to the tree panel to scroll a node into view by double click or menu.

> >+   * Highlight the visible region of the node, if any.
> 
> >+  highlightVisibleRegion: function PanelHighlighter_highlightVisibleRegion(aRect)
> 
> "of the node" is a bit confusing here - it doesn't actually use this.node,
> right? Seems like it should just either always use
> this.rectForElement(this.node), or the description should be more generic
> ("highlights the region described by rect").

That's true! I think I forgot to update the comment after changing the parameters around. Turned out, I wasn't interested in the node by the time I got into this method. Added "under aRect" to the comment in the hopes of clarifying.

> >+  rehighlight: function PanelHighlighter_rehighlight()
> >+  {
> >+    if (this.node) {
> >+      this.unhighlight();
> >+      this.highlight(false);
> 
> highlight() already calls unhighlight(), so this method seems redundant - can't
> onscroll just call highlight(false)?

Right you are. I think I missed that one.

> >+  rectForElement: function PanelHighlighter_rectForElement(anElement)
> >+  {
> >+    let correct = function(a) { return Math.round(a); };
> >+    let correctedRect = {
> 
> roundedRect and round() seem like better names (and you could just do "round =
> Math.round;", or even just use Math.round directly). Do you really want
> Math.round, though? Seems like it'd actually be best to floor() left/top, and
> ceil() right/bottom/width/height.

I had a problem with negative values. I was using floors initially, but that was increasing the negative rather than dropping it closer to zero. I was doing some additional stuff originally too (using a Math.abs to drop negatives altogether, but that was giving funny results).

There's still a bug in here somewhere (fired from mousemoves during highlighting), but roundedRect sounds like an improvement over corrected.

(and that was probably more back-story than you really cared about) :)

> >+   * Scroll the given node bounded by the given rect into view if out-of-view.
> 
> >+  scrollIfOutOfBounds: function PanelHighlighter_scrollIfOutOfBounds(node, rect)
> 
> This is a bit of an odd signature. This function doesn't appear to be used in
> this patch; assuming it's needed, is there any reason not to just have it take
> the node, and use rectForElement() to get the rect? Or is there actually a
> usecase that involves using different rects?

I *think* the origin of this was to try to prevent scrolling if the node's rect was within view. Otherwise, it would jump the display up to the node's top no matter what. I stopped using auto-scrolling in my highlighting and stopped using this method though. I can probably lose it.

> >+  handleEvent: function PanelHighlighter_handleEvent(anEvent)
> 
> >+    switch (anEvent.type) {
> >+      case "keypress": break;
> 
> unused?

yep. I plan on implementing a key handler along with the remains of Milestone 0.4. Should probably add a "todo" comment.

> >+          // TODO bugXXXXXX, highlighter panel jumping around on mousemove.
> 
> >+        // TODO need to implement this behavior, see bugXXXXXX.
> 
> don't forget to update these comments

already done with the latest updates to my user-repo. 

> >+      default: break;
> 
> just omit this?

done.

> >+var InspectorUI = {
> 
> >+  onToolbarButtonCommand: function InspectorUI_onToolbarButtonCommand(anEvent)
> 
> Is there any use in having this? Seems like the button could just call
> toggleInspectorUI directly (like the menuitem)?

Yeah, I wasn't sure if I would need to do anything differently in the two cases. Turns out I don't! Gonzo!

> >+  get isInspectorOn()
> 
> >+  get isPanelOpen()
> 
> Are these expected to be different at some later point? Can we just add the
> extra method when they are, rather than now?

Yup. Done!

> >+  openTreePanel: function InspectorUI_openTreePanel()
> 
> >+    if (!this.isPanelOpen) {
> >+      let bar = document.getElementById("status-bar");
> >+      this._log("have panel, not open");
> >+      this.treePanel.openPopup(bar, "overlap", 120, -120, false, false);
> >+      this.treePanel.sizeTo(this.win.outerWidth / 8 * 7, this.win.outerHeight / 5);
> 
> Some constants here would probably be best (e.g. outerWidth * widthFactor,
> outerHeight * heightFactor)

What if the window changes size? I'm not sure if we have any windows or panels in our UI that change based on the browser size. I'd be happy with constant sizes, but that seems arbitrary. Some useful fraction of main browser size seems like a good idea, but what ratios to use? I'm certainly open to suggestions for these.

> >+  openInspectorUI: function InspectorUI_openInspectorUI()
> 
> >+    this.browser = gBrowser.selectedTab.linkedBrowser;
> 
> gBrowser.selectedBrowser

oop, ok!

> >+  startInspecting: function InspectorUI_startInspecting()
> 
> >+    this.attachPageListeners();
> >+    this.highlighter.attachListeners();
> 
> Why does the highlighter have its own mousemove listener? I guess because it's
> possible to mouseover the highlighter panel to select a new node?

That's the idea. Currently it's a little flippy. There seems to be some offset at work moving my highlighter down and to the right. I have bug 560829 filed to cover that.

> >+  observe: function InspectorUI_observe(aSubject, aTopic, aDatum)
> 
> Does this pref really need to be live? Doesn't seem to me like the observer is
> worth the trouble.

I'm preffing out the menu option so if the pref weren't observed, we'd never be able to access the inspector without manually setting the pref and restarting. It may be overkill, but it's nice. :)

> >+  attachPageListeners: function InspectorUI_attachPageListeners()
> 
> >+    childWindowsDo(this.win, function(subWin) {
> >+      self._log("childWindow: " + subWin);
> >+      subWin.document.addEventListener("mouseover", self, true);
> >+      subWin.document.addEventListener("click", self, true);
> >+    });
> 
> Is it really necessary to add handlers to all windows? Does a capturing
> listener on the root window not work?

I'm not sure it works in the case of framesets. It probably works with iframes. I know Firebug uses a similar mechanism to handle mouseovers in subdocuments so I figured I'd do the same rather than try to reinvent the wheel in a tricky area.

> >+  detachPageListeners: function InspectorUI_detachPageListeners()
> 
> >+    childWindowsDo(this.win, function(subWin) {
> 
> Can't the set of windows have changed since the call to attachPageListeners?
> Seems like this would throw, if so. Do you actually need to remove the
> listeners?

If I don't remove them, subsequent mouse-moves within the documents will cause the highlighter to jump around to a new target.

Can the set of windows change? I suppose they could if some script added an iframe with a src attribute. I bet there's an interesting test case in there. :)

> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> 
> >+          <treecol id="col@id" label="id" persist="width,hidden,ordinal"
> 
> is "@" valid in IDs? Seems kind of odd even if it is, is there a reason for
> that?

Funny thing about the inDOMView: it uses the id attribute of the columns to do a lookup!

see: http://mxr.mozilla.org/mozilla-central/source/layout/inspector/src/inDOMView.cpp#449

This is a temporary solution. In Milestone 0.5 we're (and by we I mean I) am going to come up with a better tree view.

> >diff --git a/browser/base/content/test/browser_inspector_treeSelection.js b/browser/base/content/test/browser_inspector_treeSelection.js
> 
> >+function createDocument()
> 
> >+  doc.body.addEventListener("DOMSubtreeModified", setupSelectionTests, false);
> >+  doc.body.appendChild(div);
> 
> Why not just call setupSelectionTests() directly rather than relying on a
> mutation event listener?

I wanted to avoid a timeout.

> These tests are a good start, but need to get rid of the timeouts before they
> land to avoid random oranges.

right. This is the tricky bit. I needed the timeouts to get these tests working initially and knew I could do this to do it. Maybe if you're around for a bit tomorrow you can point me in the right direction to get these taken care of.

I tried using felipe's method with generators initially, but I didn't have much luck. I might have better luck this time now that the tests are working with timeouts. The conversion might be easier.

http://felipe.wordpress.com/2010/03/21/writing-async-tests/

> Looks like all the themeing stuff is also incomplete at this point - if you're
> looking to land this now, should probably just remove the button and do that in
> a followup?

Yeah, I got the obvious chunk for the highlighter panel into the "root" browser.css for now (in a user-repo landed patch), but there's going to be more general theming work in the future.

thanks!

Comment 26

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

btw, you can see the first pass changes here:

http://hg.mozilla.org/users/rcampbell_mozilla.com/mozilla-inspector/rev/239adcb7400b

First part of the theming bits are here:

http://hg.mozilla.org/users/rcampbell_mozilla.com/mozilla-inspector/rev/1708bd26b1a0

Comment 27

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

and an additional changeset for mostly-eliminated logging:

http://hg.mozilla.org/users/rcampbell_mozilla.com/mozilla-inspector/rev/e5e8d6a379a9

Comment 28

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

Attachment: review-cleanup

addresses first set of review comments from previous patch.

Comment 29

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

Attachment: review-cleanup-1

diffed the wrong way, correcting.

Comment 30

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

created a new try build, located at:

https://build.mozilla.org/tryserver-builds/[e-mail]-1272575233/

to enable the Inspect menu, set browser.inspector.enabled to true in about:config.

Comment 31

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

Attachment: browser-inspector-init-test

removed timeouts from browser_inspector_initialization.js

Comment 32

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

Attachment: browser-inspector-highlighter-test

modified highlighter test to get rid of timeouts, added extra test to verify highlighter panel is over correct position.

Comment 33

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

Attachment: browser-inspector-stylePanel

browser-inspector-stylePanel test file de-timeouted and generatorized.

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 440330 [details] [diff] [review]
0.1e

> anEvent

The "a" stands for "argument", afaik. So either aEvent or just event.

Please don't declare childWindowsDo and cancelEvent in the global scope.

Comment 35

[Dão Gottwald [:dao]]

(In reply to comment #8)
> > function childWindowsDo(aWindow, aFunction) {
> 
> nit: opening brace on new line

Err, no...

> >     if (this.node)
> >       this.shell.repaintElement(this.node);
> 
> nit: one line ifs are supposed to be braced

Not needed in browser.

Comment 36

[Dão Gottwald [:dao]]

Comment on attachment 442482 [details] [diff] [review]
review-cleanup-1

>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -704,7 +704,7 @@
>       <toolbarbutton id="inspect-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>                      label="&inspectButton.label;"
>                      tooltiptext="&inspectButton.tooltip;"
>-                     oncommand="InspectorUI.onToolbarButtonCommand();"/>
>+                     oncommand="InspectorUI.toggleInspectorUI();"/>

command="Tools:Inspect"

Comment 37

[Dão Gottwald [:dao]]

Comment on attachment 440330 [details] [diff] [review]
0.1e

>--- a/browser/themes/gnomestripe/browser/browser.css
>+++ b/browser/themes/gnomestripe/browser/browser.css
>@@ -551,6 +551,12 @@
>   list-style-image: url("moz-icon://stock/gtk-stop?size=toolbar&state=disabled");
> }
> 
>+#inspect-button {
>+  list-style-image: url("moz-icon://stock/gtk-stop?size=toolbar");
>+}
>+#inspect-button[disabled="true"] {
>+  list-style-image: url("moz-icon://stock/gtk-stop?size=toolbar&state=disabled");
>+

Lacks the closing }.
Is this button ever going to be disabled?

Comment 38

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

(In reply to comment #34)
> (From update of attachment 440330 [details] [diff] [review])
> > anEvent
> 
> The "a" stands for "argument", afaik. So either aEvent or just event.
> 
> Please don't declare childWindowsDo and cancelEvent in the global scope.

fixed in http://hg.mozilla.org/users/rcampbell_mozilla.com/mozilla-inspector/rev/4afa6c042ab5 …

Comment 39

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

(In reply to comment #36)
> (From update of attachment 442482 [details] [diff] [review])
> >--- a/browser/base/content/browser.xul
> >+++ b/browser/base/content/browser.xul
> >@@ -704,7 +704,7 @@
> >       <toolbarbutton id="inspect-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
> >                      label="&inspectButton.label;"
> >                      tooltiptext="&inspectButton.tooltip;"
> >-                     oncommand="InspectorUI.onToolbarButtonCommand();"/>
> >+                     oncommand="InspectorUI.toggleInspectorUI();"/>
> 
> command="Tools:Inspect"

fixed in: http://hg.mozilla.org/users/rcampbell_mozilla.com/mozilla-inspector/rev/4d5a2203e5dd

Comment 40

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

(In reply to comment #37)
> (From update of attachment 440330 [details] [diff] [review])
> >--- a/browser/themes/gnomestripe/browser/browser.css
> >+++ b/browser/themes/gnomestripe/browser/browser.css
> >@@ -551,6 +551,12 @@
> >   list-style-image: url("moz-icon://stock/gtk-stop?size=toolbar&state=disabled");
> > }
> > 
> >+#inspect-button {
> >+  list-style-image: url("moz-icon://stock/gtk-stop?size=toolbar");
> >+}
> >+#inspect-button[disabled="true"] {
> >+  list-style-image: url("moz-icon://stock/gtk-stop?size=toolbar&state=disabled");
> >+
> 
> Lacks the closing }.

Good catch! Fixed in http://hg.mozilla.org/users/rcampbell_mozilla.com/mozilla-inspector/rev/00b44641cc2c

> Is this button ever going to be disabled?

There's a pref in there now (browser.inspector.enabled) which enables the Tools menu item. I was thinking this might wire up to the button at some point as well. Probably optional, however.

Thanks for the drive-by review, Dão!

Comment 41

[Shawn Wilsher :sdwilsh]

(In reply to comment #35)
> > nit: opening brace on new line
> Err, no...
> > nit: one line ifs are supposed to be braced
> Not needed in browser.
Er, yes per style guide.  It doesn't say anything about exceptions for browser, but it does say that new code should be following it.

Comment 42

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

Attachment: inspector-addendum

incorporating Dão's review comments above.

Comment 43

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

Attachment: fewer-tree-columns+attributes

slightly-improved tree view.

Comment 44

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

(In reply to comment #25)
> Added "under aRect" to the comment in the hopes of clarifying.

I find the mention of "node" at all to be confusing. Seems like it should be:

"Highlights the visible portion of aRect, if any.", or something like that.

> > >+  rectForElement: function PanelHighlighter_rectForElement(anElement)

> I had a problem with negative values. I was using floors initially, but that
> was increasing the negative rather than dropping it closer to zero.

Why is that a problem? You should expect negative values for elements scrolled off the top of the screen, right? You really want rects to expand when adjusting them to keep things consistent - if that causes problems, its likely with the consumer's expectations (or you're using the wrong rects or coordinate space, etc.).

> > >+      this.treePanel.sizeTo(this.win.outerWidth / 8 * 7, this.win.outerHeight / 5);
> > 
> > Some constants here would probably be best (e.g. outerWidth * widthFactor,
> > outerHeight * heightFactor)
> 
> What if the window changes size?

All I meant was to use e.g. |const widthFactor = 1 / (8*7);|, so that there was a named variable, rather than just magic numbers.

> I'm preffing out the menu option so if the pref weren't observed, we'd never be
> able to access the inspector without manually setting the pref and restarting.
> It may be overkill, but it's nice. :)

What's the user-relevant use case for dynamically flipping the pref during run time? Why do we even have the pref?

> > Is it really necessary to add handlers to all windows? Does a capturing
> > listener on the root window not work?
> 
> I'm not sure it works in the case of framesets. It probably works with iframes.
> I know Firebug uses a similar mechanism to handle mouseovers in subdocuments so
> I figured I'd do the same rather than try to reinvent the wheel in a tricky
> area.

The mechanism seems error prone, though, so it'd be nice to do something smarter. Maybe Neil has some ideas?

> Can the set of windows change? I suppose they could if some script added an
> iframe with a src attribute. I bet there's an interesting test case in there.
> :)

This seems like something this code needs to deal with, at the very least.

> > >diff --git a/browser/base/content/test/browser_inspector_treeSelection.js b/browser/base/content/test/browser_inspector_treeSelection.js
> > 
> > >+function createDocument()
> > 
> > >+  doc.body.addEventListener("DOMSubtreeModified", setupSelectionTests, false);
> > >+  doc.body.appendChild(div);
> > 
> > Why not just call setupSelectionTests() directly rather than relying on a
> > mutation event listener?
> 
> I wanted to avoid a timeout.

Not sure I understand - I was suggesting changing those to lines to just:

doc.body.appendChild(div);
setupSelectionTests();

Comment 45

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

(In reply to comment #44)
> (In reply to comment #25)
> > Added "under aRect" to the comment in the hopes of clarifying.
> 
> I find the mention of "node" at all to be confusing. Seems like it should be:
> 
> "Highlights the visible portion of aRect, if any.", or something like that.

Ok, will do. The comment predated the current version of the method.

> > > >+  rectForElement: function PanelHighlighter_rectForElement(anElement)
> 
> > I had a problem with negative values. I was using floors initially, but that
> > was increasing the negative rather than dropping it closer to zero.
> 
> Why is that a problem? You should expect negative values for elements scrolled
> off the top of the screen, right? You really want rects to expand when
> adjusting them to keep things consistent - if that causes problems, its likely
> with the consumer's expectations (or you're using the wrong rects or coordinate
> space, etc.).

This is getting further from my usable memory. I found using round worked better than floor. I can change 'em and see if that changes the behavior. I expect it'll just work now as the rest of the boundary checking has gotten better.

> > > >+      this.treePanel.sizeTo(this.win.outerWidth / 8 * 7, this.win.outerHeight / 5);
> > > 
> > > Some constants here would probably be best (e.g. outerWidth * widthFactor,
> > > outerHeight * heightFactor)
> > 
> > What if the window changes size?
> 
> All I meant was to use e.g. |const widthFactor = 1 / (8*7);|, so that there was
> a named variable, rather than just magic numbers.

ah, I get you. I'll add this.

> > I'm preffing out the menu option so if the pref weren't observed, we'd never be
> > able to access the inspector without manually setting the pref and restarting.
> > It may be overkill, but it's nice. :)
> 
> What's the user-relevant use case for dynamically flipping the pref during run
> time? Why do we even have the pref?

I was asked to add it so we could leave it off by default.

> > > Is it really necessary to add handlers to all windows? Does a capturing
> > > listener on the root window not work?
> > 
> > I'm not sure it works in the case of framesets. It probably works with iframes.
> > I know Firebug uses a similar mechanism to handle mouseovers in subdocuments so
> > I figured I'd do the same rather than try to reinvent the wheel in a tricky
> > area.
> 
> The mechanism seems error prone, though, so it'd be nice to do something
> smarter. Maybe Neil has some ideas?

Not sure. I can ask him.

This seems to work well. Why do you think it's error-prone?

> > Can the set of windows change? I suppose they could if some script added an
> > iframe with a src attribute. I bet there's an interesting test case in there.
> > :)
> 
> This seems like something this code needs to deal with, at the very least.

Any suggestions on how to handle that? Add a mutation listener and walk the dom looking for new child documents? I could try something dumber that just disables live inspection if a mutation occurs forcing the user to turn it back on.

I'm open to suggestions.

> > > >diff --git a/browser/base/content/test/browser_inspector_treeSelection.js b/browser/base/content/test/browser_inspector_treeSelection.js
> > > 
> > > >+function createDocument()
> > > 
> > > >+  doc.body.addEventListener("DOMSubtreeModified", setupSelectionTests, false);
> > > >+  doc.body.appendChild(div);
> > > 
> > > Why not just call setupSelectionTests() directly rather than relying on a
> > > mutation event listener?
> > 
> > I wanted to avoid a timeout.
> 
> Not sure I understand - I was suggesting changing those to lines to just:
> 
> doc.body.appendChild(div);
> setupSelectionTests();

Pretty sure when I tried that initially, setupSelectionTests() was running before the appendChild finished laying out the document. I can try it again though.

Comment 46

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

Attachment: inspector-addendum-2

added consts, removed rectForElement entirely, updated browser_inspection_treeSelection.js test, removing setTimeout.

Comment 47

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

Attachment: remove-childWindowsDo-cancelEvent

Removed the recursive listeners gunk in favor of a simpler XUL browser mousemove event. So far it's working well, but we might need some additional logic to descend into iframes.

Comment 48

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

Comment on attachment 442741 [details] [diff] [review]
browser-inspector-stylePanel

(I think this belongs in bug 560692)

Comment 49

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

(In reply to comment #48)
> (From update of attachment 442741 [details] [diff] [review])
> (I think this belongs in bug 560692)

yeah, I misposted that here after the test code cleanup. I will move!

Comment 50

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

Attachment: additional TabSelect patch

I've rebased the patches attached to this bug onto trunk, and added this patch, which is just a port of http://hg.mozilla.org/users/rcampbell_mozilla.com/mozilla-inspector/rev/1fdc76aea35c with a tweak (only listen for TabSelect since it's also fired for close/open) and some de-logging changes rolled in.

I'll attach my rollup patch that includes all of these, then review that and then we can get it landed and rebase the patches in bug 560692 / bug 561782.

Comment 51

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

Attachment: merged patch

Comment 52

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

awesome! Thanks. :)

Comment 53

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

one other change I did downstream was rename the InspectorUI prefix as IUI because it was getting a little long on some of the method signatures. Otherwise, this looks at first glance.

Comment 54

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

Comment on attachment 444709 [details] [diff] [review]
merged patch

Can you rename browser-inspector to just inspector.js? The "browser-" prefixes are kind redundant and I'd like to avoid adding more files like that (and get rid of the other existing ones, eventually).

I think we should get rid of the pref code - I don't think there's much point in landing this disabled.

The menu item currently acts like a toggle, but it isn't visibly so - seems you should update Tools:Inspect's "checked" attribute in toggleInspectorUI(), and make menu_inspector a type="checkbox"?

diff --git a/browser/base/content/browser-inspector.js b/browser/base/content/browser-inspector.js

>+ * Contributor(s):
>+ *   Rob Campbell <[e-mail]>

should probably add an (original author) to this (and other license headers)

Is the treeview code (or anything else?) taken from Firebug? Should also include its author if so.

>+function PanelHighlighter(aBrowser, aColor, aBorderSize, anOpacity)

>+  this.browser = aBrowser;
>+  this.win = this.browser.contentWindow;

Would probably be good to explcitly release these references somewhere (called by stopInspecting?).

>+PanelHighlighter.prototype = {

>+  highlight: function PanelHighlighter_highlight(scroll)

This doesn't handle partially offscreen windows very well, I think because the panel is forced to be on-screen. Wonder if there's some kind of way to indicate that it should be allowed to be offscreen (ask Enn?)... Otherwise you'll have to clip manually, I guess. Can be done in a followup.

>+    // node is not set or node is not highlightable, bail
>+    if (!this.node || !this.isNodeHighlightable()) {
>+      return;

Might make sense to put the this.node null check in isNodeHighlightable?

>+  /**
>+   * Highlight the given node.
>+   *
>+   * @param element
>+   *        a DOM element to be highlighted
>+   * @param params
>+   *        extra paramaters object

nit: parameters

>+  highlightNode: function PanelHighlighter_highlightNode(element, params)

>+    if (params && params.scroll) {
>+      this.highlight(true)
>+    } else {
>+      this.highlight();
>+    }

can just be this.highlight(params && params.scroll);

>+  /**
>+   * Highlight the visible region of the region described by aRect, if any.
>+   *
>+   * @param aRect
>+   *        A rectangle representing this.node's region

Unnecessary/misleading reference to this.node - I'd just remove these lines, aRect's purpose is obvious enough from the previous sentence.

>+  rehighlight: function PanelHighlighter_rehighlight()

Should get rid of this and have its callers just call highlight() directly.

>+   * Does the current scroll position contain the given rectangle object?

AFAICT this method doesn't take scrolling into account at all - it just checks that aRect fits within a window-sized rect anchored at (0,0). Since you're using it with rects obtained from getBoundingClientRect() - which also doesn't take scrolling into account - this works fine, but the comment is a bit confusing. Perhaps: "Returns true if the given viewport-relative rect is within the visible area of the window."

>+  attachListeners: function PanelHighlighter_attachListeners()

>+    let self = this;
>+    this.panel.addEventListener("mousemove", self, false);
>+    this.panel.addEventListener("DOMMouseScroll", self, false);

no point using "self" here since there's no closure.

>+  handleEvent: function PanelHighlighter_handleEvent(event)

>+    switch (event.type) {
>+      case "keypress": break;

remove

>+      case "mousemove":
>+        element = this.win.document.elementFromPoint(event.clientX,
>+            event.clientY);
>+        if (element && element != this.node) {
>+          // TODO bugXXXXXX, highlighter panel jumping around on mousemove.
>+          InspectorUI.inspectNode(element);

The mousemove events in this case are in the panel's coordinate space, which means they're chromewindow-relative as opposed to viewport-relative, so this results in the wrong elements being highlighted (offset by the height of the browser chrome). You need to do coordinate adjusting here similar to what you do in highlitNode() - a generic method to convert between the two coordinate spaces might be useful.

>+function InspectorTreeView()

>+InspectorTreeView.prototype = {

>+  initialize: function ITV_init(aWindow)

Is there a good reason to have this method separate rather than just including it in the constructor?

>+  destroy: function ITV_destroy()

>+var InspectorUI = {

>+  destroy: function InspectorUI_destroy()

Neither of these destroy() methods seem to be called...

>+  onToolbarButtonCommand: function InspectorUI_onToolbarButtonCommand(event)
>+  {
>+    this.toggleInspectorUI(event);
>+  },

still need to get rid of this.

>+  closeInspectorUI: function InspectorUI_closeInspectorUI()
>+  {
>+    this.win.document.removeEventListener("scroll", this, false);
>+    gBrowser.tabContainer.removeEventListener("TabSelect", this, false);
>+    if (this.highlighter && this.highlighter.isHighlighting) {
>+      this.highlighter.unhighlight();
>+      this.stopInspecting();
>+    }

Seems to me like you should stopInspecting unconditionally?

Probably worth dropping the references to browser/win here just to be on the safe side.

>+  handleEvent: function InspectorUI_handleEvent(event)

>+      case "keypress":
>+        switch (event.keyCode) {
>+          case KeyEvent.DOM_VK_RETURN:
>+            this.stopInspecting();
>+            break;
>+          case KeyEvent.DOM_VK_ESCAPE:
>+            this.stopInspecting();
>+            break;

could collapsed these

>+      case "mousemove":

>+        if (element && element != this.node) {
>+          InspectorUI.inspectNode(element);

this.inspectNode()

>+  panelShown: function InspectorUI_panelShown(event)
>+  {
>+    // # todo
>+  },

remove for now (also caller)?

diff --git a/browser/base/content/browser-menubar.inc b/browser/base/content/browser-menubar.inc

>+              <menuitem id="menu_inspector"

>+                        commandkey="&inspectMenu.commandkey;"

commandkey attribute doesn't exist.

>+                        command="Tools:Inspect"/>

diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+    <panel id="highlighter-panel"
>+           ignorekeys="true"
>+           noautofocus="true"
>+           noautohide="true"
>+           onclick="InspectorUI.stopInspecting();"/>

Seems like the event handlers should be consolidated - I think I'd actually prefer them to be specified here rather than added/removed manually in attachListeners/detachListeners.

>+    <panel id="inspector-panel"

>+           left="100"
>+           top="600"

Is there any point in having these given that the panel is always positioned on open?

These panels should probably both be hidden="true" by default - prevents unnecessary initialization until they're used (txul/ts have been sensitive to adding non-hidden panels in the past).

>+      <toolbarbutton id="inspect-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>+                     label="&inspectButton.label;"
>+                     tooltiptext="&inspectButton.tooltip;"
>+                     command="Tools:Inspect"/>

diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY inspectButton.label          "Inspect">
>+<!ENTITY inspectButton.tooltip        "Inspect web page">

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

Can we move these toolbarbutton changes to a followup bug? I don't really want to land with the theming as-is, and don't think we need to block on this.

diff --git a/browser/base/content/test/browser_inspector_highlighter.js b/browser/base/content/test/browser_inspector_highlighter.js

>+function createDocument()

>+  doc.body.addEventListener("DOMSubtreeModified", setupHighlighterTests, false);
>+  doc.body.appendChild(div);
>+  doc.body.appendChild(div2);

This really shouldn't be necessary - appendChild is synchronous, so you should really just be able to call setupHighlighterTests() directly here. If that doesn't work, need to figure out why.

>+function performTestComparisons(evt)

>+  setTimeout(finishUp, 0);

executeSoon()

diff --git a/browser/base/content/test/browser_inspector_initialization.js b/browser/base/content/test/browser_inspector_initialization.js

>+function runInspectorTests(evt)

>+  ok(InspectorUI.isStylePanelOpen, "Inspector Style Panel is open");
>+  ok(InspectorUI.isDOMPanelOpen, "Inspector DOM Panel is open");

need to todo() these before landing obviously...

diff --git a/browser/base/content/test/browser_inspector_treeSelection.js b/browser/base/content/test/browser_inspector_treeSelection.js

>+function createDocument()

>+  // doc.body.addEventListener("DOMSubtreeModified", , false);
>+  doc.body.appendChild(div);
>+  setupSelectionTests();

exactly! :)

>+function setupSelectionTests()

>+  /* # todo attach to panelShown event */
>+  setTimeout(runSelectionTests, 200);

This this needs to be fixed before landing.

r- overall just because I want to take a quick look at the patch that addresses these, but we should be able to get this landed soon after that I think.

Comment 55

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

(In reply to comment #54)
> (From update of attachment 444709 [details] [diff] [review])
> Can you rename browser-inspector to just inspector.js? The "browser-" prefixes
> are kind redundant and I'd like to avoid adding more files like that (and get
> rid of the other existing ones, eventually).

done.

> I think we should get rid of the pref code - I don't think there's much point
> in landing this disabled.

OK! Pulled out the initialization code from browser.js and the observe() method from InspectorUI.

> The menu item currently acts like a toggle, but it isn't visibly so - seems you
> should update Tools:Inspect's "checked" attribute in toggleInspectorUI(), and
> make menu_inspector a type="checkbox"?

Ok, done. Removed the hidden attribute on the menu item as well to make it visible by default.

> diff --git a/browser/base/content/browser-inspector.js
> b/browser/base/content/browser-inspector.js
> 
> >+ * Contributor(s):
> >+ *   Rob Campbell <[e-mail]>
> 
> should probably add an (original author) to this (and other license headers)

added (original author). Not sure what to do with "and other license headers". Can't find an example in the project.

What's this for?

> Is the treeview code (or anything else?) taken from Firebug? Should also
> include its author if so.

Not in this patch. In some subsequent patches I've got some stuff borrowed from Firebug.

I looked at DOM Inspector early on for some clues on how to get the inDOMView stuff working, but the code's mine.

> >+function PanelHighlighter(aBrowser, aColor, aBorderSize, anOpacity)
> 
> >+  this.browser = aBrowser;
> >+  this.win = this.browser.contentWindow;
> 
> Would probably be good to explcitly release these references somewhere (called
> by stopInspecting?).

stopInspecting doesn't kill the UI though, and we still need those references if the user clicks around in the treeview area.

Maybe in closeInspectorUI()?

> >+PanelHighlighter.prototype = {
> 
> >+  highlight: function PanelHighlighter_highlight(scroll)
> 
> This doesn't handle partially offscreen windows very well, I think because the
> panel is forced to be on-screen. Wonder if there's some kind of way to indicate
> that it should be allowed to be offscreen (ask Enn?)... Otherwise you'll have
> to clip manually, I guess. Can be done in a followup.

added bug 565301, left a TODO comment in the code.

> >+    // node is not set or node is not highlightable, bail
> >+    if (!this.node || !this.isNodeHighlightable()) {
> >+      return;
> 
> Might make sense to put the this.node null check in isNodeHighlightable?

Outstanding suggestion! Done.

> >+  /**
> >+   * Highlight the given node.
> >+   *
> >+   * @param element
> >+   *        a DOM element to be highlighted
> >+   * @param params
> >+   *        extra paramaters object
> 
> nit: parameters

Ouch. Thank you, sir.

> >+  highlightNode: function PanelHighlighter_highlightNode(element, params)
> 
> >+    if (params && params.scroll) {
> >+      this.highlight(true)
> >+    } else {
> >+      this.highlight();
> >+    }
> 
> can just be this.highlight(params && params.scroll);

Yes, I suppose it can be. DONE!

> >+  /**
> >+   * Highlight the visible region of the region described by aRect, if any.
> >+   *
> >+   * @param aRect
> >+   *        A rectangle representing this.node's region
> 
> Unnecessary/misleading reference to this.node - I'd just remove these lines,
> aRect's purpose is obvious enough from the previous sentence.

Good enough. I left in the | @param aRect | mention though for consistency's sake.

> >+  rehighlight: function PanelHighlighter_rehighlight()
> 
> Should get rid of this and have its callers just call highlight() directly.

yes, I suppose so. Good catch.

The reasoning for the separate method was to use a different mechanism on document scrolling. That can come later.

> >+   * Does the current scroll position contain the given rectangle object?
> 
> AFAICT this method doesn't take scrolling into account at all - it just checks
> that aRect fits within a window-sized rect anchored at (0,0). Since you're
> using it with rects obtained from getBoundingClientRect() - which also doesn't
> take scrolling into account - this works fine, but the comment is a bit
> confusing. Perhaps: "Returns true if the given viewport-relative rect is within
> the visible area of the window."

You're right, of course. I did some dabbling with scrollIntoView and a few other rectangular gymnastics during the early days with these methods. Some of the comments drifted a bit from their original design.

Replaced.

> >+  attachListeners: function PanelHighlighter_attachListeners()
> 
> >+    let self = this;
> >+    this.panel.addEventListener("mousemove", self, false);
> >+    this.panel.addEventListener("DOMMouseScroll", self, false);
> 
> no point using "self" here since there's no closure.

ah, right again. I think I missed that one.

> >+  handleEvent: function PanelHighlighter_handleEvent(event)
> 
> >+    switch (event.type) {
> >+      case "keypress": break;
> 
> remove

OK.

> >+      case "mousemove":
> >+        element = this.win.document.elementFromPoint(event.clientX,
> >+            event.clientY);
> >+        if (element && element != this.node) {
> >+          // TODO bugXXXXXX, highlighter panel jumping around on mousemove.
> >+          InspectorUI.inspectNode(element);
> 
> The mousemove events in this case are in the panel's coordinate space, which
> means they're chromewindow-relative as opposed to viewport-relative, so this
> results in the wrong elements being highlighted (offset by the height of the
> browser chrome). You need to do coordinate adjusting here similar to what you
> do in highlitNode() - a generic method to convert between the two coordinate
> spaces might be useful.

I added a fix for this in follow-up bug 560829.

I didn't create a generic method for this since it's only being used in the one place. If it comes up again, I have the method for it.

> >+function InspectorTreeView()
> 
> >+InspectorTreeView.prototype = {
> 
> >+  initialize: function ITV_init(aWindow)
> 
> Is there a good reason to have this method separate rather than just including
> it in the constructor?

Probably not. Gone.

> >+  destroy: function ITV_destroy()
> 
> >+var InspectorUI = {
> 
> >+  destroy: function InspectorUI_destroy()
> 
> Neither of these destroy() methods seem to be called...

Added a call to ITV's destroy() method, removed InspectorUI's.

> >+  onToolbarButtonCommand: function InspectorUI_onToolbarButtonCommand(event)
> >+  {
> >+    this.toggleInspectorUI(event);
> >+  },
> 
> still need to get rid of this.

That goes away in a subsequent patch. I can remove it here and catch the other one when I rebase them.

> >+  closeInspectorUI: function InspectorUI_closeInspectorUI()
> >+  {
> >+    this.win.document.removeEventListener("scroll", this, false);
> >+    gBrowser.tabContainer.removeEventListener("TabSelect", this, false);
> >+    if (this.highlighter && this.highlighter.isHighlighting) {
> >+      this.highlighter.unhighlight();
> >+      this.stopInspecting();
> >+    }
> 
> Seems to me like you should stopInspecting unconditionally?

Yes, I did this in another patch downstream. I'll bring that change in here and fix the other when rebasing.

Too many patches!

> Probably worth dropping the references to browser/win here just to be on the
> safe side.

Done above.

> >+  handleEvent: function InspectorUI_handleEvent(event)
> 
> >+      case "keypress":
> >+        switch (event.keyCode) {
> >+          case KeyEvent.DOM_VK_RETURN:
> >+            this.stopInspecting();
> >+            break;
> >+          case KeyEvent.DOM_VK_ESCAPE:
> >+            this.stopInspecting();
> >+            break;
> 
> could collapsed these

done.

> >+      case "mousemove":
> 
> >+        if (element && element != this.node) {
> >+          InspectorUI.inspectNode(element);
> 
> this.inspectNode()

Quite.

> >+  panelShown: function InspectorUI_panelShown(event)
> >+  {
> >+    // # todo
> >+  },
> 
> remove for now (also caller)?

Aye-aye!

> diff --git a/browser/base/content/browser-menubar.inc
> b/browser/base/content/browser-menubar.inc
> 
> >+              <menuitem id="menu_inspector"
> 
> >+                        commandkey="&inspectMenu.commandkey;"
> 
> commandkey attribute doesn't exist.

whups. ->key

> >+                        command="Tools:Inspect"/>
> 
> diff --git a/browser/base/content/browser.xul
> b/browser/base/content/browser.xul
> 
> >+    <panel id="highlighter-panel"
> >+           ignorekeys="true"
> >+           noautofocus="true"
> >+           noautohide="true"
> >+           onclick="InspectorUI.stopInspecting();"/>
> 
> Seems like the event handlers should be consolidated - I think I'd actually
> prefer them to be specified here rather than added/removed manually in
> attachListeners/detachListeners.

Problem with that is that we're not always listening for them. Only during "inspect mode". Once you've selected a node to look at, you don't want the highlighter jumping around with errant mouse moves.

I guess we could add the listeners here and check the inspecting property in the event handler.

I'm pulling in the fix from bug 560829 while we're at it.

> >+    <panel id="inspector-panel"
> 
> >+           left="100"
> >+           top="600"
> 
> Is there any point in having these given that the panel is always positioned on
> open?

no, not anymore.

> These panels should probably both be hidden="true" by default - prevents
> unnecessary initialization until they're used (txul/ts have been sensitive to
> adding non-hidden panels in the past).

OK. That's good to know.

> >+      <toolbarbutton id="inspect-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
> >+                     label="&inspectButton.label;"
> >+                     tooltiptext="&inspectButton.tooltip;"
> >+                     command="Tools:Inspect"/>
> 
> diff --git a/browser/locales/en-US/chrome/browser/browser.dtd
> b/browser/locales/en-US/chrome/browser/browser.dtd
> 
> >+<!ENTITY inspectButton.label          "Inspect">
> >+<!ENTITY inspectButton.tooltip        "Inspect web page">
> 
> 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
> 
> Can we move these toolbarbutton changes to a followup bug? I don't really want
> to land with the theming as-is, and don't think we need to block on this.

Sure. That was largely-placeholder gunk. Removed.

> diff --git a/browser/base/content/test/browser_inspector_highlighter.js
> b/browser/base/content/test/browser_inspector_highlighter.js
> 
> >+function createDocument()
> 
> >+  doc.body.addEventListener("DOMSubtreeModified", setupHighlighterTests, false);
> >+  doc.body.appendChild(div);
> >+  doc.body.appendChild(div2);
> 
> This really shouldn't be necessary - appendChild is synchronous, so you should
> really just be able to call setupHighlighterTests() directly here. If that
> doesn't work, need to figure out why.

But... I thought it was clever! Removed. :(

> >+function performTestComparisons(evt)
> 
> >+  setTimeout(finishUp, 0);
> 
> executeSoon()

Didn't know about that. I will use it always.

> diff --git a/browser/base/content/test/browser_inspector_initialization.js
> b/browser/base/content/test/browser_inspector_initialization.js
> 
> >+function runInspectorTests(evt)
> 
> >+  ok(InspectorUI.isStylePanelOpen, "Inspector Style Panel is open");
> >+  ok(InspectorUI.isDOMPanelOpen, "Inspector DOM Panel is open");
> 
> need to todo() these before landing obviously...

oops. Yes.

> diff --git a/browser/base/content/test/browser_inspector_treeSelection.js
> b/browser/base/content/test/browser_inspector_treeSelection.js
> 
> >+function createDocument()
> 
> >+  // doc.body.addEventListener("DOMSubtreeModified", , false);
> >+  doc.body.appendChild(div);
> >+  setupSelectionTests();
> 
> exactly! :)

huzzah!

> >+function setupSelectionTests()
> 
> >+  /* # todo attach to panelShown event */
> >+  setTimeout(runSelectionTests, 200);
> 
> This this needs to be fixed before landing.

I could've sworn I fixed all these, and yet it seems to still be in my user-repo like this. Fixing...

> r- overall just because I want to take a quick look at the patch that addresses
> these, but we should be able to get this landed soon after that I think.

OK, patch inbound...

Comment 56

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

Attachment: inspector-01-redux

this took a little longer than expecting as I had a couple of bugs.

Also, hard to read as a result of the move from browser-inspector.js to inspector.js. Variations on hg mv --mq weren't working for me.

Tested this out and it seems to be working well, and includes the fix from bug 560829 to eliminate highlighter jumping on mousemoves.

Comment 57

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

Attachment: test-fixes

fixes for unittests that changed or broke as a result of previous patchery.

Comment 58

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

(In reply to comment #55)
> Not sure what to do with "and other license headers".

Just meant "and also for other license headers in this patch where you're original author".

> Maybe in closeInspectorUI()?

Yep, that's what I meant.

Comment 59

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

Comment on attachment 444920 [details] [diff] [review]
inspector-01-redux

I reviewed the diff from trunk of this patch on top of the last one:

>diff --git a/browser/base/content/browser-menubar.inc b/browser/base/content/browser-menubar.inc

>+              <menuitem id="menu_inspector"

I've discovered that this is the same menuitem ID that DOM inspector currently uses, which results in some wackyness if that's already installed. Maybe best to change this to "menu_pageinspector" or something just to avoid issues?

diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>+  this.panel.setAttribute("hidden", "false");

>+      this.treePanel.setAttribute("hidden", "false");

Just use .hidden=false for these (which ends up just calling removeAttribute).

+InspectorTreeView.prototype = {

>   destroy: function ITV_destroy()
>   {
>-    this.tree.treeBoxObject.view = null;
>+    this.tree.view = null;

Hmm, I think this also needs to clear this.view, right? (Do you actually need both references, or is just having this.tree.view enough?)

+var InspectorUI = {
+  name: "browser.content.inspector",

now unused

r=me with those addressed, let's get this puppy landed!

Comment 60

[Shawn Wilsher :sdwilsh]

(In reply to comment #59)
> r=me with those addressed, let's get this puppy landed!
SQUEEEEEEEEEEEEEEEEEEEE

Comment 61

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

(I assume we'll land attachment 444709 [details] [diff] [review] + attachment 444920 [details] [diff] [review] + attachment 444936 [details] [diff] [review] on trunk as one patch combined.)

Comment 62

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

yes, was just going to take my little patch queue and convert it to a single patch for fixing as per your most-recent comments. Once that's ready, I'm clear to land?

Comment 63

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

(In reply to comment #59)
> (From update of attachment 444920 [details] [diff] [review])
> I reviewed the diff from trunk of this patch on top of the last one:
> 
> >diff --git a/browser/base/content/browser-menubar.inc b/browser/base/content/browser-menubar.inc
> 
> >+              <menuitem id="menu_inspector"
> 
> I've discovered that this is the same menuitem ID that DOM inspector currently
> uses, which results in some wackyness if that's already installed. Maybe best
> to change this to "menu_pageinspector" or something just to avoid issues?

Good catch.

> diff --git a/browser/base/content/inspector.js
> b/browser/base/content/inspector.js
> 
> >+  this.panel.setAttribute("hidden", "false");
> 
> >+      this.treePanel.setAttribute("hidden", "false");
> 
> Just use .hidden=false for these (which ends up just calling removeAttribute).

done.

> +InspectorTreeView.prototype = {
> 
> >   destroy: function ITV_destroy()
> >   {
> >-    this.tree.treeBoxObject.view = null;
> >+    this.tree.view = null;
> 
> Hmm, I think this also needs to clear this.view, right? (Do you actually need
> both references, or is just having this.tree.view enough?)

They should be pointing to the same object, so an extra reference in this.view is probably not enough to clear it. Adding it in.

> +var InspectorUI = {
> +  name: "browser.content.inspector",
> 
> now unused
> 
> r=me with those addressed, let's get this puppy landed!

all done, adding folded patch...

Comment 64

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

Attachment: inspector-0.1-final

moving review+ forward from previous patches. Ready to land.

Comment 65

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

Comment on attachment 445091 [details] [diff] [review]
inspector-0.1-final

requesting SR per conversation with mconnor in irc.

Comment 66

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

try builds for this patch are appearing here:

https://build.mozilla.org/tryserver-builds/[e-mail]-inspector01/

Comment 67

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

Comment on attachment 445091 [details] [diff] [review]
inspector-0.1-final

rs's review certainly can't hurt, but let's not block on it - there's nothing in this patch that requires SR per the policy.

Comment 68

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

(In reply to comment #63)
> > Hmm, I think this also needs to clear this.view, right? (Do you actually need
> > both references, or is just having this.tree.view enough?)
> 
> They should be pointing to the same object, so an extra reference in this.view
> is probably not enough to clear it. Adding it in.

The parenthetical meant "does this.view need to exist at all?" - I wasn't sure why there need to be two references to the same object when only one appears to be used.

Comment 69

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

Attachment: [checked-in] inspector-0.1-really-final

fix for initialization test.

Comment 70

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

Comment on attachment 445175 [details] [diff] [review]
[checked-in] inspector-0.1-really-final

pushed 42289:f551d1be254e mozilla-central

Comment 71

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

Comment on attachment 445175 [details] [diff] [review]
[checked-in] inspector-0.1-really-final

changeset 529e4485571a - updated to change commit message

Comment 72

[Dão Gottwald [:dao]]

(In reply to comment #41)
> (In reply to comment #35)
> > > nit: opening brace on new line
> > Err, no...
> > > nit: one line ifs are supposed to be braced
> > Not needed in browser.
> Er, yes per style guide.

It doesn't spell out that there should be a line break before {. It does do it in an example, but I suspect that goes back to a quick arbitrary choice (is there a regular process for deciding these things anyway?). We've omitted the line break for years in browser/, which is most relevant here, since, as far as I can tell, there aren't any rational arguments for doing it differently. It's a question of habit and aesthetics, where breaking consistency means failure.

> It doesn't say anything about exceptions for browser,

Right, it doesn't take module-wide prevailing style into account.

Comment 73

[Shawn Wilsher :sdwilsh]

(In reply to comment #72)
> It doesn't spell out that there should be a line break before {. It does do it
> in an example, but I suspect that goes back to a quick arbitrary choice (is
> there a regular process for deciding these things anyway?). We've omitted the
> line break for years in browser/, which is most relevant here, since, as far as
> I can tell, there aren't any rational arguments for doing it differently. It's
> a question of habit and aesthetics, where breaking consistency means failure.
I think that that is the whole point of the samples.  A style guide that dictates everything with English instead of code samples would be suboptimal for a reference.

The process for deciding these things is newsgroup discussions.  While it's true that browser has had a different convention for years, brendan said that all new files should follow the style guide, not module conventions.  Maybe it's breaking a habit for you because you prominently hack in browser/, but for folks who hack in a wider range of places, having browser/ be different has the same problem you are trying to avoid.

Comment 74

[Dão Gottwald [:dao]]

(In reply to comment #73)
> The process for deciding these things is newsgroup discussions.

Has this particular point been discussed?

> While it's
> true that browser has had a different convention for years, brendan said that
> all new files should follow the style guide, not module conventions.

Well, even then this is still browser.js in the end.

> Maybe
> it's breaking a habit for you because you prominently hack in browser/, but for
> folks who hack in a wider range of places, having browser/ be different has the
> same problem you are trying to avoid.

I would think that these people's contributions are outnumbered and also wouldn't rise significantly just because of a consistent coding style across mozilla-central. Of course that consistency is purely hypothetical anyway, since loads of existing code don't conform. Breaking up modules complicates matters for the occasional contributor as well as for the regular one.

Comment 75

[Jim Jeffery not reading bug-mail 1/2/11]

Two things I noticed using an hourly with the landing.

1.  The 'shortcut' to open the Inspector Alt+t Alt+I does not open the tool
2.  When the Inspector is open, on Win7, with the larger taskbar than XP, it extends below the taskbar cutting off the bottom line or two.

Comment 76

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

(In reply to comment #75)
> Two things I noticed using an hourly with the landing.
> 
> 1.  The 'shortcut' to open the Inspector Alt+t Alt+I does not open the tool
> 2.  When the Inspector is open, on Win7, with the larger taskbar than XP, it
> extends below the taskbar cutting off the bottom line or two.

Hi Jim,

thanks for the report. Would you mind filing a separate bug on this issue? I'm closing this bug as the code is landed.

thanks!

Comment 77

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

I don't think bracing style should be enforced on a project-wide level at all. I don't think Rob should have had to fix up his patches to conform to the style guide, but once it was done I didn't feel strongly enough to suggest undoing it, either. I don't think it's a big deal either way.

Comment 78

[Alfred Kayser]

Note, the patch needlessly adds an empty line to:
browser/themes/pinstripe/browser/browser.css

Comment 79

[Robert Strong (they/them - no direct email)]

(In reply to comment #75)
> Two things I noticed using an hourly with the landing.
> 
> 1.  The 'shortcut' to open the Inspector Alt+t Alt+I does not open the tool
> 2.  When the Inspector is open, on Win7, with the larger taskbar than XP, it
> extends below the taskbar cutting off the bottom line or two.
Jim, when you file please note the bug number in this bug. Thanks

Comment 80

[Robert Strong (they/them - no direct email)]

bah... didn't see it the first time in the dependency list - bug 565872

Comment 81

[Robert Strong (they/them - no direct email)]

Comment on attachment 445175 [details] [diff] [review]
[checked-in] inspector-0.1-really-final

Not much if anything to sr here...

The only things I noticed were:
nit: trailing commas in objects... for example, INSPECTOR_INVISIBLE_ELEMENTS and I noticed others as well.
see bugXXXXXX should be a bug number instead of XXXXXX.

Comment 82

[Shawn Wilsher :sdwilsh]

(In reply to comment #81)
> nit: trailing commas in objects... for example, INSPECTOR_INVISIBLE_ELEMENTS
There's nothing wrong with that, right?  It's no longer a strict warning.

Comment 83

[Robert Strong (they/them - no direct email)]

(In reply to comment #82)
> (In reply to comment #81)
> > nit: trailing commas in objects... for example, INSPECTOR_INVISIBLE_ELEMENTS
> There's nothing wrong with that, right?  It's no longer a strict warning.
I believe that is correct and is why I wrote nit vs. asking for it to be fixed.