Bug 9197 - CSS3: Borders with border-radius and double, groove, or ridge styles should look better
: CSS3: Borders with border-radius and double, groove, or ridge styles should l...
Status: RESOLVED FIXED
: WebKit
CSS
: 420+
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
: http://www.w3.org/TR/2005/WD-css3-bac...
:
:
: 87931
  Show dependency treegraph
 
Reported: 2006-05-31 14:33 PST by
Modified: 2012-05-30 23:49 PST (History)


Attachments
Testcase (3.58 KB, text/html)
2006-06-01 00:48 PST, Joost de Valk (AlthA)
no flags Details
Rounded border rendering improvements (20.42 KB, patch)
2007-05-21 16:23 PST, mitz@webkit.org
no flags Review Patch | Details | Formatted Diff | Diff
Non-uniform stroke thickness improvements (25.74 KB, patch)
2007-05-22 03:27 PST, mitz@webkit.org
no flags Review Patch | Details | Formatted Diff | Diff
Non-uniform stroke thickness example (after and before) (16.53 KB, image/png)
2007-05-22 03:29 PST, mitz@webkit.org
no flags Details
An interactive test case (1.58 KB, text/html)
2008-06-20 00:47 PST, mitz@webkit.org
no flags Details
Image explaining the strategy for radius drawing (49.03 KB, image/png)
2009-05-17 11:20 PST, Simon Fraser (smfr)
no flags Details
Re-written border code using clipping patches (46.72 KB, patch)
2009-06-29 05:06 PST, Alex Taylor
eric: review-
Review Patch | Details | Formatted Diff | Diff
Updated, re-factored version of Alex's patch (55.66 KB, patch)
2010-06-22 10:41 PST, Beth Dakin
no flags Review Patch | Details | Formatted Diff | Diff
New version of the patch (61.41 KB, patch)
2010-06-22 21:43 PST, Beth Dakin
no flags Review Patch | Details | Formatted Diff | Diff
Patch addresses some comments (61.48 KB, patch)
2010-06-23 10:32 PST, Beth Dakin
no flags Review Patch | Details | Formatted Diff | Diff
Patch the won't break other platforms (45.48 KB, patch)
2010-06-23 12:42 PST, Beth Dakin
sam: review+
Review Patch | Details | Formatted Diff | Diff
On the left is border-radius (plus a few other things) in Firefox 4, the right shows border-radius not quite rendering right in Webkit 534.28 (52 bytes, url)
2011-04-08 11:14 PST, Kim
no flags Details


Note

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


Description From 2006-05-31 14:33:21 PST
The border arcs for double, groove, and ridge styles should be implemented differently. We should pretend like we're drawing two solid borders, so do the outer/inner clip for each line separately. These styles will probably look much nicer if they are implemented this way, particularly with smaller border widths. There are existing layout tests covering these styles in fast/borders.
------- Comment #1 From 2006-05-31 14:40:37 PST -------
This is a followup bug for http://bugzilla.opendarwin.org/show_bug.cgi?id=6755
------- Comment #2 From 2006-05-31 22:26:40 PST -------
I'll create an uber testcase for all variants of border-radius.
------- Comment #3 From 2006-06-01 00:48:28 PST -------
Created an attachment (id=8633) [details]
Testcase

This should help in fixing it :)
------- Comment #4 From 2007-02-13 09:35:33 PST -------
additionally the border arcs for dotted and dashed of thin borders look very different than the straight borders. 1px dotted even looks like a solid border...
------- Comment #5 From 2007-05-21 16:23:44 PST -------
Created an attachment (id=14645) [details]
Rounded border rendering improvements

I came across several pathological cases in the existing code while testing this patch, which currently adds at least one more pathology.
------- Comment #6 From 2007-05-22 03:27:29 PST -------
Created an attachment (id=14651) [details]
Non-uniform stroke thickness improvements

