Closed Bug 757346 Opened 8 years ago Closed 8 years ago

Draw rounded rects into mask layers without clipping

Categories

(Core :: Graphics: Layers, defect)

15 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: nrc, Assigned: nrc)

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → ncameron
Attached patch patch (obsolete) — Splinter Review
Attachment #626265 - Flags: review?(roc)
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
Attached patch patch (obsolete) — Splinter Review
Attachment #626326 - Flags: review?(roc)
Attachment #626265 - Attachment is obsolete: true
Attachment #626265 - Flags: review?(roc)
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();
Attachment #626326 - Flags: review?(roc) → review+
Attached patch patchSplinter Review
Reviewer's change and minor rebasing. Carrying r=roc.
Attachment #626326 - Attachment is obsolete: true
Attachment #626340 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/602c1435026d
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/602c1435026d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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?
(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.
I think all comments marked "REVIEW:" can be removed. I know I've got quite a few ancient ones lying around...
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.
I presume the changesets in comment 13 were supposed to be for bug 757347 and just had the wrong bug #?
You need to log in before you can comment on or make changes to this bug.