- open http://slashdot.org/ - resize the text size - go to one of the articles - notice that the new page is also using the new text size (ok) - go back - notice that the first page still has the new size (ok) - resize the text back to normal - go forward again - notice that the second page still has the changed text size, not the normal The text size has to work for all pages in the window or tab. This means that if you change it, it should invalidate the cached pages in bfcache or it should at least trigger a new text resize when you go back to that page.

IMHO We should not disable the bfcache or evict pages already in there when the text is resized. It is both faster and more consistent to just update the size once we return to the cached page.

transferring nomination to 1.8b4. If we don't get it there, we don't get it for 1.1

Targetting to fix for beta, this one should be relatively easy.

maybe RestoreFromHistory can use nsDocShell::SetupNewViewer?

- I didn't use SetupNewViewer because that did a lot of stuff, some of which should probably not be done here - copying the other properties that are copied in SetupNewViewer is probably not necessary for history traversal - I'd have done the ClearStyleDataAndReflow call in docshell but the symbol is not exported from layout.

Instead of the nsDocumentViewer.cpp change, why not move the storage of text zoom from device context to pres context? There doesn't seem to be a good reason for it to be on the device context anymore. (Given where it was stored, why did we need to do all that work to persist between document viewers in the past? I guess it was for subframes.)

I'm unsure whether the printing changes are correct... I don't know what their point was (is the DC reused for printing and screen display?)

