Last Comment Bug 757346 - Draw rounded rects into mask layers without clipping
: Draw rounded rects into mask layers without clipping
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: 15 Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-22 00:40 PDT by Nick Cameron [:nrc]
Modified: 2012-06-26 02:01 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.97 KB, patch)
2012-05-22 17:31 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch (5.28 KB, patch)
2012-05-22 22:22 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
patch (5.27 KB, patch)
2012-05-22 23:27 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review

Description Nick Cameron [:nrc] 2012-05-22 00:40:06 PDT
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] 2012-05-22 17:31:03 PDT
Created attachment 626265 [details] [diff] [review]
patch
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-22 17:36:55 PDT
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] 2012-05-22 22:22:26 PDT
Created attachment 626326 [details] [diff] [review]
patch
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-22 22:57:52 PDT
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] 2012-05-22 23:27:05 PDT
Created attachment 626340 [details] [diff] [review]
patch

Reviewer's change and minor rebasing. Carrying r=roc.
Comment 6 Nick Cameron [:nrc] 2012-05-27 12:13:19 PDT
Try push: https://tbpl.mozilla.org/?tree=Try&rev=a02fcb809219
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-05-27 13:59:52 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/602c1435026d
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-05-27 18:48:24 PDT
https://hg.mozilla.org/mozilla-central/rev/602c1435026d
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2012-05-28 03:41:34 PDT
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] 2012-05-28 14:10:17 PDT
(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) (Exited; email my personal email if necessary) 2012-05-28 14:14:13 PDT
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] 2012-05-28 17:17:28 PDT
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 14 Ed Morley [:emorley] 2012-06-26 02:01:36 PDT
I presume the changesets in comment 13 were supposed to be for bug 757347 and just had the wrong bug #?

Note You need to log in before you can comment on or make changes to this bug.