559991 - zoom flicker when switching between two tabs

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Timothy Nikkel (:tnikkel)]

Open two tabs, one with a long page that takes some time to reflow (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp works for me, http://www.whatwg.org/specs/web-apps/current-work/ is bigger), and another with anything. Increase the zoom in the short page tab. Keep hitting ctrl-tab, the zoom will flicker in both tabs if you do it enough.

Speculatively setting this to block bug 541779.

Comment 1

[Johnathan Nightingale [:johnath]]

Yep - regression and crappy looking - jesse, you flagged this as a regression, any chance of a range?

Comment 2

[Jesse Ruderman]

We're guessing it's a regression from bug 541779, since it caused a raft of similar regressions.

Comment 3

[Timothy Nikkel (:tnikkel)]

I'm pretty sure I bisected this down to the day bug 541779 landed when I filed.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

I'm working on this one too

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch - v1

Patch. Working on tests now.

tn, could you check if this patch fixes the issue for you?
(note: this is meant to only fix the flickering when switching tabs)

Comment 6

[Timothy Nikkel (:tnikkel)]

Seems to fix it for me, thank you!

Comment 7

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

I'm confused as to why that patch would help - can you explain?

Comment 8

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #7)
> I'm confused as to why that patch would help - can you explain?

Sure. On tab switch, FullZoom.onLocationChange is called without aBrowser, so the isSaneURI check here doesn't happen, and _applyPrefToSetting is called unconditionally. However, _applyPrefToSetting will pick the selectedBrowser anyway.
If you keep switching tabs fast enough, the async callbacks will queue and they can arrive when that requested tab is no longer selected, so we end up applying the zoom level from one tab (the no longer selected) to the wrong one (the current one).

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 467683 [details] [diff] [review]
Patch - v1

>     var self = this;
>     Services.contentPrefs.getPref(aURI, this.name, function (aResult) {
>       // Check that we're still where we expect to be in case this took a while.
>-      let isSaneURI = (aBrowser && aBrowser.currentURI) ?
>-        aURI.equals(aBrowser.currentURI) : false;
>-      if (!aBrowser || isSaneURI)
>-        self._applyPrefToSetting(aResult, aBrowser);
>+      let browser = aBrowser ? aBrowser : gBrowser.selectedBrowser;

nit: let browser = aBrowser || gBrowser.selectedBrowser;

>+      let currentURI = browser.currentURI;
>+      if (aURI.equals(currentURI)) {

nit: if (aURI.equals(browser.currentURI))

Comment 10

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

I will r+ when the new patch with a test is posted!

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch v2

Then here's a patch with a test!

Also fixed dao's comments.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/mozilla-central/rev/26cb1dc48637

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This caused bug 593378.