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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: ventnor.bugzilla, Assigned: ventnor.bugzilla)

Tracking

({fixed1.9.1, perf})

Trunk
mozilla1.9.2a1
fixed1.9.1, perf
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.1b99])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 373826 [details] [diff] [review]
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.
Attachment #373826 - Flags: superreview?(roc)
Attachment #373826 - Flags: review?(roc)
+  /**
+   * 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.
(Assignee)

Comment 2

10 years ago
Created attachment 374011 [details] [diff] [review]
Patch 2

Address comments.
Attachment #373826 - Attachment is obsolete: true
Attachment #374011 - Flags: superreview?(roc)
Attachment #374011 - Flags: review?(roc)
Attachment #373826 - Flags: superreview?(roc)
Attachment #373826 - Flags: review?(roc)
+// 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.
(Assignee)

Comment 4

10 years ago
Created attachment 374013 [details] [diff] [review]
Patch 3

Fix the comments.
Attachment #374011 - Attachment is obsolete: true
Attachment #374013 - Flags: superreview?(roc)
Attachment #374013 - Flags: review?(roc)
Attachment #374011 - Flags: superreview?(roc)
Attachment #374011 - Flags: review?(roc)
Comment on attachment 374013 [details] [diff] [review]
Patch 3

You forgot to rename the function FrameContainsVisibleBounds. Other than that, you're done.
Attachment #374013 - Flags: superreview?(roc)
Attachment #374013 - Flags: superreview+
Attachment #374013 - Flags: review?(roc)
Attachment #374013 - Flags: review+
(Assignee)

Comment 6

10 years ago
Created attachment 374015 [details] [diff] [review]
Patch 3.1
[Checkin: Comment 7 & 10]

Oh bugger it, you're right.
Attachment #374013 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed

Comment 7

10 years ago
http://hg.mozilla.org/mozilla-central/rev/bdce7766c3c0
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 8

10 years ago
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". :)
Attachment #374015 - Flags: approval1.9.1?

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 landing]
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
}
Attachment #374015 - Attachment description: Patch 3.1 → Patch 3.1 [Checkin: Comment 7 & 10]
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [needs 1.9.1 landing] → [fixed1.9.1b5]
Whiteboard: [fixed1.9.1b5] → [fixed1.9.1b99]
You need to log in before you can comment on or make changes to this bug.