[webkit-reviews] review denied: [Bug 65769] New renderer for SVGRectElement : [Attachment 105479] Propsed 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 105479: Propsed patch
https://bugs.webkit.org/attachment.cgi?id=105479&action=review

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


Some more comments.

> Source/WebCore/rendering/svg/RenderSVGPath.h:34
> +class RenderSVGPath : public RenderSVGShape {

Add a FIXME that you plan to remove RenderSVGPath with all the changes in DRT
afterwards in a followup.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:51
> +    SVGRectElement* rect = static_cast<SVGRectElement*>(node());

Can you add an assert here for rect please?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:55
> +    if (m_hasRx || m_hasRy ||
!style()->svgStyle()->strokeDashArray().isEmpty()) {

just create the path for rects with rounded corners. More later on this
review...

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:96
> +    if (hasPath())
> +	   RenderSVGShape::fillShape(context);
> +    else
> +	   context->fillRect(m_rect);

even if we have to create paths for pointer events, it could be faster to use
context->fillRect instead of filling the path (of course just if fillRect
respects dashArray and different styles; you have to check this first). We just
need path for rects with rounded corners.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:105
> +    if (hasPath()) {
> +	   RenderSVGShape::strokeShape(context);
> +	   return;
> +    }
> +    context->strokeRect(m_rect, this->strokeWidth());

Ditto

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:108
> +bool RenderSVGRect::strokeContainsSlowCase(const FloatPoint& point)

what is the fast case?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:110
> +    if (hasPath())

You just created a path for dash arrays. But there is more stuff to respect:
lineCap, mitterLimit. The W3C test suite has more examples. This is not needed
for stroking or filling a rect, since the graphic libraries support it with
rects automatically. But you have to think about this on pointer events.

I'd just create the path, for _pointer events_ if we have either have a
dahsarray, some special settings for mitterlimit or lineCap, and not on every
layout call. Use the stored rects as much as possible. Just fallback to the
path if there is no other way to determine something. Would it make sense to
store a boolean for isPath() in RenderShape instead of checking the pointer?
Not sure if we loose time on the check.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:121
> +    FloatRect innerStrokeRect = m_rect;
> +    FloatRect outerStrokeRect = m_rect;
> +    innerStrokeRect.inflate(-strokeWidth / 2);
> +    outerStrokeRect.inflate(strokeWidth / 2);
> +    return (determinePointLocation(point, innerStrokeRect) != InsideShape
> +	       && determinePointLocation(point, outerStrokeRect) !=
OutsideShape) ?  true : false;

you should store inner and outer rect. But don't forget to clear the rects on
style changes. This could be

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:126
> +    return (determinePointLocation(point, m_rect) != OutsideShape) ? true :
false;

isn't m_rect.contains(point.x(), point.y()) enough here? Also, you are missing
the rounded corner case.

And I'd like to see a test case for pointer events with rounded corners, if you
didn't see regressions.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:134
> +    // Test whether the rect contains the certain point or not
> +    if (!rect.contains(point))
> +	   return OutsideShape;
> +    return (rect.x() < point.x() && rect.maxX() > point.x() && rect.y() <
point.y() && rect.maxY() > point.y()) ? InsideShape : OnStroke;

Can we add a comprises() method in FloatRect, that does not include the
boundingBox of the rect? So we would not need this method here.

> Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.cpp:83
> +    if (/*path && */!(resourceMode & ApplyToTextMode)) {

can you remove this comment?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:75
> +    return path().isEmpty();

why not just m_path->isEmpty();?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:91
> +    return path().strokeBoundingRect(&strokeStyle);

m_path->stroke...

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:102
> +    return path().strokeContains(&applier, point);

m_path->strokeC...

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:107
> +    return path().contains(point, fillRule);

m_path->contains(...

> Source/WebCore/rendering/svg/RenderSVGShape.h:108
> +protected:

The order should be public, protected, private sections

> Source/WebCore/rendering/svg/RenderSVGShape.h:127
> +    Path* pathPtr() const
> +    {
> +	   ASSERT(m_path.get());
> +	   return m_path.get();
> +    };

Looks like you don't use it. Remove it please.