I was going to write a testcase with a mask layer that's fixed to the outer page and shouldn't be scrolled along with the subframe contents... However, it seems like the clip is completely ignored, so this bug might not become testable until bug 1148581 is fixed.

Here's a testcase that shows that our current model of one clip rect + one mask layer per layer may just not be enough to express what we want to express. The transformed layer in this case is clipped both by something that moves in sync with it (#rounded-clip), and by something that doesn't (#scrollbox). The layer's clip rect is an intersection of these two clips. Same for the mask layer: The mask image is an intersection of two masks. Without APZ, we end up updating the mask on each scroll. We need to think of a plan for this, instead of just hacking around it again.

Can we attach a clip rect and a mask layer to each FrameMetrics? These would be the clip of the scrollable element up to (but not including) the parent scrollable thing. And the clip + mask layer of a layer also wouldn't include any clips from scrollable parents. Then the compositor would apply the intersection of all the clips when composting.

*compositing.

That sounds like a reasonable plan to me. (In reply to Markus Stange [:mstange] from comment #2) > Can we attach a clip rect and a mask layer to each FrameMetrics? These would > be the clip of the scrollable element up to (but not including) the parent > scrollable thing. So this would include clips that come from places other than actively scrollable scroll frames? If so then sounds good.

(In reply to Timothy Nikkel (:tn) from comment #4) > So this would include clips that come from places other than actively > scrollable scroll frames? Yes.

Some reftests based on mstange's test cases. Going to attempt fixing this based on discussions with mstange.

This patch just moves the composition bounds clip, so that instead of being intersected with the layer's clip in Layout, it is intersected in AsyncCompositionManager instead. It doesn't attempt to fix any bugs, it's just getting things set up while (I think) preserving the current behavior.

Comment on attachment 8599130 [details] [diff] [review] part 1, move clip rect Review of attachment 8599130 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +8227,5 @@ > > + ParentLayerRect clip = LayoutDeviceRect::FromAppUnits(aClipRect, auPerDevPixel) > + * metrics.GetCumulativeResolution() > + * layerToParentLayerScale; > + metrics.SetClipRect(RoundedOut(clip)); This should be Rounded(), not RoundedOut(), I think. The old code did ViewAs<ParentLayerPixel>(ScaleToNearestPixels(clipRect)); ::: layout/generic/nsGfxScrollFrame.cpp @@ +3055,1 @@ > if (!omitClip && (!gfxPrefs::LayoutUseContainersForRootFrames() || mAddClipRectToLayer)) { Wow, this if condition is a complete mystery to me. Hopefully it can be cleaned up soon.

(In reply to Markus Stange [:mstange] from comment #8) > ::: layout/generic/nsGfxScrollFrame.cpp > @@ +3055,1 @@ > > if (!omitClip && (!gfxPrefs::LayoutUseContainersForRootFrames() || mAddClipRectToLayer)) { > > Wow, this if condition is a complete mystery to me. Hopefully it can be > cleaned up soon. Sorry, I piled onto it in bug 1130982 by adding the 'omitClip' bit. The idea behind 'omitClip' is that on APZ platforms, if a layer has multiple scrollable metrics, the async transforms for these metrics need to be applied "in between" the clips to produce an accurate final clip for the layer. Intersecting the clips in Layout, where the async transforms aren't available, means the information needed to perform this calculation is lost, so we don't intersect the clips for metrics other than the bottom-most in Layout, we do that in AsyncCompositionManager instead. I can't really comment on the other parts of the condition.

The previous patch breaks containerful scrolling, so reworking things to keep it as-is. First, change ComputeFrameMetrics to return a Maybe<nsRect> which is a little cleaner.

This patch makes the behavior of ScrollFrameHelper::ComputeFrameMetrics a little clearer. The big |if| condition is split out into containerful/containerless, intentionally duplicating some of the other checks to make it easier to follow (and later, delete). This patch has one behavioral change. Previously, CFM always included the scrollport clip when containerless scrolling. Now, it only includes it when we want a FrameMetrics, since (1) otherwise it's already on display items and (2) the next patch moves it to the FrameMetrics anyway.

Same as the previous patch, but with a lot more Maybe<> and some hacks for containerful scrolling.

Comment on attachment 8600174 [details] [diff] [review] part 3, move clip to FrameMetrics Review of attachment 8600174 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/FrameMetrics.h @@ +708,5 @@ > // Whether or not the frame can be vertically scrolled with a mouse wheel. > bool mAllowVerticalScrollWithWheel; > > + // The clip rect to use when compositing a layer with this FrameMetrics. > + ParentLayerIntRect mClipRect; Could you make this a Maybe<>, too? Since you didn't, I assume there is some reason not to, and I'm curious what it is.

(In reply to Markus Stange [:mstange] from comment #13) > Comment on attachment 8600174 [details] [diff] [review] > part 3, move clip to FrameMetrics > > Review of attachment 8600174 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/FrameMetrics.h > @@ +708,5 @@ > > // Whether or not the frame can be vertically scrolled with a mouse wheel. > > bool mAllowVerticalScrollWithWheel; > > > > + // The clip rect to use when compositing a layer with this FrameMetrics. > > + ParentLayerIntRect mClipRect; > > Could you make this a Maybe<>, too? Since you didn't, I assume there is some > reason not to, and I'm curious what it is. It makes it harder to do the IPC gunk. I could move that logic into the IPC marshaller though.

Comment on attachment 8600115 [details] [diff] [review] part 2, refactor ComputeFrameMetrics This is how I _think_ the logic should go if (!mShouldBuildScrollableLayer || mIsScrollableLayerInRootContainer) return; // we don't need frame metrics, or clip bool needsParentLayerClip = true; if (gfxPrefs::LayoutUseContainersForRootFrames() && !mAddClipRectToLayer) needsParentLayerClip = false; if (aOutput->Length() > 0) needsParentLayerClip = false; if (needsParentLayerClip) // add clip // add metrics unconditionally Does that jive with your understanding?

(In reply to David Anderson [:dvander] from comment #14) > (In reply to Markus Stange [:mstange] from comment #13) > > Comment on attachment 8600174 [details] [diff] [review] > > part 3, move clip to FrameMetrics > > > > Review of attachment 8600174 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: gfx/layers/FrameMetrics.h > > @@ +708,5 @@ > > > // Whether or not the frame can be vertically scrolled with a mouse wheel. > > > bool mAllowVerticalScrollWithWheel; > > > > > > + // The clip rect to use when compositing a layer with this FrameMetrics. > > > + ParentLayerIntRect mClipRect; > > > > Could you make this a Maybe<>, too? Since you didn't, I assume there is some > > reason not to, and I'm curious what it is. > > It makes it harder to do the IPC gunk. I could move that logic into the IPC > marshaller though. Yeah, I think it would be useful to have a template<typename T> struct ParamTraits<mozilla::Maybe<T>> { ... } in IPCMessageUtils.h.

(In reply to Timothy Nikkel (:tn) from comment #15) > Comment on attachment 8600115 [details] [diff] [review] > part 2, refactor ComputeFrameMetrics > > This is how I _think_ the logic should go > > if (!mShouldBuildScrollableLayer || mIsScrollableLayerInRootContainer) > return; // we don't need frame metrics, or clip > > bool needsParentLayerClip = true; > if (gfxPrefs::LayoutUseContainersForRootFrames() && !mAddClipRectToLayer) > needsParentLayerClip = false; > if (aOutput->Length() > 0) > needsParentLayerClip = false; > > if (needsParentLayerClip) > // add clip > > // add metrics unconditionally > > > Does that jive with your understanding? I wasn't sure if that would mess up containerful-scrolling when no FrameMetrics are added. But if that's supposed to work - sounds good, I'll test it.

Comment on attachment 8600244 [details] [diff] [review] part 3, move clip to FrameMetrics >@@ -4244,31 +4244,18 @@ ContainerState::SetupScrollingMetadata(NewLayerEntry* aEntry) > return; > } > > nsIScrollableFrame* scrollFrame = nsLayoutUtils::GetScrollableFrameFor(f); > if (!scrollFrame) { > continue; > } > >- Maybe<nsRect> clipRect; > scrollFrame->ComputeFrameMetrics(aEntry->mLayer, mContainerReferenceFrame, >- mParameters, &clipRect, &metricsArray); >- if (clipRect) { >- ParentLayerIntRect pixClip = ViewAs<ParentLayerPixel>(ScaleToNearestPixels(*clipRect)); >- if (layerClip) { >- tmpClipRect.IntersectRect(pixClip, *layerClip); >- } else { >- tmpClipRect = pixClip; >- } >- layerClip = &tmpClipRect; >- // XXX this could cause IPC churn due to cliprects being updated >- // twice during layer building --- for non-PaintedLayers that have >- // both CSS and scroll clipping. >- } >+ mParameters, &metricsArray); > } > aEntry->mLayer->SetClipRect(ToMaybe(layerClip)); > // Watch out for FrameMetrics copies in profiles > aEntry->mLayer->SetFrameMetrics(metricsArray); > } So these means we get the clip rect from the layer and then set it. We should be able to remove all the clip rect stuff from this function then. I want to think through the clip rect units calculations, but otherwise fine with this patch.

Comment on attachment 8602996 [details] [diff] [review] part 4, fix the scrollport clip I think you want to add the clip we calculate in ComputeFrameMetrics to the saved clip from BuildDisplayList, otherwise there isn't anything doing the clipping of the scroll frame itself.

You could also move the non-APZ from the else branch of that if out of the else and just before the if, so that it happens with and without APZ. Then the APZ branch in the if would pick up that clip.

I kept the original clip in ComputeFrameMetrics as a callback since (I think) the displayport one won't be set with containerful root scrolling.

Comment on attachment 8603163 [details] [diff] [review] part 4, fix the scrollport clip Clipping to mScrollPort isn't correct for root scroll frames (that have resolution and scroll position clamping scroll port rects). What your previous patch did was good, it just needed to combine the existing clip computed in :ComputeFrameMetrics with the saved mScrollPortClip. Nit, change the name of mScrollPortClip. Maybe mSavedClip? mAncestorClip?

Comment on attachment 8604119 [details] [diff] [review] part 4, fix the async scrollport clip Almost. We don't want to add the mScrollPort clip in BuildDisplayList. mScrollPort isn't the right clip to use for root scroll frames (the clip we add in ComputeFrameMetrics is instead of the mScrollPort clip in BuildDisplayList). Instead just save the current clip in BuildDisplayList (before clearing it). And then use the saved clip in ComputeFrameMetrics to intersect with the clip that is computed in ComputeFrameMetrics.

Oh, so my suggestion was no good then. Okay.

Update PostprocessRetainedLayers to use the combined clip of a layer (its base clip + all async clips).

Comment on attachment 8604262 [details] [diff] [review] part 5, update PostprocessRetainedLayers Review of attachment 8604262 [details] [diff] [review]: ----------------------------------------------------------------- TBH I think Timothy is the better reviewer for all these patches. He keeps finding bugs that I miss.

Revert the premature mScrollPort clip.

Comment on attachment 8604278 [details] [diff] [review] part 4, fix the async scrollport clip Thanks

So do parts 1 through 5 already fix bug 1148581?

(In reply to Markus Stange [:mstange] from comment #33) > So do parts 1 through 5 already fix bug 1148581? Yup.

This patch removes the ComputeFrameMetrics() restriction, that only the first FrameMetrics can have an ancestor scrollframe clip. AsyncCompositionManager has been updated to include that clip.

Slightly better patch.

(In reply to David Anderson [:dvander] from comment #10) > Created attachment 8600108 [details] [diff] [review] > part 1, use Maybe for optional clips in CFM > > The previous patch breaks containerful scrolling, so reworking things to > keep it as-is. First, change ComputeFrameMetrics to return a Maybe<nsRect> > which is a little cleaner. The attached patch doesn't change ComputeFrameMetrics to return a Maybe<nsRect>...

Comment on attachment 8600244 [details] [diff] [review] part 3, move clip to FrameMetrics Review of attachment 8600244 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by. ::: gfx/layers/FrameMetrics.h @@ +533,5 @@ > + void SetClipRect(const Maybe<ParentLayerIntRect>& aClipRect) > + { > + mClipRect = aClipRect; > + } > + const Maybe<ParentLayerIntRect> GetClipRect() const Please return this by reference-to-const. @@ +540,5 @@ > + } > + bool HasClipRect() const { > + return mClipRect.isSome(); > + } > + const ParentLayerIntRect &ClipRect() const { nit: the style in these lands is to put the & before the space ::: ipc/glue/IPCMessageUtils.h @@ +777,5 @@ > + T tmp; > + if (!ReadParam(msg, iter, &tmp)) { > + return false; > + } > + *result = Some(Move(tmp)); Don't we also need to qualify Some() with mozilla:: ?

Comment on attachment 8605273 [details] [diff] [review] part 6, fix nested scrollframe clips Review of attachment 8605273 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/AsyncCompositionManager.cpp @@ +585,5 @@ > + // transforms, and its scrollframe clips, which are the clips between each > + // scrollframe and its ancestor scrollframe. Scrollframe clips include the > + // composition bounds and any other clips induced by layout. > + // > + // The final clip for the layer is the intersection of these clips. Much nicer than what we had before! Thanks for doing this.

Factor mask layer creation out of SetupMaskLayer.

If the ancestor scrollframe clip requires a mask layer, this creates that mask layer and then attaches it to the Layer and FrameMetrics. TODO still: get it to composite, make sure they're cached/retained properly.

Split SetupMaskLayer so it can be used for either normal or async mask layers. This adds a flag to CreateOrRecycleMaskImageLayer so only the base mask layer gets cached (the cache changes are in another patch).

This adds an extra array to layers, storing any extra mask layers that may need to be applied during async composition. FrameMetrics now has an index into this array. This patch doesn't actually get the extra mask layers compositing yet, that's coming next. I don't like that this is pretty open-coded, but I think keeping Layer* fields on Layer and not directly on the FrameMetrics is ultimately cleaner. There's probably a better way of expressing this though.

Comment on attachment 8606801 [details] [diff] [review] part 8, attach mask layers to FrameMetrics Review of attachment 8606801 [details] [diff] [review]: ----------------------------------------------------------------- Since we have to keep the mask layer array on the Layer, I think it would be nice to move the clips from FrameMetrics into an array on the Layer as well to keep them consistent. This has the added benefit that we can compute the final combined clip to pass to the compositor using the Layer only without having to use FrameMetrics. When we async scroll a given FrameMetrics (and update the transform on the Layer), do we also immediately move the Masks transform? Do we shift the clips (of the layer and any 'child' FrameMetrics) immediately too? Is it worth differentiating 'ancestor' mask layers from the base one, or could we just combine these into a single array? I guess they're set in different place, so determining when to 'Mutated' gets a bit harder. Code all looks really good :)

(In reply to Matt Woodrow (:mattwoodrow) from comment #45) > > Since we have to keep the mask layer array on the Layer, I think it would be > nice to move the clips from FrameMetrics into an array on the Layer as well > to keep them consistent. This has the added benefit that we can compute the > final combined clip to pass to the compositor using the Layer only without > having to use FrameMetrics. I was reluctant to do that for two reasons: (1) it gets really hacky to sync indices with FrameMetrics, and (2) the clips are already all combined quite cleanly in AsyncCompositionManager. The mask layers can't do that though. If we wanted to have them both be the same mechanism, I'd rather move the mask layer pointers fully into the FM. I don't feel very strongly about it though. > When we async scroll a given FrameMetrics (and update the transform on the > Layer), do we also immediately move the Masks transform? Do we shift the > clips (of the layer and any 'child' FrameMetrics) immediately too? Not yet - I will do that in one of the next patches. The clips currently do get shifted in patch 6. > Is it worth differentiating 'ancestor' mask layers from the base one, or > could we just combine these into a single array? I guess they're set in > different place, so determining when to 'Mutated' gets a bit harder. I thought about that, but yeah for that exact reason I decided against it. It's hard to differentiate the owned mask from the ancestor masks. Clips have the same problem. If it's confusing as-is, maybe there's a better name, I didn't think about the names enough.

Same as the previous patch, but with a few more "GetMaskLayer" tests updated and LayerTreeInvalidation updated. I *think* this is all the machinery sans the actual compositing/transform changes that will be coming next. r?ing again but if you feel strongly about using the same mechanism for masks+clip rects I'll go back to it.

Comment on attachment 8606802 [details] [diff] [review] part 9, recycle ancestor mask layers Review of attachment 8606802 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +1244,5 @@ > + > + PLDHashNumber Hash() const { > + // Hash the layer and add the layer index to the hash. > + return (NS_PTR_TO_UINT32(mLayer) >> 2) > + + (mAncestorIndex ? (*mAncestorIndex + 1) : 0); Use AddUintptrToHash/AddToHash?

Comment on attachment 8607316 [details] [diff] [review] part 8, attach mask layers to FrameMetrics Review of attachment 8607316 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerTreeInvalidation.cpp @@ +183,5 @@ > } > > + for (size_t i = 0; > + i < std::min(mAncestorMaskLayers.Length(), mLayer->GetAncestorMaskLayerCount()); > + i++) Just skip this loop entirely if ancestorMaskChanged == true, since we've already invalidated the entire old/new region in that case.

(In reply to David Anderson [:dvander] from comment #46) > > Since we have to keep the mask layer array on the Layer, I think it would be > > nice to move the clips from FrameMetrics into an array on the Layer as well > > to keep them consistent. This has the added benefit that we can compute the > > final combined clip to pass to the compositor using the Layer only without > > having to use FrameMetrics. If we ever make FrameMetrics a first-class IPDL object and share them between multiple layers, then we'll need to move anything Layer specific out. This has been discussed before as an option if the cost of creating/copying FrameMetrics objects ever shows up in profiles, but we can worry about that when/if it happens.

This attempts to apply FrameMetrics mask layers. It almost works but there's some kind of weird 1-pixel misalignment that goes away with a content repaint. It also doesn't support multiple mask layers but I'll do that in another patch.

The original version of this patch broke displayports that don't have APZs, since the compositor would no longer have a clip. This version adds the clip back if APZ is not enabled.

part 6 fails on b2g due to containerful scrolling. Previously we did not move the layer clip with the first async transform. The patch in part 6 changed this. The layer's clip is always transformed by the first APZC. With containerful scrolling, the clip (as it would apply to its children) is effectively pre-transform, so transforming the clip would transform it twice. As a quick hack around this (since we're close to dropping containerful scrolling on B2G), I think we can just keep the old behavior for B2G.

Comment on attachment 8609916 [details] [diff] [review] part 6 addendum, fix for containerful scrolling I think I have figured out what this patch is trying to do. You want to not move the clip on the root scrollable container layer that exists with containerful scrolling because it doesn't move when that scroll frame is scrolled. So it looks like you are trying to write a condition that will only match the root scrollable container layer that exists with containerful scrolling. The condition you have will match the first (inner most) metrics of any container layer when containerful root is enabled. Which doesn't seem to be what you want. I think you want gfxPrefs::LayoutUseContainersForRootFrames() && aLayer->AsContainerLayer() && aLayer->GetFrameMetricsCount() == 1 && metrics.IsRootScrollable() Although you probably don't need to check that it is a container layer.

(In reply to Timothy Nikkel (:tn) from comment #54) > Comment on attachment 8609916 [details] [diff] [review] > part 6 addendum, fix for containerful scrolling > > I think I have figured out what this patch is trying to do. You want to not > move the clip on the root scrollable container layer that exists with > containerful scrolling because it doesn't move when that scroll frame is > scrolled. > > So it looks like you are trying to write a condition that will only match > the root scrollable container layer that exists with containerful scrolling. > > The condition you have will match the first (inner most) metrics of any > container layer when containerful root is enabled. Which doesn't seem to be > what you want. > > I think you want > gfxPrefs::LayoutUseContainersForRootFrames() && > aLayer->AsContainerLayer() && > aLayer->GetFrameMetricsCount() == 1 && > metrics.IsRootScrollable() > > Although you probably don't need to check that it is a container layer. This condition doesn't work, I think it has to match the first metrics of any container layer used for containerful scrolling. See the existing code here: https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#628 This seems to assume containerful scrolling and only transforms the clip if i > 0 (since the clip is already effectively transformed).

(In reply to David Anderson [:dvander] from comment #55) > This condition doesn't work, I think it has to match the first metrics of > any container layer used for containerful scrolling. This will match the first metrics of any container layer used for containerful scrolling, but it will also match the first metrics of any container layer when we have containerful scrolling enabled for root scroll frames (but disabled for all others). So for example it will match whatever the first metrics (they may correspond to a scrollable div that is using containerless scrolling) happen to be for a container layer used for a transform anywhere in the document. So I'm guessing the problem occurs with content iframes? The subdocument clip gets put on the scrollable container layer. So the IsRootScrollable part won't work then. I don't think we have a way to tell if a container layer is for a containerful scrollframe (at least I can't think of a way to do it). So we may need to add a field to FrameMetrics to indicate such. > See the existing code > here: > > https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ > AsyncCompositionManager.cpp#628 > > This seems to assume containerful scrolling and only transforms the clip if > i > 0 (since the clip is already effectively transformed). My understanding of that code is that it is not assuming containerful scrolling (which is good because it is run on both types of layer trees). The clip of a layer is applied after the layer contents are transform. The clip of the layer is the composition bounds. This is the "hole" through which we see the scrolled content for this scroll frame. When this scroll frame is scrolled the layer contents moves, but the hole that we see the scrolled content through does not move. However when an ancestor scroll frame scrolls then the hole that we see the scrolled content (for what is now a descendant scrollframe) does move. (In reply to David Anderson [:dvander] from comment #53) > part 6 fails on b2g due to containerful scrolling. Previously we did not > move the layer clip with the first async transform. The patch in part 6 > changed this. The layer's clip is always transformed by the first APZC. The layer clip used to be the composition bounds, so didn't move with the first metrics. When we changed the layer clip to no longer include any clipping that is induced by scroll frames that made this invalid, we now had to move the clip because the clip is not related to any scrollframes. So the distinction is if the clip is scroll related or not. Not containerful vs containerless. Because if the clip is the composition bounds in a containerless scroll frame then it still shouldn't be moved because it is a scroll related clip. If what I say above about the clip coming from the subdocument clip above is correct then this clip is effectively a scroll related clip.

https://hg.mozilla.org/integration/mozilla-inbound/rev/caa1f9974731 https://hg.mozilla.org/integration/mozilla-inbound/rev/498b1d8f9d71 https://hg.mozilla.org/integration/mozilla-inbound/rev/21393b5c6963 https://hg.mozilla.org/integration/mozilla-inbound/rev/507b6aba4555

Backed out the followup in https://hg.mozilla.org/integration/mozilla-inbound/rev/70a376c0f23d for android reftest orange: https://treeherder.mozilla.org/logviewer.html#?job_id=10167401&repo=mozilla-inbound http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android-api-11/1432674856/mozilla-inbound_ubuntu64_vm_armv7_large_test-plain-reftest-6-bm117-tests1-linux64-build14.txt.gz&only_show_unexpected=1

Thanks for the explanation. Propagating the "is containerful scrolling" bit through FrameMetrics appears to work.

https://hg.mozilla.org/mozilla-central/rev/caa1f9974731 https://hg.mozilla.org/mozilla-central/rev/498b1d8f9d71 https://hg.mozilla.org/mozilla-central/rev/21393b5c6963 https://hg.mozilla.org/mozilla-central/rev/507b6aba4555

Comment on attachment 8611546 [details] [diff] [review] part 6 addendum, fix for containerful scrolling v2 Can you add an assert that the containerfull pref is enabled in FrameMetrics::SetUsesContainerScrolling when it's setting the value to true? That way we can find all the containerful code easier later and remove it.

Comment on attachment 8609806 [details] [diff] [review] part 3, move clip to FrameMetrics v2 >@@ -178,16 +178,19 @@ AppendToString(std::stringstream& aStream, const FrameMetrics& m, > AppendToString(aStream, m.GetDisplayPort(), "] [dp="); > AppendToString(aStream, m.GetCriticalDisplayPort(), "] [cdp="); > AppendToString(aStream, m.GetBackgroundColor(), "] [color="); > if (!detailed) { > AppendToString(aStream, m.GetScrollId(), "] [scrollId="); > if (m.GetScrollParentId() != FrameMetrics::NULL_SCROLL_ID) { > AppendToString(aStream, m.GetScrollParentId(), "] [scrollParent="); > } >+ if (m.HasClipRect()) { >+ AppendToString(aStream, m.ClipRect(), "] [clip="); >+ } > AppendToString(aStream, m.GetZoom(), "] [z=", "] }"); > } else { > AppendToString(aStream, m.GetDisplayPortMargins(), " [dpm="); > aStream << nsPrintfCString("] um=%d", m.GetUseDisplayPortMargins()).get(); > AppendToString(aStream, m.GetRootCompositionSize(), "] [rcs="); > AppendToString(aStream, m.GetViewport(), "] [v="); > aStream << nsPrintfCString("] [z=(ld=%.3f r=%.3f", > m.GetDevPixelsPerCSSPixel().scale, Presumably if you are outputing this in the not detailed case you also want it for the detailed case.

Based on irc discussion with tn, PostprocessRetainedLayers should only be testing occlusion using non-asynchronous clips.

Comment on attachment 8613032 [details] [diff] [review] part 5, fix PostprocessRetainedLayers >+GetNonAsyncClipForLayer(Layer* aLayer) I'm not thrilled about the name, because the clip from a scroll frame that we are scrolling async like the others. The important part is that it is stationary in the container. So perhaps GetStationaryClipInContainer? >@@ -4328,17 +4337,17 @@ ContainerState::PostprocessRetainedLayers(nsIntRegion* aOpaqueRegionForContainer > nsIntRegion clippedOpaque = e->mOpaqueRegion; >- const Maybe<ParentLayerIntRect>& clipRect = e->mLayer->GetClipRect(); >+ const Maybe<ParentLayerIntRect>& clipRect = GetNonAsyncClipForLayer(e->mLayer); > if (clipRect) { > clippedOpaque.AndWith(ParentLayerIntRect::ToUntyped(*clipRect)); > } > data->mOpaqueRegion.Or(data->mOpaqueRegion, clippedOpaque); > if (e->mHideAllLayersBelow) { > hideAll = true; > } For this clip, we can, and do want to use the full combined clip. We can use the combined clip because the opaque region we are adding to is for the animated geometry root: so everything that shares this opaque region will move with the same scroll frames (async or async). We want to use the combined clip because that is what we will composite on screen, so it would be incorrect to consider areas outside of the combined clip as opaque because they will be clipped out. This differs from the first use of GetNonAsyncClipForLayer above because in that case we are comparing the root opaque region for the container. That opaque region doesn't necessarily move with us, or our clip rect(s).

The followup from comment 59 was backed out because it was assuming that all scroll frames on android use async scrolling. Only the root scroll frame of the root content doc uses async scrolling (until we move from the java pan zoom code to the APZ code). Without this containerless is red/orange on try server.

(In reply to Timothy Nikkel (:tn) from comment #66) > Created attachment 8613315 [details] [diff] [review] > part 3 followup for android > > The followup from comment 59 was backed out because it was assuming that all > scroll frames on android use async scrolling. Only the root scroll frame of > the root content doc uses async scrolling (until we move from the java pan > zoom code to the APZ code). > > Without this containerless is red/orange on try server. After bug 1162064 lands can we make nsLayoutUtils::UsesAsyncScrolling(nsIFrame*) include this check for Android? Or does it depend on the context?

whoops, wrong patch

(In reply to David Anderson [:dvander] from comment #68) > After bug 1162064 lands can we make > nsLayoutUtils::UsesAsyncScrolling(nsIFrame*) include this check for Android? > Or does it depend on the context? You mean make a new function that takes a frame? The current function with that name takes no arguments. We could create such a function, and then convert the callers as appropriate (some may not need it).

https://hg.mozilla.org/integration/mozilla-inbound/rev/bef5cc51f3db https://hg.mozilla.org/integration/mozilla-inbound/rev/38a6cc715880 https://hg.mozilla.org/integration/mozilla-inbound/rev/4acac1be1f6b

https://hg.mozilla.org/mozilla-central/rev/bef5cc51f3db https://hg.mozilla.org/mozilla-central/rev/38a6cc715880 https://hg.mozilla.org/mozilla-central/rev/4acac1be1f6b

Rebasing this patch. (I'm just trying to get the transforms right here, it doesn't support multiple mask layers yet). In theory, the mask layer on the FrameMetrics should not move with the layer (as its associated clip does not), but adjusting for that makes it very clearly wrong.

This patch applies on top of part 10 and seems to fix mask layer offsets for me. I've only tested this with containerless scrolling.

It looks like we never get multiple mask layers without this fix.

This seems to work but the code can definitely be improved.

I improved the code and hopefully kept masks on 3d transforms working, though I don't really trust that part.

Bug 1148582 - Factor mask layer creation out of ContainerState::SetupMaskLayer. r=mstange

Bug 1148582 - Add mask layers to FrameMetrics for ancestor scroll frame clips. r=mattwoodrow

Bug 1148582 - Recycle mask layers attached to FrameMetrics. r=mattwoodrow

Bug 1148582 - Apply async transforms to (ancestor) mask layers correctly. r=botond

Bug 1148582 - Include the rounded clip of the async scrolled scroll frame in its mAncestorClip.

Bug 1148582 - Support multiple mask layers per layer in LayerManagerComposite.

https://reviewboard.mozilla.org/r/11897/#review10345 ::: gfx/layers/composite/ContainerLayerComposite.cpp:529 (Diff revision 1) > - > + aManager->GetCompositor()->DrawQuad(visibleRect, clipRect, effectChain, Our static analysis complains about me capturing aManager here (which is refcounted), so I'll fix that by adding a compositor variable and only capturing that. This usage of lambdas is safe anyway because the function that we're passing the lambda to doesn't hold on to the lambda.

Other than the static analysis issue, this try push is looking as green as you could hope for. :)

Comment on attachment 8625636 [details] MozReview Request: Bug 1148582 - Apply async transforms to (ancestor) mask layers correctly. r=botond https://reviewboard.mozilla.org/r/11893/#review10487 r+ with comments addressed. ::: gfx/layers/Layers.h:1512 (Diff revision 1) > + void ComputeEffectiveTransformForMaskLayer(Layer* aMaskLayer, const gfx::Matrix4x4& aTransformToSurface); I think this function can be made static and private. If so, please make it so. ::: gfx/layers/composite/AsyncCompositionManager.cpp:696 (Diff revision 1) > + ancestorMaskLayer->GetLocalTransform() * asyncTransform); GetLocalTransform() includes the pre- and post-scales, but LayerComposite::SetShadowTransform() expects a transform that does not include the pre- and post-scales, so they cannot be used together in this way. Replacing LayerComposite::SetShadowTransform() with a call to the non-member SetShadowTransform() function in this file should work. ::: gfx/layers/composite/AsyncCompositionManager.cpp:724 (Diff revision 1) > + maskLayer->AsLayerComposite()->SetShadowTransform( Same issue with using LayerComposite::SetShadowTransform() directly. ::: gfx/layers/composite/AsyncCompositionManager.cpp:626 (Diff revision 1) > + // we iterate over scroll frames from inside to outside. I would rephrase the last two sentences as: "In the loop below, we iterate over scroll frames from inside to outside. At each iteration, this array contains the layer's ancestor mask layers for all scroll frames inside the current one."

Comment on attachment 8625638 [details] MozReview Request: Bug 1148582 - Support multiple mask layers per layer in LayerManagerComposite. https://reviewboard.mozilla.org/r/11897/#review10677 Ship It!

Comment on attachment 8625637 [details] MozReview Request: Bug 1148582 - Include the rounded clip of the async scrolled scroll frame in its mAncestorClip. We should have two follow up to figure out what we should do for root scroll frames and border-radius.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b40d5e2bc9d7 https://hg.mozilla.org/integration/mozilla-inbound/rev/745366787bca https://hg.mozilla.org/integration/mozilla-inbound/rev/be49031960aa https://hg.mozilla.org/integration/mozilla-inbound/rev/6234e37d912a https://hg.mozilla.org/integration/mozilla-inbound/rev/aff9ea7ce4fe https://hg.mozilla.org/integration/mozilla-inbound/rev/4a28b2cb9562

Sorry, I had to back it all out again because I got the static analysis fix wrong. Compositor objects are refcounted, too. I'll fix it up tomorrow.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d185f800411c https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa640a81855 https://hg.mozilla.org/integration/mozilla-inbound/rev/1a8dd2f36086 https://hg.mozilla.org/integration/mozilla-inbound/rev/656bffca3b9c https://hg.mozilla.org/integration/mozilla-inbound/rev/bc56084c502c https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1115daa8c4

https://hg.mozilla.org/mozilla-central/rev/d185f800411c https://hg.mozilla.org/mozilla-central/rev/5fa640a81855 https://hg.mozilla.org/mozilla-central/rev/1a8dd2f36086 https://hg.mozilla.org/mozilla-central/rev/656bffca3b9c https://hg.mozilla.org/mozilla-central/rev/bc56084c502c https://hg.mozilla.org/mozilla-central/rev/fd1115daa8c4