418468 - Zoom manager preferences are not live

Status: NEW

(Firefox :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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...

Comment 1

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

(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.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

This will not block the final release of Firefox 3.

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

To see this you need to adjust a pref in about:config to see this, so removing the polish keywords.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

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.

Comment 6

[Nochum Sossonko [:Natch]]

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

[Dão Gottwald [:dao]]

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

the former

Comment 8

[Nochum Sossonko [:Natch]]

Attachment: 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?

Comment 9

[Dão Gottwald [:dao]]

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

Comment 10

[Neil Deakin]

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.

Comment 11

[Nochum Sossonko [:Natch]]

(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?

Comment 12

[Nochum Sossonko [:Natch]]

Attachment: Like so?

Dao, you've done some work on this, can you review it?

Comment 13

[Dão Gottwald [: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.

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> You also need to observe quit-application and remove all observers.

Or xul-window-destroyed, which seems more suitable...

Comment 15

[Nochum Sossonko [:Natch]]

Attachment: Patch ver. 2

How's this? I used xul-window-destroyed as you suggested.

Comment 16

[Nochum Sossonko [:Natch]]

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.

Comment 17

[Nochum Sossonko [:Natch]]

Attachment: Patch ver. 2.1

Using quit-application, this works now.

Comment 18

[Nochum Sossonko [:Natch]]

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) ;)

Comment 19

[Dão Gottwald [:dao]]

You should be able to compare aSubject to the current window for xul-window-destroyed.

Comment 20

[Nochum Sossonko [:Natch]]

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.

Comment 21

[Dão Gottwald [:dao]]

Oh, right, I was looking at dom-window-destroyed.

Comment 22

[Nochum Sossonko [:Natch]]

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?

Comment 23

[Dão Gottwald [:dao]]

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.

Comment 24

[Nochum Sossonko [:Natch]]

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?

Comment 25

[Nochum Sossonko [:Natch]]

Attachment: 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.

Comment 26

[Nochum Sossonko [:Natch]]

> the patch because the prefs were a bit confusing to me and I do see a real

s/do see/don't see

Comment 27

[Nochum Sossonko [:Natch]]

Dao, what do you think of this latest approach?

Comment 28

[Dão Gottwald [: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.

Comment 29

[Nochum Sossonko [:Natch]]

In that case let's get a decision for wontfix.

Comment 30

[Nochum Sossonko [:Natch]]

-> default owner