Boris Zbarsky [:bzbarsky] Reporter
	Description • 17 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...

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 1 • 17 years ago

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

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 2 • 17 years ago

This will not block the final release of Firefox 3.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 3 • 16 years ago

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

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 4 • 16 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.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 5 • 16 years ago

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.

	Nochum Sossonko [:Natch]
	Comment 6 • 16 years ago

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

	Dão Gottwald [:dao]
	Comment 7 • 16 years ago

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

the former

	Nochum Sossonko [:Natch]
	Comment 8 • 16 years ago

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?

	Dão Gottwald [:dao]
	Comment 9 • 16 years ago

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

	Neil Deakin
	Comment 10 • 16 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.

	Nochum Sossonko [:Natch]
	Comment 11 • 16 years ago

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

	Nochum Sossonko [:Natch]
	Comment 12 • 16 years ago

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

	Dão Gottwald [:dao]
	Comment 13 • 16 years ago

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.

	Dão Gottwald [:dao]
	Comment 14 • 16 years ago

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

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

	Nochum Sossonko [:Natch]
	Comment 15 • 16 years ago

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

	Nochum Sossonko [:Natch]
	Comment 16 • 16 years ago

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.

	Nochum Sossonko [:Natch]
	Comment 17 • 16 years ago

Using quit-application, this works now.

	Nochum Sossonko [:Natch]
	Comment 18 • 16 years ago

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

	Dão Gottwald [:dao]
	Comment 19 • 16 years ago

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

	Nochum Sossonko [:Natch]
	Comment 20 • 16 years ago

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.

	Dão Gottwald [:dao]
	Comment 21 • 16 years ago

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

	Nochum Sossonko [:Natch]
	Comment 22 • 16 years ago

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?

	Dão Gottwald [:dao]
	Comment 23 • 16 years ago

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.

	Nochum Sossonko [:Natch]
	Comment 24 • 16 years ago

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?

	Nochum Sossonko [:Natch]
	Comment 25 • 16 years ago

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.

	Nochum Sossonko [:Natch]
	Comment 26 • 16 years ago

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

s/do see/don't see

	Nochum Sossonko [:Natch]
	Comment 27 • 16 years ago

Dao, what do you think of this latest approach?

	Dão Gottwald [:dao]
	Comment 28 • 16 years ago

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

	Nochum Sossonko [:Natch]
	Comment 29 • 16 years ago

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

	Nochum Sossonko [:Natch]
	Comment 30 • 16 years ago

-> default owner