Fennec's browser.js currently calls window.top.setResolution(). This means that the resolution stays the same across document loads which is not ideal. We want the resolution to be reset to 1 on the new document. Right now, we try to set it at the right time, but it's rather finicky and we have some paint suppression code to work around this. It would be nice to know what the right thing to do here is.

We should not need to add a new display item for this, I don't think.

We don't end up adding one in the common case of the top most content document. We add a nsDisplaySubDocument instead of a nsDisplayOwnLayer. For content iframes that have resolution set we do end up with an extra display item, but I don't know if the frontend code sets resolution on content iframes.

(In reply to Timothy Nikkel (:tn) from comment #3) > We don't end up adding one in the common case of the top most content > document. We add a nsDisplaySubDocument instead of a nsDisplayOwnLayer. Hmm, after a bit more thought, this may not be totally insane. We could potentially do something cleaner, but that requires more extensive surgery... > For > content iframes that have resolution set we do end up with an extra display > item, but I don't know if the frontend code sets resolution on content > iframes. It currently does not. We would like to be able to set the resolution on the content window's pres shell.

(In reply to Ehsan Akhgari [:ehsan] from comment #4) > (In reply to Timothy Nikkel (:tn) from comment #3) > Hmm, after a bit more thought, this may not be totally insane. We could > potentially do something cleaner, but that requires more extensive surgery... I'm open to hearing other ways, I whipped this up quickly. > It currently does not. We would like to be able to set the resolution on > the content window's pres shell. Thats the idea of this patch.

(In reply to Timothy Nikkel (:tn) from comment #1) > Created attachment 603024 [details] [diff] [review] > quick hack The quick hack doesn't seem to work.

FWIW, we do get to nsDisplaySubDocument::BuildLayer() but it seems not to work.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7) > FWIW, we do get to nsDisplaySubDocument::BuildLayer() but it seems not to > work. Having a transform of NULL partially breaks drawing. (We get a bunch of black when we zoom in)

This, being mostly cleanup, probably doesn't need to block.

This blocks beta because we currently do not clamp scroll position to work around this issue. Not clamping scroll position is believed to be the cause of ~6 other blockers such as being able to scroll to negative coordinates.

To test a fix to the bug, apply this patch and load a page in Fennec (such as CNN.com). Zoom in as far as you can go - if it renders blurry content then the fix isn't working. If the content is sharp then the fix may be working properly (there might still be other repercussions that need to be handled).

Using the two attached patches here it seems to be working for me, but could someone else please verify?

(In reply to Timothy Nikkel (:tn) from comment #12) > Using the two attached patches here it seems to be working for me, but could > someone else please verify? Please create a try build and when its available mark this bug as qawanted.

I imagine QA can do about the same as me. I would like someone who has seen the problem before to confirm or deny that it is fixed.

This was a GFX filed bug. QA has no insight on this bug.

With this patch, the document does not redraw at the appropriate resolution (i.e the text doesn't get sharp when you zoom in). So something looks to be broken.

For bug posterity: On 12-04-10 09:47 , Kartikaya Gupta wrote: > On 12-04-10 09:43 , Jeff Muizelaar wrote: >> >> It did not work for me. Zooming in on cnn.com just shows the blurry content not the content at the new resolution. >> > Ditto on nytimes.com for me. The outer pres shell remains at 1.0 zoom (I verified that by checking the scale the compositor is getting) and the compositor is scaling the content to the required zoom level. This results in a blurry view of the content.

I can see the problem now. I wasn't testing right.

No longer believed to block, 732016 and others can be fixed independent of this.

Fixing this will let us cleanup the tab code because we can store the resolution on the tab's presshell instead of having to change it when we switch tabs.

So I tried doing this and it seemed to work locally. (Although the first time I ran it I got some really weird results and I'm not sure if I screwed up my build). Anyway, pushed to try [1] and will test more once I get a build from that. [1] https://tbpl.mozilla.org/?tree=Try&rev=2b96eb68ee3e

The try run shows a failure in testVkbOverlap across all android builds. I'll need to investigate.

Running the try build on my device shows that the patch is actually completely busted. I guess my first local build was correct and the second one was wrong.

Turns out we were really close to getting this right. We had all the right pieces but I just didn't put them together correctly. Cleaned up patches incoming.

This is tn's original patch, unbitrotted. :tn, can you verify I unbitrotted it correctly (there were some tricky bits) and then bounce the review to somebody else? Since you wrote the original patch it's probably best that somebody else reviews it.

Chris, the code in ThebesLayerComposite::GetCompositionBounds is still full of type mismatches. I didn't try to fix there (bug 881451 covers it) but I updated it to not use the root transform any more, so it should remain exactly as broken as it was before.

This is the patch from bug 903460, unmodified (so carrying the r=BenWa). I had landed that yesterday but it broke panning on Fennec, because this bug wasn't fixed. Turns out these two bugs go hand in hand, and need to be fixed together. If landed separately they both cause regressions. I will dupe that bug over to here.

Try push of the tree patches at https://tbpl.mozilla.org/?tree=Try&rev=6642b51851c4 I will still need to do some manual testing of all this on B2G and Metro; all of my testing has been on Fennec so far.

Comment on attachment 789296 [details] [diff] [review] Part 2 - Update gfx code to use mResolution instead of rootTransform Review of attachment 789296 [details] [diff] [review]: ----------------------------------------------------------------- LGTM - I'd double-check the fixed-pos async transform stuff still works correctly (I have a load of pages at chrislord.net/files/mozilla/ (fixed*.html) that might help). If it doesn't, it needn't necessarily block this, but we'd need to track it. ::: gfx/layers/composite/ThebesLayerComposite.cpp @@ +252,5 @@ > parentMetrics.mCompositionBounds.width, > parentMetrics.mCompositionBounds.height); > > + const FrameMetrics& metrics = scrollableLayer->GetFrameMetrics(); > + // The below is wrong wrong wrong. See bug 881451. Should this replace the comment a few lines below that says the same thing?

(In reply to Chris Lord [:cwiiis] from comment #32) > LGTM - I'd double-check the fixed-pos async transform stuff still works > correctly (I have a load of pages at chrislord.net/files/mozilla/ > (fixed*.html) that might help). If it doesn't, it needn't necessarily block > this, but we'd need to track it. My patches regress behaviour on these pages, but I don't know why. Halp!

Comment on attachment 789296 [details] [diff] [review] Part 2 - Update gfx code to use mResolution instead of rootTransform Review of attachment 789296 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +465,5 @@ > > // Always abort updates if the resolution has changed. There's no use > // in drawing at the incorrect resolution. > if (!FloatUtils.fuzzyEquals(resolution, viewportMetrics.zoomFactor)) { > + Log.d(LOGTAG, "Aborting draw due to resolution change: " + resolution + " != " + viewportMetrics.zoomFactor); This will cause us to build strings and cause garbage even if we're not logging this :(. This case is common while zooming.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #33) > (In reply to Chris Lord [:cwiiis] from comment #32) > > LGTM - I'd double-check the fixed-pos async transform stuff still works > > correctly (I have a load of pages at chrislord.net/files/mozilla/ > > (fixed*.html) that might help). If it doesn't, it needn't necessarily block > > this, but we'd need to track it. > > My patches regress behaviour on these pages, but I don't know why. Halp! So I debugged this a bit, and it looks like the difference in behaviour occurs in ChooseScaleAndSetTransform [1]. Without these patches, the "aTransform" passed in to that function is null, so the aOutgoingScale is set to the same scaling factor as the aIncomingScale. With these patches, the nsDisplaySubDocument class passes in an "aTransform" that is the inverse of the resolution, and so when ChooseScaleAndSetTransform is called, the aOutgoingScale ends up being 1.0 regardless of the resolution. That means further down the tree, when the fixed-layer margins are determined [1], the aContainerParameters.mXScale and mYScale are 1.0 and the value is different. This results in the fixed-position layers getting translated differently. I don't really know this code well enough to understand which part of this is wrong. :tn, any ideas? I tried taking out the transform as well, but then other things started behaving strangely so I'm not sure that's the right solution either. [1] http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#2654 [2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#3111

I think passing in the inverse transform might be the wrong thing to do. I think we might just want to set the post scale transform on the layer like we do when taking into account resolution on a root document in nsDisplayList::PaintForFrame.

I tried just setting the post scale transform but that didn't have the desired effect either. I'm beginning to think these patches are correct and maybe it's a bug somewhere else in the fixed-position code. I'll try to dig into it a bit more, but if you have some time to look into it or other ideas for me to try they would be most welcome.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37) > I tried just setting the post scale transform but that didn't have the > desired effect either. What effect was desired that didn't happen exactly?

So the patches as attached to the bug seem to work except that fixed-position items attached to the bottom aren't postioned properly, in some hard-to-explain ways. The modification I'm looking for would fix that without affecting any other behaviour. Setting the post-scale instead of the transform, however, breaks panning/zooming in that when you are zoomed out the page will move faster than your finger, the focus point seems to be incorrect, the page content gets detached from the background layer behind it, and other such misbehaviours. I don't know if the fixed-position problem is fixed because the other bad beahviours would mask any attempt to verify that.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #35) > With these patches, > the nsDisplaySubDocument class passes in an "aTransform" that is the inverse > of the resolution, and so when ChooseScaleAndSetTransform is called, the > aOutgoingScale ends up being 1.0 regardless of the resolution. Wouldn't that mean that we are also rendering the layer without taking into account the resolution, thus losing any benefit of setting the resolution at all?

Comment on attachment 789293 [details] [diff] [review] Part 1 - Add a TYPE_SUBDOCUMENT layer for subdocuments with a resolution I think we should give nsDisplaySubDocument a better name like nsDisplayResolution and add a comment about it's purpose. > if (constructZoomItem) { > nsDisplayZoom* zoomItem = > new (aBuilder) nsDisplayZoom(aBuilder, subdocRootFrame, &childItems, > subdocAPD, parentAPD, > nsDisplayOwnLayer::GENERATE_SUBDOC_INVALIDATIONS); > childItems.AppendToTop(zoomItem); >+ } else if (constructResolutionItem) { >+ nsDisplaySubDocument* resolutionItem = >+ new (aBuilder) nsDisplaySubDocument(aBuilder, subdocRootFrame, &childItems, >+ nsDisplayOwnLayer::GENERATE_SUBDOC_INVALIDATIONS); >+ childItems.AppendToTop(resolutionItem); If constructZoomItem and constructResolutionItem are both true (I don't think we hit that situation though) then we need both a zoom item and a resolution item, not just one of them.

(In reply to Timothy Nikkel (:tn) from comment #40) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #35) > > With these patches, > > the nsDisplaySubDocument class passes in an "aTransform" that is the inverse > > of the resolution, and so when ChooseScaleAndSetTransform is called, the > > aOutgoingScale ends up being 1.0 regardless of the resolution. > > Wouldn't that mean that we are also rendering the layer without taking into > account the resolution, thus losing any benefit of setting the resolution at > all? Sort of. If I understand correctly though this is expected - the way async zoom works is that layout sets a resolution (say 2.0) and a corresponding transform (0.5x) on the layer. This renders the layer at twice the density but also causes the compositor to scale it down by half, making it look exactly the same as it would without the resolution/transform. However, the compositor also picks an "async zoom" from the Java code (or AsyncPanZoomController on B2G) that tells it what to actually scale it up by. In this case the async zoom would be 2.0 (or maybe something else like 2.5 if the user has pinched in some more since the gecko draw request) and so the net scale would be 2.0 x 0.5 x 2.0 = 2.0, and the layer gets drawn at the expected size. (In reply to Timothy Nikkel (:tn) from comment #41) > I think we should give nsDisplaySubDocument a better name like > nsDisplayResolution and add a comment about it's purpose. Ok. > If constructZoomItem and constructResolutionItem are both true (I don't > think we hit that situation though) then we need both a zoom item and a > resolution item, not just one of them. Agreed. I wasn't entirely sure if I should bother handling that case but I probably should. Do you know which layer should get "AppendToTop"'d first? The zoom should get applied before the resolution, but I don't know what order that corresponds to in the childItems list.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #42) > Sort of. If I understand correctly though this is expected - the way async > zoom works is that layout sets a resolution (say 2.0) and a corresponding > transform (0.5x) on the layer. This renders the layer at twice the density > but also causes the compositor to scale it down by half, making it look > exactly the same as it would without the resolution/transform. I think that if ChooseScaleAndSetTransform chooses a scale of 1 then the layer will not be rendered at twice the density, it will be rendered at the normal density. Or am I missing something? > Agreed. I wasn't entirely sure if I should bother handling that case but I > probably should. Do you know which layer should get "AppendToTop"'d first? > The zoom should get applied before the resolution, but I don't know what > order that corresponds to in the childItems list. We paint bottom to top, so the zoom should be placed on the top, then the resolution.

Comment on attachment 789293 [details] [diff] [review] Part 1 - Add a TYPE_SUBDOCUMENT layer for subdocuments with a resolution Dropping review flag for now. I haven't had a chance to look into this since my last comment, and won't be able to for a little bit - but I think you might be right in that the fixed-position layers might always get rendered at a 1.0 resolution with this patch. I might be misremembering but I think I saw them get blurrier as I zoomed in. The fixed position frames aren't outside the frame tree corresponding to the sub-presshell, are they?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #44) > Comment on attachment 789293 [details] [diff] [review] > Part 1 - Add a TYPE_SUBDOCUMENT layer for subdocuments with a resolution > > Dropping review flag for now. I haven't had a chance to look into this since > my last comment, and won't be able to for a little bit - but I think you > might be right in that the fixed-position layers might always get rendered > at a 1.0 resolution with this patch. I might be misremembering but I think I > saw them get blurrier as I zoomed in. It would be everything in the sub document that is affected, not just the fixed pos items I would expect. > The fixed position frames aren't outside the frame tree corresponding to the > sub-presshell, are they? No, they are in the frame tree for the sub-presshell.

(In reply to Timothy Nikkel (:tn) from comment #45) > It would be everything in the sub document that is affected, not just the > fixed pos items I would expect. > The main content part of it was definitely fine though; it was just fixed-position items that had the problem.

These patches work on Fennec with no regressions that I can see. I will need to test other platforms too. Try run at https://tbpl.mozilla.org/?tree=Try&rev=1559412bf375

The try run is green but I'm seeing some incorrect behaviour on B2G which I'm trying to debug.

So it turns out that on Fennec these patches end up creating a new layer between the root presShell's layer and the "primary scrollable layer". This new layer corresponds to the content presShell, and it picks up a scaling transform but doesn't have a properly-initialized FrameMetrics instance. The primary scrollable layer is created from a nsDisplayScrollLayer instance somewhere down the tree and is the one that actually ends up with the APZC. I'll need to talk to somebody more about the expected displaylist and layer tree in this case to figure out what to do.

Comment on attachment 819803 [details] [diff] [review] Part 1 - Add a TYPE_SUBDOCUMENT layer for subdocuments with a resolution (v2) Subdocument is a poor name for this item (my fault). It's all about resolution, so maybe nsDisplaySubdocumentResolution or nsDisplayResolution.

Comment on attachment 819806 [details] [diff] [review] Part 3 - Fix order of async/CSS transforms (v2) Removing this third patch fixes the issue I was seeing on B2G, and doesn't change anything for Fennec. Apparently on Fennec we have a container layer for the root presShell (where the scale previously lived), a container layer for the content presShell (where the scale now lives), and a container layer for the content document (which is scrollable and where we apply the async transform). On B2G the content process has just one container layer, which has both the scale and is scrollable. The scale affects the layer->GetTransform() value, so if I pre-multiply the layer transform to the async transform then it comes out differently than if I post-multiply. For the longest time I thought the pre-multiply was correct but now I realize that the layer transform is not supposed to affect the layer itself (in this case at least) so the post-multiply is actually correct. With the post-multiply the layer transform cancels out with the post-scale and so the net effect is only the async transform gets applied, which is correct.

(In reply to Timothy Nikkel (:tn) from comment #52) > Comment on attachment 819803 [details] [diff] [review] > Part 1 - Add a TYPE_SUBDOCUMENT layer for subdocuments with a resolution (v2) > > Subdocument is a poor name for this item (my fault). It's all about > resolution, so maybe nsDisplaySubdocumentResolution or nsDisplayResolution. I'm updating the patches to address this, as well as the comments in comment 32 and comment 41. Will re-test and upload new patches in a bit.

Updated to rename the layer nsDisplayResolution, and to allow creating both nsDisplayResolution and nsDisplayZoom items if the subdocument has both.

Mostly the same as the patch previously r+'d by Cwiiis and BenWa, but it's been rebased and a thing or two might have changed so I'd like to get a re-review just to be safe.

Try run with the latest patches: https://tbpl.mozilla.org/?tree=Try&rev=a4f312789d05

Comment on attachment 820601 [details] [diff] [review] Part 2 - Update gfx code to use mResolution instead of rootTransform (v3) Review of attachment 820601 [details] [diff] [review]: ----------------------------------------------------------------- I'm ok with the changes but I can't really review for correctness. Feel free to land if you're ok with that.

I'm fine with that, I think my testing was reasonably thorough and I don't think we'll find bugs here by code inspection anyway. I'm ready to land this, but I'll wait until after the merge on Monday and put it into the 28 train. That will also give me time to check what the effects are on Metro and hopefully fix that up.

28 is on Nightly, so it's go time! https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd222e1ae40 https://hg.mozilla.org/integration/mozilla-inbound/rev/546d07bdc09c

Your commit message says "new layer type" but it's actually a new display item type, which is much less scary :-). Don't bother fixing it though.

https://hg.mozilla.org/mozilla-central/rev/8bd222e1ae40 https://hg.mozilla.org/mozilla-central/rev/546d07bdc09c