[webkit-reviews] review denied: [Bug 72103] box-shadow creates incorrect shadow when border-radius is too large : [Attachment 156055] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 72103: box-shadow creates incorrect shadow when border-radius is too large
https://bugs.webkit.org/show_bug.cgi?id=72103

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=156055&action=review


> Source/WebCore/platform/graphics/Path.cpp:138
> +	   float maxRadiusWidth = std::max(topLeftRadius.width() +
topRightRadius.width(), bottomLeftRadius.width() + bottomRightRadius.width());
> +	   float maxRadiusHeight = std::max(topLeftRadius.height() +
bottomLeftRadius.height(), topRightRadius.height() +
bottomRightRadius.height());

We normally put a 'using namespace std' at the top of the file, then use min()
and max() without the namespace qualifier.

> Source/WebCore/platform/graphics/Path.cpp:148
> +	   float scale = maxRadiusWidth > maxRadiusHeight ? rect.width() /
maxRadiusWidth : rect.height() / maxRadiusHeight;
> +	   FloatSize adjustedTopLeftRadius(topLeftRadius);
> +	   FloatSize adjustedTopRightRadius(topRightRadius);
> +	   FloatSize adjustedBottomLeftRadius(bottomLeftRadius);
> +	   FloatSize adjustedBottomRightRadius(bottomRightRadius);
> +	   adjustedTopLeftRadius.scale(scale);
> +	   adjustedTopRightRadius.scale(scale);
> +	   adjustedBottomLeftRadius.scale(scale);
> +	   adjustedBottomRightRadius.scale(scale);
> +	   addPathForRoundedRect(rect, adjustedTopLeftRadius,
adjustedTopRightRadius, adjustedBottomLeftRadius, adjustedBottomRightRadius,
strategy);

Is it appropriate to have this code here, where it affects all path drawing,
rather than in rendering code? What is the impact on other uses, like SVG and
canvas?

It might be better to make RoundedFloatRect, then have a utility method on it
that does the radius scaling, so we can call it from anywhere.

> LayoutTests/fast/borders/border-radius-larger-than-rect.html:12
> +    background-color: red;

Please don't use red in a passing test. Also, make the divs bigger so the issue
is more apparent.