Closed Bug 481391 Opened 11 years ago Closed 11 years ago

Image document test in FullZoom._applyPrefToSetting fails for background tabs

Categories

(Firefox :: General, defect, trivial)

3.5 Branch
defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: fixed1.9.1)

Attachments

(2 files)

Attached patch patchSplinter Review
No description provided.
Attachment #365415 - Flags: review?(gavin.sharp)
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?
Attachment #365415 - Flags: review?(gavin.sharp) → review+
The test for bug 386835 can probably extended to cover this.
I've streamlined browser_bug386835.js so that it can be extended more easily:

http://hg.mozilla.org/mozilla-central/rev/cf078a985bcd
Attached patch with testSplinter Review
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...
Attachment #365655 - Flags: review?(gavin.sharp)
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...
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.
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...
(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.
(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.
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 on attachment 365655 [details] [diff] [review]
with test

Makes sense.
Attachment #365655 - Flags: review?(gavin.sharp) → review+
http://hg.mozilla.org/mozilla-central/rev/c9e0320499d9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Attachment #365655 - Flags: approval1.9.1?
Severity: minor → trivial
Attachment #365655 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.