672002 - Move HTML panel to a docked "panel" in the main browser window (highlighter)

Status: VERIFIED FIXED

(DevTools :: General, defect, P1)

Description

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

The HTML panel should be docked into the main browser window by default. It should be animated on opening and closing similar to the way the Web Console animates.

Comment 1

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

Could take a few days if the styling turns out to be tricky. Not anticipating issues though.

Comment 2

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

Attachment: docked tree panel WIP

have a look.

There's a dead method in TreePanel.jsm (createSplitter) that I'm not using and can remove. The createResizer sets a style attribute on the resizer that should be moved into the theme files probably with a bumpy background so users can see there's something there before hovering over it.

There are no tests and the code path to access the previous panel-based layout is currently unavailable.

other than that... it's based on the patch to move the tree panel and it's dependencies in bug 681651.

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 559249 [details] [diff] [review]
docked tree panel WIP

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

Patch looks great! r+ for a WIP, but please address the comments below.

One general comment: panel size is not remembered. This is "weird" when the user opens/reopens the panel, or when he switches tabs. We should have persistence, at least per browser session. (follow up bug report?)

::: browser/devtools/highlighter/TreePanel.jsm
@@ +66,4 @@
>  
>    /**
>     * container.
> +   * @returns xul:panel|xul:vbox|Object element

/**
 * The tree panel container element.
 * @returns xul:panel|xul:vbox|null
 *          xul:panel is returned when the tree panel is not docked, or xul:vbox when when the tree panel is docked. null is returned when no container is available.
 */

@@ +71,5 @@
>    get container()
>    {
> +    if (this.openInDock) {
> +      let vbox = this.document.getElementById("inspector-tree-box");
> +      return vbox ? vbox : { state: "closed" };

This should just return vbox, even if it's null. The object mock is not something we'd like in the longer run. Callers need to deal with the "no container" case.

@@ +144,5 @@
>  
> +  loadedInitializeTreePanel: function TP_loadedInitializeTreePanel()
> +  {
> +    this.treeIFrame.removeEventListener("load", this.loadedInitializeTreePanel,
> +      true);

This event listener removal doesn't work. You do:

this.treeIFrame.addEventListener("load",
  this.loadedInitializeTreePanel.bind(this), true);

The bound function is a different function, so the removeEventListener call is ineffective.

@@ +170,4 @@
>      }
>  
> +    if (this.openInDock) { // Create vbox
> +      return this.openDocked();

The open() method returns void. openDocked() also returns void.

I would suggest: this.openDocked(); return; (to avoid confusion)

@@ +181,1 @@
>          treePanelShown, false);

This removeEventListener() call is also ineffective. The bound function is different.

@@ +208,5 @@
>    },
>  
> +  openDocked: function TP_openDocked()
> +  {
> +    let treeBox = null;

This line is not needed. Below you can just do: let treeBox = this.document...

@@ +210,5 @@
> +  openDocked: function TP_openDocked()
> +  {
> +    let treeBox = null;
> +    let toolbar = this.IUI.toolbar.nextSibling; // Addons bar, typically
> +    let toolbarParent = toolbar.parentNode;

This code is probably prone to errors. Can't we get to that element in a more specific way? Does it have an ID? or something

@@ +213,5 @@
> +    let toolbar = this.IUI.toolbar.nextSibling; // Addons bar, typically
> +    let toolbarParent = toolbar.parentNode;
> +    treeBox = this.document.createElement("vbox");
> +    treeBox.id = "inspector-tree-box";
> +    treeBox.state = "open"; // for the registerTools API.

Instead of this change I'd prefer updating our isOpen() implementation to correctly handle the docked mode.

@@ +215,5 @@
> +    treeBox = this.document.createElement("vbox");
> +    treeBox.id = "inspector-tree-box";
> +    treeBox.state = "open"; // for the registerTools API.
> +    treeBox.minHeight = 10;
> +    treeBox.setAttribute("flex", "1");

Doesn't treeBox.flex = 1 work here?

@@ +218,5 @@
> +    treeBox.minHeight = 10;
> +    treeBox.setAttribute("flex", "1");
> +    toolbarParent.insertBefore(treeBox, toolbar);
> +    this.createResizer();
> +    this.treeIFrame = treeBox.appendChild(this.treeIFrame);

appendChild() returns the appended child element - no need to do the prop update. Just do:

treeBox.appendChild(this.treeIFrame);

@@ +225,5 @@
> +
> +    let src = this.treeIFrame.getAttribute("src");
> +    if (src != "chrome://browser/content/inspector.html") {
> +      this.treeIFrame.setAttribute("src",
> +        "chrome://browser/content/inspector.html");

We should put this URI into a constant at the top of the JSM.

@@ +229,5 @@
> +        "chrome://browser/content/inspector.html");
> +    } else {
> +      this.treeIFrame.contentWindow.location.reload();
> +    }
> +    return;

This is not needed here.

@@ +242,5 @@
> +    resizer.id = "inspector-horizontal-splitter";
> +    resizer.setAttribute("dir", "top");
> +    resizer.setAttribute("flex", "1");
> +    resizer.setAttribute("element", "inspector-tree-box");
> +    resizer.setAttribute("style", "background: none !important; -moz-appearance: none; cursor: n-resize;");

This should probably be in the inspector stylesheet?

@@ +251,5 @@
> +
> +  /**
> +   * Create a splitter before the inspector toolbar. NOT USED. (but I'd like to)
> +   */
> +  createSplitter: function TP_createSplitter()

In the final patch I suggest that this is either dropped or used. It doesn't make sense to add unused code.

Comment 4

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

Attachment: docked tree panel v1

Addressed Mihai's comments. Have a quick look-over to see that I caught everything.

Will add a followup to persist tree panel height.

Comment 5

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

Attachment: docked tree panel v1.1 - requires patch from bug 687854

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 561450 [details] [diff] [review]
docked tree panel v1

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

Patch looks good! I only have two minor comments. See below.

::: browser/devtools/highlighter/TreePanel.jsm
@@ +145,5 @@
>      if (this.IUI.selection)
>        this.select(this.IUI.selection, true);
>    },
>  
> +  loadedInitializeTreePanel: function TP_loadedInitializeTreePanel()

I really don't like this method.

@@ +192,5 @@
> +      this.container.removeEventListener("popupshown",
> +        boundTreePanelShown, false);
> +
> +        this.treeIFrame.addEventListener("load",
> +          boundLoadedInitializeTreePanel, true);

Wrong indentation level for these two lines.

@@ +236,5 @@
> +    treeBox.appendChild(this.treeIFrame);
> +
> +    let boundLoadedInitializeTreePanel =
> +      this.boundLoadedInitializeTreePanel =
> +        this.loadedInitializeTreePanel.bind(this);

I would prefer that instead of the boundLoadedInitializeTreePanel() function, you'd do something like the treePanelShown() above.

(same in both locations where the treeIframe is loaded)

Comment 7

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

(In reply to Mihai Sucan [:msucan] from comment #6)
> Comment on attachment 561450 [details] [diff] [review]
> docked tree panel v1
> 
> Review of attachment 561450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good! I only have two minor comments. See below.
> 
> ::: browser/devtools/highlighter/TreePanel.jsm
> @@ +145,5 @@
> >      if (this.IUI.selection)
> >        this.select(this.IUI.selection, true);
> >    },
> >  
> > +  loadedInitializeTreePanel: function TP_loadedInitializeTreePanel()
> 
> I really don't like this method.

Well, it's better than duplicating it in both locations, IMO. Have any recommendations?

> @@ +192,5 @@
> > +      this.container.removeEventListener("popupshown",
> > +        boundTreePanelShown, false);
> > +
> > +        this.treeIFrame.addEventListener("load",
> > +          boundLoadedInitializeTreePanel, true);
> 
> Wrong indentation level for these two lines.

oops!

> @@ +236,5 @@
> > +    treeBox.appendChild(this.treeIFrame);
> > +
> > +    let boundLoadedInitializeTreePanel =
> > +      this.boundLoadedInitializeTreePanel =
> > +        this.loadedInitializeTreePanel.bind(this);
> 
> I would prefer that instead of the boundLoadedInitializeTreePanel()
> function, you'd do something like the treePanelShown() above.
> 
> (same in both locations where the treeIframe is loaded)

You mean inline the function definition? Again, I'm not keen on duplicating it if at all possible, though it is only a few lines...

Comment 8

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

ok, I tried it out and inlined the two functions. Looks somewhat better. :)

