577721 - allow repositioning of the web console: above, below and out into a panel/window

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[David Dahl :ddahl]

We should allow the console to pop out into a window so we do not use so much screen real estate

Comment 1

[Biju]

It looks like for each kind of tabs we are trying to write custom code.
Why can it be like Eclipse IDE http://www.eclipse.org/screenshots/
The TAB frame itself provide Detach, Move, Dock facilities.

We should just enhance XUL TAB/TABPANEL elements. 
That should automatically take care of things like this.

Comment 2

[David Dahl :ddahl]

(In reply to comment #1)
> It looks like for each kind of tabs we are trying to write custom code.
> Why can it be like Eclipse IDE http://www.eclipse.org/screenshots/
> The TAB frame itself provide Detach, Move, Dock facilities.
> 
> We should just enhance XUL TAB/TABPANEL elements. 
> That should automatically take care of things like this.

Except in Firefox, the "tab" dom node is actually a notificationBox node. The code to do this is rather trivial, it just hasen't hit my queue yet.

Comment 3

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

I think if we switch to using a panel, we can make use of the "dockable panels" mechanism to be implemented later. (bug 554926)

Rather than duplicate that effort, we could move this to "future".

Comment 4

[David Dahl :ddahl]

(In reply to comment #3)
> I think if we switch to using a panel, we can make use of the "dockable panels"
> mechanism to be implemented later. (bug 554926)
> 
> Rather than duplicate that effort, we could move this to "future".

Yes, that will be the first avenue to try.

Comment 6

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

David, are you working on this? If not, you should take yourself out of the assignee field so someone else can work on it.

Comment 7

[David Dahl :ddahl]

I am not actively working on this if you want to take a crack at it.

Comment 8

[David Dahl :ddahl]

(In reply to comment #7)
> I am not actively working on this if you want to take a crack at it.

I have started to get this working via our devtools webconsole api repo on github:

https://github.com/daviddahl/devtools

Comment 10

[David Dahl :ddahl]

Attachment: v 0.1 WIP patch

Comment 11

[David Dahl :ddahl]

Attachment: v 0.2 WIP

Comment 12

[David Dahl :ddahl]

Attachment: v 0.3 folded all positioning patches intot his one

Dao: Trying to figure out the CSS to make the panel resize properly, right now I have to set the height attr to the height of the panel to get the console node to fill up the space, but resizing to a shorter height is not working. Also, the panel itself seems to not take a background-color property. What have I done wrong?

david

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 521642 [details] [diff] [review]
v 0.3 folded all positioning patches intot his one

>+// Remember the Web Console position
>+pref("devtools.hud.position", "above");

No more "hud", please.

(This is just drive-by at this point, not a full review.)

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #12)
> Dao: Trying to figure out the CSS to make the panel resize properly, right now
> I have to set the height attr to the height of the panel to get the console
> node to fill up the space, but resizing to a shorter height is not working.

You might be doing it the wrong way around... The panel should automatically grow with its content, as I understand it. So maybe you should resize the container instead of the panel.

> Also, the panel itself seems to not take a background-color property. What have
> I done wrong?

You need to set -moz-appearance:none in order to set a background color.

Comment 16

[David Dahl :ddahl]

(In reply to comment #14)
> Comment on attachment 521642 [details] [diff] [review]
> v 0.3 folded all positioning patches intot his one
> 
> >+// Remember the Web Console position
> >+pref("devtools.hud.position", "above");
> 
> No more "hud", please.
> 
> (This is just drive-by at this point, not a full review.)

Cool. I appreciate that - however we have several clean up bugs to remove hud naming, etc... I do not want to expand this bug too much.

Comment 17

[Dão Gottwald [:dao]]

I wasn't asking for cleanup here, I'm just saying "hud" shouldn't creep into new code.

Comment 18

[David Dahl :ddahl]

(In reply to comment #15)

> You might be doing it the wrong way around... The panel should automatically
> grow with its content, as I understand it. So maybe you should resize the
> container instead of the panel.

Ah. ok.

Is there a difference between setting the height attr and using css in this case? 

> 
> You need to set -moz-appearance:none in order to set a background color.

ok, so that must override all other selectors

Comment 19

[David Dahl :ddahl]

(In reply to comment #17)
> I wasn't asking for cleanup here, I'm just saying "hud" shouldn't creep into
> new code.

ok. I'll just add devtools.webconsole.position

Comment 20

[David Dahl :ddahl]

(In reply to comment #18)
> (In reply to comment #15)
> 
> > You might be doing it the wrong way around... The panel should automatically
> > grow with its content, as I understand it. So maybe you should resize the
> > container instead of the panel.

I specifically set the height when the panel opens to an arbitrary value of 300 when going from in browser frame UI to panel, and this is the result:

http://img6.imagebanana.com/img/mypzg511/Selection_022.png

The height is always too short.

then, if I set the height of the panel and set the height of the inner vbox, it looks ok, but  when you resize the panel you cannot resize height.

Comment 21

[David Dahl :ddahl]

Attachment: Weird Mac OS behavior

When I close the window-panel console on mac, the panel is still visible, but not usable - as if all widgets are disabled. Windows and Linux work fine.

Comment 22

[David Dahl :ddahl]

Attachment: v 0.3.1 Tweaks while testing on mac

Comment 23

[David Dahl :ddahl]

Attachment: Screencast of Mac Panel problem

This is a screencast of the panel not closing on macos only when moving the console nodes back into the notification box.

Comment 24

[David Dahl :ddahl]

Attachment: v 0.3.2 Fixed weird panel issue

Comment 25

[David Dahl :ddahl]

Attachment: v 0.4 Added tests

Comment 26

[David Dahl :ddahl]

Still dealing with some DOM and hudreference problems. tests are written, will try to submit for review tomorrow. Ugh.

Comment 27

[David Dahl :ddahl]

Attachment: 0.5 Patch with Tests

I still think there are a few tweaks to make here. this patch needs another set of eyeballs on it - I will still be working on the tests a bit.

Comment 28

[Mihai Sucan [:msucan]]

Comment on attachment 523692 [details] [diff] [review]
0.5 Patch with Tests

This looks good. Really nice stuff!

Review comments:

+// Remember the Web Console position
+pref("devtools.hud.position", "above");

The comment needs to tell which are the possible values.

+    try {
+      // try to remove the panel node if it exists
+      let panelNode = ownerDoc.getElementById("console_window_" + id);
+      panelNode.parentNode.removeChild(panelNode);
+    }
+    catch (ex) { }

You do not need to wrap that in a try-catch. Just do:

+    // remove the panel node if it exists
+    let panelNode = ownerDoc.getElementById("console_window_" + id);
+    if (panelNode) {
+      panelNode.parentNode.removeChild(panelNode);
+    }

If it fails like this, then there's a bug in some other place in the code which needs a fix - I doubt it fails.

- I think the changes in HUDService.animate() are unrelated and not necessary. Why did you wrap the code into try-catch which does Cu.reportError(ex)? Exceptions in the code show up anyway in the error console, without Cu.reportError().


- In HUDService.windowInitializer():

+    let hudPanelId = "console_window_" + hudId;
+    let windowUI = nBox.ownerDocument.getElementById(hudPanelId);

The hudPanelId variable is not used in other places. Why not just do getElementById("console_window_" + hudId)?

- In the HeadsUpDisplay object constructor:

-  this.notificationBox.insertBefore(splitter,
+  this.splitter = splitter;
+  if (Services.prefs.getCharPref("devtools.hud.position") != "window") {
+      this.notificationBox.insertBefore(splitter,
                                     this.notificationBox.childNodes[1]);
+  }
+  else {
+    this.consoleSplitter = splitter;
+  }

This code is confusing, and confusion stayed with me when I read the other parts of the code that use the two splitters. I understand why you do it, but it's not needed to have two properties on the HUD object. Just keep the HUD.splitter and work with it - you can remove it from the document and keep the ref in the HUD object.

(I would like this fixed, it's not just a nit-pick request.)

- In the HeadsUpDisplay object:

+  get mainPopupSet()
+  {
+    return this.notificationBox.ownerDocument.getElementById("mainPopupSet");
+  },

You have this.chromeDocument that you can use.


- In the HUD_createOwnWindowPanel() function:

+    panel.addEventListener("popupshown", function onPopupShow() {
+      panel.removeEventListener("popupshown", onPopupShow, false);
+    }, false);

Empty event handler that needs to be removed.


- In the same function you have:

+    if (!this.HUDBox.parentNode) {
+      // we have no existing console open to snip DOM Nodes from
+      panel.appendChild(this.HUDBox);
+    }
+    else {
+      let consoleNode = this.HUDBox.parentNode.removeChild(this.HUDBox);
+      let consoleSplitter = this.splitter.parentNode.removeChild(this.splitter);
+       // keep splitter around to re-attach if needed
+      this.consoleSplitter = consoleSplitter;
+      panel.appendChild(consoleNode);
+      this.HUDBox = consoleNode;
+    }

I would rewrite that as:

+    if (this.HUDBox.parentNode) {
+      this.splitter.parentNode.removeChild(this.splitter);
+    }
+    panel.appendChild(this.HUDBox);

... it does the same: (1) it moves the HUDBox DOM node and its children into panel, (2) it removes the splitter from the DOM while keeping a ref.

Tip: if HUDBox has no parentNode then panel.appendChild() works as you wanted, but if HUDBox has a parentNode you do not need to do removeChild, store a ref, and then appendChild - you simply do appendChild() - the browser removes the HUDBox from its original parentNode and appends the node into the panel node.

The this.splitter reference is unaffected by removals, appends and other changes - making this.consoleSplitter an unneeded reference. Same goes for the HUDBox.

(please correct me if I am wrong!)

- In the same function you have:

+    let space = this.notificationBox.ownerDocument.createElement("spacer");
...
+    let bottomBox = this.notificationBox.ownerDocument.createElement("hbox");
...
+    let resizer = this.notificationBox.ownerDocument.createElement("resizer");

You can use this.chromeDocument.

- In the same function you have:

+    panel.sizeTo(800, this.HUDBox.style.height);

this.HUDBox.style.height is a string which includes "px". I believe you should do parseInt(this.HUDBox.style.height).

The panel width should be the computed width of the hudBox.

- In HUD_createPositionUI() you do not need let self = this.

+    let menuItem;
+
+    menuItem = this.makeXULNode("menuitem");

Why not let menuItem = ...?

+    menuItem.addEventListener("command", self.positionAbove.bind(self), false);

s/self/this/g

- In HUD_makeCloseButton():

         if (ownerDocument.getElementById(hudId)) {
           HUDService.deactivateHUDForContext(tab, true);
         }
+        if (self.consolePanel) {
+          self.consolePanel.hidePopup();
+        }

Somehow, I think that's wrong - it must lead to errors. For one, you have a popuphidden event handler that calls unregisterDisplay(), and here you also call deactivateHUDForContext() ... which does the same.

I suggest removing the call to hidePopup(). If the code fails, I think we need a better solution than what is now in the patch.

This might as well lead to unexpected errors like the one you told me about on IRC: the web console ends up in a state that it no longer opens again.

- In HUD_createHUD() you have:

+          let panel = this.createOwnWindowPanel(this.HUDBox);

... the createOwnWindowPanel() method takes no arguments. I believe this remained from some previous code iteration.

- In the mochitest file:

+  let hudId = HUDService.displaysIndex()[0];
+  let hudRef = HUDService.hudReferences[hudId];
+  let hudBox = HUDService.getHeadsUpDisplay(hudId);

The two methods are deprecated - they will be removed in an upcoming cleanup patch that is waiting for review.

getHeadsUpDisplay() method actually does this.hudReferences[aHudId].HUDBox.

Please rewrite this as:

+  let hudId = HUDService.getHudIdByWindow(content);
+  let hudRef = HUDService.hudReferences[hudId];
+  let hudBox = hudRef.hudBox;

Later in the test you do:

+  hudRef.positionConsole("below");
+  let node = hudRef.HUDBox.parentNode;
+  let id = hudRef.HUDBox.parentNode.childNodes.item(2).getAttribute("id");
+  ok(id == hudId, "below position is correct");

You can reuse the hudBox variable you already have. The reference stays valid across repositioning, as long as the node is only moved (not re-created).

- The mochitest fails to run for me, and all subsequent HUDService mochitests fail to run - it leaves the HUDService in a broken state.


Comments based on user testing:

- the Position menupopup is fine, or you can move it into the context menu (right-click in the web console).

- the menuitem checkboxes do not update. You need to mark the current position as checked.

- the windowed (as a panel) web console is awesome!

- the panel is resizable but the web console output box does not flex. The output node height must change based on the panel size - otherwise it looks like unfinished code. :)

- and it was easy to bump into the bug you mentioned. The Web Console cannot reopen after you play with its position. hudBox is null in HS_animate() (line 2829), which means getOutputNodeById() fails. I believe there's a bug with when you leave the web console in the windowed position and you try to re-open the console.

- I also get this.jsterm is null (after restart, with position=window), line 3185:
    this.jsterm.inputNode.focus();

- after restart with position=window the panel opens with height=0, and if i resize the panel, the console output doesn't show up.


After code investigation I see some problems:

- In createHUD() you call createOwnWindowPanel() which assumes that this.jsterm is available, but createConsoleInput() is called later in the HUD constructor.

- Another problem is with the panel height. After restart the outputNode height is 0 until resetHeight() is called by the animate() method. Still, the createOwnWindowPanel() expects that HUDBox.style.height is correct. Again it looks like the call to createOwnWindowPanel() is too early in the HUD initialization.

That's all for now. Sorry for the long comment.

The patch and the new functionality is awesome. It's almost there, indeed. Giving the patch r- until the main problems are fixed.

Comment 29

[David Dahl :ddahl]

(In reply to comment #28)
> Comment on attachment 523692 [details] [diff] [review]

> - I think the changes in HUDService.animate() are unrelated and not necessary.
> Why did you wrap the code into try-catch which does Cu.reportError(ex)?
> Exceptions in the code show up anyway in the error console, without
> Cu.reportError().
> 
my bad. That was from a debugging session.

> 
> - In HUDService.windowInitializer():
> 
> +    let hudPanelId = "console_window_" + hudId;
> +    let windowUI = nBox.ownerDocument.getElementById(hudPanelId);
> 
> The hudPanelId variable is not used in other places. Why not just do
> getElementById("console_window_" + hudId)?
> 
check. 

> - In the HeadsUpDisplay object constructor:
> 
> -  this.notificationBox.insertBefore(splitter,
> +  this.splitter = splitter;
> +  if (Services.prefs.getCharPref("devtools.hud.position") != "window") {
> +      this.notificationBox.insertBefore(splitter,
>                                      this.notificationBox.childNodes[1]);
> +  }
> +  else {
> +    this.consoleSplitter = splitter;
> +  }
> 
> This code is confusing, and confusion stayed with me when I read the other
> parts of the code that use the two splitters. I understand why you do it, but
> it's not needed to have two properties on the HUD object. Just keep the
> HUD.splitter and work with it - you can remove it from the document and keep
> the ref in the HUD object.
> 
> (I would like this fixed, it's not just a nit-pick request.)
> 
But, Mihai: I really enjoy making thins over-complicated. (no I don't, thanks:))

> - In the HeadsUpDisplay object:
> 
> +  get mainPopupSet()
> +  {
> +    return this.notificationBox.ownerDocument.getElementById("mainPopupSet");
> +  },
> 
> You have this.chromeDocument that you can use.
> 
check.

> 
> - In the HUD_createOwnWindowPanel() function:
> 
> +    panel.addEventListener("popupshown", function onPopupShow() {
> +      panel.removeEventListener("popupshown", onPopupShow, false);
> +    }, false);
> 
> Empty event handler that needs to be removed.
> 
Good catch. That was a copy-paste issue.

> 
> - In the same function you have:
> 
> +    if (!this.HUDBox.parentNode) {
> +      // we have no existing console open to snip DOM Nodes from
> +      panel.appendChild(this.HUDBox);
> +    }
> +    else {
> +      let consoleNode = this.HUDBox.parentNode.removeChild(this.HUDBox);
> +      let consoleSplitter =
> this.splitter.parentNode.removeChild(this.splitter);
> +       // keep splitter around to re-attach if needed
> +      this.consoleSplitter = consoleSplitter;
> +      panel.appendChild(consoleNode);
> +      this.HUDBox = consoleNode;
> +    }
> 
> I would rewrite that as:
> 
> +    if (this.HUDBox.parentNode) {
> +      this.splitter.parentNode.removeChild(this.splitter);
> +    }
> +    panel.appendChild(this.HUDBox);

Well done!

> - In the same function you have:
> 
> +    let space = this.notificationBox.ownerDocument.createElement("spacer");
> ...
> +    let bottomBox = this.notificationBox.ownerDocument.createElement("hbox");
> ...
> +    let resizer = this.notificationBox.ownerDocument.createElement("resizer");
> 
> You can use this.chromeDocument.
check.

> 
> - In the same function you have:
> 
> +    panel.sizeTo(800, this.HUDBox.style.height);
> 
> this.HUDBox.style.height is a string which includes "px". I believe you should
> do parseInt(this.HUDBox.style.height).
> 
> The panel width should be the computed width of the hudBox.
> 
check.

> - In HUD_createPositionUI() you do not need let self = this.
> 
> +    let menuItem;
> +
> +    menuItem = this.makeXULNode("menuitem");
> 
> Why not let menuItem = ...?
Probably detritus there. check.

> 
> +    menuItem.addEventListener("command", self.positionAbove.bind(self),
> false);
> 
> s/self/this/g
check.

> 
> - In HUD_makeCloseButton():
> 
>          if (ownerDocument.getElementById(hudId)) {
>            HUDService.deactivateHUDForContext(tab, true);
>          }
> +        if (self.consolePanel) {
> +          self.consolePanel.hidePopup();
> +        }
> 
> Somehow, I think that's wrong - it must lead to errors. For one, you have a
> popuphidden event handler that calls unregisterDisplay(), and here you also
> call deactivateHUDForContext() ... which does the same.
> 
> I suggest removing the call to hidePopup(). If the code fails, I think we need
> a better solution than what is now in the patch.
> 
> This might as well lead to unexpected errors like the one you told me about on
> IRC: the web console ends up in a state that it no longer opens again.
> 
ok. will test this again.


> - In HUD_createHUD() you have:
> 
> +          let panel = this.createOwnWindowPanel(this.HUDBox);
> 
> ... the createOwnWindowPanel() method takes no arguments. I believe this
> remained from some previous code iteration.
> 
yep. 

> - In the mochitest file:
> 
> +  let hudId = HUDService.displaysIndex()[0];
> +  let hudRef = HUDService.hudReferences[hudId];
> +  let hudBox = HUDService.getHeadsUpDisplay(hudId);
> 
> The two methods are deprecated - 
check.

Will continue tomorrow - thanks1

Comment 30

[David Dahl :ddahl]

Attachment: 0.5.1 saving work

Comment 31

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch.

Changes from the previous patch:

- addressed all the review comments from comment 28.

- made the menuitem checkboxes to show the current console position.

- the web console output now resizes when you resize the web console panel.

- now the web console reopens properly after playing with the position choice.

- fixed the jsterm focus error.

- if you start firefox with the web console as a panel, first open remembers the last panel dimensions and location on screen.

- added prefs for web console width, top and left location.

- fixed the way hudservice activation/deactivation works for a tab. this prevented the web console positioning patch from working properly. for example we called unregisterDisplay() but deactivateHUDForContext() did more stuff we needed. now with the changes in this patch we can always, reliably, just call deactivateHUDForContext() - never directly call unregisterDisplay().

- for the above fix we needed a new HUD.tab property - to find the tab of the HUD. I added this, but a better fix would be in bug 656231 that I reported as a followup. I discussed this with ddahl and he agrees that would be better.

- made sure that the output scroll location is remembered when the user changes the web console position. the previous patch did always reset the scroll to 0. (it's something I forgot to mention in the review)

- updated the test accordingly, and fixed it so it runs fine.

Looking forward to your reviews, thanks!

Comment 32

[Shawn Wilsher :sdwilsh]

Comment on attachment 531650 [details] [diff] [review]
proposed patch

Please flag me for review after robcee has reviewed (and you've addressed any comments he's raised).

Comment 33

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

this looks really solid.

I asked in #fx-team if there was a preferred way to handle positioning of window elements, thinking localstore.rdf and the document.persist method might be preferable, but because there is no concrete ID for the panel elements, that wouldn't work.

Prefs seem to be ok.

The removal of this.unregisterDisplay from shutdown seems like a good addition.

I wonder if we'll need some additional CSS for the createPositionUI toolbar items? The styling seemed reasonable enough, though without any indication of hover or pressed state.

Also in createPositionUI, it seems like we could break out a new function to set the different attributes on each item. That feels like a nit, however, and the cost of the extra function isn't really worth the effort, imo.

All hudtests passed locally with this applied.

Comment 34

[Mihai Sucan [:msucan]]

Comment on attachment 531650 [details] [diff] [review]
proposed patch

Thank you Robert for the r+! I also thought of using the persist feature of XUL, but afaik that doesn't work with panels as we use them here. Given I don't know all the xul features, maybe it's possible. If you have any ideas on how to do it, please let me know.

I think createPositionUI is small enough - doesn't seem to need to be split. (IMO)

Asking Shawn for review.

Comment 35

[Shawn Wilsher :sdwilsh]

Comment on attachment 531650 [details] [diff] [review]
proposed patch

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

r=sdwilsh

::: browser/app/profile/firefox.js
@@ +1010,5 @@
> +// The last Web Console window panel width. This is initially 0 which means that
> +// the Web Console will use the default width next time it shows. That's the
> +// browser window width.
> +// Change to -1 if you do not want the Web Console to remember its last width.
> +pref("devtools.hud.width", 0);

I don't see value in putting this preference in firefox.js

@@ +1015,5 @@
> +
> +// The last Web Console window panel location, relative to the web page
> +// viewport.
> +pref("devtools.hud.top", 0);
> +pref("devtools.hud.left", 0);

Same with these two.

::: toolkit/components/console/hudservice/HUDService.jsm
@@ +1760,5 @@
>        }
>      }
> +
> +    ownerDoc = outputNode.ownerDocument;
> +    // remove the outputNode first

Obviously you are removing it first.  This comment doesn't add much value unless you indicate why you are removing it first.

@@ +2798,5 @@
>      let nBoxId = nBox.getAttribute("id");
>      let hudId = "hud_" + nBoxId;
> +    let windowUI = nBox.ownerDocument.getElementById("console_window_" + hudId);
> +    if (windowUI) {
> +      // we have an existing console in a panel 'window', exit

This comment isn't using proper grammar, and it doesn't really tell me why we are exiting early apart from we have an existing console.  Please elaborate.

@@ +3177,5 @@
> +   */
> +  createOwnWindowPanel: function HUD_createOwnWindowPanel()
> +  {
> +    if (this.uiInOwnWindow)
> +      return this.consolePanel;

nit: brace all ifs

@@ +3191,5 @@
> +    let left = Services.prefs.getIntPref("devtools.hud.left");
> +
> +    let panel = this.chromeDocument.createElementNS(XUL_NS, "panel");
> +
> +    let self = this;

either move this down to just before you use it, or call .bind(this) on the event listeners you use it in (preferred).

@@ +3348,5 @@
> +    // get the node position index
> +    let nodeIdx = this.positions[aPosition];
> +
> +    // check to see if console is already positioned in aPosition
> +    if (this.notificationBox.childNodes[nodeIdx] == this.HUDBox) {

Given the frequency that you use this.notificationBox and this.notificationBox.childNodes below, I think it's a good idea to create two locals:
let notificationBox = this.notificationBox;
let childNodes = notificationBox.childNodes;
(and update all consumers below)

@@ +3829,3 @@
>      function HUD_closeButton_onCommand() {
> +      HUDService.animate(self.hudId, ANIMATE_OUT, function() {
> +        HUDService.deactivateHUDForContext(self.tab, true);

likewise, I'd prefer you used bind here and not self.

@@ +3861,5 @@
>  
>      aToolbar.appendChild(clearButton);
>    },
>  
>    createHUD: function HUD_createHUD()

You are changing it, so please add a javadoc-style comment here explaining what it does.

::: toolkit/components/console/hudservice/tests/browser/browser_webconsole_position_ui.js
@@ +4,5 @@
> +
> +const TEST_URI = "data:text/html,<p>test for bug 577721";
> +
> +function test() {
> +  addTab(TEST_URI);

While I realize that your test harness cleans up any opened tabs, it makes it a lot easier to verify that the test cleans up after itself when the registerCleanup invocation is paired with the action that needs to be cleaned up.

@@ +79,5 @@
> +      Services.prefs.setCharPref(POSITION_PREF, "above");
> +      Services.prefs.setIntPref(WIDTH_PREF, 0);
> +      Services.prefs.setIntPref(HEIGHT_PREF, 0);
> +      Services.prefs.setIntPref(TOP_PREF, 0);
> +      Services.prefs.setIntPref(LEFT_PREF, 0);

You are just restoring the defaults, right?  I really think you want to 1) do this in a cleanup function and 2) use clearUserPref to reset it to the default.

::: toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
@@ +119,5 @@
> +webConsolePositionAbove=Above
> +webConsolePositionBelow=Below
> +webConsolePositionTooltip=Position the Web Console above or below the document
> +webConsoleOwnWindowTitle=Web Console
> +webConsolePositionWindow=Window

I think you want some localization notes explaining what these things mean to localizers.

::: toolkit/themes/gnomestripe/global/webConsole.css
@@ +309,5 @@
> +.web-console-panel > .hud-box {
> +    height: 100%;
> +    width: 100%;
> +    background-color: white;
> +}

this file uses two space indents, but you are using four here.

::: toolkit/themes/pinstripe/global/webConsole.css
@@ +393,5 @@
> +.web-console-panel > .hud-box {
> +    height: 100%;
> +    width: 100%;
> +    background-color: white;
> +}

this file uses two space indents, but you are using four here.

::: toolkit/themes/winstripe/global/webConsole.css
@@ +326,5 @@
> +.web-console-panel > .hud-box {
> +    height: 100%;
> +    width: 100%;
> +    background-color: white;
> +}

this file uses two space indents, but you are using four here.

Comment 36

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the patch to address the review comments.

Thank you Shawn for the review+!

I have pushed the patch to the try server as well.
- Test results:
http://tbpl.mozilla.org/?tree=Try&rev=325a11c4e59c
- Builds and logs:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-325a11c4e59c

Comment 37

[Mihai Sucan [:msucan]]

Attachment: patch update 2

The previous patch had test failures on Windows machines.

This patch fixes the test failures.

Try server results:

http://tbpl.mozilla.org/?tree=Try&rev=1c1184818de2

Builds and logs:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1c1184818de2

Comment 38

[Dão Gottwald [:dao]]

Please get rid of "hud" in new pref names, ids, etc. as mentioned earlier in this bug.

Comment 40

[Mihai Sucan [:msucan]]

Attachment: [checked-in] [in-devtools] patch update 3

Updated the patch as requested by Dão. I forgot about comment 14, sorry!

I changed the new prefs to use devtools.webconsole. The only new ID is that of the web console panel, and the class name. Otherwise, I can't really change class names or other IDs, because that would mean cleanup/changes in other places of the code - something I saw you do not want.

Comment 41

[Dão Gottwald [:dao]]

(In reply to comment #40)
> Otherwise, I can't really
> change class names or other IDs, because that would mean cleanup/changes in
> other places of the code - something I saw you do not want.

That's cleanup I'd certainly like to see get done, but yes, that wouldn't belong in this bug.

Comment 42

[Mihai Sucan [:msucan]]

Dão: We have established within the team that we do not use the generic checkin-needed keyword because we want to land our patches when we see fit. The use of the generic keyword means that others can land our patches in m-c "randomly" - something we want to avoid.

Removing bug 554926 from the dependency, because this patch does not really need it.

Thank you!

Comment 43

[Dão Gottwald [:dao]]

(In reply to comment #42)
> Dão: We have established within the team that we do not use the generic
> checkin-needed keyword because we want to land our patches when we see fit.
> The use of the generic keyword means that others can land our patches in m-c
> "randomly" - something we want to avoid.

Others use [needs landing] or [can land] for that purpose. checkin-needed has a stricter established meaning and actually used to be a whiteboard tag before it became a keyword.

Comment 44

[Mihai Sucan [:msucan]]

Comment on attachment 533232 [details] [diff] [review]
[checked-in] [in-devtools] patch update 3

Pushed to the devtools repo:

http://hg.mozilla.org/projects/devtools/rev/27bd5bdf1445

Comment 45

[Dave Camp (:dcamp)]

Comment on attachment 533232 [details] [diff] [review]
[checked-in] [in-devtools] patch update 3

http://hg.mozilla.org/mozilla-central/rev/27bd5bdf1445

Comment 46

[AndreiD[QA]]

Verified fixed on:
Windows 7:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
Window XP:
Mozilla/5.0 (Windows NT 5.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
Mac OS 10.6
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110525 Firefox/6.0a2 
Linux i686:
Mozilla/5.0 (X11; Linux i686; rv:6.0a2) Gecko/20110525 Firefox/6.0a2

*Note: The position of the web console is managed through the "Position" button - with the options "Above", "Below" and "Window". Marking this bug as Verified.

Comment 47

[Eric Shepherd [:sheppy]]

Already documented.