757346 - Draw rounded rects into mask layers without clipping

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

ContainerState::SetupMaskLayer in FrameLayerBuilder.cpp currently uses clipping to draw rounded rects into the mask layer. We ought to be able to do better by drawing the rounded rects directly.

Comment 1

[Nick Cameron [:nrc]]

Attachment: patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 626265 [details] [diff] [review]
patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2648,5 @@
> +  aEnd = NS_MIN<PRUint32>(aEnd, mRoundedClipRects.Length());
> +
> +  for (PRUint32 i = aBegin; i < aEnd; ++i) {
> +    RoundedRectTo(aContext, A2D, mRoundedClipRects[i]);
> +    aContext->Fill();

bogus!

::: layout/base/FrameLayerBuilder.h
@@ +376,5 @@
> +    // Draw (fill) the rounded rects in this clip to aContext
> +    void DrawRoundedRectsTo(gfxContext* aContext, PRInt32 A2D,
> +                            PRUint32 aBegin, PRUint32 aEnd) const;
> +    // 'Draw' (create as a path, does not stroke or fill) aRoundRect to aContext
> +    void RoundedRectTo(gfxContext* aContext, PRInt32 A2D,

AddRoundedRectPathTo

Comment 3

[Nick Cameron [:nrc]]

Attachment: patch

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 626326 [details] [diff] [review]
patch

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

r+ with that

::: layout/base/FrameLayerBuilder.cpp
@@ +2658,5 @@
> +  }
> +
> +  // If there are more than one rounded rects, we need the intersection, so we must clip
> +  ApplyRoundedRectsTo(aContext, A2D, aBegin, aEnd);
> +  aContext->Paint();

It's actually slightly simpler (and potentially more efficient) to do
  ApplyRoundedRectsTo(aContext, A2D, aBegin, aEnd - 1);
  AddRoundedRectPathTo(aContext, A2D, mRoundedClipRects[aEnd - 1]);
  aContext->Fill();

Comment 5

[Nick Cameron [:nrc]]

Attachment: patch

Reviewer's change and minor rebasing. Carrying r=roc.

Comment 6

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=a02fcb809219

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/602c1435026d

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/602c1435026d

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 626340 [details] [diff] [review]
patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2668,5 @@
> +
> +  gfxRect clip = nsLayoutUtils::RectToGfxRect(aRoundRect.mRect, A2D);
> +  clip.Round();
> +  clip.Condition();
> +  // REVIEW: This might make clip empty.  Is that OK?

Was this comment meant to make it into the tree?

Comment 10

[Nick Cameron [:nrc]]

(In reply to :Ms2ger from comment #9)
> Comment on attachment 626340 [details] [diff] [review]
> patch
> 
> Review of attachment 626340 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +2668,5 @@
> > +
> > +  gfxRect clip = nsLayoutUtils::RectToGfxRect(aRoundRect.mRect, A2D);
> > +  clip.Round();
> > +  clip.Condition();
> > +  // REVIEW: This might make clip empty.  Is that OK?
> 
> Was this comment meant to make it into the tree?

That comment has been hanging around for ages, at least as long as I've been working on this code. I've just left it there when I've moved code around.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think all comments marked "REVIEW:" can be removed. I know I've got quite a few ancient ones lying around...

Comment 12

[Nick Cameron [:nrc]]

I'll remove it in my next relevant patch, probably for Bug 757347; doesn't seem worth filing a bug and/or making a separate patch just for this.

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ec5d136e203d
https://hg.mozilla.org/mozilla-central/rev/5a327af385df
https://hg.mozilla.org/mozilla-central/rev/d5a60bfaa968

Comment 14

[Ed Morley [:emorley]]

I presume the changesets in comment 13 were supposed to be for bug 757347 and just had the wrong bug #?