This version varies the stroke thickness along arcs joining borders of different widths.
------- Comment #7 From 2007-05-22 03:29:28 PST -------
Created an attachment (id=14652) [details]
Non-uniform stroke thickness example (after and before)

Rendering of arcs between borders with different widths, with attachment 14651 [details] (left) and with the current code (right).
------- Comment #8 From 2007-05-24 09:52:22 PST -------
Those last two patches are actually wrong for all but the uniform stroke and equal radii case, since in other cases the outside of the curve is not exactly an ellipse. I think those cases will have to be done using a clipping path (which I tried to avoid in the patches).
------- Comment #9 From 2007-09-30 12:24:52 PST -------
Looks like this was another casualty to leopard stabilization. ;)
------- Comment #10 From 2008-02-21 07:08:25 PST -------
See also bug 17468.
------- Comment #11 From 2008-06-19 17:19:03 PST -------
(In reply to comment #8)
> Those last two patches are actually wrong for all but the uniform stroke and
> equal radii case, since in other cases the outside of the curve is not exactly
> an ellipse. I think those cases will have to be done using a clipping path
> (which I tried to avoid in the patches).
> 

That statement is a little ambiguous to me, are you saying that using this patch, the outside of the curve does not draw as an ellipse (and it should). Or are you saying that with this patch, the outside of the curve follows an ellipse (and it shouldn't).

I have a patch which is very close to this but instead I kept the Inner Clipping Rectangle in there, is that what you're hinting should be done or is there another equation for the corners that probably should be followed instead of clipping to rounded rectangles?
------- Comment #12 From 2008-06-19 17:54:30 PST -------
(In reply to comment #11)
> That statement is a little ambiguous to me, are you saying that using this
> patch, the outside of the curve does not draw as an ellipse (and it should). Or
> are you saying that with this patch, the outside of the curve follows an
> ellipse (and it shouldn't).

Sorry about the ambiguity. I meant the former.

> I have a patch which is very close to this but instead I kept the Inner
> Clipping Rectangle in there, is that what you're hinting should be done or is
> there another equation for the corners that probably should be followed instead
> of clipping to rounded rectangles?

It has been a while since I worked on this bug, but I think that with Core Graphics at least, one would have to clip to a rounded rectangle (that is, the correct shape cannot be achieved by just stroking a path in the non-uniform case).
------- Comment #13 From 2008-06-20 00:47:38 PST -------
Created an attachment (id=21853) [details]
An interactive test case

A test file I used when I worked on this bug.
------- Comment #14 From 2008-06-20 01:08:43 PST -------
Oh wow, awesome test case you had there. Shows some massive problems with border-radius.

When drawing rounded borders, an outer rounded clipping region is applied always.
Also currently, in cases where it needs to, an inner rounded clipping region is also applied.

You last patch removes the inner clipping region in all cases explicitly but you're saying it is required? 
Those clipping regions perform elliptically at the corners so are they what you're saying is required? 
If so, what was the reasoning for removing them in the patch? 

I'm interested in getting this all fixed up so any pointers would be appreciated.

(The double border case would probably need additional clipping regions than what is currently there to cater for the gap between the border strokes)
------- Comment #15 From 2008-06-20 16:47:48 PST -------
As a correction, I can see why you removed the clipping regions. They only clip circles, not other ellipses. I got confused as the patch I had done added elliptical support on those clipping regions. 

Sorry about that.
------- Comment #16 From 2009-01-14 00:13:44 PST -------
Has the patch on this bug fallen through the cracks? The latest nightly still doesn't anti-alias double and dotted curves properly and rounds the width of the gap in double border badly.
------- Comment #17 From 2009-05-15 23:27:08 PST -------
Would be nice to get these patches in.
------- Comment #18 From 2009-05-15 23:49:02 PST -------
(In reply to comment #17)
> Would be nice to get these patches in.

Would be nice to fix this bug. The patches have issues, though (comment #8).
------- Comment #19 From 2009-05-17 11:20:25 PST -------
Created an attachment (id=30423) [details]
Image explaining the strategy for radius drawing
------- Comment #20 From 2009-05-17 13:06:24 PST -------
I modified the interactive test case and had a play with it in Firefox 3 yesterday. Their rendering seems to match the strategy image that Simon uploaded.

Unless one of the recent commenters is keen to fix this themselves, I'll take another look at a this bug over the next few days. By the looks, I think our code is a little too convoluted and results in far too many broken edge cases currently.

I won't assign it to me until tomorrow in case someone else has more of a chance to create a patch.
------- Comment #21 From 2009-06-28 05:09:06 PST -------
Taking bug for another spin, have a patch almost done using clipping paths instead of stroking arcs.
------- Comment #22 From 2009-06-29 05:06:10 PST -------
Created an attachment (id=32006) [details]
Re-written border code using clipping patches

This patch changes all the border rendering code to use elliptical clipping masks when there is a border radius defined instead of approximating the curve by stroking an arc on the corners. The clipping masks are anti-aliased. 

This patch isn't finished at all, I need to do a few more bits and pieces and clean up the code.

Mitz or Hyatt, can I please get one of you to do some performance testing on this code (using PLT?) to see how much optimisation work is required.
------- Comment #23 From 2009-07-05 04:12:46 PST -------
I would expect minimal performance impact in normal browser benchmarks, since border-radius is rarely used, let alone non-uniformly. If you want to check performance, I would suggest making some artificial test cases, but you may need to pile a lot of boxes with rounded corners onto the screen to get something that takes measurable time.
------- Comment #24 From 2009-08-04 19:33:56 PST -------
(From update of attachment 32006 [details])
Wow.  We can come up with some way to abstract some of this math.  This is way too much copy/paste.

Huge sections of:
topLeftInnerRadius.setWidth(max(0, topLeftInnerRadius.width() - style->borderLeftWidth()));
 825         topLeftInnerRadius.setHeight(max(0, topLeftInnerRadius.height() - style->borderTopWidth()));


This huge section of checks is repeated at least twice too:
             && (bottomLeft.width() == 0 || bottomLeft.width() >= style->borderLeftWidth())
 895             && (bottomLeft.height() == 0 || bottomLeft.height() >= style->borderBottomWidth())
 8

Style:
 736             int innerBorderTopWidth = style->borderTopWidth()*2/3;
 737             int innerBorderRightWidth = style->borderRightWidth()*2/3;
 738             int innerBorderBottomWidth = style->borderBottomWidth()*2/3;
 739             int innerBorderLeftWidth = style->borderLeftWidth()*2/3;
 740 
Can't we use IntRects here?

Bah!  copy/paste galore:
53             if (style->borderTopWidth() % 3 == 1)
 754                 innerBorderTopWidth += 1;
 755             if (style->borderRightWidth() % 3 == 1)
 756                 innerBorderRightWidth += 1;
 757             if (style->borderBottomWidth() % 3 == 1)
 758                 innerBorderBottomWidth += 1;
 759             if (style->borderLeftWidth() % 3 == 1)
 760                 innerBorderLeftWidth += 1;

I like this change... but It needs to be about half this size.  Using some nice abstractions. :)  Way too much copy paste here to land this.
------- Comment #25 From 2009-08-06 03:24:27 PST -------
(In reply to comment #24)
> (From update of attachment 32006 [details] [details])
> Wow.  We can come up with some way to abstract some of this math.  This is way
> too much copy/paste.

Thanks for setting review- on this, I hadn't realised Maciej didn't with his comment. 
Definitely needs some refactoring and abstraction, I had submitted this and marked review? to get some performance testing but I haven't had a chance to do this myself yet.

> 
> Huge sections of:
> topLeftInnerRadius.setWidth(max(0, topLeftInnerRadius.width() -
> style->borderLeftWidth()));
>  825         topLeftInnerRadius.setHeight(max(0, topLeftInnerRadius.height() -
> style->borderTopWidth()));
> 
> 
> This huge section of checks is repeated at least twice too:
>              && (bottomLeft.width() == 0 || bottomLeft.width() >=
> style->borderLeftWidth())
>  895             && (bottomLeft.height() == 0 || bottomLeft.height() >=
> style->borderBottomWidth())
>  8
> 
> Style:
>  736             int innerBorderTopWidth = style->borderTopWidth()*2/3;
>  737             int innerBorderRightWidth = style->borderRightWidth()*2/3;
>  738             int innerBorderBottomWidth = style->borderBottomWidth()*2/3;
>  739             int innerBorderLeftWidth = style->borderLeftWidth()*2/3;
>  740 
> Can't we use IntRects here?
> 
> Bah!  copy/paste galore:
> 53             if (style->borderTopWidth() % 3 == 1)
>  754                 innerBorderTopWidth += 1;
>  755             if (style->borderRightWidth() % 3 == 1)
>  756                 innerBorderRightWidth += 1;
>  757             if (style->borderBottomWidth() % 3 == 1)
>  758                 innerBorderBottomWidth += 1;
>  759             if (style->borderLeftWidth() % 3 == 1)
>  760                 innerBorderLeftWidth += 1;
> 
> I like this change... but It needs to be about half this size.  Using some nice
> abstractions. :)  Way too much copy paste here to land this.

I'm sure most of the copy/paste can be reduced with some functions for shrinking an IntRect and rounding the sizes of an IntRect etc, I really just needed to confirm that my maths was actually working correctly before making some generic functions.

Thanks for the comments, I really need to get some more work done on this patch!
------- Comment #26 From 2010-06-01 11:44:45 PST -------
I know I'm chiming in late on this, and I'm really glad someone is tackling this problem, but I think there might be a better way to tackle this.

It seems to me that once you're calculating inner and outer paths, using fills would be a lot simpler than stroke/clip. (Except for dashes. Dashes almost justify stroke/clip, but not quite, in my mind.) You can also control bezels and the like more precisely.

Fills are also a lot more flexible as far as applying gradient colors and other effects (as far as I understand things). If I were to tackle this problem, I'd start by checking the SVG library - it's much easier to build paths with elliptical segments using SVG than with NSPath. Hopefully somewhere in there is a bridge.

Just my two cents; I don't want to diminish or discourage your efforts in any way, but ultimately I think using fills gives the code the best long-term viability.
------- Comment #27 From 2010-06-22 10:41:20 PST -------
Created an attachment (id=59390) [details]
Updated, re-factored version of Alex's patch

Here is an updated, re-factored, and ever-so-slightly behaviorally different version of Alex's patch. This is probably best reviewed in person where I can show all of the improvements and the trade-offs, but I will try to summarize here in case anyone who is not right HERE wants to review:

-Solid, inset/outset, groove/ridge border are WAY better. I am tempted to say this patch fixes all of the bugs with these styles based on Dan's interactive test case, but there might be some edge cases.
-Double border are way better too. There are a few edge-case bugs that can be found with the interactive test case, but I think these could be fine-tuned and fixed in a follow-up patch.
-Dotted and Dashed borders are not better, but I am tempted to say they are not worse either. They are just…different. This is the area that would need the most follow-up work if we decide to go with this patch. These layout tests are probably worse with this patch:

fast/borders/borderRadiusDashed01.html
fast/borders/borderRadiusDashed02.html
fast/borders/borderRadiusDashed03.html

But these ones are better:

fast/borders/borderRadiusDotted01.html
fast/borders/borderRadiusDotted02.html
fast/borders/borderRadiusDotted03.html

This one is a good example of "just different." I don't think the new rendering is better than the old or vice versa.

fast/borders/borderRadiusAllStylesAllCorners.html

Playing with the interactive test case with and without the patch make me feel like it's just trading one set of bugs for another. 

Other obvious problems with this patch are that I need to implement clipConvexPolygon() for non-CG platforms, and need to add many news tests. I will work on that while all of you reviewers out there consider taking a look ;-)
------- Comment #28 From 2010-06-22 10:42:47 PST -------
Attachment 59390 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1000:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1000:  ignore_right is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1019:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1019:  ignore_left is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1022:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1022:  ignore_right is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1042:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1042:  ignore_top is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1045:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1045:  ignore_bottom is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1061:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1061:  ignore_top is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1062:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1065:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1065:  ignore_bottom is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1066:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 22 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #29 From 2010-06-22 10:50:37 PST -------
Attachment 59390 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3299588
------- Comment #30 From 2010-06-22 12:13:17 PST -------
Attachment 59390 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3269600
------- Comment #31 From 2010-06-22 12:14:07 PST -------
Attachment 59390 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3316576
------- Comment #32 From 2010-06-22 13:13:44 PST -------
(From update of attachment 59390 [details])
Taking the review flag off this for now. I fixed the style errors. The compilation errors are probably just that I haven't implemented that GraphicsContext function on non-CG platforms, and I will get to that.

Simon and I looked at this a little in person, and he convinced me to put a little more effort into getting dashed and dotted styles right. I am looking at that now, but if anyone wants to look at the patch and comment on it in any way, I still encourage that!!
------- Comment #33 From 2010-06-22 21:43:28 PST -------
Created an attachment (id=59473) [details]
New version of the patch

Here's a new version of the patch. Dashed and dotted border looks WAY better now. I also fleshed out stubs in all of the different ports of GraphicsContext. I should really fill these in because border-radius implementations on other platforms will break without this function, but it is a little scary to right them without configurations to compile and test. Hmm…any advice anyone? 

Other know problems:
-Double borders need a little tweaking. This will be easy to fix in a follow-up patch. I added a FIXME to borderWillArcInnerEdge in RenderBoxModelObject.
-Dashed and dotted borders could be even better if we tried centering the dash pattern. Added a FIXME for this too.
-The angle of border sides is a little off sometimes. Should be fix-able in a follow-up patch.
-Need to add more tests. Dan't interactive test case has been invaluable in working on this, and we need more representation in the layout tests. I would like to do this in a follow-up patch as well.
------- Comment #34 From 2010-06-22 21:45:54 PST -------
Attachment 59473 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/RenderBoxModelObject.cpp:916:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/RenderBoxModelObject.cpp:917:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/RenderBoxModelObject.cpp:918:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/RenderBoxModelObject.cpp:919:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/RenderObject.cpp:923:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/RenderObject.cpp:924:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #35 From 2010-06-22 21:53:06 PST -------
Attachment 59473 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3329672
------- Comment #36 From 2010-06-22 22:00:39 PST -------
(From update of attachment 59473 [details])
Some brief comments:
WebCore/platform/graphics/GraphicsContext.h:242
 +          void clipConvexPolygon(size_t numPoints, const FloatPoint*);
It's a bit odd to see size_t there, rather than the normal int or unsigned.

WebCore/platform/graphics/cg/GraphicsContextCG.cpp:443
 +  static void addConvexPolygonToContext(CGContextRef context, size_t npoints, const FloatPoint* points)
Use numPoints, rather than npoints.

WebCore/rendering/RenderBoxModelObject.cpp:913
 +  static bool borderWillArcInnerEdge(IntSize firstRadius, IntSize secondRadius, int firstBorderWidth, int secondBorderWidth, int middleBorderWidth)
Use const IntSize& for params.

WebCore/rendering/RenderBoxModelObject.cpp:954
 +      IntRect borderRect = IntRect(tx, ty, w, h);
IntRect borderRect(tx, ty, w, h)

WebCore/rendering/RenderBoxModelObject.cpp:1085
 +      FloatPoint quad[4];
We have FloatQuad for this.

WebCore/rendering/RenderBoxModelObject.h:118
 +      void clipBorderSidePolygon(GraphicsContext*, IntRect, IntSize topLeft, IntSize topRight, IntSize bottomLeft, IntSize bottomRight, const BoxSide side, const RenderStyle* style);
Use const reference parameters.

WebCore/rendering/RenderObject.cpp:881
 +  IntRect RenderObject::borderInnerRect(const IntRect& borderRect, unsigned short topWidth, unsigned short bottomWidth, unsigned short leftWidth, unsigned short rightWidth) const
I don't know why those are unsigned shorts.

WebCore/rendering/style/RenderStyle.h:790
 +      void getInnerBorderRadiiForRectWithBorderWidths(const IntRect&, unsigned short topWidth, 
Ditto
------- Comment #37 From 2010-06-22 22:23:50 PST -------
Attachment 59473 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3302626
------- Comment #38 From 2010-06-22 22:26:16 PST -------
Attachment 59473 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3293650
------- Comment #39 From 2010-06-23 06:42:11 PST -------
Attachment 59473 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3315676
------- Comment #40 From 2010-06-23 10:14:23 PST -------
(In reply to comment #36)
> (From update of attachment 59473 [details] [details])
> Some brief comments:
> WebCore/platform/graphics/GraphicsContext.h:242
>  +          void clipConvexPolygon(size_t numPoints, const FloatPoint*);
> It's a bit odd to see size_t there, rather than the normal int or unsigned.
> 

I made it a size_t to match drawConvexPolygon()'s parameters. Not sure there is much value in that though. I didn't change this for now since I have a justification for it, however weak. I will happily change it if you think I should after reading the explanation. 

> WebCore/platform/graphics/cg/GraphicsContextCG.cpp:443
>  +  static void addConvexPolygonToContext(CGContextRef context, size_t npoints, const FloatPoint* points)
> Use numPoints, rather than npoints.
> 

Fixed.

> WebCore/rendering/RenderBoxModelObject.cpp:913
>  +  static bool borderWillArcInnerEdge(IntSize firstRadius, IntSize secondRadius, int firstBorderWidth, int secondBorderWidth, int middleBorderWidth)
> Use const IntSize& for params.
> 

Fixed.

> WebCore/rendering/RenderBoxModelObject.cpp:954
>  +      IntRect borderRect = IntRect(tx, ty, w, h);
> IntRect borderRect(tx, ty, w, h)
> 

Fixed.

> WebCore/rendering/RenderBoxModelObject.cpp:1085
>  +      FloatPoint quad[4];
> We have FloatQuad for this.
> 

I started changing this, and then I changed it back. The point of constructing this FloatPoint array is to send it to GraphicsContext::clipConvexPolygon(), which takes a FloatPoint array as a parameter and not a FloatQuad. Of course, GraphicsContext::clipConvexPolygon() is a brand new function called only from new code, so we can make it take a FloatQuad, but then it will be clipFloatQuad() instead of clipConvexPolygon() which seems less flexible for potential future use in other code. 

Anyway, I am open to changing this, but I also think there is a good justification for keeping this the way it is. What do you think?

> WebCore/rendering/RenderBoxModelObject.h:118
>  +      void clipBorderSidePolygon(GraphicsContext*, IntRect, IntSize topLeft, IntSize topRight, IntSize bottomLeft, IntSize bottomRight, const BoxSide side, const RenderStyle* style);
> Use const reference parameters.
> 

Fixed.

> WebCore/rendering/RenderObject.cpp:881
>  +  IntRect RenderObject::borderInnerRect(const IntRect& borderRect, unsigned short topWidth, unsigned short bottomWidth, unsigned short leftWidth, unsigned short rightWidth) const
> I don't know why those are unsigned shorts.
> 
> WebCore/rendering/style/RenderStyle.h:790
>  +      void getInnerBorderRadiiForRectWithBorderWidths(const IntRect&, unsigned short topWidth, 
> Ditto

They are unsigned shorts since RenderStyle stores and returns border widths as unsigned shorts. I didn't see any value in changing the type. I guess there is some value in just having a shorter type name, so I will change these to just unsigned if you want. I didn't change this for now, but I would be fine with changing it. What do you think?

I will attach a new patch with the above changes momentarily.
------- Comment #41 From 2010-06-23 10:32:49 PST -------
Created an attachment (id=59529) [details]
Patch addresses some comments
------- Comment #42 From 2010-06-23 12:42:45 PST -------
Created an attachment (id=59552) [details]
Patch the won't break other platforms

After talking with Darin and Sam, I decided to keep around the old version of RenderBoxModelObject::paintBorder() and RenderObject::drawArcForBoxSide() for non-CG platforms. After I get an r+, I will file bugs for the other platforms to implement GraphicsContext::clipConvexPolygon() and email WebKit dev about it. But in the meantime, non-CG platforms will just have our same old border-radius support, and they can switch to the new hotness once they implement that one function.

Just realized I should update the Changlog to reflect this. Will do that now, but I won't bother posting a new patch for it unless someone really wants me to.
------- Comment #43 From 2010-06-26 19:13:54 PST -------
Awesome, cheers for picking this up Beth. 

There are still a few little ugly edge cases around extreme non-uniform borders but they probably need to be in a separate bug.

The only thing I can see with the outcome of this patch is a nasty 1px anti-aliasing regression on the diagonals at the corners. You can see it on the top-left corner in the interactive test case when there is an inner clipping radius on the corners. (I think it is the border-radius > border-width case?)
------- Comment #44 From 2010-06-28 12:08:19 PST -------
(In reply to comment #43)
> Awesome, cheers for picking this up Beth. 
> 
> There are still a few little ugly edge cases around extreme non-uniform borders but they probably need to be in a separate bug.
> 
> The only thing I can see with the outcome of this patch is a nasty 1px anti-aliasing regression on the diagonals at the corners. You can see it on the top-left corner in the interactive test case when there is an inner clipping radius on the corners. (I think it is the border-radius > border-width case?)

Hey Alex! Thanks for your patches! My patch is basically a modernized version of yours. 

I have a list of a few known bugs that remain from this patch, and I know how to fix most of them, but I am holding out to do the work in follow-up patches. I am not sure I *totally* understand the bug you mention above…it might be one of the bugs I have found, but it might be something I didn't find. It's easy to make a stand-alone test case from the interactive one by finding out the values of the various change-able parameters in the web inspector. If you could make one or just tell me what the values are in the web inspector when you see the bug you describe, that would be very helpful.

Thanks again!
------- Comment #45 From 2010-06-28 13:54:45 PST -------
Thanks Sam!! Fixed with r62035. I will be filing follow-up bugs momentarily.
------- Comment #46 From 2010-06-28 14:20:10 PST -------
Filed:
https://bugs.webkit.org/show_bug.cgi?id=41301
https://bugs.webkit.org/show_bug.cgi?id=41302
https://bugs.webkit.org/show_bug.cgi?id=41303
https://bugs.webkit.org/show_bug.cgi?id=41304

I still need to file bugs for other ports to implement GraphicsContext::clipConvexPolygon().
------- Comment #47 From 2010-06-28 15:09:36 PST -------
http://trac.webkit.org/changeset/62035 might have broken GTK Linux 32-bit Debug
------- Comment #49 From 2010-06-28 15:13:06 PST -------
(In reply to comment #47)
> http://trac.webkit.org/changeset/62035 might have broken GTK Linux 32-bit Debug

Looking at the logs, I don't think so.
------- Comment #50 From 2011-04-08 11:14:58 PST -------
Created an attachment (id=88844) [details]
On the left of the image is a border-radius (plus a few other things) in Firefox 4 (it looks like this is according to spec), the right shows border-radius not quite rendering right in Webkit 534.28

It would be nice to see the curve between two borders with different widths look smooth -- I really don't want to use two elements here to have the same effect <.<;

This bug has some better examples linked:
https://bugs.webkit.org/show_bug.cgi?id=38787