650794 - Disable HTML panel and make it use registerTools API

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

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

The current HTML view implementation is going to be made obsolete and should be largely unnecessary with the next version of the highlighter (bug 642471).

Comment 1

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

backed out due to testfailures. iframe removal patch landed on central. lol.

http://tinderbox.mozilla.org/showlog.cgi?log=Devtools/1310599510.1310600924.985.gz&fulltext=1

Comment 2

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

Morphing this bug.

Rather than remove the HTML panel, we want to make it disabled by default and use the registerTools API in the Inspector code to show and hide the Panel.

Comment 3

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

lots of entanglement with the HTML panel currently. Disconnecting it is going to require brain surgery. Also, lots of tests will need to be updated.

Comment 4

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

Attachment: disable html panel, register it - WIP

WIP patch. Behavior is working well, but need to patch up the existing unittests and create a new one to verify this is working as intended.

Comment 5

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

Attachment: disable html panel v2, register it - WIP

Comment 6

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

Attachment: disable html panel v3

ok, this is ready for feedback. Still need to write a test to enable the HTML panel and verify it's working. Might need another notification to let tests know when the HTML panel is "up".

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 551777 [details] [diff] [review]
disable html panel v3

Review of attachment 551777 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks fine. Please address the comments below. f+ with that!

