489319 - Box shadow's OptimizeVisibility() should take border radii into account

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Michael Ventnor]

Attachment: Patch

Currently what we do is simply bail and redraw the box shadow when we encounter a frame with a border radius. But apparently the prospect of easy rounded corners is too tempting to the kind of people who also love easy drop shadows. For example: look at the screenshot of the WIP dialog on bug 479808. That's supposed to go on mobile phones. We really should avoid using our pure-CPU blurring algorithm, as good as it is, if we don't need it.

I've tested this and it works beautifully.

Comment 1

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

+  /**
+   * Get the size, in twips, of the border radii. It returns FALSE iff all
+   * returned twips radii == 0 (so no border radii), TRUE otherwise.
+   * For the aTwipsRadii indexes, use the NS_CORNER_* constants in nsStyleConsts.h
+   */
+  static PRBool GetBorderRadiusTwips(const nsStyleCorners& aBorderRadius,
+                                     const nscoord& aFrameWidth,
+                                     nscoord aTwipsRadii[8]);
 
Can you fix this comment to refer to "appunits" instead of twips? Also change the name of the parameter to just "aRadii".

+    nsRect frameRectFullLength = frameRect;

Call it frameFullHeight?

It would be nice to factor out this code into a helper function that takes a rect, the radii, and another rect, and tests if the first rect minus the radii contains the last rect. Then we could use it to optimize nsDisplayBorder the same way.

Comment 2

[Michael Ventnor]

Attachment: Patch 2

Address comments.

Comment 3

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

+// Returns TRUE if aVisibleBounds is guaranteed to be contained in
+// aFrameRect. This function attempts to account for border radii,
+// but will not return TRUE for corner cases (literally!)

// Returns TRUE if aVisibleBounds is guaranteed to be contained in
// the rounded rect defined by aFrameRect and aRadii. Complex cases are
// handled conservatively by return PR_FALSE in some situations where
// a more complicated analysis could return PR_TRUE.

Actually I think you shouldn't use the term Frame anywhere here, just call it RoundedRectContainsRect, and the parameters aRoundedRect, aRadii, and aContainedRect.

Comment 4

[Michael Ventnor]

Attachment: Patch 3

Fix the comments.

Comment 5

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

Comment on attachment 374013 [details] [diff] [review]
Patch 3

You forgot to rename the function FrameContainsVisibleBounds. Other than that, you're done.

Comment 6

[Michael Ventnor]

Attachment: Patch 3.1 [Checkin: Comment 7 & 10]

Oh bugger it, you're right.

Comment 7

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/bdce7766c3c0

Comment 8

[Michael Ventnor]

Comment on attachment 374015 [details] [diff] [review]
Patch 3.1
[Checkin: Comment 7 & 10]

I'd certainly like to get this into the 1.9.1 tree after beta 4 finishes. It's simply a big performance improvement for a new feature in one (undoubtedly) common case featuring rounded corners and box shadows.

This would make a big improvement to the "dialogs" that the Fennec team is implementing due to them using both shadows and rounded corners, and would also be helpful to those new Web 2.0 Cloud Computing Rich Internet Application Browser Operating Systems whose design principles are summed up as "do what Apple does". :)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 374015 [details] [diff] [review]
Patch 3.1
[Checkin: Comment 7 & 10]

a1.9.1=dbaron

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 374015 [details] [diff] [review]
Patch 3.1
[Checkin: Comment 7 & 10]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6dd6259f2121

After fixing context for
{
patching file layout/base/nsCSSRendering.cpp
Hunk #1 FAILED at 329
1 out of 2 hunks FAILED
}