Comment 9

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

Attachment: [in-fx-team] docked tree panel v1.2 - requires patch from bug 687854

updated based on mihai's comments.

Comment 10

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (robcee) from comment #7)
> (In reply to Mihai Sucan [:msucan] from comment #6)
> > Comment on attachment 561450 [details] [diff] [review]
> > docked tree panel v1
> > 
> > Review of attachment 561450 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Patch looks good! I only have two minor comments. See below.
> > 
> > ::: browser/devtools/highlighter/TreePanel.jsm
> > @@ +145,5 @@
> > >      if (this.IUI.selection)
> > >        this.select(this.IUI.selection, true);
> > >    },
> > >  
> > > +  loadedInitializeTreePanel: function TP_loadedInitializeTreePanel()
> > 
> > I really don't like this method.
> 
> Well, it's better than duplicating it in both locations, IMO. Have any
> recommendations?
> 
> > @@ +236,5 @@
> > > +    treeBox.appendChild(this.treeIFrame);
> > > +
> > > +    let boundLoadedInitializeTreePanel =
> > > +      this.boundLoadedInitializeTreePanel =
> > > +        this.loadedInitializeTreePanel.bind(this);
> > 
> > I would prefer that instead of the boundLoadedInitializeTreePanel()
> > function, you'd do something like the treePanelShown() above.
> > 
> > (same in both locations where the treeIframe is loaded)
> 
> You mean inline the function definition? Again, I'm not keen on duplicating
> it if at all possible, though it is only a few lines...

Exactly, it's only a line. We have the event handler, initializeIFrame(), but we wrap it in a function that removes the event listener. My point was that for a single line there's no point to add a method to the TreePanel object.

Thanks for the patch update!

Comment 11

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

Comment on attachment 561709 [details] [diff] [review]
[in-fx-team] docked tree panel v1.2 - requires patch from bug 687854

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

Comment 12

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

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

Comment 13

[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 14

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

relanded:

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

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 16

[Ioan C.]

Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111123 Firefox/10.0a2
Verified fixed. Screencast: http://screencast.com/t/s50of7J58