Maybe I misunderstand this bug, but AIUI, it says that all pages in the fastback cache should have the same zoom level set, i.e. you change zoom in one page and go back/forward it should be applied to the other page as well. If this is correct, this will break fastback cache for some embedders (Epiphany and Galeon at least) which do apply different zoom levels on a *per-page basis*. So, say, you go to page A with zoom z_A, load page B and change zoom to zoom z_B, go back, change z_A, and forward again. Comment 0 says that the new z_A should apply to page B; but the app wants to keep applying z_B. Will this patch cause an unnecessary reflow because it tries to apply z_A to page B, and then another one when the embedding app resets the zoom to z_B ? (In reply to comment #7) > Created an attachment (id=191807) [edit] + if (mPresContext && aTextZoom != mPresContext->TextZoom()) { + mPresContext->SetTextZoom(aTextZoom); Just one thing I noticed in the patch: I thought one should never directly compare floats with == or != ?

(In reply to comment #8) > Maybe I misunderstand this bug, but AIUI, it says that all pages in the fastback > cache should have the same zoom level set, i.e. you change zoom in one page and > go back/forward it should be applied to the other page as well. If this is > correct, this will break fastback cache for some embedders (Epiphany and Galeon > at least) which do apply different zoom levels on a *per-page basis*. I filed this because it would be the expected behaviour would be that when you change the text-size (which works per window or tab), you would expect that it works for the entire window/tab, pages in the cache included. Actually, the fact that it didn't work for the cache until I reloaded, was proof for me that the bfcache worked :-) Those extensions (which implement this per page or per website) actually have the same problem. You might change the text-size for a specific website, and then go back in the cache. The cached copy should also have the changed text-size. > So, say, you go to page A with zoom z_A, load page B and change zoom to zoom > z_B, go back, change z_A, and forward again. Comment 0 says that the new z_A > should apply to page B; but the app wants to keep applying z_B. Will this patch > cause an unnecessary reflow because it tries to apply z_A to page B, and then > another one when the embedding app resets the zoom to z_B ? > No. The point is that when you go back to A, z_B should be applied. Currently, the cache contains a copy of A with z_A. When you have an extension that tries to force page A to use z_A, then it should do so (invalidating the page when you go back, for example). But if you don't have that extension, then it should always show it to me in the current zoom-level.

(In reply to comment #9) > I filed this because it would be the expected behaviour would be that when you > change the text-size (which works per window or tab), you would expect that it > works for the entire window/tab, pages in the cache included. Actually, the fact > that it didn't work for the cache until I reloaded, was proof for me that the > bfcache worked :-) > > Those extensions (which implement this per page or per website) actually have > the same problem. You might change the text-size for a specific website, and > then go back in the cache. The cached copy should also have the changed text-size. It's not an extension, it's an embedding application. What I'm saying is that docshell shouldn't hardcode firefox client policy (to always apply the same zoon to all pages in the tab). > > So, say, you go to page A with zoom z_A, load page B and change zoom to zoom > > z_B, go back, change z_A, and forward again. Comment 0 says that the new z_A > > should apply to page B; but the app wants to keep applying z_B. Will this patch > > cause an unnecessary reflow because it tries to apply z_A to page B, and then > > another one when the embedding app resets the zoom to z_B ? > > > > No. The point is that when you go back to A, z_B should be applied. Currently, > the cache contains a copy of A with z_A. Which is exactly how I want it :) > When you have an extension that tries to force page A to use z_A, then it should > do so (invalidating the page when you go back, for example). Which will cause unnecessary reflows, slowing down back/forward for my application. > But if you don't > have that extension, then it should always show it to me in the current zoom-level. Yes, this is firefox zoom policy. So IMHO this should be done in frontend, not enforced in docshell. ---- Anyway, I talked with biesi about this, and concluded that implementing this in docshell for now is ok, but that is should be moved to frontend later.

this sets the text zoom earlier, so that onpageshow etc handlers can override it

HEAD: Checking in docshell/base/nsDocShell.cpp; /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp new revision: 1.722; previous revision: 1.721 done Checking in gfx/public/nsDeviceContext.h; /cvsroot/mozilla/gfx/public/nsDeviceContext.h,v <-- nsDeviceContext.h new revision: 1.40; previous revision: 1.39 done Checking in gfx/public/nsIDeviceContext.h; /cvsroot/mozilla/gfx/public/nsIDeviceContext.h,v <-- nsIDeviceContext.h new revision: 1.50; previous revision: 1.49 done Checking in gfx/src/nsDeviceContext.cpp; /cvsroot/mozilla/gfx/src/nsDeviceContext.cpp,v <-- nsDeviceContext.cpp new revision: 3.115; previous revision: 3.114 done Checking in gfx/src/cairo/nsCairoDeviceContext.cpp; /cvsroot/mozilla/gfx/src/cairo/nsCairoDeviceContext.cpp,v <-- nsCairoDeviceContext.cpp new revision: 1.12; previous revision: 1.11 done Checking in layout/base/nsDocumentViewer.cpp; /cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v <-- nsDocumentViewer.cpp new revision: 1.445; previous revision: 1.444 done Checking in layout/base/nsPresContext.cpp; /cvsroot/mozilla/layout/base/nsPresContext.cpp,v <-- nsPresContext.cpp new revision: 3.289; previous revision: 3.288 done Checking in layout/base/nsPresContext.h; /cvsroot/mozilla/layout/base/nsPresContext.h,v <-- nsPresContext.h new revision: 3.151; previous revision: 3.150 done Checking in layout/printing/nsPrintData.cpp; /cvsroot/mozilla/layout/printing/nsPrintData.cpp,v <-- nsPrintData.cpp new revision: 1.6; previous revision: 1.5 done Checking in layout/printing/nsPrintData.h; /cvsroot/mozilla/layout/printing/nsPrintData.h,v <-- nsPrintData.h new revision: 1.7; previous revision: 1.6 done Checking in layout/printing/nsPrintEngine.cpp; /cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v <-- nsPrintEngine.cpp new revision: 1.98; previous revision: 1.97 done Checking in layout/style/nsStyleStruct.cpp; /cvsroot/mozilla/layout/style/nsStyleStruct.cpp,v <-- nsStyleStruct.cpp new revision: 3.130; previous revision: 3.129 done MOZILLA_1_8_BRANCH: Checking in docshell/base/nsDocShell.cpp; /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp new revision: 1.719.2.1; previous revision: 1.719 done Checking in gfx/public/nsDeviceContext.h; /cvsroot/mozilla/gfx/public/nsDeviceContext.h,v <-- nsDeviceContext.h new revision: 1.39.18.1; previous revision: 1.39 done Checking in gfx/public/nsIDeviceContext.h; /cvsroot/mozilla/gfx/public/nsIDeviceContext.h,v <-- nsIDeviceContext.h new revision: 1.49.20.1; previous revision: 1.49 done Checking in gfx/src/nsDeviceContext.cpp; /cvsroot/mozilla/gfx/src/nsDeviceContext.cpp,v <-- nsDeviceContext.cpp new revision: 3.114.12.1; previous revision: 3.114 done Checking in gfx/src/cairo/nsCairoDeviceContext.cpp; /cvsroot/mozilla/gfx/src/cairo/nsCairoDeviceContext.cpp,v <-- nsCairoDeviceContext.cpp new revision: 1.11.8.1; previous revision: 1.11 done Checking in layout/base/nsDocumentViewer.cpp; /cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v <-- nsDocumentViewer.cpp new revision: 1.442.4.1; previous revision: 1.442 done Checking in layout/base/nsPresContext.cpp; /cvsroot/mozilla/layout/base/nsPresContext.cpp,v <-- nsPresContext.cpp new revision: 3.288.12.1; previous revision: 3.288 done Checking in layout/base/nsPresContext.h; /cvsroot/mozilla/layout/base/nsPresContext.h,v <-- nsPresContext.h new revision: 3.150.4.1; previous revision: 3.150 done Checking in layout/printing/nsPrintData.cpp; /cvsroot/mozilla/layout/printing/nsPrintData.cpp,v <-- nsPrintData.cpp new revision: 1.5.14.1; previous revision: 1.5 done Checking in layout/printing/nsPrintData.h; /cvsroot/mozilla/layout/printing/nsPrintData.h,v <-- nsPrintData.h new revision: 1.6.28.1; previous revision: 1.6 done Checking in layout/printing/nsPrintEngine.cpp; /cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v <-- nsPrintEngine.cpp new revision: 1.97.4.1; previous revision: 1.97 done Checking in layout/style/nsStyleStruct.cpp; /cvsroot/mozilla/layout/style/nsStyleStruct.cpp,v <-- nsStyleStruct.cpp new revision: 3.129.6.1; previous revision: 3.129 done

This seems to be fixed in the latest branch release (1.8 branch, but the name isn't changed yet). I tested by changing max_viewers and clearing the cache, and the behaviour stayed the same. Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050813 Firefox/1.0+