[webkit-reviews] review denied: [Bug 95373] Clickable area is incorrect for elements with border-radius : [Attachment 191695] 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 191695: patch
https://bugs.webkit.org/attachment.cgi?id=191695&action=review

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


> Source/WebCore/rendering/HitTestLocation.cpp:217
> +static inline bool isLineIntersectsEllipse(const FloatPoint& center, const
FloatSize& size, const FloatPoint& p0, const FloatPoint& p1)
> +{
> +    float x0 = (p0.x() - center.x()) * size.height() / size.width();
> +    float y0 = p0.y() - center.y();
> +    float x1 = (p1.x() - center.x()) * size.height() / size.width();
> +    float y1 = p1.y() - center.y();
> +    float radius2 = size.height() * size.height();
> +    if ((x0 * x0 + y0 * y0) <= radius2 || (x1 * x1 + y1 * y1) <= radius2)
> +	   return true;
> +
> +    float a = y0 - y1;
> +    float b = x1 - x0;
> +    float c = x0 * y1 - x1 * y0;
> +    float distance2 = c * c / (a * a + b * b);
> +    // If distance between the center point and the line > the radius,
> +    // the line doesn't cross (or is contained by) the ellipse.
> +    if (distance2 > radius2)
> +	   return false;
> +
> +    // The nearest point on the line is between p0 and p1?
> +    float x = - a * c / (a * a + b * b);
> +    float y = - b * c / (a * a + b * b);
> +    return (((x0 <= x && x <= x1) || (x0 >= x && x >= x1))
> +	   && ((y0 <= y && y <= y1) || (y1 <= y && y <= y0)));
> +}
> +
> +static inline bool isQuadIntersectsEllipse(const FloatPoint& center, const
FloatSize& size, const FloatQuad& quad)
> +{
> +    if (isLineIntersectsEllipse(center, size, quad.p1(), quad.p2())
> +	   || isLineIntersectsEllipse(center, size, quad.p2(), quad.p3())
> +	   || isLineIntersectsEllipse(center, size, quad.p3(), quad.p4())
> +	   || isLineIntersectsEllipse(center, size, quad.p4(), quad.p1()))
> +	   return true;
> +    return false;
> +}

Please move this code out into a separate file; it is generally useful. I
suggest platform/graphics/GeometryUtilities.h/cpp. That file could also contain
the

float findSlope(const FloatPoint& p1, const FloatPoint& p2, float& c);
bool findIntersection(const FloatPoint& p1, const FloatPoint& p2, const
FloatPoint& d1, const FloatPoint& d2, FloatPoint& intersection);

functions that are currently in FloatPoint.cpp.

isLineIntersectsEllipse() should be called lineIntersectsEllipse().
isQuadIntersectsEllipse() should be called quadIntersectsEllipse().

Why don't you just add RoundedRect::containsPoint()?