464962 - Zoom-Level reset to default when switching tabs while in Private Browsing Mode

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P2)

Description

[Luke Iliffe (Harlequin99)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081114 Minefield/3.1b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081114 Minefield/3.1b2pre

When in the new private browsing mode setting the zoom level to anything other than default will be lost after switch to a second tab then back to the zoomed page.

Reproducible: Always

Steps to Reproduce:
1.Switch to Private Browsing Mode
2.Open tab#1
3.Increase the zoom level for Tab#1
4.Open tab #2
5.Switch back to Tab#1 -> zoom level will be set to default a few milliseconds later
Actual Results:  
Zoom level is reset

Expected Results:  
Site zoom level maintained whilst in the same private browsing session.

Comment 1

[(no longer active)]

Gavin, are you familiar with page zoom code?  We disable saving anything in the content prefs module, but the page should remember its zoom setting once set manually across tab switches, isn't it?

Comment 2

[(no longer active)]

Confirming on Windows.  Marcia, can you check Mac as well?

Comment 3

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

We set the page zoom from prefs on every tab switch:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-textZoom.js#229

Seems like we should avoid doing that while PB is enabled. Or perhaps just have siteSpecific be false in PB mode.

Comment 5

[Marcia Knous [:marcia]]

Yes, this happens on mac as well.

(In reply to comment #2)
> Confirming on Windows.  Marcia, can you check Mac as well?

Comment 6

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

(In reply to comment #3)
> Or perhaps just have siteSpecific be false in PB mode.

That seems sensible to me. This blocks final release.

Comment 7

[(no longer active)]

Attachment: Patch (v1)

Patch with test.

Comment 8

[(no longer active)]

Attachment: Patch (v1)

I just realized that I had forgot to qrefresh the patch, so please ignore the previous patch.  :-)

Comment 9

[(no longer active)]

Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.

Comment 10

[(no longer active)]

Gavin: ping?

Comment 11

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

Comment on attachment 351351 [details] [diff] [review]
Patch (v1)

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

>+    if (gPrivateBrowsingUI.privateBrowsingEnabled)
>+      this._onEnterPrivateBrowsing();
>+    else
>+      this._onLeavePrivateBrowsing();

I'd get rid of _onEnter and _onLeave and just assign to this._inPrivateBrowsing, with a similar change to have the pref set _siteSpecificPref, and then turn siteSpecific into a getter that just returns !_inPrivateBrowsing && _siteSpecificPref. I think the function call overhead of the getter is outweighed by the convenience of not having to check all of the siteSpecific setting codepaths to ensure they don't conflict.

Could also just always have it check the service directly and avoid the observer, since the getter for privateBrowsingEnabled is just a JS property get, but this method is called on every tab switch and page load, so perhaps it's not a bad idea to avoid the xpconnect overhead... Things like this certainly make a difference on Fennec, but for Firefox it probably matters less.

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

>+  // initialize the private browsing UI
>+  gPrivateBrowsingUI.init();

>   // Initialize the full zoom setting.

>-  // initialize the private browsing UI
>-  gPrivateBrowsingUI.init();

Why is this change needed?

Comment 12

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #11)
> I'd get rid of _onEnter and _onLeave and just assign to
> this._inPrivateBrowsing, with a similar change to have the pref set
> _siteSpecificPref, and then turn siteSpecific into a getter that just returns
> !_inPrivateBrowsing && _siteSpecificPref. I think the function call overhead of
> the getter is outweighed by the convenience of not having to check all of the
> siteSpecific setting codepaths to ensure they don't conflict.

Done.

> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >+  // initialize the private browsing UI
> >+  gPrivateBrowsingUI.init();
> 
> >   // Initialize the full zoom setting.
> 
> >-  // initialize the private browsing UI
> >-  gPrivateBrowsingUI.init();
> 
> Why is this change needed?

Because FullZoom_init tries to use gPrivateBrowsingUI, so it should be inited before the full zoom object.

Comment 13

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

Comment on attachment 358150 [details] [diff] [review]
Patch (v2)

Oh, I didn't notice the dependency. I don't think it's worth the inter-file dependency just to avoid a getService call, so I think you should omit that change and just get the privateBrowsingService directly rather than relying on gPrivateBrowsingUI initialization happening first.

Comment 14

[(no longer active)]

Landed with the requested change on mozilla-central:
<http://hg.mozilla.org/mozilla-central/rev/e1962b8ae522>

Comment 15

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e5033a8c5472

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified on trunk and 1.9.1 branch with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090124 Minefield/3.2a1pre ID:20090124020327

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre (.NET CLR 3.5.30729) ID:20090123033224

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090124 Shiretoko/3.1b3pre ID:20090124020525

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090124 Shiretoko/3.1b3pre (.NET CLR 3.5.30729) Ubiquity/0.1.5 ID:20090124033656