Persist web console height

VERIFIED FIXED

Status

()

Firefox
Developer Tools
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: Kevin Dangoor, Assigned: msucan)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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+
(Reporter)

Updated

7 years ago
Assignee: nobody → mihai.sucan
(Assignee)

Comment 1

7 years ago
Created attachment 483505 [details] [diff] [review]
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.
Attachment #483505 - Flags: feedback?(ddahl)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
Whiteboard: [patchclean:1015]
(Assignee)

Updated

7 years ago
Duplicate of this bug: 597139

Comment 3

7 years ago
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-
(Assignee)

Comment 4

7 years ago
(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

7 years ago
(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?
(Reporter)

Comment 6

7 years ago
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.
(Assignee)

Comment 7

7 years ago
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

7 years ago
(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.
(Assignee)

Comment 9

7 years ago
Created attachment 484342 [details] [diff] [review]
updated patch

Updated patch. Now the height is remembered across browser restarts as well.
Attachment #483505 - Attachment is obsolete: true
Attachment #484342 - Flags: feedback?(ddahl)
(Assignee)

Updated

7 years ago
Whiteboard: [patchclean:1015] → [patchclean:1019]

Comment 10

7 years ago
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+
(Assignee)

Comment 11

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

Thanks for the feedback+!
(Assignee)

Comment 12

7 years ago
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?
(Assignee)

Comment 15

7 years ago
(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!
(Assignee)

Comment 16

7 years ago
(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.
(Assignee)

Comment 17

7 years ago
Created attachment 486027 [details] [diff] [review]
patch update 2

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)
(Assignee)

Updated

7 years ago
Whiteboard: [patchclean:1019] → [patchclean:1026]
Attachment #486027 - Flags: review?(dietrich) → review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [patchclean:1026] → [patchclean:1026][checkin]
(Reporter)

Comment 18

7 years ago
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]
(Assignee)

Comment 20

7 years ago
Created attachment 491880 [details] [diff] [review]
[checked-in] rebased patch

Rebased patch.
Attachment #486027 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Depends on: 597151
Whiteboard: [needs-rebase:1119] → [patchclean:1119][checkin]
(Assignee)

Updated

7 years ago
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
Last Resolved: 7 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
(Assignee)

Comment 23

7 years ago
Thanks Gavin!

Updated

7 years ago
Status: RESOLVED → VERIFIED
Whiteboard: [patchclean:1119] → [patchclean:1119] [Web-Console-Testday]

Updated

7 years ago
Blocks: 615408
You need to log in before you can comment on or make changes to this bug.