Closed Bug 1228602 Opened 4 years ago Closed 4 years ago

about:firefox is truncated with APZ enabled (proper fix)

Categories

(Core :: Layout, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(6 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1225508 +++

If APZ is enabled on Fennec without bug 1225508 then about:firefox is truncated. Note that this only happens on devices with a large enough screen that about:firefox is NOT scrollable. It should be easy enough to create a test page that replicates this; I think all that's needed is that the page be not scrollable, but have a resolution < 1.0 (via the meta-viewport tag). This results in the visible region on the layer being smaller than what's actually visible, because it gets scaled by the resolution at some point. Setting a displayport (which is what we do in bug 1225508) seems to fix the problem.
So it seems that the presence of frame metrics on layer for the root content document is necessary to fix this. Is that expected? Do we maybe need some fix on the apz/compositor side?
Flags: needinfo?(bugmail.mozilla)
I guess it depends on what data are in the metrics. The metrics by itself should not have this sort of effect but maybe there is a clip rect in the metrics that is making a difference? Do you have layer tree dumps?
Flags: needinfo?(bugmail.mozilla)
Don't have a layer dump handy at the moment, but the clip rect on the metrics in question would be none because it comes via nsDisplaySubDocument::ComputeFrameMetrics which passes Nothing() for the cliprect. Would that mean we ignore the clip set on the Layer?
Attached file Layer dumps
I got layer dumps from three different scenarios: (1) is current m-c with about:firefox rendering in its entirety. This shows the RCD layer has a metrics and displayport. (2) is current m-c with the fix backed out. This layer dump was taking upon loading about:firefox in a fresh tab, so the clipping was visible. (3) was taking after loading about:firefox, then going to about:config in the same tab, and then hitting back. After going back the about:firefox page was fully rendered.

The layer dumps from (1) and (3) look the same, but (2) is different. There are two differences in particular: one is that the root content's container layer has a metrics in (1) and (3) and doesn't in (2). The other is that the valid region for the last PaintedLayerComposite in (2) has dimensions of 512x692 whereas in (1) and (3) the same layer has a valid region of 768x1038.

Note in particular that in all cases the visible region and clip on the root content's container layer is the same, so I don't think that's a factor here. It's must be either the displayport or the valid region on that PaintedLayer that's the problem. I don't think this is a compositor-side problem, although I might be wrong still.
(I stripped out the addresses in the above two dumps to be able to diff them easier. There's a lot of (0,0,34558,46708) clips in the truncated case that aren't in the non-truncated one.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> clipping was visible. (3) was taking after loading about:firefox, then going
> to about:config in the same tab, and then hitting back. After going back the
> about:firefox page was fully rendered.

That's interesting. I wonder why that fixes the bug.
Sorry, I wasn't entirely clear with my explanation. Starting with m-c before the fix for this bug (the 'less proper' one) I wrote patches that I think should fix any clipping or visible region problem. These patches did not fix the problem. If I then change nsDisplaySubDocument::ComputeFrameMetrics so that we return the non-null framemetrics when we have a non-1.0 resolution (in addition to if we have the generate scrollable flag) the bug is then fixed.
Hm. Can you post the patches? I can take a look to see if there are any compositor issues with them applied.
Attached patch wip / debugging patch (obsolete) — Splinter Review
Comment on attachment 8696674 [details] [diff] [review]
wip / debugging patch

I blame sleep deprivation.
Attachment #8696674 - Attachment is obsolete: true
So even with the patch applied we get a valid region of 512x692 on that PaintedLayer. It occurs to me now that this is a tiled layer, and that it might be a bug in the tiling code. I'll check that.
Note that in the patch I uploaded you still have to uncomment the code in nsDisplySubDocument::ComputeFrameMetrics to "fix" this bug.
Attached patch Tiling fix (obsolete) — Splinter Review
So yeah, there was a tiling bug here that was causing the presShell resolution to not get picked up because it was on the layer rather than the metrics. This explains why as soon as a metrics was added it worked. This patch fixes the issue in the tiling code, and fixes the about:firefox clipping problem even when applied without your patch.

As we discussed, it's probably still good to clean up your patch and get it landed since it fixes other issues in nsDisplayResolution.
Attachment #8696849 - Flags: review?(botond)
Comment on attachment 8696849 [details] [diff] [review]
Tiling fix

Review of attachment 8696849 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/LayerMetricsWrapper.h
@@ +367,5 @@
>  
>      return sNoClipRect;
>    }
>  
> +  const float GetPresShellResolution() const

Toplevel const on a return type is unnecessary.

::: gfx/layers/client/ClientTiledPaintedLayer.cpp
@@ +80,5 @@
>        // compositor in turn cancels out this post-scale (i.e., scales by the
>        // pres shell resolution), and to get correct calculations, we need to do
> +      // so here, too. Note that the presshell could be stored on either the
> +      // metrics or the layer itself (but at most one of these places), and we
> +      // want to apply it regardless of where it is. Hence the code below.

What you describe here is not my understanding.

My understanding is that the layer is an additional place where we store the pres shell resolution, so it's stored both on the metrics (if there is one) and on the layer.

(Observe that both places that set the resolution on the layer [1] [2] do so unconditionally.)

[1] https://dxr.mozilla.org/mozilla-central/rev/2bdd9ec79799eff3ceec0a318f5a0632d918a527/layout/base/nsDisplayList.cpp
[2] https://dxr.mozilla.org/mozilla-central/rev/2bdd9ec79799eff3ceec0a318f5a0632d918a527/layout/base/nsDisplayList.cpp
Attached patch Tiling fix v2Splinter Review
Ah, sorry, I misinterpreted what you said then. This patch should be better - it will pick up the resolution from the layer, and if that's 1.0 (i.e. doesn't exist, or is actually 1.0) then we will pick it up from the metrics. Presumably if it is actually 1.0 it will be 1.0 on the metrics as well so things should work out.
Attachment #8696849 - Attachment is obsolete: true
Attachment #8696849 - Flags: review?(botond)
Attachment #8696962 - Flags: review?(botond)
Comment on attachment 8696962 [details] [diff] [review]
Tiling fix v2

Review of attachment 8696962 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/ClientTiledPaintedLayer.cpp
@@ +84,5 @@
>        // With containerless scrolling, the offending post-scale is on the
>        // parent layer of the displayport-ancestor, which we don't reach in this
>        // loop, so we don't need to worry about it.
> +      float presShellResolution = iter.GetPresShellResolution();
> +      if (presShellResolution == 1.0f) {

Would you ever expect the resolution on the layer to be 1 but the resolution on the metrics something other than 1?
Based on my interpretation of what you said and my reading of the code, I wouldn't expect that, particularly since the LayerMetrics::GetPresShellResolution only returns the resolution if you're at the top LayerMetrics. But I don't remember (or never knew) about the details here so I could be wrong - feel free to correct me!
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> Based on my interpretation of what you said and my reading of the code, I
> wouldn't expect that, particularly since the
> LayerMetrics::GetPresShellResolution only returns the resolution if you're
> at the top LayerMetrics. But I don't remember (or never knew) about the
> details here so I could be wrong - feel free to correct me!

I agree. Where I was going is, doesn't that make the entire if block unnecessary?
Ah, I was thinking that it was still possible to have a scenario where you have a resolution on a metrics, but it's one of a bunch of metrics on a PaintedLayer or something, and so there's no container layer in the tree with the resolution.

However I guess this is inside a LayoutUseContainersForRootFrames block so maybe that won't ever happen?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23)
> Ah, I was thinking that it was still possible to have a scenario where you
> have a resolution on a metrics, but it's one of a bunch of metrics on a
> PaintedLayer or something, and so there's no container layer in the tree
> with the resolution.

Note that even if the metrics with the resolution is on a PaintedLayer, the resolution will be stored on its parent ContainerLayer. Basically, it needs to be stored on a layer for the compositor to apply it. 

That said ...

> However I guess this is inside a LayoutUseContainersForRootFrames block so
> maybe that won't ever happen?

... in this case the resolution will be on the same layer as the metrics with the resolution.
Comment on attachment 8696962 [details] [diff] [review]
Tiling fix v2

Review of attachment 8696962 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the if block removed as per the last couple of comments
Attachment #8696962 - Flags: review?(botond) → review+
I'll back out the patch from bug 1225508 once the above landing makes it to m-c.
Keywords: leave-open
Actually I should wait until tn lands his nsDisplayResolution fixes before I do the backout, because otherwise not having a displayport might break other things (the visibility computation etc.). At least in theory.
I spun off the outstanding issue here into bug 1243514.
Keywords: leave-open
Target Milestone: --- → mozilla45
Assignee: nobody → bugmail.mozilla
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.