Bug 90514 – Refactor RenderSVGShape to not contain fallback logic

• Component: SVG
• Status: RESOLVED FIXED

[Philip Rogers]

The interaction between RenderSVGShape and {RenderSVGEllipse, RenderSVGRect} is unnecessarily confusing.

(part of https://bugs.webkit.org/show_bug.cgi?id=88231)

[Philip Rogers]

Created attachment 150712 [details]
First pass

[Nikolas Zimmermann]

Comment on attachment 150712 [details]
First pass

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?

[Philip Rogers]

Created attachment 151122 [details]
Update after reviewer comments

@Niko, do you mind taking one more look?

[Philip Rogers]

Comment on attachment 150712 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=150712&action=review

>> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:63
>> +    } else
> 
> No need for the else branch here.

This is actually needed. Here's the scenario:
Imagine a rect has vector-effects=non-scaling-stroke set, and then someone unsets it. Because we no longer re-create the renderer, only createShape() is called from the layout and this will be incorrect the second time through.

>> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:64
>> +        m_shapeFallback = false;
> 
> How about using m_usePathFallback? Sounds more explanatory to me.

Agreed! Done.

>> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:145
>> +            RenderSVGShape::createShape();
> 
> Shall we name this createPath?

Yes and no :P
For now, I think we should leave it as createShape, even though in this case the effect is to create the path. The reason being, RenderSVGShape is the superclass to all of these shape renderers but only RenderSVGShape and RenderSVGPath will have a use for createPath.

I was thinking in a future patch we could extract the path-specific code in RenderSVGShape into RenderSVGShapeWithOptionalPath, and have RenderSVGRect and RenderSVGShape inherit from that. If we did this, RenderSVGShape would be entirely unaware of any path code and would only have functions such as objectBoundingBox() and strokeShape(), etc. In this scenario it would make sense to have createPath().

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:153
>> +        || svgStyle->capStyle() != svgStyle->initialCapStyle()) {
> 
> Can we share this code between RenderSVGRect&Ellipse in RenderSVGShape::canUseOptimizedRenderingPath.. (or something like that..)

Agreed!

I added RenderSVGShape::hasSmoothStroke(). Sound like a good name to you?

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:71
>> +    ASSERT(RenderSVGShape::isEmpty());
> 
> isEmpty() is virtual and you do want to invoke only the RenderSVGShape method?

Because we lazily call createShape() in RenderSVGRect::shapeDependentStrokeContains(), the isEmpty() method of RenderSVGRect will actually return false. This problem was introduced because I changed RenderSVGRect::isEmtpy() conditional to be based on m_usePathFallback instead of hasPath() (see RenderSVGRect.h).

The effect in this case should be the same--we ended up calling RenderSVGShape::isEmpty() either way, now we're just explicit about it.

[Nikolas Zimmermann]

Comment on attachment 151122 [details]
Update after reviewer comments

r=me.

[Philip Rogers]

Comment on attachment 151122 [details]
Update after reviewer comments

Thanks for the review! In the queue it goes

[WebKit Review Bot]

Comment on attachment 151122 [details]
Update after reviewer comments

Clearing flags on attachment: 151122

Committed r122079: <http://trac.webkit.org/changeset/122079>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.