487656 - zoomed in by default while entering Private Browsing

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Krzysiek Wróblewski]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3

Every time I enter Private Browsing mode, the page is zoomed in and the text and images are bigger. 

Reproducible: Always

Comment 1

[(no longer active)]

(In reply to comment #0)
> Every time I enter Private Browsing mode, the page is zoomed in and the text
> and images are bigger. 

Which page are you talking about?

Comment 2

[Krzysiek Wróblewski]

All web pages are bigger, because when I enter Private Browsing mode, Firefox sets zoom level to more than 100%. When I press Ctrl+0, zoom goes back to normal.

Comment 3

[(no longer active)]

(In reply to comment #2)
> All web pages are bigger, because when I enter Private Browsing mode, Firefox
> sets zoom level to more than 100%. When I press Ctrl+0, zoom goes back to
> normal.

Hmm, can you please provide a detailed set of steps to reproduce this problem?  This looks very odd...  Do you think you would be able to provide me with a copy of your profile if needed for debugging this?

Comment 4

[Marcia Knous [:marcia]]

I can try to repro this on Windows, but I have not seen any problem with zooming behavior in any of my testing during the B3 cycle. It would be great to get a good set of STR with the websites that you are launching in PB mode that are zoomed in.

Comment 5

[(no longer active)]

Yes, it would be very nice if you can try to reproduce this, Marcia.

The odd thing is that inside the private browsing mode, we shouldn't retain the zoom settings for any sites at all, so all sites should appear at 100% by default...

Comment 6

[Krzysiek Wróblewski]

I just click Tools -> Start Private Browsing. On the welcome page (about:privatebrowsing), font is bigger and the mask image is in bad resolution. All other pages are also zoomed in. I have had this problem since I installed Firefox 3.1 beta 2.
I can provide you a copy of my profile if you need, but please tell me how to send it.

Comment 7

[(no longer active)]

(In reply to comment #6)
> I just click Tools -> Start Private Browsing. On the welcome page
> (about:privatebrowsing), font is bigger and the mask image is in bad
> resolution. All other pages are also zoomed in. I have had this problem since I
> installed Firefox 3.1 beta 2.
> I can provide you a copy of my profile if you need, but please tell me how to
> send it.

Before I look at your actual profile, can you please install the latest Firefox 3.5 nightly from <http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-1.9.1/> and see if the same problem happens in that version as well?  Thanks!

Also, can you please open about:config, enter "zoom" in the Filter text box, and watch for the below preferences, and see if any of them appear in bold?  If they do, can you please post their values here?

* browser.zoom.full
* browser.zoom.siteSpecific
* toolkit.zoomManager.zoomValues
* zoom.maxPercent
* zoom.minPercent

Comment 8

[Krzysiek Wróblewski]

I installed the latest nightly and the problem still happens. Preferences that you posted aren't bold.

I found this in the web:
http://groups.google.com/group/mozilla.feedback.firefox.prerelease/browse_thread/thread/5ceaa6b4be7b3f49
It seems that somebody had the same problem.

Comment 9

[(no longer active)]

(In reply to comment #8)
> I installed the latest nightly and the problem still happens. Preferences that
> you posted aren't bold.

OK.  Let's move on with the profile.  Can you please ZIP up your profile folder and send it to my email address as an extension (or upload it somewhere and post a link to it if you prefer)?  Thanks!

Comment 10

[Krzysiek Wróblewski]

I uploaded my profile. Here's the link:
http://www.megaupload.com/?d=YJD4OJ01

Comment 11

[Scott F]

Odd; I'm having a similar issue but when entering private mode it's zoomed out by 100% for me.

Comment 12

[[e-mail]]

(In reply to comment #11)
> Odd; I'm having a similar issue but when entering private mode it's zoomed out
> by 100% for me.

im having the same problem

> Also, can you please open about:config, enter "zoom" in the Filter text box,
> and watch for the below preferences, and see if any of them appear in bold?  If
> they do, can you please post their values here?

tried this, all the values aren't bold

Comment 13

[(no longer active)]

Confirming using the test profile.  I'm still investigating this...

Comment 14

[(no longer active)]

The cause of this problem is that inside FullZoom._applyPrefToSetting, we bail out if FullZoom.siteSpecific is false (among other things), which causes the zoom level of the previous window to persist.

Steps to reproduce (for private browsing):
1. Open about:blank.
2. Change the zoom level.
3. Switch to the PB mode.

This happens because about:blank is first entered before about:privatebrowsing is shown.

Steps to reproduce (general case):
1. Open about:blank.
2. Change the zoom level.
3. Navigate to about:logo.  This page which shows an image only document will have a modified zoom level.

Patch + tests forthcoming.

Comment 15

[(no longer active)]

Attachment: WIP

This is a WIP patch.  It solves this problem, but is wrong in the sense that the zoom applied to a tab is not persisted when you switch to it from another tab unless the site-specific prefs are enabled.  This means that with browser.zoom.siteSpecific set to false, or inside private browsing mode, the zoom value for tabs is not persistant.

We need a way to associate a zoom value with a tab indicating that it already has a zoom value applied to it.  Gavin, what do you think?

Comment 16

[(no longer active)]

Attachment: Patch (v1)

OK, here's a patch which works correctly, plus tests.

Comment 17

[Marcia Knous [:marcia]]

Ehsan: Should we make a tryserver build so we can test the patch?

Comment 18

[(no longer active)]

(In reply to comment #17)
> Ehsan: Should we make a tryserver build so we can test the patch?

Sure, I'm uploading a try server build right now, I'll post the link here when it's available.

Comment 19

[(no longer active)]

(In reply to comment #18)
> Sure, I'm uploading a try server build right now, I'll post the link here when
> it's available.

<https://build.mozilla.org/tryserver-builds/2009-05-07_08:[e-mail]-zoom/>

Comment 20

[Marcia Knous [:marcia]]

I tested the Build in Comment 19 on a Windows XP VM and followed the STR in Comment 14, and everything looked good to me - when I entered PB mode the sites showed normal size, no zoom.

Will next test on Mac.

Comment 21

[Marcia Knous [:marcia]]

Looks good as well using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090507 Minefield/3.6a1pre, which is the Tryserver build noted in Comment 19.

Comment 22

[(no longer active)]

I think this needs to block the release of Firefox 3.5, because once a user has a custom zoom setting for about:blank, they'd be affected by this; it's a very highly visible bug for those affected by it; there is no obvious way to circumvent the problem by users, and even if they reset the zoom level on about:privatebrowsing, the same zoom level won't be used the next time they enter the private browsing mode.

Comment 23

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

Don't think we'd block on this, but I'd approve the patch if it gets ready in time. Dao, can you review this as it's in browser/base/content?

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #14)
> Steps to reproduce (general case):
> 1. Open about:blank.
> 2. Change the zoom level.
> 3. Navigate to about:logo.  This page which shows an image only document will
> have a modified zoom level.

I can't reproduce this.

Comment 25

[(no longer active)]

(In reply to comment #24)
> (In reply to comment #14)
> > Steps to reproduce (general case):
> > 1. Open about:blank.
> > 2. Change the zoom level.
> > 3. Navigate to about:logo.  This page which shows an image only document will
> > have a modified zoom level.
> 
> I can't reproduce this.

Yes, this is not the right set of STRs.  I forgot to update it here:

1. Open about:logo in a new tab.
2. Type about:blank in the location bar and press enter.
3. Change the zoom level.
4. Click Back.

Comment 26

[Dão Gottwald [:dao]]

(In reply to comment #15)
> Created an attachment (id=375997) [details]
> WIP
> 
> This is a WIP patch.  It solves this problem, but is wrong in the sense that
> the zoom applied to a tab is not persisted when you switch to it from another
> tab unless the site-specific prefs are enabled.  This means that with
> browser.zoom.siteSpecific set to false, or inside private browsing mode, the
> zoom value for tabs is not persistant.

So how about not calling FullZoom.onLocationChange in that case, or passing more data so that onLocationChange can distinguish tab switches from loads?

The second patch looks gross.

Comment 27

[(no longer active)]

(In reply to comment #26)
> (In reply to comment #15)
> > Created an attachment (id=375997) [details] [details]
> > WIP
> > 
> > This is a WIP patch.  It solves this problem, but is wrong in the sense that
> > the zoom applied to a tab is not persisted when you switch to it from another
> > tab unless the site-specific prefs are enabled.  This means that with
> > browser.zoom.siteSpecific set to false, or inside private browsing mode, the
> > zoom value for tabs is not persistant.
> 
> So how about not calling FullZoom.onLocationChange in that case, or passing
> more data so that onLocationChange can distinguish tab switches from loads?

I'm not really following what you're suggesting here.

> The second patch looks gross.

Yeah, I feel guilty even thinking it, let alone writing it.  :-(

Comment 28

[Dão Gottwald [:dao]]

You said that with attachment 375997 [details] [diff] [review], the zoom didn't persist when switching back to a tab, assuming FullZoom.siteSpecific == false. The solution to that seems to be to not touch the zoom level in that case (i.e. don't call onLocationChange or return early).

Comment 29

[Dão Gottwald [:dao]]

I think you need to look at aRequest in TabsProgressListener.onLocationChange... once you've figured out that it's a tab switch, you can pass that information to FullZoom.onLocationChange and act appropriately there.

Comment 30

[(no longer active)]

(In reply to comment #29)
> I think you need to look at aRequest in
> TabsProgressListener.onLocationChange... once you've figured out that it's a
> tab switch, you can pass that information to FullZoom.onLocationChange and act
> appropriately there.

If onLocationChange gets called because of a tab switch, would aRequest be null?

Comment 31

[Dão Gottwald [:dao]]

I think so, yes.

Comment 32

[(no longer active)]

Attachment: Patch (v2)

This patch detects and ignores tab switches in case siteSpecific is false in a clean manner.  It also makes sure that toggling the print preview mode doesn't affect the zoom settings whether siteSpecific is true or false.

I also modified the unit tests to test print preview mode switches for both HTML and image documents and both with and without siteSpecific test.  In addition, the private browsing test now tests to make sure that toggling the print preview mode does not touch the zoom settings on HTML and image documents inside the private browsing mode.

Requesting review on browser/base/content parts from Dao and on privatebrowsing parts (which only includes a test) from mconnor.

Comment 33

[Dão Gottwald [:dao]]

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

>+    var resetZoom = (!this.siteSpecific || gInPrintPreviewMode ||
>+                    browser.contentDocument instanceof Ci.nsIImageDocument);

nit: "browser" should line up with "!this" from the previous line.

>+    if (typeof aDontReset != "undefined" && aDontReset && resetZoom)
>       return;

drop the |typeof aDontReset != "undefined"| part?

>   // simulate all change notifications after switching tabs
>   onUpdateCurrentBrowser: function (aStateFlags, aStatus, aMessage, aTotalProgress) {
>     if (FullZoom.updateBackgroundTabs)
>-      FullZoom.onLocationChange(gBrowser.currentURI);
>+      FullZoom.onLocationChange(gBrowser.currentURI, undefined, true);

What about the other FullZoom.onLocationChange call?

Comment 34

[Dão Gottwald [:dao]]

> >+      FullZoom.onLocationChange(gBrowser.currentURI, undefined, true);

Btw, I kinda prefer passing null rather than undefined. Or you could make aIsTabSwitch mandatory and move aBrowser to the last position.

Comment 35

[(no longer active)]

Attachment: Patch (v2.1)

(In reply to comment #33)
> (From update of attachment 381048 [details] [diff] [review])
> >+    var resetZoom = (!this.siteSpecific || gInPrintPreviewMode ||
> >+                    browser.contentDocument instanceof Ci.nsIImageDocument);
> 
> nit: "browser" should line up with "!this" from the previous line.
> 
> >+    if (typeof aDontReset != "undefined" && aDontReset && resetZoom)
> >       return;
> 
> drop the |typeof aDontReset != "undefined"| part?

Addressed both nits.

> >   // simulate all change notifications after switching tabs
> >   onUpdateCurrentBrowser: function (aStateFlags, aStatus, aMessage, aTotalProgress) {
> >     if (FullZoom.updateBackgroundTabs)
> >-      FullZoom.onLocationChange(gBrowser.currentURI);
> >+      FullZoom.onLocationChange(gBrowser.currentURI, undefined, true);
> 
> What about the other FullZoom.onLocationChange call?

As it turns out, tabbrowser.xml doesn't call onLocationChange for tabs progress listeners when a tab switch occurs, but only calls it on regular progress listeners.  So the only onLocationChange which happens because of tab switch is the one that I've patched.

Comment 36

[(no longer active)]

Attachment: Patch (v2.2)

(In reply to comment #34)
> > >+      FullZoom.onLocationChange(gBrowser.currentURI, undefined, true);
> 
> Btw, I kinda prefer passing null rather than undefined. Or you could make
> aIsTabSwitch mandatory and move aBrowser to the last position.

I think making that null would be better because leaving aIsTabSwitch optional makes more sense (since we're only using it in one location).

Comment 37

[Dão Gottwald [:dao]]

Making aIsTabSwitch optional isn't beneficial, IMHO. There are only two cases where FullZoom.onLocationChange is called, and in fact I think it's better to be explicit about what's going on here:

> As it turns out, tabbrowser.xml doesn't call onLocationChange for tabs progress
> listeners when a tab switch occurs, but only calls it on regular progress
> listeners.

And anything calling FullZoom.onLocationChange in the future should really know about whether it's a tab switch. Otherwise your current code will just assume that it's not a tab switch, which is potentially quite wrong.

Comment 38

[Dão Gottwald [:dao]]

Comment on attachment 381052 [details] [diff] [review]
Patch (v2.2)

>+  onLocationChange: function FullZoom_onLocationChange(aURI, aBrowser, aIsTabSwitch) {
>+    if (!aURI || (aIsTabSwitch && !this.siteSpecific))
>       return;

does the !aURI case really happen?

Comment 39

[(no longer active)]

Attachment: Patch (v2.3)

(In reply to comment #37)
> Making aIsTabSwitch optional isn't beneficial, IMHO. There are only two cases
> where FullZoom.onLocationChange is called, and in fact I think it's better to
> be explicit about what's going on here:
> 
> > As it turns out, tabbrowser.xml doesn't call onLocationChange for tabs progress
> > listeners when a tab switch occurs, but only calls it on regular progress
> > listeners.
> 
> And anything calling FullZoom.onLocationChange in the future should really know
> about whether it's a tab switch. Otherwise your current code will just assume
> that it's not a tab switch, which is potentially quite wrong.

OK, I did that.

(In reply to comment #38)
> (From update of attachment 381052 [details] [diff] [review])
> >+  onLocationChange: function FullZoom_onLocationChange(aURI, aBrowser, aIsTabSwitch) {
> >+    if (!aURI || (aIsTabSwitch && !this.siteSpecific))
> >       return;
> 
> does the !aURI case really happen?

I'm not 100% sure, but I don't think so.  Let's leave it there and file a follow-up about investigating whether it's really needed there.

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 381061 [details] [diff] [review]
Patch (v2.3)

>+  /**
>+   * Called when the location displayed in the primary user interface changes.

So do we rely on the location change happening in the foreground tab? In that case, the aBrowser argument would be redundant...

>+    this._applyPrefToSetting(value, undefined, true);

nit: s/undefined/null/

Comment 41

[Dão Gottwald [:dao]]

(In reply to comment #39)
> I'm not 100% sure, but I don't think so.  Let's leave it there and file a
> follow-up about investigating whether it's really needed there.

Oh right, the !aURI check was already there before. Fine by me.

Comment 42

[(no longer active)]

Can someone with a Mac please apply this patch and run browser-chrome tests?  I'm getting failures on try server which seem to be related to print stuff, and IINM no other test in the tree invokes the print preview window, so I'm suspecting this is something we don't handle somewhere...

<http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1243948198.1243956574.22149.gz>
<http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1243939618.1243947833.32371.gz>

Comment 43

[(no longer active)]

Attachment: Patch (v2.4)

(In reply to comment #40)
> (From update of attachment 381061 [details] [diff] [review])
> >+  /**
> >+   * Called when the location displayed in the primary user interface changes.
> 
> So do we rely on the location change happening in the foreground tab? In that
> case, the aBrowser argument would be redundant...

No, sorry that was an error in the documentation.  If I'm reading the code correctly, this could happen for background tabs as well.

> >+    this._applyPrefToSetting(value, undefined, true);
> 
> nit: s/undefined/null/

Done.

Comment 44

[(no longer active)]

(In reply to comment #42)
> Can someone with a Mac please apply this patch and run browser-chrome tests? 
> I'm getting failures on try server which seem to be related to print stuff, and
> IINM no other test in the tree invokes the print preview window, so I'm
> suspecting this is something we don't handle somewhere...

Could anyone help here, please?

Comment 45

[Dão Gottwald [:dao]]

(In reply to comment #35)
> As it turns out, tabbrowser.xml doesn't call onLocationChange for tabs progress
> listeners when a tab switch occurs, but only calls it on regular progress
> listeners.

Can you explain this? I'm looking at tabbrowser.xml, and don't see where these location changes would be filtered, except for a suspicious early return depending on this.mBlank.

Comment 46

[(no longer active)]

(In reply to comment #45)
> (In reply to comment #35)
> > As it turns out, tabbrowser.xml doesn't call onLocationChange for tabs progress
> > listeners when a tab switch occurs, but only calls it on regular progress
> > listeners.
> 
> Can you explain this? I'm looking at tabbrowser.xml, and don't see where these
> location changes would be filtered, except for a suspicious early return
> depending on this.mBlank.

This is the only call site to onLocationChange for tab switches: <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#892>, and it is made on regular progress listeners, not tabs progress listeners.

Comment 47

[Dão Gottwald [:dao]]

Comment on attachment 381275 [details] [diff] [review]
Patch (v2.4)

>   setSettingValue: function FullZoom_setSettingValue() {
>     var value = this._cps.getPref(gBrowser.currentURI, this.name);
>-    this._applyPrefToSetting(value);
>+    this._applyPrefToSetting(value, null, true);
>   },

>+   * If aDontReset is true, we skip resetting the zoom value.  This can happen
>+   * when exiting from the print preview mode.
>    **/
>-  _applyPrefToSetting: function FullZoom__applyPrefToSetting(aValue, aBrowser) {
>+  _applyPrefToSetting: function FullZoom__applyPrefToSetting(aValue, aBrowser,
>+                                                             aDontReset) {

If I zoom a page and open the print preview, the whole preview area is zoomed, which seems wrong. And if you zoom within the print preview using the scroll wheel, that value won't be reset after exiting print preview (assuming site-specific zoom is off).

Seems like the print preview code should instead store ZoomManager.zoom and set it to 1 when entering the print preview, and set it back to the stored value when exiting print preview. No need to access FullZoom at all, right?

Comment 48

[(no longer active)]

(In reply to comment #47)
> If I zoom a page and open the print preview, the whole preview area is zoomed,
> which seems wrong.

Yes, but this happens even without this patch.

> And if you zoom within the print preview using the scroll
> wheel, that value won't be reset after exiting print preview (assuming
> site-specific zoom is off).

Again, this happens without this patch as well, both with and without siteSpecific enabled.

In other words, this patch does not change the behavior of print preview with respect to zoom levels.

> Seems like the print preview code should instead store ZoomManager.zoom and set
> it to 1 when entering the print preview, and set it back to the stored value
> when exiting print preview. No need to access FullZoom at all, right?

What if we don't have a stored value?  (Anyway I prefer to defer this to a follow-up bug because this bug has already grown much bigger than what comment 0 was about...)

Comment 49

[Dão Gottwald [:dao]]

(In reply to comment #48)
> Yes, but this happens even without this patch.

Right, but you're adjusting the internal FullZoom API to support something that shouldn't be supported this way.

> > And if you zoom within the print preview using the scroll
> > wheel, that value won't be reset after exiting print preview (assuming
> > site-specific zoom is off).
> 
> Again, this happens without this patch as well, both with and without
> siteSpecific enabled.

Not quite. If there's a siteSpecific value, that would be used.

> > Seems like the print preview code should instead store ZoomManager.zoom and set
> > it to 1 when entering the print preview, and set it back to the stored value
> > when exiting print preview. No need to access FullZoom at all, right?
> 
> What if we don't have a stored value?

You simply store ZoomManager.zoom in an old-school variable. Again, FullZoom shouldn't be involved there.

Comment 50

[(no longer active)]

(In reply to comment #49)
> > > And if you zoom within the print preview using the scroll
> > > wheel, that value won't be reset after exiting print preview (assuming
> > > site-specific zoom is off).
> > 
> > Again, this happens without this patch as well, both with and without
> > siteSpecific enabled.
> 
> Not quite. If there's a siteSpecific value, that would be used.

So from the user's perspective, the same zoom value that was active before entering print preview is observed after exiting from it, right?

> > > Seems like the print preview code should instead store ZoomManager.zoom and set
> > > it to 1 when entering the print preview, and set it back to the stored value
> > > when exiting print preview. No need to access FullZoom at all, right?
> > 
> > What if we don't have a stored value?
> 
> You simply store ZoomManager.zoom in an old-school variable. Again, FullZoom
> shouldn't be involved there.

Do you want me to do that in the toolkit print preview component, or at the browser level?

To confirm that I have a good understanding about the desired behavior here, you want the zoom level inside the print preview mode to be "sealed" from the page zoom level (i.e., init it to 1.0, and discard the changed value after exiting the print preview), right?

Comment 51

[Dão Gottwald [:dao]]

(In reply to comment #50)
> > > > And if you zoom within the print preview using the scroll
> > > > wheel, that value won't be reset after exiting print preview (assuming
> > > > site-specific zoom is off).
> > > 
> > > Again, this happens without this patch as well, both with and without
> > > siteSpecific enabled.
> > 
> > Not quite. If there's a siteSpecific value, that would be used.
> 
> So from the user's perspective, the same zoom value that was active before
> entering print preview is observed after exiting from it, right?

Yes, as long as the page has a site-specific value and FullZoom is in the site-specific mode.

> > > > Seems like the print preview code should instead store ZoomManager.zoom and set
> > > > it to 1 when entering the print preview, and set it back to the stored value
> > > > when exiting print preview. No need to access FullZoom at all, right?
> > > 
> > > What if we don't have a stored value?
> > 
> > You simply store ZoomManager.zoom in an old-school variable. Again, FullZoom
> > shouldn't be involved there.
> 
> Do you want me to do that in the toolkit print preview component, or at the
> browser level?

I haven't looked at the toolkit component. If you think that would be as simple, toolkit might be preferable.
browser would be fine, as that's where we call setSettingValue currently. In the long run, bug 133787 should get fixed and make this unnecessary.

> To confirm that I have a good understanding about the desired behavior here,
> you want the zoom level inside the print preview mode to be "sealed" from the
> page zoom level (i.e., init it to 1.0, and discard the changed value after
> exiting the print preview), right?

Yep.

Comment 52

[(no longer active)]

Attachment: Patch (v2.5)

I've updated the patch to achieve what Dao suggested.  I had to do that in toolkit, because of the strange way that the print preview zoom level is implemented (it's applied as a coefficient to the normal zoom level: <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#2795>) so we have to reset the zoom level before the print preview is initialized, and doing that in browser would cause a visible change in the zoom level while the print preview progress dialog is active.

Comment 53

[Dão Gottwald [:dao]]

Comment on attachment 382024 [details] [diff] [review]
Patch (v2.5)

>+  saveZoomValue: function () {

The important part of that method seems to be the resetting, the saving seems to be secondary...

>+    var browser = getPPBrowser();
>+    browser.zoomValue = ZoomManager.zoom;

Why browser.zoomValue rather than something like this._originalZoomValue?

>+    ZoomManager.setZoomForBrowser(browser, value);

ZoomManager.zoom = value?

Comment 54

[(no longer active)]

Attachment: Patch (v2.6)

(In reply to comment #53)
> (From update of attachment 382024 [details] [diff] [review])
> >+  saveZoomValue: function () {
> 
> The important part of that method seems to be the resetting, the saving seems
> to be secondary...

OK, changed the function name to resetZoomValue.

> >+    var browser = getPPBrowser();
> >+    browser.zoomValue = ZoomManager.zoom;
> 
> Why browser.zoomValue rather than something like this._originalZoomValue?

I had this code in FullZoom originally, and I forgot to change this when I moved it to printUtils

> >+    ZoomManager.setZoomForBrowser(browser, value);
> 
> ZoomManager.zoom = value?

Done.

Comment 55

[Dão Gottwald [:dao]]

Comment on attachment 382055 [details] [diff] [review]
Patch (v2.6)

>+  resetZoomValue: function () {
>+    var browser = getPPBrowser();
>+    this._originalZoomValue = ZoomManager.zoom;
>+    ZoomManager.reset();
>+  },

var browser is unused

>+  restoreZoomValue: function () {
>+    var browser = getPPBrowser();

ditto

>+    var value = 1.0;
>+    if (this._originalZoomValue)

Which case is this trying to protect against?

>     try {
>+      this.resetZoomValue();

This doesn't look like it should be part of the try block.

>       webBrowserPrint.printPreview(printSettings, null, this._webProgressPP.value);
>     } catch (e) {
>       // Pressing cancel is expressed as an NS_ERROR_ABORT return value,
>       // causing an exception to be thrown which we catch here.
>       // Unfortunately this will also consume helpful failures, so add a
>       // dump(e); // if you need to debug
>       return;

Should restoreZoomValue be called here?

Comment 57

[(no longer active)]

Attachment: Patch (v2.7)

(In reply to comment #55)
> (From update of attachment 382055 [details] [diff] [review])
> >+  resetZoomValue: function () {
> >+    var browser = getPPBrowser();
> >+    this._originalZoomValue = ZoomManager.zoom;
> >+    ZoomManager.reset();
> >+  },
> 
> var browser is unused

Fixed.

> >+  restoreZoomValue: function () {
> >+    var browser = getPPBrowser();
> 
> ditto

Fixed.

> >+    var value = 1.0;
> >+    if (this._originalZoomValue)
> 
> Which case is this trying to protect against?

I read around the code and it seems like enterPrintPreview is guaranteed to be called before exitPrintPreview.  So, I removed the check, and also removed the two functions and made them inline in their call site.

> >     try {
> >+      this.resetZoomValue();
> 
> This doesn't look like it should be part of the try block.

Fixed.

> >       webBrowserPrint.printPreview(printSettings, null, this._webProgressPP.value);
> >     } catch (e) {
> >       // Pressing cancel is expressed as an NS_ERROR_ABORT return value,
> >       // causing an exception to be thrown which we catch here.
> >       // Unfortunately this will also consume helpful failures, so add a
> >       // dump(e); // if you need to debug
> >       return;
> 
> Should restoreZoomValue be called here?

Yes, fixed.

I'm not asking for review yet until the Mac failures in comment 42 are solved.

Comment 58

[(no longer active)]

Attachment: Patch (v2.8)

OK, the reason for Mac OS X test failure was simply the fact that Mac OS X does not support the toolkit-based print preview UI.  So I simply disabled the print preview related tests on Mac.

With this change, the whole patch is ready for review.

Comment 59

[Dão Gottwald [:dao]]

Comment on attachment 390697 [details] [diff] [review]
Patch (v2.8)

>+   * @param aURI A URI object representing the new location.
>+   * @param aIsTabSwitch Whether this location change has happened because 
>+   *        of a tab switch.
>+   * @param aBrowser (optional) browser object displaying the document

Should be formatted like this:

>+   * @param aURI
>+   *        A URI object representing the new location.
>+   * @param aIsTabSwitch
>+   *        Whether this location change has happened because
>+   *        of a tab switch.
>+   * @param aBrowser
>+   *        (optional) browser object displaying the document

I haven't really reviewed the test.

Comment 60

[(no longer active)]

Comment on attachment 390697 [details] [diff] [review]
Patch (v2.8)

(In reply to comment #59)
> (From update of attachment 390697 [details] [diff] [review])
> >+   * @param aURI A URI object representing the new location.
> >+   * @param aIsTabSwitch Whether this location change has happened because 
> >+   *        of a tab switch.
> >+   * @param aBrowser (optional) browser object displaying the document
> 
> Should be formatted like this:
> 
> >+   * @param aURI
> >+   *        A URI object representing the new location.
> >+   * @param aIsTabSwitch
> >+   *        Whether this location change has happened because
> >+   *        of a tab switch.
> >+   * @param aBrowser
> >+   *        (optional) browser object displaying the document

Will fix in the next iteration.

> I haven't really reviewed the test.

Can you please review the browser/base/content/test/browser_bug386835.js test?  I'll ask mconnor for the browser/components/privatebrowsing/test/browser/browser_privatebrowsing_zoomrestore.js test review.

Comment 61

[Dão Gottwald [:dao]]

Comment on attachment 390697 [details] [diff] [review]
Patch (v2.8)

>-  finishTest();
>+  setTimeout(imageZoomSwitch, 0);
>+}

Why not just imageZoomSwitch();?

>+function imageZoomSwitch() {
>+  navigate("back", function() {

The first argument is weird as a string, but given that it's a test, I don't really care...

>+function testPrintPreview(aTab, aCallback) {
>+  gBrowser.selectedTab = aTab;
>+  FullZoom.enlarge();
>+  let level = ZoomManager.getZoomForBrowser(gBrowser.getBrowserForTab(aTab));

let level = ZoomManager.zoom;

>+  function onEnterPP(aHide) {
>+    toggleAffectedChromeOrig(aHide);

function onEnterPP() {
  toggleAffectedChromeOrig.apply(null, arguments);

and the same for onExitPP.

Comment 62

[(no longer active)]

Attachment: Patch (v2.9)

(In reply to comment #61)
> (From update of attachment 390697 [details] [diff] [review])
> >-  finishTest();
> >+  setTimeout(imageZoomSwitch, 0);
> >+}
> 
> Why not just imageZoomSwitch();?

Because in that case, the pageshow event listener set up in the |navigate| function is installed during the load event listener set up in the |load| function, and it catches the pageshow event for the previous load, instead of for the goBack command.

> >+function imageZoomSwitch() {
> >+  navigate("back", function() {
> 
> The first argument is weird as a string, but given that it's a test, I don't
> really care...

Oops, you're right.  This was never supposed to appear in my test!  I fixed it in this iteration.

> >+function testPrintPreview(aTab, aCallback) {
> >+  gBrowser.selectedTab = aTab;
> >+  FullZoom.enlarge();
> >+  let level = ZoomManager.getZoomForBrowser(gBrowser.getBrowserForTab(aTab));
> 
> let level = ZoomManager.zoom;

Done.

> >+  function onEnterPP(aHide) {
> >+    toggleAffectedChromeOrig(aHide);
> 
> function onEnterPP() {
>   toggleAffectedChromeOrig.apply(null, arguments);
> 
> and the same for onExitPP.

Done.

Comment 63

[Dão Gottwald [:dao]]

Comment on attachment 391044 [details] [diff] [review]
Patch (v2.9)

>+  function onEnterPP(aHide) {
>+    toggleAffectedChromeOrig.apply(null, arguments);
>+
>+    function onExitPP(aHide) {
>+      toggleAffectedChromeOrig.apply(null, arguments);

aHide is unused.

Comment 64

[(no longer active)]

Attachment: Patch (v2.10)

(In reply to comment #63)
> (From update of attachment 391044 [details] [diff] [review])
> >+  function onEnterPP(aHide) {
> >+    toggleAffectedChromeOrig.apply(null, arguments);
> >+
> >+    function onExitPP(aHide) {
> >+      toggleAffectedChromeOrig.apply(null, arguments);
> 
> aHide is unused.

Right, fixed.

Comment 65

[Mike Connor [:mconnor]]

Comment on attachment 391046 [details] [diff] [review]
Patch (v2.10)


>diff --git a/toolkit/components/printing/content/printUtils.js b/toolkit/components/printing/content/printUtils.js
>--- a/toolkit/components/printing/content/printUtils.js
>+++ b/toolkit/components/printing/content/printUtils.js

>@@ -213,21 +214,26 @@ var PrintUtils = {
>       throw Components.results.NS_NOINTERFACE;
>     }
>   },
> 
>   enterPrintPreview: function (aWindow)
>   {
>     gFocusedElement = document.commandDispatcher.focusedElement;
> 
>+    // Reset the zoom value and save it to be restored later.
>+    this._originalZoomValue = ZoomManager.zoom;
>+    ZoomManager.reset();

You're assuming ZoomManager is in scope, which is quite possibly not true for all consumers of printUtils.js.  Otherwise, looks good.  Thanks to Dao for all of the rounds of review before this!

Comment 66

[(no longer active)]

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

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

So this breaks browser.zoom.siteSpecific=false. A new page is always reset to
zoom level 1. Note, we have code in nsDocShell to explicitly set the
zoom level to be the same as what the previous page had. This change basically
undoes that, but in Firefox code.

Comment 69

[Boris Zbarsky [:bzbarsky]]

Sounds like we need a bug filed on comment 68.

Comment 70

[Olli Pettay [:smaug][[e-mail]]]

https://bugzilla.mozilla.org/show_bug.cgi?id=510187

Comment 72

[Daniel Veditz [:dveditz]]

Comment on attachment 391046 [details] [diff] [review]
Patch (v2.10)

Doesn't seem like the kind of fix we have to take on the stable branch. 1.9.1 approval denied.