Closed Bug 628745 Opened 10 years ago Closed 10 years ago

skip rounded rect clips whenever possible

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(1 file, 3 obsolete files)

Rounded rect clips are slow, and a good portion of the time they are useless as their rounded corners don't clip anything. We should replace them with their non-rounded rect cousins whenever possible.
Attached patch patch (obsolete) — Splinter Review
Attachment #506839 - Flags: review?(roc)
+  if (!mHaveClipRect)
+    return;
+
+  mClipRect = NonRoundedIntersection();
+  mRoundedClipRects.Clear();

The early returns for !mHaveClipRect seem unnecessary. How about
  if (mRoundedClipRects.IsEmpty())
    return;
  mClipRect = NonRoundedIntersection();

and have NonRoundedIntersection skip using mClipRect if !mHaveClipRect? Assert in NonRoundedIntersection that !mRoundedClipRects.IsEmpty() instead of checking mHaveClipRect. IsRectClippedByRoundedCorner can exit early if !mRoundedClipRects.IsEmpty() too.

I think you need to do something for zero radii here. Right now it looks like we can die in IsInsideEllipse for zero radii. I think changing
+    if (rect.x <= rr.mRect.x + rr.mRadii[NS_CORNER_TOP_LEFT_X] &&
+        rect.y <= rr.mRect.y + rr.mRadii[NS_CORNER_TOP_LEFT_Y]) {
to use < (and similar changes too) would avoid that, and optimize a little bit more too.
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to comment #2)
> and have NonRoundedIntersection skip using mClipRect if !mHaveClipRect?

The way it is coded right now if mHaveClipRect is true then both mClipRect and mRoundedClipRects are valid (http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1782), so if mRoundedClipRects is not empty then mClipRect must be present and valid.

Otherwise made the other changes.
Attachment #506839 - Attachment is obsolete: true
Attachment #506877 - Flags: review?(roc)
Attachment #506839 - Flags: review?(roc)
Attached patch interdiff (obsolete) — Splinter Review
Comment on attachment 506877 [details] [diff] [review]
patch v2

Please make sure we have a test for zero-radii.
Attachment #506877 - Flags: review?(roc) → review+
Just a test that we execute this code with zero-radii and don't crash?
Yes --- a test that would have crashed with the first version of the patch :-).
Attached patch patch v3Splinter Review
Added a test.
Attachment #506877 - Attachment is obsolete: true
Attachment #506878 - Attachment is obsolete: true
Attachment #506889 - Flags: approval2.0?
Comment on attachment 506889 [details] [diff] [review]
patch v3

Let's get this in ASAP!
Attachment #506889 - Flags: approval2.0? → approval2.0+
This might make bug 601512 much less common.
Blocks: 601512
Try server of an intermediate version of this patch was green. Try server run of final patch is running now. I'll probably land tomorrow unless someone is landing late tonight and wants to land this.
Blocks: 629012
http://hg.mozilla.org/mozilla-central/rev/8149e1a06476
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 613406
You need to log in before you can comment on or make changes to this bug.