[webkit-reviews] review granted: [Bug 21819] background color of elements with border-radius shows around outer edge of border at corners : [Attachment 90294] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Simon Fraser (smfr) <simon.fraser at apple.com>'s
request for review:
Bug 21819: background color of elements with border-radius shows around outer
edge of border at corners
https://bugs.webkit.org/show_bug.cgi?id=21819

Attachment 90294: Patch
https://bugs.webkit.org/attachment.cgi?id=90294&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=90294&action=review

> Source/WebCore/ChangeLog:12
> +	   If the border is opaque on all sides, we can inset the border by

I think you mean “inset the background”

> Source/WebCore/ChangeLog:35
> +	   Helper method to determine if this border side will totally
> +	   the border edge, allowing us to inset it.

You totally a word

> Source/WebCore/rendering/RenderBoxModelObject.cpp:622
> +		   roundedBorder.inflate(-1);

Does this do the right thing for 1-by-n and 2-by-n borderRects?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1091
> +	   if (!isPresent || isTransparent || width < 2 || color.hasAlpha() ||
style == BHIDDEN)
> +	       return false;
> +
> +	   if (style == DOTTED || style == DASHED)
> +	       return false;
> +
> +	   if (style == DOUBLE)
> +	       return width >= 5; // The outer band needs to be >= 2px wide.

These comparisons should be in device (well, painting root) space, not user
space. For example, if you have a plain .5 scale transform, the width needs to
be at least 4, etc. Worth a comment, perhaps.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2079
> +    edges[BSTop] = BorderEdge(style->borderTopWidth(),
> +		       style->visitedDependentColor(CSSPropertyBorderTopColor),

> +		       style->borderTopStyle(),
> +		       style->borderTopIsTransparent(),
> +		       horizontal || includeLogicalLeftEdge);
> +
> +    edges[BSRight] = BorderEdge(style->borderRightWidth(),
> +		      
style->visitedDependentColor(CSSPropertyBorderRightColor),
> +		       style->borderRightStyle(),
> +		       style->borderRightIsTransparent(),
> +		       !horizontal || includeLogicalRightEdge);
> +
> +    edges[BSBottom] = BorderEdge(style->borderBottomWidth(),
> +		      
style->visitedDependentColor(CSSPropertyBorderBottomColor),
> +		       style->borderBottomStyle(),
> +		       style->borderBottomIsTransparent(),
> +		       horizontal || includeLogicalRightEdge);
> +
> +    edges[BSLeft] = BorderEdge(style->borderLeftWidth(),
> +		      
style->visitedDependentColor(CSSPropertyBorderLeftColor),
> +		       style->borderLeftStyle(),
> +		       style->borderLeftIsTransparent(),
> +		       !horizontal || includeLogicalLeftEdge);

Questionable alignment.