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

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


Can you check if the cursor "hand"is just visible over the visible parts of the
rect on this example please? :

<svg xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink">
<clipPath id="clip">
    <rect width="55" height="110"/>	   
</clipPath>
<a xlink:href="#">
<rect x="5" y="5" width="100" height="100" stroke="blue" fill="green"
stroke-width="10" clip-path="url(#clip)"/>
</a>
</svg>

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:50
> +    // Clear m_rect.
> +    m_rect = FloatRect();

Can you call it m_boundingBox?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:55
> +    // Fallback to the parent if rect has special features.

Fallback to RenderSVGShape...

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:73
> +    // 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.

To decide if the stroke contains a point.... A stroke contains the point, if
...

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:93
> +    // If we inflate the m_rect with the half size strokeWidth, we get the
strokeBoundingBox in non-fallback path mode.

s/m_rect/bounding box/

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:95
> +    FloatRect strokeRect = m_rect;
> +    strokeRect.inflate(this->strokeWidth() / 2);

hm, isn't it just m_outerStrokeRect?

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

I really dislike this name, because this is clearly not the slow case. What
about shapeDependentStrokeContains?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:115
> +bool RenderSVGRect::fillContainsSlowCase(const FloatPoint& point, const
WindRule& fillRule) const

same here, what about shapeDependentFillContains?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:166
> +    if (m_needsBoundariesUpdate)
> +	   updateCachedBoundariesInParents = true;

Don't you need to clear the rects for RenderSVGRect here as well and look if
you must set/remove fallback flag? I thought this is called for stroke style
changes.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:179
> +    if (needsShapeUpdate || m_needsBoundariesUpdate) {
> +	   updateCachedBoundaries();
> +	   m_needsBoundariesUpdate = false;
> +    }

Maybe you can make updateCachedBoundaries() virtual and add it ti RenderSVGRect
as well. After calling this function you have to decide if you need to update
boundaries or may need to call RenderSVGShape::updateCachedBoundaries(). Make
sure that you don't do work twice. Would be insane to refresh all boundaries in
createShape() and right afterwards in updateCacheBoundaries().

> Source/WebCore/rendering/svg/RenderSVGShape.h:85
> +    void setIsFillFallback() { m_fillFallback = true; }
> +    bool isFillFallback() const { return m_fillFallback; }

Shouldn't setIsFallback take a argument, so that you can reset it later if
needed?


I couldn't review everything. I'll definitely come back to the review, but must
go... I didn't not check the correct use of isFallback right now. Still some
comments that need to address.