It is quite annoying to see normal pages zoomed when using pagezoom to zoom images. Perhaps there should be !(event.target.ownerDocument instanceof Components.interfaces.nsIImageDocument) check somewhere?

This seems to work for me (if I understand this bug correctly).

That prevents us from setting a pref *from* an image document, but I think what we really want is to avoid applying any existing prefs *to* image documents, right? That means the check needs to be in _applySettingToPref. Shouldn't be too hard to write a browser chrome test for this as well.

browser chrome test coming... I'll post it seperately.

Have to check in _removePref as well (good thing is I found this while preparing the browser chrome test). I'll upload the tests soon(hopefully, having some problems, gonna try to straighten them out now).

Having problems with this, it doesn't work :(

BTW should this be changed for a video document (ogg video) now that it landed?

Ok here's the browser chrome test, forgot to addEventListener on the linkedBrowser... :(

(In reply to comment #7) If you're asking whether the bug should morph to be "...apply to video/audio/image documents", I'd think so as video fullscreening better addresses the need to enlarge playback size. I haven't followed video controls discussions closely enough to know whether fullscreening will have obvious UI (I see none in a recent trunk build), but I'd be astounded if it won't.

(In reply to comment #9) I don't really understand what that means, the patch in this bug doesn't disable zooming for images, it just doesn't update the zoom preference for the domain and doesn't get affected by the domain's preference. If zooming for video/audio should be entirely disabled that would probably be a different bug. Then again, maybe I'm just being dense...

Comment on attachment 345916 [details] [diff] [review] Utter Stupidity :( ... >diff --git a/browser/base/content/test/browser_bug416661.js b/browser/base/content/test/browser_bug416661.js >+var tabDoc, zoomLevel; "tabDoc" is kind of an odd name for something that holds a <xul:tab>. >+function start_test_prefNotSet() { >+ FullZoom.enlarge(); >+ >+ //capture the zoom level to test later >+ zoomLevel = ZoomManager.zoom; Might add in a test to make sure that enlarge() caused the zoomLevel to change? And add a test for the initial zoom being 1? >+function continue_test_prefNotSet () { >+ var isGlobal = ZoomManager.zoom == 1; >+ ok(isGlobal, "An image document should not be affected by the" + >+ " site specific zoom level "); >+ FullZoom.reset(); Test that ZoomManager.zoom is still 1 here? >+function end_test_prefNotSet() { >+ isSiteSpecific = ZoomManager.zoom == zoomLevel; >+ ok(isSiteSpecific, "The site specific zoom level should not be" + >+ " affected by an image document "); Prefer is(a, b) rather than ok(a == b), since is() will print out actual/expected results for failures (this comment applies to a bunch of places in this test). >+function test() { >+ content.location = >+ "chrome://mochikit/content/browser/browser/base/content/test/zoom_test.html"; I guess this works because the content prefs service uses nsIURI.host, and for chrome URIs host is the package name ("mochikit")... It would be best to use one of the fake http://example.com URIs for this test instead, I think, so that we're testing the most common use case (and avoid problems if the CPS URI rules change). r=me with those addressed.

All nits addressed, besides the one discussed on irc.

Comment on attachment 345602 [details] [diff] [review] Yet another oversight :( Er, I forgot to post this earlier. I reviewed the patch and test separately, don't assume that just because I marked the test r+ I reviewed the patch as well! >diff --git a/browser/base/content/browser-textZoom.js b/browser/base/content/browser-textZoom.js > _applyPrefToSetting: function FullZoom__applyPrefToSetting(aValue) { >- if (!this.siteSpecific || gInPrintPreviewMode) >+ if (!this.siteSpecific || gInPrintPreviewMode || >+ content.document instanceof Ci.nsIImageDocument) I wonder whether we want this change or not. Would it be useful for existing site specific prefs to apply to images by default? I suppose that might be confusing and/or annoying. r=me either way. You should probably get ui-review for this change, not sure who would be best.

Comment on attachment 345602 [details] [diff] [review] Yet another oversight :( sorry about that, I had combined the two patches locally so I thought it was finished with review. Beltzner: comment 13, do we want site-specific zoom prefs to apply to image documents?

Does something similar to this need to be done for the new audio or video documents that we now use when directly loading video and audio files in the browser?

Comment on attachment 345602 [details] [diff] [review] Yet another oversight :( Giving this a review to move the bug forward, and the UI makes sense. To follow up we need to make sure we are doing the same thing for video and audio.

Comment on attachment 347217 [details] [diff] [review] Nits Addressed annoyance fix.

Comment on attachment 347217 [details] [diff] [review] Nits Addressed a191=beltzner

filed bug 466909 as a follow-up for video/audio documents (that will have to wait though for bug 462892 and possibly others...).

adding checkin-needed so that it doesn't get lost (for 1.9.1)

Backed out of 1.9.1 due to Mac test failures: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.1/1228916771.1228921242.32424.gz

That's weird, it's passing on m-c on mac http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228949978.1228954124.20407.gz&fulltext=1 There is quite a bit of loading+load listeners and removers, but I figured that's safer than setTimeout. I guess this will just have to be bumped to 1.9.2, unless someone can figure this out.

Scratch that, comment 24 's log was from 10.5.2 while mine was from 9.2.2, still pretty weird though ;)

Are there any known weirdnesses with: 1) addEventListener, 2) the mochitest "fake" servers (e.g. localhost:8888). Maybe they don't get recognized as the same domain? (or anything else) I use them in the test and if it's problematic on 10.5.2 that may be the cause of the failure. I'm pretty sure the problem is with the test as the other tests were passing. Also this may be worth investigating as there is no mochitest tbox for 10.5.2 on m-c AFAICT, so it may, hypothetically, be failing there too?

See also bug 463923 comment 15. Might be some weirdness on that box.

The proxy hack for the servers isn't going to be at fault here, or if it did we'd have huge problems with corporate areas that use proxy autoconfig. (Not to mention it's exercised pretty heavily via the Mochitest harness anyway.)

(In reply to comment #29) > The proxy hack for the servers isn't going to be at fault here, or if it did > we'd have huge problems with corporate areas that use proxy autoconfig. (Not Not really, see below. > to mention it's exercised pretty heavily via the Mochitest harness anyway.) The problem probably wouldn't manifest itself in a very open manner, the proxy works perfectly, the question is does the browser see localhost:8888 as the same domain (for site specific prefs) when loaded a second time (this is specifically what is tested in this case). A good test for this would be to scrap the fake proxy and use chrome urls instead and then see if it passes, otherwise this doesn't really make sense, as it's passing on every other machine and all the tests on this machine are passing, besides the one that relies on the aforementioned.

Write the test again, add dumps of interesting information and the JS stack, see what's being called wrongly (and conceivably from where). I assert the proxying stuff is not at fault here, although what is, I don't know.

This applies on top of the previous patch (iow on m-c tip). Not sure if this is what you wanted, but if anyone's got a Mac (with 10.5.2) and can test this, then paste the debug information here it'd be great. ;) Hope this helps.

(In reply to comment #33) > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b89527e65352 Ummm... this caused failures on the mac box last time, anything change since then?

Last time there was something wrong with moz2-darwin9-slave05.

Just to be sure other tests don't fail unexpectedly, reset the zoom level to default.

http://hg.mozilla.org/mozilla-central/rev/df612b3cc3d8 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/067649fefb8d

verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090511 Minefield/3.6a1pre ID:20090511031307 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090511 Shiretoko/3.5b5pre ID:20090511031352

This bug's priority relative to the set of other polish bugs is: P4 - Polish issue that is rarely encountered, and is easily identifiable.