Last Comment Bug 577721 - allow repositioning of the web console: above, below and out into a panel/window
: allow repositioning of the web console: above, below and out into a panel/window
Status: VERIFIED FIXED
[console-1][patchclean:0518][fixed-in...
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 6
Assigned To: Mihai Sucan [:msucan]
:
:
Mentors:
: 577722 591384 597200 625851 (view as bug list)
Depends on: 645163
Blocks: 529086
  Show dependency treegraph
 
Reported: 2010-07-09 11:20 PDT by David Dahl :ddahl
Modified: 2012-05-03 07:36 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v 0.1 WIP patch (4.88 KB, patch)
2011-03-21 20:35 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.2 WIP (10.66 KB, patch)
2011-03-23 15:31 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.3 folded all positioning patches intot his one (18.10 KB, patch)
2011-03-24 14:53 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Weird Mac OS behavior (144.80 KB, image/png)
2011-03-25 13:09 PDT, David Dahl :ddahl
no flags Details
v 0.3.1 Tweaks while testing on mac (19.01 KB, patch)
2011-03-28 11:39 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Screencast of Mac Panel problem (1.59 MB, application/octet-stream)
2011-03-29 14:27 PDT, David Dahl :ddahl
no flags Details
v 0.3.2 Fixed weird panel issue (19.13 KB, patch)
2011-03-30 09:29 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.4 Added tests (28.79 KB, patch)
2011-03-31 13:51 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
0.5 Patch with Tests (25.40 KB, patch)
2011-04-01 14:55 PDT, David Dahl :ddahl
mihai.sucan: review-
Details | Diff | Splinter Review
0.5.1 saving work (23.60 KB, patch)
2011-04-11 20:30 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
proposed patch (34.25 KB, patch)
2011-05-11 09:08 PDT, Mihai Sucan [:msucan]
rcampbell: review+
sdwilsh: review+
Details | Diff | Splinter Review
updated patch (35.09 KB, patch)
2011-05-17 05:52 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
patch update 2 (35.45 KB, patch)
2011-05-18 03:24 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
[checked-in] [in-devtools] patch update 3 (35.35 KB, patch)
2011-05-18 04:13 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description David Dahl :ddahl 2010-07-09 11:20:12 PDT
We should allow the console to pop out into a window so we do not use so much screen real estate
Comment 1 Biju 2010-08-09 20:44:40 PDT
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 2010-08-09 21:25:48 PDT
(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) 2010-08-10 06:51:20 PDT
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 2010-08-10 08:50:09 PDT
(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 5 Kevin Dangoor 2010-09-03 10:42:13 PDT
*** Bug 591384 has been marked as a duplicate of this bug. ***
Comment 6 Rob Campbell [:rc] (:robcee) 2010-10-29 05:58:35 PDT
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 2010-10-29 07:14:39 PDT
I am not actively working on this if you want to take a crack at it.
Comment 8 David Dahl :ddahl 2011-03-09 09:23:20 PST
(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 9 David Dahl :ddahl 2011-03-15 12:42:58 PDT
*** Bug 625851 has been marked as a duplicate of this bug. ***
Comment 10 David Dahl :ddahl 2011-03-21 20:35:04 PDT
Created attachment 520843 [details] [diff] [review]
v 0.1 WIP patch
Comment 11 David Dahl :ddahl 2011-03-23 15:31:45 PDT
Created attachment 521331 [details] [diff] [review]
v 0.2 WIP
Comment 12 David Dahl :ddahl 2011-03-24 14:53:51 PDT
Created attachment 521642 [details] [diff] [review]
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 13 David Dahl :ddahl 2011-03-24 14:54:59 PDT
*** Bug 577722 has been marked as a duplicate of this bug. ***
Comment 14 Dão Gottwald [:dao] 2011-03-24 15:11:09 PDT
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] 2011-03-24 15:21:00 PDT
(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 2011-03-24 15:25:20 PDT
(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] 2011-03-24 15:31:32 PDT
I wasn't asking for cleanup here, I'm just saying "hud" shouldn't creep into new code.
Comment 18 David Dahl :ddahl 2011-03-24 15:33:10 PDT
(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 2011-03-24 15:34:15 PDT
(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 2011-03-24 15:48:34 PDT
(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 2011-03-25 13:09:40 PDT
Created attachment 521919 [details]
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 2011-03-28 11:39:12 PDT
Created attachment 522414 [details] [diff] [review]
v 0.3.1 Tweaks while testing on mac
Comment 23 David Dahl :ddahl 2011-03-29 14:27:28 PDT
Created attachment 522800 [details]
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 2011-03-30 09:29:25 PDT
Created attachment 523031 [details] [diff] [review]
v 0.3.2 Fixed weird panel issue
Comment 25 David Dahl :ddahl 2011-03-31 13:51:34 PDT
Created attachment 523417 [details] [diff] [review]
v 0.4 Added tests
Comment 26 David Dahl :ddahl 2011-03-31 15:05:00 PDT
Still dealing with some DOM and hudreference problems. tests are written, will try to submit for review tomorrow. Ugh.
Comment 27 David Dahl :ddahl 2011-04-01 14:55:27 PDT
Created attachment 523692 [details] [diff] [review]
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] 2011-04-04 11:48:05 PDT
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 2011-04-11 20:27:28 PDT
(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 2011-04-11 20:30:16 PDT
Created attachment 525290 [details] [diff] [review]
0.5.1 saving work
Comment 31 Mihai Sucan [:msucan] 2011-05-11 09:08:36 PDT
Created attachment 531650 [details] [diff] [review]
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 2011-05-11 12:08:22 PDT
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) 2011-05-11 16:16:21 PDT
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] 2011-05-12 01:10:00 PDT
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 2011-05-12 12:21:06 PDT
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] 2011-05-17 05:52:20 PDT
Created attachment 532941 [details] [diff] [review]
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/mihai.sucan@gmail.com-325a11c4e59c
Comment 37 Mihai Sucan [:msucan] 2011-05-18 03:24:54 PDT
Created attachment 533230 [details] [diff] [review]
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/mihai.sucan@gmail.com-1c1184818de2
Comment 38 Dão Gottwald [:dao] 2011-05-18 03:40:28 PDT
Please get rid of "hud" in new pref names, ids, etc. as mentioned earlier in this bug.
Comment 39 Mihai Sucan [:msucan] 2011-05-18 03:41:00 PDT
*** Bug 597200 has been marked as a duplicate of this bug. ***
Comment 40 Mihai Sucan [:msucan] 2011-05-18 04:13:37 PDT
Created attachment 533232 [details] [diff] [review]
[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] 2011-05-18 04:25:43 PDT
(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] 2011-05-18 05:12:34 PDT
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] 2011-05-18 05:21:43 PDT
(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] 2011-05-18 05:49:19 PDT
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) 2011-05-21 21:17:28 PDT
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] 2011-05-26 04:50:33 PDT
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] 2012-05-03 07:36:18 PDT
Already documented.

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