Zoom manager preferences are not live

NEW
Unassigned

Status

()

11 years ago
10 years ago

People

(Reporter: bzbarsky, Unassigned)

Tracking

Trunk
Points:
---
Bug Flags:
blocking-firefox3 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [wontfix?])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 years ago
We read the zoom manager preferences once at startup, and don't observe the prefbranch for changes.  This means that changing those preferences requires a browser restart.  In particular, it makes it impossible for extensions to improve on the zoom UI dynamically based on page (e.g. offer different stops on different pages).

I ran into this when trying to zoom to below 50%; when I changed the prefs to allow this, it didn't work.

I'm a little surprised we're still checking in "read the pref once at startup and ignore later changes" code, to be honest...
Flags: blocking-firefox3?
(In reply to comment #0)
> I'm a little surprised we're still checking in "read the pref once at startup
> and ignore later changes" code, to be honest...

I don't see a problem with reading some prefs only at startup if there's no real need for the pref to be "live". It avoids the code+performance overhead of an observer.

I agree that this pref could use an observer, though.
This will not block the final release of Firefox 3.
Flags: blocking-firefox3? → blocking-firefox3-
Keywords: polish
Whiteboard: [polish-easy] [polish-interactive]
To see this you need to adjust a pref in about:config to see this, so removing the polish keywords.
Keywords: polish
Whiteboard: [polish-easy] [polish-interactive]
(Reporter)

Comment 4

10 years ago
Alex, did you read comment 0?  I ran into this problem with an extension that was attempting to provide a richer zoom UI...  no about:config involved.
Right, but if you don't have an extension installed you would need to change the pref yourself in order to experience the problem.  I'm not saying it isn't a bug, or that we shouldn't fix it, just that it doesn't really meet the requirements to be a visual or interactive Firefox polish issue, since it doesn't impact the default UI.
Assignee: nobody → highmind63
Whiteboard: [good first bug]
Is this to be done in toolkit's ZoomManager or just for browser's FullZoom. Note, either way both files would need to be modified...

Comment 7

10 years ago
(In reply to comment #6)
> Is this to be done in toolkit's ZoomManager or just for browser's FullZoom.

the former
Created attachment 357746 [details] [diff] [review]
Patch ver. 1

I set it up that all the values get updated when the observer is called since there are dependencies on each other. Does this need to be a weak ref, or is it fine like this?
Attachment #357746 - Flags: review?(enndeakin)

Comment 9

10 years ago
The zoom manager is also used elsewhere. We really shouldn't do the init/destroy thing.

Comment 10

10 years ago
Comment on attachment 357746 [details] [diff] [review]
Patch ver. 1

I don't know this code at all. A better reviewer would be someone who wrote or reviewed this code in the past.
Attachment #357746 - Flags: review?(enndeakin)
(In reply to comment #9)
> The zoom manager is also used elsewhere. We really shouldn't do the
> init/destroy thing.

How do I get rid of the observers? Is that unnecessary?
Status: NEW → ASSIGNED
Created attachment 357901 [details] [diff] [review]
Like so?

Dao, you've done some work on this, can you review it?
Attachment #357746 - Attachment is obsolete: true
Attachment #357901 - Flags: review?(dao)
Comment on attachment 357901 [details] [diff] [review]
Like so?

Just make the getters call a single method that deletes all getters and set the values.

You also need to observe quit-application and remove all observers.
Attachment #357901 - Flags: review?(dao) → review-
(In reply to comment #13)
> You also need to observe quit-application and remove all observers.

Or xul-window-destroyed, which seems more suitable...
Created attachment 358066 [details] [diff] [review]
Patch ver. 2

How's this? I used xul-window-destroyed as you suggested.
Attachment #357901 - Attachment is obsolete: true
Attachment #358066 - Flags: review?(dao)
Comment on attachment 358066 [details] [diff] [review]
Patch ver. 2

This doesn't work because we hit "xul-window-destroyed" for some reason when the pref is set! I'll post a new patch with quit-application.
Attachment #358066 - Attachment is obsolete: true
Attachment #358066 - Flags: review?(dao)
Created attachment 358135 [details] [diff] [review]
Patch ver. 2.1

Using quit-application, this works now.
Attachment #358135 - Flags: review?(dao)
Oh, and I think the reason xul-window-destroyed is being hit is because when you edit the pref it closes a pop-up window, which I *think* triggers the xul-window-destroyed notification (as will any alert, which I learned the hard way) ;)
You should be able to compare aSubject to the current window for xul-window-destroyed.
I don't think that will work, http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp?mark=549-549#538 passes in nsnull for the other two arguments, but I can give it a try later today.
Oh, right, I was looking at dom-window-destroyed.
Is that better? With dom-window-destroyed I think we're getting much more notifications which means more observer overhead, and the end result will probably be the same. quit-application is probably the safest way to go, and the code is relatively simple so there shouldn't be a perf problem here. Alternatively, can I register an onunload handler?
With quit-application, I'm not sure what happens if multiple browser windows are closed independently. Listening to the unload event should work.

But either way, this all seems like too much overhead just for making the prefs live. Ideally, we'd read them directly. Assuming that this would be too costly, I'd rather leave this bug unfixed.
Ok, perhaps add a pref to make the prefs live :) . Either way, I'll wait for a decision on comment 23 to be made before I continue on this one...

OTOH, maybe we could make the prefs live just for browser-textZoom.js and do the init/destroy thing, only this time in consideration of apps that don't want the prefs to be live. I know this is not really optimal, but any other suggestions?
Whiteboard: [good first bug] → [good first bug][pending decision, comment 23]
Created attachment 362525 [details] [diff] [review]
Alternative

Dao, how about this? We use the zoomValues until we run out of them, in which case we just push a higher/lower value. This avoids the observer overhead for at least the zoomValues pref.

Alternatively we can just grab the pref anytime we don't have anymore values to see if it changed, which shouldn't be a problem because that means that the page was zoomed already to the max capacity of zoomValues. I chose the way in the patch because the prefs were a bit confusing to me and I do see a real reason to have to provide zoom values.

I also switched the quit-application observer to quit-application-granted, as it seems to be the standard way of observing end of life for destroying components.
Attachment #358135 - Attachment is obsolete: true
Attachment #362525 - Flags: review?(dao)
Attachment #358135 - Flags: review?(dao)
> the patch because the prefs were a bit confusing to me and I do see a real

s/do see/don't see
Dao, what do you think of this latest approach?
Whiteboard: [good first bug][pending decision, comment 23] → [good first bug][needs review dao]
- unload still seems better than quit-application-granted, since this code is bound to a window.

- I don't like how the zoomValues are extended magically. You'll get very odd fractions, which isn't good (see bug 389628 comment 89). And even if you fix this, it's still unexpected for a user who modifies toolkit.zoomManager.zoomValues.

- I still think all this isn't worthwhile.
In that case let's get a decision for wontfix.
Whiteboard: [good first bug][needs review dao] → [good first bug][wontfix?]
Attachment #362525 - Flags: review?(dao)

Updated

10 years ago
Whiteboard: [good first bug][wontfix?] → [wontfix?]
-> default owner
Assignee: highmind63 → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.