zoomed in by default while entering Private Browsing

RESOLVED FIXED in Firefox 3.6a1

Status

()

RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: krzysiek.wr, Assigned: Ehsan)

Tracking

3.5 Branch
Firefox 3.6a1
Points:
---
Bug Flags:
blocking-firefox3.5 -
in-testsuite +

Firefox Tracking Flags

(status1.9.1 wontfix)

Details

Attachments

(1 attachment, 12 obsolete attachments)

(Reporter)

Description

10 years ago
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
(Assignee)

Comment 1

10 years ago
(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?
(Reporter)

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
(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?
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.
(Assignee)

Comment 5

10 years ago
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...
(Reporter)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
(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
(Assignee)

Updated

10 years ago
Version: unspecified → 3.1 Branch
(Reporter)

Comment 8

10 years ago
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.
(Assignee)

Comment 9

10 years ago
(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!
(Reporter)

Comment 10

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

Comment 11

9 years ago
Odd; I'm having a similar issue but when entering private mode it's zoomed out by 100% for me.
(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
(Assignee)

Comment 13

9 years ago
Confirming using the test profile.  I'm still investigating this...
Assignee: nobody → ehsan.akhgari
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 14

9 years ago
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.
(Assignee)

Comment 15

9 years ago
Created attachment 375997 [details] [diff] [review]
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?
(Assignee)

Comment 16

9 years ago
Created attachment 376004 [details] [diff] [review]
Patch (v1)

OK, here's a patch which works correctly, plus tests.
Attachment #375997 - Attachment is obsolete: true
Attachment #376004 - Flags: review?(gavin.sharp)
Ehsan: Should we make a tryserver build so we can test the patch?
(Assignee)

Comment 18

9 years ago
(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.
(Assignee)

Comment 19

9 years ago
(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:46-ehsan.akhgari@gmail.com-zoom/>
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.
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.
(Assignee)

Comment 22

9 years ago
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.
Flags: blocking-firefox3.5?
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?
Flags: blocking-firefox3.5? → blocking-firefox3.5-
(Assignee)

Updated

9 years ago
Attachment #376004 - Flags: review?(gavin.sharp) → review?(johnath)
(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.
(Assignee)

Comment 25

9 years ago
(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.
(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.
(Assignee)

Comment 27

9 years ago
(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.  :-(
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).
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.
(Assignee)

Comment 30

9 years ago
(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?
I think so, yes.
(Assignee)

Comment 32

9 years ago
Created attachment 381048 [details] [diff] [review]
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.
Attachment #376004 - Attachment is obsolete: true
Attachment #381048 - Flags: review?(mconnor)
Attachment #381048 - Flags: review?(dao)
Attachment #376004 - Flags: review?(johnath)
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?
> >+      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.
(Assignee)

Comment 35

9 years ago
Created attachment 381051 [details] [diff] [review]
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.
Attachment #381048 - Attachment is obsolete: true
Attachment #381051 - Flags: review?(mconnor)
Attachment #381051 - Flags: review?(dao)
Attachment #381048 - Flags: review?(mconnor)
Attachment #381048 - Flags: review?(dao)
(Assignee)

Comment 36

9 years ago
Created attachment 381052 [details] [diff] [review]
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).
Attachment #381051 - Attachment is obsolete: true
Attachment #381052 - Flags: review?(mconnor)
Attachment #381052 - Flags: review?(dao)
Attachment #381051 - Flags: review?(mconnor)
Attachment #381051 - Flags: review?(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 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?
(Assignee)

Comment 39

9 years ago
Created attachment 381061 [details] [diff] [review]
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.
Attachment #381052 - Attachment is obsolete: true
Attachment #381061 - Flags: review?(mconnor)
Attachment #381061 - Flags: review?(dao)
Attachment #381052 - Flags: review?(mconnor)
Attachment #381052 - Flags: review?(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/
(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.
(Assignee)

Comment 42

9 years ago
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>
(Assignee)

Comment 43

9 years ago
Created attachment 381275 [details] [diff] [review]
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.
Attachment #381061 - Attachment is obsolete: true
Attachment #381275 - Flags: review?(mconnor)
Attachment #381275 - Flags: review?(dao)
Attachment #381061 - Flags: review?(mconnor)
Attachment #381061 - Flags: review?(dao)
(Assignee)

Comment 44

9 years ago
(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?
(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.
(Assignee)

Comment 46

9 years ago
(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 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?
(Assignee)

Comment 48

9 years ago
(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...)
(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.
(Assignee)

Comment 50

9 years ago
(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?
(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.
(Assignee)

Comment 52

9 years ago
Created attachment 382024 [details] [diff] [review]
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.
Attachment #381275 - Attachment is obsolete: true
Attachment #382024 - Flags: review?(dao)
Attachment #381275 - Flags: review?(mconnor)
Attachment #381275 - Flags: review?(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?
(Assignee)

Comment 54

9 years ago
Created attachment 382055 [details] [diff] [review]
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.
Attachment #382024 - Attachment is obsolete: true
Attachment #382055 - Flags: review?(dao)
Attachment #382024 - Flags: review?(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?
(Assignee)

Updated

9 years ago
Duplicate of this bug: 499394

Updated

9 years ago
Attachment #382055 - Flags: review?(dao)
(Assignee)

Comment 57

9 years ago
Created attachment 390696 [details] [diff] [review]
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.
Attachment #382055 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
status1.9.1: --- → ?
(Assignee)

Comment 58

9 years ago
Created attachment 390697 [details] [diff] [review]
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.
Attachment #390696 - Attachment is obsolete: true
Attachment #390697 - Flags: review?(dao)

Updated

9 years ago
Attachment #390697 - Flags: review?(dao) → review+
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.
(Assignee)

Comment 60

9 years ago
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.
Attachment #390697 - Flags: review?(mconnor)
(Assignee)

Updated

9 years ago
Whiteboard: [needs r=mconnor]
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.
(Assignee)

Comment 62

9 years ago
Created attachment 391044 [details] [diff] [review]
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.
Attachment #390697 - Attachment is obsolete: true
Attachment #391044 - Flags: review?(mconnor)
Attachment #390697 - Flags: review?(mconnor)
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.
(Assignee)

Comment 64

9 years ago
Created attachment 391046 [details] [diff] [review]
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.
Attachment #391044 - Attachment is obsolete: true
Attachment #391046 - Flags: review?(mconnor)
Attachment #391044 - Flags: review?(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!
Attachment #391046 - Flags: review?(mconnor) → review+
(Assignee)

Comment 66

9 years ago
http://hg.mozilla.org/mozilla-central/rev/bf18f4b1e91a
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
OS: Windows XP → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [needs r=mconnor]
Target Milestone: --- → Firefox 3.6a1
(Assignee)

Updated

9 years ago
Attachment #391046 - Flags: approval1.9.1.3?
(Assignee)

Updated

9 years ago
Duplicate of this bug: 509355

Updated

9 years ago
Depends on: 503729
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.
Sounds like we need a bug filed on comment 68.

Updated

9 years ago
Depends on: 510187

Updated

9 years ago
No longer depends on: 503729
(Assignee)

Updated

9 years ago
Duplicate of this bug: 511239
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.
Attachment #391046 - Flags: approval1.9.1.3? → approval1.9.1.3-
status1.9.1: ? → wontfix
(Assignee)

Updated

9 years ago
Duplicate of this bug: 524323
(Assignee)

Updated

9 years ago
Duplicate of this bug: 534432

Updated

8 years ago
Depends on: 575830
No longer depends on: 575830
You need to log in before you can comment on or make changes to this bug.