Closed Bug 601909 Opened 11 years ago Closed 11 years ago

Persist web console height

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(blocking2.0 final+)

VERIFIED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: dangoor, Assigned: msucan)

References

Details

(Whiteboard: [patchclean:1119] [Web-Console-Testday])

Attachments

(1 file, 3 obsolete files)

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.
blocking2.0: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
blocking2.0: ? → final+
Assignee: nobody → mihai.sucan
Attached patch proposed patch (obsolete) — Splinter Review
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.
Attachment #483505 - Flags: feedback?(ddahl)
Status: NEW → ASSIGNED
Whiteboard: [patchclean:1015]
Duplicate of this bug: 597139
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
Attachment #483505 - Flags: feedback?(ddahl) → feedback-
(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?
(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?
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.
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?
(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.
Attached patch updated patch (obsolete) — Splinter Review
Updated patch. Now the height is remembered across browser restarts as well.
Attachment #483505 - Attachment is obsolete: true
Attachment #484342 - Flags: feedback?(ddahl)
Whiteboard: [patchclean:1015] → [patchclean:1019]
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?
Attachment #484342 - Flags: feedback?(ddahl) → feedback+
For me that's the behavior I'd expect. Please let me know if I should make further changes.

Thanks for the feedback+!
Comment on attachment 484342 [details] [diff] [review]
updated patch

Asking for review. This is a nice bit of Web Console polish.
Attachment #484342 - Flags: review?(dietrich)
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 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?
(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!
(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.
Attached patch patch update 2 (obsolete) — Splinter Review
Updated patch. Removed .replace("px", ""), as requested by Gavin.
Attachment #484342 - Attachment is obsolete: true
Attachment #486027 - Flags: review?(dietrich)
Attachment #484342 - Flags: review?(dietrich)
Whiteboard: [patchclean:1019] → [patchclean:1026]
Attachment #486027 - Flags: review?(dietrich) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [patchclean:1026] → [patchclean:1026][checkin]
does this patch still apply cleanly? if so, I don't see any reason for this to not have the checkin-needed keyword.
this is going to need some rework since the addition of the console animation bug landing. bug 596315
Whiteboard: [patchclean:1026][checkin] → [needs-rebase:1119]
Rebased patch.
Attachment #486027 - Attachment is obsolete: true
Depends on: 597151
Whiteboard: [needs-rebase:1119] → [patchclean:1119][checkin]
Blocks: 597756
Comment on attachment 491880 [details] [diff] [review]
[checked-in] rebased patch

http://hg.mozilla.org/mozilla-central/rev/46edc39233b3
Attachment #491880 - Attachment description: rebased patch → [checked-in] rebased patch
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:1119][checkin] → [patchclean:1119]
I fixed what looks like a rebase mistake in the patch:
https://hg.mozilla.org/mozilla-central/rev/6f28a60c1c71
Thanks Gavin!
Status: RESOLVED → VERIFIED
Whiteboard: [patchclean:1119] → [patchclean:1119] [Web-Console-Testday]
Blocks: 615408
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.