Markus Stange [:mstange] Reporter
	Description • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 1 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 2 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 3 • 9 years ago

*compositing.

	Timothy Nikkel (:tnikkel)
	Comment 4 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 5 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 6 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 7 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 8 • 9 years ago

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.

	Botond Ballo [:botond]
	Comment 9 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 10 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 11 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 12 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 13 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 14 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 15 • 9 years ago

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?

	Markus Stange [:mstange] Reporter
	Comment 16 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 17 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 20 • 9 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 22 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 23 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 24 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 25 • 9 years ago

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?

	Timothy Nikkel (:tnikkel)
	Comment 27 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 28 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 29 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 30 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 31 • 9 years ago

Revert the premature mScrollPort clip.

	Timothy Nikkel (:tnikkel)
	Comment 32 • 9 years ago

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

Thanks

	Markus Stange [:mstange] Reporter
	Comment 33 • 9 years ago

So do parts 1 through 5 already fix bug 1148581?

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 34 • 9 years ago

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

Yup.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 35 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 36 • 9 years ago

Slightly better patch.

	Botond Ballo [:botond]
	Comment 37 • 9 years ago

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

	Botond Ballo [:botond]
	Comment 38 • 9 years ago

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:: ?

	Botond Ballo [:botond]
	Comment 39 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 40 • 9 years ago

Factor mask layer creation out of SetupMaskLayer.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 41 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 42 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 43 • 9 years ago

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.

	Matt Woodrow (:mattwoodrow)
	Comment 45 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 46 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 47 • 9 years ago

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.

	Matt Woodrow (:mattwoodrow)
	Comment 48 • 9 years ago

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?

	Matt Woodrow (:mattwoodrow)
	Comment 49 • 9 years ago

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.

	Matt Woodrow (:mattwoodrow)
	Comment 50 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 51 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 52 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 53 • 9 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 54 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 55 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 56 • 9 years ago

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

	Pulsebot
	Comment 57 • 9 years ago

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

	Pulsebot
	Comment 58 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/93943a21b457

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 59 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 60 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 61 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 62 • 9 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 63 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 64 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 65 • 9 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 66 • 9 years ago

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.

	Pulsebot
	Comment 67 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e044d8526a1a

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 68 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 70 • 9 years ago

whoops, wrong patch

	Timothy Nikkel (:tnikkel)
	Comment 71 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 72 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/e044d8526a1a

	Pulsebot
	Comment 73 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 74 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 75 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 76 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 77 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 78 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 79 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 80 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=590b30a2336d

	Markus Stange [:mstange] Reporter
	Comment 81 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe31018583fa

	Markus Stange [:mstange] Reporter
	Comment 82 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 83 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 84 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 85 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 86 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 87 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 88 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 89 • 9 years ago

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

	Botond Ballo [:botond]
	Comment 90 • 9 years ago

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

	Matt Woodrow (:mattwoodrow)
	Comment 91 • 9 years ago

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!

	Timothy Nikkel (:tnikkel)
	Comment 92 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 93 • 9 years ago

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

	Markus Stange [:mstange] Reporter
	Comment 94 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a3396411829

	Markus Stange [:mstange] Reporter
	Comment 95 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6920f5c41fd

	Markus Stange [:mstange] Reporter
	Comment 96 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 97 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ebf2527074f

	Markus Stange [:mstange] Reporter
	Comment 98 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05f8110b0f7a

	Markus Stange [:mstange] Reporter
	Comment 99 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ffc08677503

	Markus Stange [:mstange] Reporter
	Comment 100 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 101 • 9 years ago

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