1026777 - Port DisplayItemClip to Moz2D

Status: ASSIGNED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

Attachment: wip

https://tbpl.mozilla.org/?tree=Try&rev=1140d10302af

Comment 1

[Markus Stange [:mstange]]

Attachment: v1

Comment 2

[Markus Stange [:mstange]]

Attachment: v2

forgot to remove the now unneeded snapping changes

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8442057 [details] [diff] [review]
v2

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

This seems like it might confuse gfxContext. In BasicLayers at least we use GetClipExtents, which is determined based off of the clip state tracked by gfxContext. If we've manually added some extra clips to the DT, then these won't be included.

::: layout/base/DisplayItemClip.cpp
@@ +428,5 @@
>    }
>    return str;
>  }
>  #endif
> +   

Trailing whitespace

@@ +443,5 @@
> +
> +void
> +DisplayItemClipSetter::ApplyClipFor(const DisplayItemClip& aClip, const nsRect& aClippedBounds)
> +{
> +  const DisplayItemClip& clip = SimplifiedClip(aClip, aClippedBounds);

Why is this a reference?

@@ +448,5 @@
> +  if (mPushedClipCount > 0 && clip != mAppliedClip) {
> +    UnapplyClip();
> +  }
> +  if (mPushedClipCount == 0 && clip.HasClip()) {
> +    mPushedClipCount = clip.ApplyTo(mDT, mPresContext, mCommonClipCount);  

Whitespace

Comment 4

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Comment on attachment 8442057 [details] [diff] [review]
> v2
> 
> Review of attachment 8442057 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like it might confuse gfxContext. In BasicLayers at least we use
> GetClipExtents, which is determined based off of the clip state tracked by
> gfxContext. If we've manually added some extra clips to the DT, then these
> won't be included.

True. What should we do about that? Should we just wait for the new PushLayer API (or whatever Moz2D changes we make so that BasicLayers can use it) before landing this?

> @@ +443,5 @@
> > +
> > +void
> > +DisplayItemClipSetter::ApplyClipFor(const DisplayItemClip& aClip, const nsRect& aClippedBounds)
> > +{
> > +  const DisplayItemClip& clip = SimplifiedClip(aClip, aClippedBounds);
> 
> Why is this a reference?

Because I thought that might avoid one more copy, but I think not using a reference here would achieve the same thing (through RVO). I can test whether it makes a difference. Note using a reference here is still safe because const references to temporaries keep them alive.

Comment 5

[Matt Woodrow (:mattwoodrow)]

(In reply to Markus Stange [:mstange] from comment #4)
 
> True. What should we do about that? Should we just wait for the new
> PushLayer API (or whatever Moz2D changes we make so that BasicLayers can use
> it) before landing this?

We use GetClipExtents for computing the draw region to pass to the thebes layer callback, and for computing the size of 3d transformed layers, neither of these are likely to be fixed by Moz2D API changes. I think we'll just have to pass some information about the clip state into BasicLayerManager to solve this.

So we really just need to write the patch converting the rest of BasicLayers to Moz2D.

> 
> > @@ +443,5 @@
> > > +
> > > +void
> > > +DisplayItemClipSetter::ApplyClipFor(const DisplayItemClip& aClip, const nsRect& aClippedBounds)
> > > +{
> > > +  const DisplayItemClip& clip = SimplifiedClip(aClip, aClippedBounds);
> > 
> > Why is this a reference?
> 
> Because I thought that might avoid one more copy, but I think not using a
> reference here would achieve the same thing (through RVO). I can test
> whether it makes a difference. Note using a reference here is still safe
> because const references to temporaries keep them alive.

Ok, didn't know that :)

Comment 6

[Markus Stange [:mstange]]

Attachment: v3