[webkit-reviews] review denied: [Bug 65769] New renderer for SVGRectElement : [Attachment 105641] Proposed patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Renata Hodovan <reni at webkit.org>'s
request for review:
Bug 65769: New renderer for SVGRectElement
https://bugs.webkit.org/show_bug.cgi?id=65769

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105641&action=review


Some more snippets.

> LayoutTests/svg/custom/pointer-events-on-rounded-rect.svg:19
> +<rect x="40" y="40" rx="30" ry="30" width="400" height="300"
pointer-events="visibleStroke" />

the test looks good, just missing the mousmove commends :)

> Source/WebCore/platform/graphics/FloatRect.cpp:67
> +	   return (x() < point.x() && maxX() > point.x() && y() < point.y() &&
maxY() > y()) ? true : false;

remove ? true : false and the unnecessary braces.

> Source/WebCore/platform/graphics/FloatRect.cpp:68
> +    return false;

what about early return if rect does not contain point?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:110
> +    // To deceide the stroke containing we create two rects which represent
the inner and
> +    // the outer stroke borders. The containing exists, if the point is
between them.
> +    float strokeWidth = this->strokeWidth();

You forgot to check for mitterlimit, dash array and line cap, if they do not
use the default values, you might need to create the path and check the style
in RenderSVGShape::strokeContainsSlowCase.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:116
> +    return ((!innerStrokeRect.contains(point) ||
!innerStrokeRect.comprises(point))
> +	       && outerStrokeRect.contains(point)) ?  true : false;

Remove ?  true : false; and unnecessary braces.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:122
> +    if (isPath())
> +	   return RenderSVGShape::fillContainsSlowCase(point, fillRule);

Because you might create a path just for strokeContains (because of the
different stroke styles), it might be unnecessary to call
RenderSVGShape::fillContainsSlowCase. You just need this call for rounded
corners. Check for the rounded corner instead.