[webkit-reviews] review granted: [Bug 90514] Refactor RenderSVGShape to not contain fallback logic : [Attachment 150712] First pass

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted Philip Rogers
<pdr at google.com>'s request for review:
Bug 90514: Refactor RenderSVGShape to not contain fallback logic
https://bugs.webkit.org/show_bug.cgi?id=90514

Attachment 150712: First pass
https://bugs.webkit.org/attachment.cgi?id=150712&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=150712&action=review


Nice improvement! r=me, with some comments:

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:63
> +    } else

No need for the else branch here.

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:64
> +	   m_shapeFallback = false;

How about using m_usePathFallback? Sounds more explanatory to me.

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:145
> +	       RenderSVGShape::createShape();

Shall we name this createPath?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:64
> +    } else

Same unnecessary else branch.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:153
> +    if (m_shapeFallback
> +	   || !svgStyle->strokeDashArray().isEmpty()
> +	   || svgStyle->strokeMiterLimit() !=
svgStyle->initialStrokeMiterLimit()
> +	   || svgStyle->joinStyle() != svgStyle->initialJoinStyle()
> +	   || svgStyle->capStyle() != svgStyle->initialCapStyle()) {

Can we share this code between RenderSVGRect&Ellipse in
RenderSVGShape::canUseOptimizedRenderingPath.. (or something like that..)

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:71
> -    ASSERT(isEmpty());
> +    ASSERT(RenderSVGShape::isEmpty());

isEmpty() is virtual and you do want to invoke only the RenderSVGShape method?