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

[bugzilla-daemon at webkit.org]

Andreas Kling <kling 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 107681: Propsed patch
https://bugs.webkit.org/attachment.cgi?id=107681&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107681&action=review


> Source/WebCore/platform/graphics/FloatRect.h:154
> +    bool contains(const FloatPoint&, ContainsMode = ContainsRect) const;

I am not a fan of this API for three reasons:

1) We're out-of-lining FloatRect::contains() which may have performance
implications. Do we know it doesn't? Could we keep it inline?
2) The enum values in ContainsMode (ContainsRect and InsideRect) are bad. What
is the difference between ContainsRect and InsideRect? The names of the modes
should make it perfectly clear what they do, I shouldn't have to look inside
FloatRect.cpp for this information.
3) I don't see the InsideRect mode used anywhere!

> Source/WebCore/rendering/svg/RenderSVGPath.h:36
> -    explicit RenderSVGPath(SVGStyledTransformableElement*);
> +    RenderSVGPath(SVGStyledTransformableElement*);

Why no longer explicit?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:112
> +    return !m_innerStrokeRect.contains(point, FloatRect::ContainsRect);

Since ContainsRect is the default value of the 2nd parameter, you can (and
should) omit it here.