Port DisplayItemClip to Moz2D

ASSIGNED
Assigned to

Status

()

Core
Layout
ASSIGNED
4 years ago
3 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Comment 1

4 years ago
Created attachment 8442054 [details] [diff] [review]
v1
Attachment #8441707 - Attachment is obsolete: true
Attachment #8442054 - Flags: review?(matt.woodrow)
(Assignee)

Comment 2

4 years ago
Created attachment 8442057 [details] [diff] [review]
v2

forgot to remove the now unneeded snapping changes
Attachment #8442054 - Attachment is obsolete: true
Attachment #8442054 - Flags: review?(matt.woodrow)
Attachment #8442057 - Flags: review?(matt.woodrow)
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
(Assignee)

Comment 4

4 years ago
(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.
(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 :)
Attachment #8442057 - Flags: review?(matt.woodrow)
(Assignee)

Comment 6

3 years ago
Created attachment 8511111 [details] [diff] [review]
v3
Attachment #8442057 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.