[webkit-reviews] review denied: [Bug 95373] Clickable area is incorrect for elements with border-radius : [Attachment 192139] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Xidorn Quan
<quanxunzhen at gmail.com>'s request for review:
Bug 95373: Clickable area is incorrect for elements with border-radius
https://bugs.webkit.org/show_bug.cgi?id=95373

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

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


> Source/WebCore/platform/graphics/FloatQuad.cpp:181
> +// tests whether the line is contained by or intersected with the circle

Please uppercase Tests, and end with a period.

> Source/WebCore/platform/graphics/FloatQuad.cpp:193
> +    float distance2 = c * c / (a * a + b * b);

You compute (a * a + b * b) possibly 3 times in this function.

Can (a * a + b * b) be 0, resulting in a divide by zero?

>> Source/WebCore/platform/graphics/FloatQuad.cpp:208
>> +	return containsPoint(center)   // the circle may be totally contained
by the quad
> 
> One space before end of line comments  [whitespace/comments] [5]

And Sentence case and punctuation please.

> Source/WebCore/platform/graphics/FloatQuad.cpp:217
> +    FloatQuad transformedQuad(m_p1, m_p2, m_p3, m_p4);

Can't you just say FloatQuad transformedQuad(*this)?

> Source/WebCore/platform/graphics/FloatQuad.cpp:222
> +    transformedQuad.move(-center.x(), -center.y());
> +    transformedQuad.scale(size.height(), size.width());
> +
> +    FloatPoint originPoint;
> +    return transformedQuad.intersectsCircle(originPoint, size.height() *
size.width());

I'm not really clear on what you're doing here. Please add a comment. How well
have you tested this code?

> Source/WebCore/platform/graphics/FloatQuad.h:98
> +    bool intersectsCircle(const FloatPoint&, float) const;
> +    bool intersectsEllipse(const FloatPoint&, const FloatSize&) const;

Please name the parameters. It's not obvious what they are..

> Source/WebCore/platform/graphics/RoundedRect.cpp:180
> +bool RoundedRect::intersectsQuad(const FloatQuad& quad) const

For platforms that don't do area hit-testing, I assume that all corners of the
quad are the same point. I think you should add a fast-path for this case.