::: browser/base/content/inspector.js
@@ +780,5 @@
> +      if (!this.treePanel) {
> +        this.treePanel = document.getElementById("inspector-tree-panel");
> +        this.treePanel.hidden = false;
> +      }
> +      this.registerTool({

General comment here: I would like to see the tree panel more decoupled than this.

inspector.js will become a JSM, so I would like the TreePanel object to be independent. The openTreePanel(), closeTreePanel() and all tree panel methods and properties should move there, in the new object.

The tree panel code in this patch is still intertwined with the main inspector UI code. (albeit, at a lesser extent)

@@ +900,5 @@
>      this.selection = null;
>      this.treeLoaded = false;
> +    this.closing = false;
> +    delete this.treePanel;
> +    // delete this.toolbar;

Why is this line commented?

Also, please note that the Highlighter.destroy() method has a delete this.toolbar line that was added in the patch of bug 666650. That should've been in here, IIANM. (There is no toolbar property in the Highlighter.) See line 225 in inspector.js.

::: browser/base/content/test/inspector/browser_inspector_initialization.js
@@ +52,5 @@
>    Services.obs.addObserver(finishInspectorTests,
>      INSPECTOR_NOTIFICATIONS.CLOSED, false);
>  
> +  // let iframe = document.getElementById("inspector-tree-iframe");
> +  // is(InspectorUI.treeIFrame, iframe, "Inspector IFrame matches");

Please remove these lines if they are no longer need, to avoid confusion in the future.

::: browser/base/content/test/inspector/browser_inspector_treePanel_output.js
@@ +73,5 @@
>    Services.obs.removeObserver(inspectorOpened,
>      INSPECTOR_NOTIFICATIONS.OPENED, false);
>  
>    ok(InspectorUI.inspecting, "Inspector is highlighting");
> +//  ok(InspectorUI.isTreePanelOpen, "Inspector Tree Panel is open");

This should be fixed. Commenting out is not a fix. :)

@@ +93,5 @@
>  
> +//  ok(InspectorUI.treePanelDiv, "InspectorUI.treePanelDiv is available");
> +//  is(InspectorUI.treePanelDiv.innerHTML.replace(/^\s+|\s+$/mg, ''),
> +//    expectedResult, "treePanelDiv.innerHTML is correct");
> +//  expectedResult = null;

Same here please.

Comment 8

[Mihai Sucan [:msucan]]

Also, please note that I get test failures:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_initialization.js | Inspector Tree Panel is not open
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_tab_switch.js | Inspector Tree Panel is not open
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_tab_switch.js | Inspector Tree Panel is not open
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_tab_switch.js | Inspector Tree Panel is not open
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_tab_switch.js | Inspector Tree Panel is not open
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_tab_switch.js | Inspector Tree Panel is not open

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 551777 [details] [diff] [review]
disable html panel v3

Review of attachment 551777 [details] [diff] [review]:
-----------------------------------------------------------------

Forgot something yesterday. See below.

::: browser/base/content/inspector.js
@@ +784,5 @@
> +      this.registerTool({
> +        id: "treepanel",
> +        label: "HTML",
> +        tooltiptext: "HTML panel",
> +        accesskey: "H",

These strings need to be in inspector.properties.

Comment 10

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

Attachment: v4 (WIP)

WIP patch migrating tree panel code to a module. Also moved insideOutBox.js to InsideOutBox.jsm.

Patch is currently working, but I still have to address the commented out tests mentioned in the review comments, make sure TreePanel cleans up properly after itself and investigate an annoying error message coming from the registerTools method.

... And verify that the unittests are working properly again.

Comment 11

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

Attachment: disable and modularize html panel v5

Updated patch, tests passing and augmented to check for activation of html panel. inspector.properties readded.

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 552873 [details] [diff] [review]
disable and modularize html panel v5

Review of attachment 552873 [details] [diff] [review]:
-----------------------------------------------------------------

Great improvements! All tests pass locally. Thanks for the updated patch.

Giving the patch r- only because it still has some problems, hehe. Please see the comments below. Once addressed, we can go forward. Thank you!

::: browser/base/content/insideOutBox.js
@@ +134,5 @@
>    this.box = aBox;
>  
>    this.rootObject = null;
>  
>    this.rootObjectBox = null;

As a measure of safety, to avoid memory leaks we should have an InsideOutBox.destroy() method. This object holds to a number of DOM objects that we should delete.

@@ +453,5 @@
>            try {
>              this.box.removeChild(this.rootObjectBox);
>            } catch (exc) {
> +            //InspectorUI._log("this.box.removeChild(this.rootObjectBox) FAILS " +
> +            //  this.box + " must not contain " + this.rootObjectBox);

Cu.reportError() or remove this?

::: browser/base/content/TreePanel.jsm
@@ +65,5 @@
> +
> +  /**
> +   * Initialization function for the TreePanel.
> +   */
> +  initialize: function TP_initialize()

Maybe rename this method to initializeIframe().

@@ +109,5 @@
> +      this.treeIFrame = this.document.createElement("iframe");
> +      this.treeIFrame.setAttribute("id", "inspector-tree-iframe");
> +      this.treeIFrame.setAttribute("flex", "1");
> +      this.treeIFrame.setAttribute("type", "content");
> +      this.treeIFrame.setAttribute("onclick", "InspectorUI.TreePanel.onTreeClick(event)");

Why doesn't this use addEventListener()?

@@ +232,5 @@
> +      child = this.getNextSibling(previousSibling);
> +    else
> +      child = this.getFirstChild(node);
> +
> +    if (this.showTextNodesWithWhitespace)

this.IUI.showTextNodesWithWhitespace

@@ +285,5 @@
> +      if (hitTwisty) {
> +        this.ioBox.toggleObject(node);
> +      } else {
> +        if (this.inspecting) {
> +          this.stopInspecting(true);

stopInspecting() doesn't exist here. I think this needs to be changed to this.IUI.stopInspecting().

The same for this.inspecting.

@@ +287,5 @@
> +      } else {
> +        if (this.inspecting) {
> +          this.stopInspecting(true);
> +        } else {
> +          this.select(node, true, false);

this.select() exists, but in the previous code iteration it meant that IUI.select() is called (not only the tree panel select code).

Please change this line to call this.IUI.select().

@@ +311,5 @@
> +   * @param aClass
> +   *        The class string.
> +   * @returns boolean
> +   */
> +  hasClass: function IUI_hasClass(aNode, aClass)

Please update the function name (IUI_hasClass to TP_hasClass).

The same goes for the rest of the functions that still have the IUI_ prefix.

@@ +382,5 @@
> +
> +    if (this.treeIFrame)
> +      delete this.treeIFrame;
> +
> +    delete this.ioBox;

You still keep some DOM refs: treePanelDiv, treeBrowserDocument and treeWalker. The first two are removed in the close() method, but there's no guarantee that close() was called. Either call it here, or just remove the DOM refs.

I would also do this.treeLoaded = false.

::: browser/base/content/inspector.js
@@ +571,5 @@
>     * @returns boolean
>     */
>    get isTreePanelOpen()
>    {
> +    return this.TreePanel && this.TreePanel.container.state == "open";

Why not have return this.TreePanel.isOpen() ?

I would suggest adding an isOpen() method (or getter) to the TreePanel.jsm code.

@@ +609,5 @@
> +    if (!this.TreePanel) {
> +      Cu.import("resource:///modules/TreePanel.jsm", this);
> +      this.TreePanel.document = document;
> +      this.TreePanel.window = window;
> +      this.TreePanel.IUI = this;

Why not this.TreePanel.init(this) ?

IUI has sufficient objects that you can derive both the chrome document and the chrome window objects. Also, document can be derived from window (and vice-versa). No need to pass both.

(note that there's a TreePanel.initialize() method that actually deals with the first initialization of the iframe. that's a different method, and we should have a clearer naming distinction, to avoid confusion.)

@@ +623,5 @@
> +        context: this,
> +        onShow: this.openTreePanel,
> +        onHide: this.closeTreePanel,
> +        onSelect: this.treePanelSelect,
> +        panel: this.TreePanel.container

I would expect that context: this.TreePanel,
onShow: this.TreePanel.onShow,
onHide: this.TreePanel.onHide,
onSelect: this.TreePanel.onSelect,

In the case of "context" I understand you already pass IUI to the TreePanel. If the code there needs to access IUI, it can do so.

And actually, why shouldn't the treepanel register itself? Move this call into the new proposed init().

@@ +668,5 @@
>    },
>  
> +  openTreePanel: function IUI_openTreePanel()
> +  {
> +    this.TreePanel.open(this);

Why is this needed to be passed to open()? The initialization part already did this.

@@ +730,5 @@
>      this.winID = null;
>      this.selection = null;
>      this.treeLoaded = false;
> +    this.closing = false;
> +    delete this.treePanel;

TreePanel.destroy() is never called. I believe we'll have memleaks. The JSM continues to hold to DOM nodes.

@@ +798,4 @@
>      }
>      this.toolsDo(function IUI_toolsOnSelect(aTool) {
>        if (aTool.panel.state == "open") {
> +        aTool.onSelect.apply(aTool.context, [aNode, aScroll]);

.apply() is not needed here. You can use aTool.onSelect.call(aTool.context, aNode, aScroll).

@@ +802,5 @@
>        }
>      });
>    },
>  
> +  treePanelSelect: function IUI_treePanelSelect(aNode, aScroll)

I would expect this method to be only in the TreePanel code, not in IUI.

@@ +1072,1 @@
>            aRegObj.onSelect.apply(aRegObj.context, [InspectorUI.selection]);

Same as above. .apply() is not needed here. You can use .call().

::: browser/base/content/test/inspector/Makefile.in
@@ +57,5 @@
>  		browser_inspector_registertools.js \
>  		browser_inspector_bug_665880.js \
>  		$(NULL)
>  
> +# 		browser_inspector_treePanel_click.js \

Why is this test disabled?

::: browser/base/content/test/inspector/browser_inspector_tab_switch.js
@@ +166,5 @@
> +
> +  // Switch back to tab 1.
> +  Services.obs.addObserver(inspectorSecondFocusTab1,
> +    INSPECTOR_NOTIFICATIONS.TREEPANELREADY, false);
> +  gBrowser.selectedTab = tab1;

Why do you wait for TREEPANELREADY here? It fires after IUI OPENED?

Once we have more panels, we'll want to have a mechanism that tells us "all panels are ready". (worth a follow up bug report?)

Comment 13

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

(In reply to Mihai Sucan [:msucan] from comment #12)
> Comment on attachment 552873 [details] [diff] [review]
> disable and modularize html panel v5
> 
> Review of attachment 552873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great improvements! All tests pass locally. Thanks for the updated patch.
> 
> Giving the patch r- only because it still has some problems, hehe. Please
> see the comments below. Once addressed, we can go forward. Thank you!

Well, if there were no problems, it would've been an r+! :)

> ::: browser/base/content/insideOutBox.js
> @@ +134,5 @@
> >    this.box = aBox;
> >  
> >    this.rootObject = null;
> >  
> >    this.rootObjectBox = null;
> 
> As a measure of safety, to avoid memory leaks we should have an
> InsideOutBox.destroy() method. This object holds to a number of DOM objects
> that we should delete.

Ah, good call. Added.

> @@ +453,5 @@
> >            try {
> >              this.box.removeChild(this.rootObjectBox);
> >            } catch (exc) {
> > +            //InspectorUI._log("this.box.removeChild(this.rootObjectBox) FAILS " +
> > +            //  this.box + " must not contain " + this.rootObjectBox);
> 
> Cu.reportError() or remove this?

Switched that to this.view._log which is equivalent to calling InspectorUI._log.

> ::: browser/base/content/TreePanel.jsm
> @@ +65,5 @@
> > +
> > +  /**
> > +   * Initialization function for the TreePanel.
> > +   */
> > +  initialize: function TP_initialize()
> 
> Maybe rename this method to initializeIframe().

I'll disagree here. It's not like we have more than one initialization function. Makes the method signature longer.

> @@ +109,5 @@
> > +      this.treeIFrame = this.document.createElement("iframe");
> > +      this.treeIFrame.setAttribute("id", "inspector-tree-iframe");
> > +      this.treeIFrame.setAttribute("flex", "1");
> > +      this.treeIFrame.setAttribute("type", "content");
> > +      this.treeIFrame.setAttribute("onclick", "InspectorUI.TreePanel.onTreeClick(event)");
> 
> Why doesn't this use addEventListener()?

Historical reasons, I think. When we first landed this, I wasn't getting a listener to fire on the iframe itself. Probably have to add it to the document. I will do so.

> @@ +232,5 @@
> > +      child = this.getNextSibling(previousSibling);
> > +    else
> > +      child = this.getFirstChild(node);
> > +
> > +    if (this.showTextNodesWithWhitespace)
> 
> this.IUI.showTextNodesWithWhitespace

oops. Wonder if that needs to be in TreePanel.jsm as well? Probably since it's the only thing that refers to it.

> @@ +285,5 @@
> > +      if (hitTwisty) {
> > +        this.ioBox.toggleObject(node);
> > +      } else {
> > +        if (this.inspecting) {
> > +          this.stopInspecting(true);
> 
> stopInspecting() doesn't exist here. I think this needs to be changed to
> this.IUI.stopInspecting().

ah yes.

> The same for this.inspecting.

Same for this.inspecting.

> @@ +287,5 @@
> > +      } else {
> > +        if (this.inspecting) {
> > +          this.stopInspecting(true);
> > +        } else {
> > +          this.select(node, true, false);
> 
> this.select() exists, but in the previous code iteration it meant that
> IUI.select() is called (not only the tree panel select code).
> 
> Please change this line to call this.IUI.select().

OK!

> @@ +311,5 @@
> > +   * @param aClass
> > +   *        The class string.
> > +   * @returns boolean
> > +   */
> > +  hasClass: function IUI_hasClass(aNode, aClass)
> 
> Please update the function name (IUI_hasClass to TP_hasClass).
> 
> The same goes for the rest of the functions that still have the IUI_ prefix.
> 
> @@ +382,5 @@
> > +
> > +    if (this.treeIFrame)
> > +      delete this.treeIFrame;
> > +
> > +    delete this.ioBox;
> 
> You still keep some DOM refs: treePanelDiv, treeBrowserDocument and
> treeWalker. The first two are removed in the close() method, but there's no
> guarantee that close() was called. Either call it here, or just remove the
> DOM refs.

done!

> I would also do this.treeLoaded = false.

also done.

> ::: browser/base/content/inspector.js
> @@ +571,5 @@
> >     * @returns boolean
> >     */
> >    get isTreePanelOpen()
> >    {
> > +    return this.TreePanel && this.TreePanel.container.state == "open";
> 
> Why not have return this.TreePanel.isOpen() ?

I guess that's a thing we could do. I will do that!

> I would suggest adding an isOpen() method (or getter) to the TreePanel.jsm
> code.

done.

> @@ +609,5 @@
> > +    if (!this.TreePanel) {
> > +      Cu.import("resource:///modules/TreePanel.jsm", this);
> > +      this.TreePanel.document = document;
> > +      this.TreePanel.window = window;
> > +      this.TreePanel.IUI = this;
> 
> Why not this.TreePanel.init(this) ?

true true.

> IUI has sufficient objects that you can derive both the chrome document and
> the chrome window objects. Also, document can be derived from window (and
> vice-versa). No need to pass both.

true.

> (note that there's a TreePanel.initialize() method that actually deals with
> the first initialization of the iframe. that's a different method, and we
> should have a clearer naming distinction, to avoid confusion.)

well, ok. I guess you sold me on the initializeIFrame method renaming. Though I still hate having "IFrame" in method names. It capitalizes poorly.

> @@ +623,5 @@
> > +        context: this,
> > +        onShow: this.openTreePanel,
> > +        onHide: this.closeTreePanel,
> > +        onSelect: this.treePanelSelect,
> > +        panel: this.TreePanel.container
> 
> I would expect that context: this.TreePanel,
> onShow: this.TreePanel.onShow,
> onHide: this.TreePanel.onHide,
> onSelect: this.TreePanel.onSelect,

I think the API is somewhat misleading. Those aren't "onShow" and "onHide" events. They're "show" and "hide" events. How does the tool show and hide itself?

> In the case of "context" I understand you already pass IUI to the TreePanel.
> If the code there needs to access IUI, it can do so.
> 
> And actually, why shouldn't the treepanel register itself? Move this call
> into the new proposed init().

done.

> 
> @@ +668,5 @@
> >    },
> >  
> > +  openTreePanel: function IUI_openTreePanel()
> > +  {
> > +    this.TreePanel.open(this);
> 
> Why is this needed to be passed to open()? The initialization part already
> did this.

That was a leftover. Removed... the WHOLE METHOD.

> @@ +730,5 @@
> >      this.winID = null;
> >      this.selection = null;
> >      this.treeLoaded = false;
> > +    this.closing = false;
> > +    delete this.treePanel;
> 
> TreePanel.destroy() is never called. I believe we'll have memleaks. The JSM
> continues to hold to DOM nodes.

Eek! Lemme fix that.

> @@ +798,4 @@
> >      }
> >      this.toolsDo(function IUI_toolsOnSelect(aTool) {
> >        if (aTool.panel.state == "open") {
> > +        aTool.onSelect.apply(aTool.context, [aNode, aScroll]);
> 
> .apply() is not needed here. You can use aTool.onSelect.call(aTool.context,
> aNode, aScroll).

um, same thing, only different?

> @@ +802,5 @@
> >        }
> >      });
> >    },
> >  
> > +  treePanelSelect: function IUI_treePanelSelect(aNode, aScroll)
> 
> I would expect this method to be only in the TreePanel code, not in IUI.

OK.

> @@ +1072,1 @@
> >            aRegObj.onSelect.apply(aRegObj.context, [InspectorUI.selection]);
> 
> Same as above. .apply() is not needed here. You can use .call().
> 
> ::: browser/base/content/test/inspector/Makefile.in
> @@ +57,5 @@
> >  		browser_inspector_registertools.js \
> >  		browser_inspector_bug_665880.js \
> >  		$(NULL)
> >  
> > +# 		browser_inspector_treePanel_click.js \
> 
> Why is this test disabled?

It was disabled previously when paul landed another test, I think. afaict, it was never enabled and never worked. Needs work.

> ::: browser/base/content/test/inspector/browser_inspector_tab_switch.js
> @@ +166,5 @@
> > +
> > +  // Switch back to tab 1.
> > +  Services.obs.addObserver(inspectorSecondFocusTab1,
> > +    INSPECTOR_NOTIFICATIONS.TREEPANELREADY, false);
> > +  gBrowser.selectedTab = tab1;
> 
> Why do you wait for TREEPANELREADY here? It fires after IUI OPENED?

Yes.

> Once we have more panels, we'll want to have a mechanism that tells us "all
> panels are ready". (worth a follow up bug report?)

not sure how we'll do that. The panels could be opening in any given order and will certainly be ready in a non-deterministic order. Maybe we could register a listener in IUI that waits for all of the panel ready notifications to arrive and then fires a "ALLREADY!" notice to its observers.

But yes, definitely worth a followup. Bug 679711.

Comment 14

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

Attachment: disable and modularize html panel v8, wip

the registerTools api provided no way to deregister. Worse, event listeners weren't getting cleaned up from the various XUL appendages (toolbar buttons, panels). Fixed that. Created a real instance of TreePanel to address multiple window problems.

need to update the tests.

Comment 15

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

Attachment: test fixes

passing all but iframeTests and tab_switch. I'll get these tomorrow.

Comment 16

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

Attachment: test fixes

tests passing.

Comment 17

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

Attachment: disable and modularize html panel v10

Comment 18

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

Attachment: move treepanel code to devtools

patch moves the tree panel and friends into browser/devtools/highlighter

Comment 19

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

Comment on attachment 555393 [details] [diff] [review]
move treepanel code to devtools

moved this patch to bug 681651.

Comment 20

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

Attachment: disable and modularize html panel v12

updated based on moved registerTools API bug 681653.

Comment 21

[Mihai Sucan [:msucan]]

Comment on attachment 555419 [details] [diff] [review]
disable and modularize html panel v12

Review of attachment 555419 [details] [diff] [review]:
-----------------------------------------------------------------

All tests pass, really good progress! Giving the patch r+, but please address the comments below.

This patch has a lot of trailing white space, please fix this.

(I'll take a quick pass through the updated patch, when you submit it.)

::: browser/base/content/TreePanel.jsm
@@ +54,5 @@
> + * @param aContext nsIDOMWindow (xulwindow)
> + * @param aIUI global InspectorUI object
> + */
> +
> +function TreePanel(aContext, aIUI) {

Nit: there's a new line before the function and the comment that describes it. Please remove it.

(same comment applies for TreePanel.destroy())

@@ +74,5 @@
> +   * originating context and the InspectorUI global.
> +   * @param aContext nsIDOMWindow (xulwindow)
> +   * @param aIUI global InspectorUI object
> +   */
> +  _init: function TP_init(aContext, aIUI)

Nit: function TP__init(aContext, aIUI)

@@ +76,5 @@
> +   * @param aIUI global InspectorUI object
> +   */
> +  _init: function TP_init(aContext, aIUI)
> +  {
> +    this.showTextNodesWithWhitespace = false;

You could put this and the id properties on the prototype directly, no need to set them in _init().

@@ +77,5 @@
> +   */
> +  _init: function TP_init(aContext, aIUI)
> +  {
> +    this.showTextNodesWithWhitespace = false;
> +    this.id = "treepanel"; // DO NOT LOCALIZE

Why is the "DO NOT LOCALIZE" comment needed here? Do localizers check the source code?

@@ +81,5 @@
> +    this.id = "treepanel"; // DO NOT LOCALIZE
> +    this.IUI = aIUI;
> +    this.window = aContext;
> +    this.document = this.window.document;
> +    this.editingContext = null;

Is this needed here?

@@ +85,5 @@
> +    this.editingContext = null;
> +
> +    if (!this.domplate) {
> +      Cu.import("resource:///modules/domplate.jsm", this);
> +      this.domplateUtils.setDOM(this.window);

We shouldn't do Cu.import() for every init() call. We should have the Cu.import() at the top of the file, only once. We are now in a JSM, more freedom.

(maybe not worth doing this right now, maybe a follow up?)

@@ +126,5 @@
> +    delete this.initializingTreePanel;
> +    Services.obs.notifyObservers(null,
> +      this.window.INSPECTOR_NOTIFICATIONS.TREEPANELREADY, null);
> +    if (this.IUI.selection)
> +      this.select(this.IUI.selection, true);

Please keep the coding style consistent through the whole file: curly braces for one liners or not. This method (and the whole file) mixes both styles.

(I, for one, prefer enforcing braces for one liners, but it really doesn't matter whatever we chose.)

@@ +132,5 @@
> +
> +  /**
> +   * Open the inspector's tree panel and initialize it.
> +   */
> +  open: function TP_openTreePanel()

open: function TP_open()

::: browser/base/content/inspector.js
@@ +537,5 @@
>  var InspectorUI = {
>    browser: null,
>    tools: {},
> +  toolButtonEvents: {},
> +  toolPanelEvents: {},

These should be in bug 681653.

@@ +618,1 @@
>      }

Similar to the comment I had for the domplate Cu.import() from the TreePanel.jsm: I wish this would be at the top of the inspector.jsm (when we switch to jsm).

Also, this.treePanel versus this.TreePanel is quite confusing. We should aim here for something better.

@@ +670,5 @@
>    {
>      // if currently editing an attribute value, closing the
>      // highlighter/HTML panel dismisses the editor
> +    if (this.treePanel.editingContext)
> +      this.treePanel.closeEditor();

I see this check in multiple places. This check fails (it throws) when you have the tree panel disabled from the prefs (when treePanelEnabled is false), because this.treePanel is null (it doesn't initialize, see openInspectorUI()).

Please update this to fix the failure, in all places where it exists.

@@ +721,5 @@
>  
> +    delete this.treePanel;
> +    delete this.toolbar;
> +    delete this.TreePanel;
> +    Services.obs.notifyObservers(null, INSPECTOR_NOTIFICATIONS.CLOSED, null);

We need a way to announce registered tools, at once, to destroy() themselves. Here you call destroy() manually for the treePanel.

The common way to register should be to just add a Cu.import() at the top of this inspector.jsm (when it becomes a jsm). To unregister, maybe we could add a notification, or a callback to registerTool objects, like destroy() which we invoke here when destroy() happens. This would avoid the need to do aTool.panel.hidePopup() above.

(maybe file a follow up bug?)

@@ +1158,5 @@
> +        this.toolPanelEvents[aPanel.id + "popupshowing"], false);
> +      this.toolPanelEvents[aPanel.id + "popupshowing"] = null;
> +      delete this.toolPanelEvents[aPanel.id + "popupshowing"]
> +    }
> +  },

The (un)bindTool*Events() and getToolbarButtonId() methods come from bug 681653 as well. These shouldn't be here, right?

::: browser/base/content/test/inspector/browser_inspector_initialization.js
@@ +57,5 @@
> +  ok(InspectorUI.inspecting, "Inspector is inspecting");
> +  ok(!InspectorUI.isTreePanelOpen, "Inspector Tree Panel is not open");
> +  ok(InspectorUI.highlighter, "Highlighter is up");
> +
> +  InspectorUI.treePanel.open();

What happens here if the tree panel is disabled by the pref? (and in the other test files)

Comment 22

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

(In reply to Mihai Sucan [:msucan] from comment #21)
> Comment on attachment 555419 [details] [diff] [review]
> disable and modularize html panel v12
> 
> Review of attachment 555419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All tests pass, really good progress! Giving the patch r+, but please
> address the comments below.
> 
> This patch has a lot of trailing white space, please fix this.

it does? that's ... odd.

/me scans.

Oh. Wow. How'd I miss that? *fix*

Think I got them all.

> (I'll take a quick pass through the updated patch, when you submit it.)

Thanks. I can usually catch those when eyeballing my patch through qdiff as it highlights them. I think I forgot to this time. They weren't mine. :|

> ::: browser/base/content/TreePanel.jsm
> @@ +54,5 @@
> > + * @param aContext nsIDOMWindow (xulwindow)
> > + * @param aIUI global InspectorUI object
> > + */
> > +
> > +function TreePanel(aContext, aIUI) {
> 
> Nit: there's a new line before the function and the comment that describes
> it. Please remove it.
> 
> (same comment applies for TreePanel.destroy())

AUGH! :)

> > @@ +74,5 @@
> > +   * originating context and the InspectorUI global.
> > +   * @param aContext nsIDOMWindow (xulwindow)
> > +   * @param aIUI global InspectorUI object
> > +   */
> > +  _init: function TP_init(aContext, aIUI)
> 
> Nit: function TP__init(aContext, aIUI)
> 
> @@ +76,5 @@
> > +   * @param aIUI global InspectorUI object
> > +   */
> > +  _init: function TP_init(aContext, aIUI)
> > +  {
> > +    this.showTextNodesWithWhitespace = false;
> 
> You could put this and the id properties on the prototype directly, no need
> to set them in _init().

ah yes.

> 
> @@ +77,5 @@
> > +   */
> > +  _init: function TP_init(aContext, aIUI)
> > +  {
> > +    this.showTextNodesWithWhitespace = false;
> > +    this.id = "treepanel"; // DO NOT LOCALIZE
> 
> Why is the "DO NOT LOCALIZE" comment needed here? Do localizers check the
> source code?

Mostly in case there's an l10n review on this and I'm told to stick it in a stringBundle.

> @@ +81,5 @@
> > +    this.id = "treepanel"; // DO NOT LOCALIZE
> > +    this.IUI = aIUI;
> > +    this.window = aContext;
> > +    this.document = this.window.document;
> > +    this.editingContext = null;
> 
> Is this needed here?

probably not. I figure it was added during the editor patch to "declare" it ahead of use and to match a potential clean up in the destroy function. I'll remove it.

> @@ +85,5 @@
> > +    this.editingContext = null;
> > +
> > +    if (!this.domplate) {
> > +      Cu.import("resource:///modules/domplate.jsm", this);
> > +      this.domplateUtils.setDOM(this.window);
> 
> We shouldn't do Cu.import() for every init() call. We should have the
> Cu.import() at the top of the file, only once. We are now in a JSM, more
> freedom.

good point.

> (maybe not worth doing this right now, maybe a follow up?)

Nah, might as well move it now. 

> @@ +126,5 @@
> > +    delete this.initializingTreePanel;
> > +    Services.obs.notifyObservers(null,
> > +      this.window.INSPECTOR_NOTIFICATIONS.TREEPANELREADY, null);
> > +    if (this.IUI.selection)
> > +      this.select(this.IUI.selection, true);
> 
> Please keep the coding style consistent through the whole file: curly braces
> for one liners or not. This method (and the whole file) mixes both styles.

my file. I get to call it! (this is a silly nit).

Seriously though, most of this code was copied over straight from inspector.js. It was good enough for there, it's good enough for this.

> (I, for one, prefer enforcing braces for one liners, but it really doesn't
> matter whatever we chose.)

I'm a little less strict on requiring unnecessary brackets. I don't.

> @@ +132,5 @@
> > +
> > +  /**
> > +   * Open the inspector's tree panel and initialize it.
> > +   */
> > +  open: function TP_openTreePanel()
> 
> open: function TP_open()

good.

> ::: browser/base/content/inspector.js
> @@ +537,5 @@
> >  var InspectorUI = {
> >    browser: null,
> >    tools: {},
> > +  toolButtonEvents: {},
> > +  toolPanelEvents: {},
> 
> These should be in bug 681653.

ah yes. Moved!

> @@ +618,1 @@
> >      }
> 
> Similar to the comment I had for the domplate Cu.import() from the
> TreePanel.jsm: I wish this would be at the top of the inspector.jsm (when we
> switch to jsm).
> 
> Also, this.treePanel versus this.TreePanel is quite confusing. We should aim
> here for something better.

TreePanel's the class, treePanel the instance. It's pretty common in object oriented programming.

> @@ +670,5 @@
> >    {
> >      // if currently editing an attribute value, closing the
> >      // highlighter/HTML panel dismisses the editor
> > +    if (this.treePanel.editingContext)
> > +      this.treePanel.closeEditor();
> 
> I see this check in multiple places. This check fails (it throws) when you
> have the tree panel disabled from the prefs (when treePanelEnabled is
> false), because this.treePanel is null (it doesn't initialize, see
> openInspectorUI()).

ehh.

> Please update this to fix the failure, in all places where it exists.

ok.

> @@ +721,5 @@
> >  
> > +    delete this.treePanel;
> > +    delete this.toolbar;
> > +    delete this.TreePanel;
> > +    Services.obs.notifyObservers(null, INSPECTOR_NOTIFICATIONS.CLOSED, null);
> 
> We need a way to announce registered tools, at once, to destroy()
> themselves. Here you call destroy() manually for the treePanel.
> 
> The common way to register should be to just add a Cu.import() at the top of
> this inspector.jsm (when it becomes a jsm). To unregister, maybe we could
> add a notification, or a callback to registerTool objects, like destroy()
> which we invoke here when destroy() happens. This would avoid the need to do
> aTool.panel.hidePopup() above.
> 
> (maybe file a follow up bug?)

Sure. Bug 681975.

> @@ +1158,5 @@
> > +        this.toolPanelEvents[aPanel.id + "popupshowing"], false);
> > +      this.toolPanelEvents[aPanel.id + "popupshowing"] = null;
> > +      delete this.toolPanelEvents[aPanel.id + "popupshowing"]
> > +    }
> > +  },
> 
> The (un)bindTool*Events() and getToolbarButtonId() methods come from bug
> 681653 as well. These shouldn't be here, right?

Right. Oops.

> ::: browser/base/content/test/inspector/browser_inspector_initialization.js
> @@ +57,5 @@
> > +  ok(InspectorUI.inspecting, "Inspector is inspecting");
> > +  ok(!InspectorUI.isTreePanelOpen, "Inspector Tree Panel is not open");
> > +  ok(InspectorUI.highlighter, "Highlighter is up");
> > +
> > +  InspectorUI.treePanel.open();
> 
> What happens here if the tree panel is disabled by the pref? (and in the
> other test files)

*boom*

Is it worth changing the tests for now? I think if we ever disable it by default, we should update the tests then. It's hard-coded to true.

Comment 23

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

Attachment: disable and modularize html panel v13

updated post review.

Comment 24

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

Attachment: disable and modularize html panel v14 (rebased 08/26)

rebased

Comment 25

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

Attachment: disable and modularize html panel v15 (rebased 08/26)

missed a reject in the inspector.properties file.

Comment 26

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

changes pending based on blocking bug 681653. Patch forth-coming...

Comment 27

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

Comment on attachment 556064 [details] [diff] [review]
disable and modularize html panel v15 (rebased 08/26)

canceling review pending updated patch.

Comment 28

[Mihai Sucan [:msucan]]

Attachment: disable and modularize html panel v16 (rebased 09/05)

Since today is a vacation for US (and Canada?) I took the liberty to rebase this patch so I can work on bug 562168. Attached the work I did:

- rebased on top of the latest fxteam.
- found that the TreePanel.jsm didn't get the new code from the previous rebase. in saveEditor() we need this.IUI.isDirty = true, to prevent users from loosing page changes.
- the InspectorProgressListener was not updated to check isInspectorOpen (it still had the check for isTreePanelOpen, which broke the code).
- the page navigation test file browser_inspector_bug_566084_location_changed.js failed. Fixed it.
- the InspectorUI.isDirty flag wasn't properly remembered across tab switches, nor was it cleared on page navigation. Fixed.
- there was some code confusion over InspectorUI.inspectCmd. The menuitem checkbox needs to be enabled/disabled when the inspector opens/closes. The Inspect toolbutton needs to be checked/unchecked when the inspecting (highlighting) mode is active/inactive. These were not working properly and this patch did touch related code. Fixed.
- removed the getToolbarButtonId(), bindToolButtonEvent(), unbindToolButtonEvent(), bindToolPanelEvents() and unbindToolPanelEvents() methods from IUI, as agreed in comment 22.

I hope my patch update makes it easier for you to update it again when bug 681653 changes.

Comment 29

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

Attachment: disable and modularize html panel v17 (rebased 09/06)

new version based on updated register style and augmented registerTools API.

Comment 30

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

Attachment: disable and modularize html panel v17 (rebased 09/07)

updated based on new registerTools API. All tests are passing.

Comment 31

[Mihai Sucan [:msucan]]

This patch actually depends on the patch from bug 681653, which in turn depends on bug 681653. (updating to make it easier for others to follow the deps tree)

Comment 32

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

Attachment: disable and modularize html panel v18 (rebased 09/20)

Folded contents of bug 681651 into this patch as requested.

Comment 33

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

also, there's a scratchpad makefile change included in this patch. I can leave it out, but included it as this set because I was "in there".

Comment 34

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

Attachment: disable and modularize html panel v19 (rebased 09/21)

cleaned and polished.

Comment 35

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

Attachment: disable and modularize html panel v19 (requires patch from bug 687854)

Comment 36

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

Attachment: disable and modularize html panel v19 (requires patch from bug 687854)

rebased onto context menu patch.

Comment 37

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

Attachment: [in-fx-team] disable and modularize html panel v20 (requires patch from bugs 687854, 668558)

fixes for re-entrant inspect context menu.

Comment 38

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

Comment on attachment 561818 [details] [diff] [review]
[in-fx-team] disable and modularize html panel v20 (requires patch from bugs 687854, 668558)

https://hg.mozilla.org/integration/fx-team/rev/121518f3df44

Comment 39

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

https://hg.mozilla.org/mozilla-central/rev/121518f3df44

Comment 40

[Dão Gottwald [::dao]]

Backed out because of various new mochitest-browser-chrome leaks. Unfortunately, this landed with a bunch of other interweaved patches. I only felt comfortable ruling out f297a626dd13 and 7d5311c92e04.

Comment 41

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

relanded:

https://hg.mozilla.org/integration/fx-team/rev/c3cc5ae0e2a9

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c3cc5ae0e2a9