481391 - Image document test in FullZoom._applyPrefToSetting fails for background tabs

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

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

Comment on attachment 365415 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser-textZoom.js b/browser/base/content/browser-textZoom.js

>   _applyPrefToSetting: function FullZoom__applyPrefToSetting(aValue, aBrowser) {
>+    var browser = aBrowser || gBrowser.selectedTab.linkedBrowser;

gBrowser.selectedBrowser

Is there a test that covers this?

Comment 2

[Dão Gottwald [:dao]]

The test for bug 386835 can probably extended to cover this.

Comment 3

[Dão Gottwald [:dao]]

I've streamlined browser_bug386835.js so that it can be extended more easily:

http://hg.mozilla.org/mozilla-central/rev/cf078a985bcd

Comment 4

[Dão Gottwald [:dao]]

Attachment: with test

This is what I had in mind, but it passes without the patch. I guess I don't really understand the site-specific zoom code, or the "zoom background tabs" code, or both.

I'm tempted to push this anyway, since the current code is quite obviously wrong...

Comment 5

[Nochum Sossonko [:Natch]]

I'm not sure I understand the intent of this bug, the image document checking is done already, so what exactly isn't working? i'm not at my computer now so i'll have to check this later...

Comment 6

[Dão Gottwald [:dao]]

onLocationChange passes a browser to _applyPrefToSetting:

http://hg.mozilla.org/mozilla-central/annotate/5e81fbdd55e4/browser/base/content/browser-textZoom.js#l269

which comes from:

http://hg.mozilla.org/mozilla-central/annotate/5e81fbdd55e4/browser/base/content/tabbrowser.xml#l499

_applyPrefToSetting ignores that browser and checks whether the /current browser/ has an image document.

Comment 7

[Nochum Sossonko [:Natch]]

Doesn't onLocationChange get called when the tab is switched to? If so, content.document should correctly point to it, which is why this works even now (just tested this, changing the zoom of another tab, with an image doc in a background tab doesn't affect the image tab). This still seems right, though, so it should probably be pushed anyhow...

Comment 8

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

(In reply to comment #7)
> Doesn't onLocationChange get called when the tab is switched to?

It does, but it also gets called for tabs in the background. When background tabs load, we need to check their document, not the selected tab's.

Comment 9

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

(In reply to comment #4)
> This is what I had in mind, but it passes without the patch. I guess I don't
> really understand the site-specific zoom code, or the "zoom background tabs"
> code, or both.

It's failing because a call to DocumentViewerImpl::SetFullZoom is clobbering the value set by _applyPrefToSetting for the background image tab, and setting it back to 1.0 - one of the calls to SetZoomLevel in nsImageDocument, presumably. I haven't sorted out what the expected behavior is, but bug 389756 is probably relevant.

Comment 10

[Dão Gottwald [:dao]]

Per bug 389756 comment 17, what landed is different from the attached patch:

http://hg.mozilla.org/mozilla-central/rev/78ab3750ebc9

Must be OnStartContainer. This seems correct, so I think the test is good to go.

Comment 11

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

Comment on attachment 365655 [details] [diff] [review]
with test

Makes sense.

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/c9e0320499d9

Comment 13

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

Comment on attachment 365655 [details] [diff] [review]
with test

a191=beltzner

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/934c2b1f4d7b