732971 - setResolution only seems to work on top most presShell

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: quick hack

Comment 2

[(no longer active)]

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

Comment 3

[Timothy Nikkel (:tnikkel)]

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.

Comment 4

[(no longer active)]

(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.

Comment 5

[Timothy Nikkel (:tnikkel)]

(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.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

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

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(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)

Comment 9

[Joe Drew (not getting mail)]

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

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch to test any attempted fixes

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).

Comment 12

[Timothy Nikkel (:tnikkel)]

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

Comment 13

[JP Rosevear [:jpr]]

(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.

Comment 14

[Timothy Nikkel (:tnikkel)]

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.

Comment 15

[Kevin Brosnan [Ex-Mozilla]]

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

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Attachment: Combined+rebased patch

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.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch to test any attempted fixes (rebased)

Comment 18

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 19

[Timothy Nikkel (:tnikkel)]

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

Comment 21

[JP Rosevear [:jpr]]

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

Comment 22

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

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

Comment 24

[Kartikaya Gupta (email:[e-mail])]

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

Comment 25

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add a TYPE_SUBDOCUMENT layer for subdocuments with a resolution

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.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Update gfx code to use mResolution instead of rootTransform

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.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Fix order of async/CSS transforms

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.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

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 32

[Chris Lord [:cwiiis]]

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?

Comment 33

[Kartikaya Gupta (email:[e-mail])]

(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 34

[Benoit Girard (:BenWa)]

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.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 36

[Timothy Nikkel (:tnikkel)]

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.

Comment 37

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 38

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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?

Comment 39

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 40

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 41

[Timothy Nikkel (:tnikkel)]

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.

Comment 42

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tn) from comment #40)
> (In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 43

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 44

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 45

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 46

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 47

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add a TYPE_SUBDOCUMENT layer for subdocuments with a resolution (v2)

Comment 48

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Update gfx code to use mResolution instead of rootTransform (v2)

Comment 49

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Fix order of async/CSS transforms (v2)

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

Comment 50

[Kartikaya Gupta (email:[e-mail])]

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

Comment 51

[Kartikaya Gupta (email:[e-mail])]

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 52

[Timothy Nikkel (:tnikkel)]

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 53

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 54

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 55

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add a TYPE_RESOLUTION layer for subdocuments with a resolution (v3)

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

Comment 56

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Update gfx code to use mResolution instead of rootTransform (v3)

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.

Comment 57

[Kartikaya Gupta (email:[e-mail])]

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

Comment 58

[Benoit Girard (:BenWa)]

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.

Comment 59

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 60

[Kartikaya Gupta (email:[e-mail])]

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

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 62

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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