Last Comment Bug 672002 - Move HTML panel to a docked "panel" in the main browser window (highlighter)
: Move HTML panel to a docked "panel" in the main browser window (highlighter)
Status: VERIFIED FIXED
[minotaur][best: 2h; likely: 1d; wors...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 9
Assigned To: Rob Campbell [:rc] (:robcee)
:
:
Mentors:
Depends on: 650794 681651
Blocks: 663830 671294
  Show dependency treegraph
 
Reported: 2011-07-15 16:55 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-11-25 06:47 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
docked tree panel WIP (8.85 KB, patch)
2011-09-08 12:38 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review+
Details | Diff | Splinter Review
docked tree panel v1 (11.81 KB, patch)
2011-09-21 06:39 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review+
Details | Diff | Splinter Review
docked tree panel v1.1 - requires patch from bug 687854 (11.81 KB, patch)
2011-09-21 07:41 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
[in-fx-team] docked tree panel v1.2 - requires patch from bug 687854 (11.37 KB, patch)
2011-09-22 05:33 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2011-07-15 16:55:33 PDT
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) 2011-07-25 17:21:50 PDT
Could take a few days if the styling turns out to be tricky. Not anticipating issues though.
Comment 2 Rob Campbell [:rc] (:robcee) 2011-09-08 12:38:02 PDT
Created attachment 559249 [details] [diff] [review]
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] 2011-09-09 04:08:01 PDT
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) 2011-09-21 06:39:21 PDT
Created attachment 561450 [details] [diff] [review]
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) 2011-09-21 07:41:41 PDT
Created attachment 561462 [details] [diff] [review]
docked tree panel v1.1 - requires patch from bug 687854
Comment 6 Mihai Sucan [:msucan] 2011-09-21 10:28:38 PDT
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) 2011-09-22 05:27:38 PDT
(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) 2011-09-22 05:31:11 PDT
ok, I tried it out and inlined the two functions. Looks somewhat better. :)
Comment 9 Rob Campbell [:rc] (:robcee) 2011-09-22 05:33:11 PDT
Created attachment 561709 [details] [diff] [review]
[in-fx-team] docked tree panel v1.2 - requires patch from bug 687854

updated based on mihai's comments.
Comment 10 Mihai Sucan [:msucan] 2011-09-22 09:05:11 PDT
(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) 2011-09-23 15:38:18 PDT
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) 2011-09-23 15:52:37 PDT
https://hg.mozilla.org/mozilla-central/rev/b1c0b12a5f65
Comment 13 Dão Gottwald [:dao] 2011-09-24 03:37:23 PDT
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) 2011-09-26 10:40:23 PDT
relanded:

https://hg.mozilla.org/integration/fx-team/rev/e9e8df65e841
Comment 15 Tim Taubert [:ttaubert] 2011-09-27 04:28:38 PDT
https://hg.mozilla.org/mozilla-central/rev/e9e8df65e841
Comment 16 Ioan C. 2011-11-25 06:47:30 PST
Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111123 Firefox/10.0a2
Verified fixed. Screencast: http://screencast.com/t/s50of7J58

Note You need to log in before you can comment on or make changes to this bug.