601909 - Persist web console height

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Kevin Dangoor]

The Web Console opens with the same height every time. Opening/closing the console can happen quite frequently, so we should save the last height used.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch.

Please note that I am adding a closeConsole() sister function for openConsole() because in the code I just wrote for the mochitest it felt wrong to call openConsole() then HUDService.deactivateHUDForContext(tab).

I also took the liberty to not just remember the console height, but to also put in place safeties for minimum and maximum console height, such that, for example, the Web Console won't open to only a height of 5px nor will it open and cover the entire web page content. Thus, this patch also fixes bug 597139.

Comment 3

[David Dahl :ddahl]

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

This patch is looking good, the only issue is are we going to persist over restarts? If so, you need to set the default in the preferences service - as part of the patch, add it to http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js -  and update the pref in the Services.prefs.getBranch("devtools.hud.") branch

Comment 4

[Mihai Sucan [:msucan]]

(In reply to comment #3)
> Comment on attachment 483505 [details] [diff] [review]
> proposed patch
> 
> This patch is looking good, the only issue is are we going to persist over
> restarts? If so, you need to set the default in the preferences service - as
> part of the patch, add it to
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js - 
> and update the pref in the Services.prefs.getBranch("devtools.hud.") branch

Hm? There was no intent in the patch to make the height persist over browser restarts. Is there something in the patch that makes it look like I am trying to persist the Web Console height across restarts?

Do you want that?

Comment 5

[David Dahl :ddahl]

(In reply to comment #4)

> Hm? There was no intent in the patch to make the height persist over browser
> restarts. Is there something in the patch that makes it look like I am trying
> to persist the Web Console height across restarts?
> 
> Do you want that?

I thought that was the intent, Kevin, what do you think?

Comment 6

[Kevin Dangoor]

I think Dietrich originally requested this (in the bug to animate the opening/closing of the console). He may have some other opinion on it...

My opinion is that we should keep track between browser runs as david suggests in comment 3.

Comment 7

[Mihai Sucan [:msucan]]

In my opinion as a user, it seems wrong to remember the Web Console height across restarts. Then again, no problem. :)

Shall I do that, then?

Comment 8

[David Dahl :ddahl]

(In reply to comment #7)
> In my opinion as a user, it seems wrong to remember the Web Console height
> across restarts. Then again, no problem. :)
> 
> Shall I do that, then?

I think so, and you can add a second pref that keeps the persistence at all between restarts, which should be the default behavior - as most of firefox does persist ui tweaks.

Comment 9

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch. Now the height is remembered across browser restarts as well.

Comment 10

[David Dahl :ddahl]

Comment on attachment 484342 [details] [diff] [review]
updated patch

The only thing we did not take into account is that the height pref that is set will be the last closed console's height.

Not too big of a deal to me. What do you think?

Comment 11

[Mihai Sucan [:msucan]]

For me that's the behavior I'd expect. Please let me know if I should make further changes.

Thanks for the feedback+!

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 484342 [details] [diff] [review]
updated patch

Asking for review. This is a nice bit of Web Console polish.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 484342 [details] [diff] [review]
updated patch

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>   deactivateHUDForContext: function HS_deactivateHUDForContext(aContext)

>+      let style = chromeWindow.getComputedStyle(displayNode, null);
>+      let height = parseInt(style.height.replace("px", ""));
>+      height += parseInt(style.borderTopWidth.replace("px", ""));
>+      height += parseInt(style.borderBottomWidth.replace("px", ""));

You don't need to strip away the "px" for parseInt() to work - it ignores trailing junk.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 484342 [details] [diff] [review]
updated patch

is the reason you're using a pref because there's no permanent piece of xul to set the persist attribute on?

Comment 15

[Mihai Sucan [:msucan]]

(In reply to comment #13)
> Comment on attachment 484342 [details] [diff] [review]
> updated patch
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >   deactivateHUDForContext: function HS_deactivateHUDForContext(aContext)
> 
> >+      let style = chromeWindow.getComputedStyle(displayNode, null);
> >+      let height = parseInt(style.height.replace("px", ""));
> >+      height += parseInt(style.borderTopWidth.replace("px", ""));
> >+      height += parseInt(style.borderBottomWidth.replace("px", ""));
> 
> You don't need to strip away the "px" for parseInt() to work - it ignores
> trailing junk.

Indeed, good point. Will update the patch. Thanks!

Comment 16

[Mihai Sucan [:msucan]]

(In reply to comment #14)
> Comment on attachment 484342 [details] [diff] [review]
> updated patch
> 
> is the reason you're using a pref because there's no permanent piece of xul to
> set the persist attribute on?

Yes. The Web Console UI not permanent in any way - it's all dynamically generated when the Web Console is opened, and removed when closed.

Comment 17

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Updated patch. Removed .replace("px", ""), as requested by Gavin.

Comment 18

[Kevin Dangoor]

does this patch still apply cleanly? if so, I don't see any reason for this to not have the checkin-needed keyword.

Comment 19

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

this is going to need some rework since the addition of the console animation bug landing. bug 596315

Comment 20

[Mihai Sucan [:msucan]]

Attachment: [checked-in] rebased patch

Rebased patch.

Comment 21

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

Comment on attachment 491880 [details] [diff] [review]
[checked-in] rebased patch

http://hg.mozilla.org/mozilla-central/rev/46edc39233b3

Comment 22

[:Gavin Sharp [email: [e-mail]]]

I fixed what looks like a rebase mistake in the patch:
https://hg.mozilla.org/mozilla-central/rev/6f28a60c1c71

Comment 23

[Mihai Sucan [:msucan]]

Thanks Gavin!