Bug 90514 - Refactor RenderSVGShape to not contain fallback logic
Summary: Refactor RenderSVGShape to not contain fallback logic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philip Rogers
URL:
Keywords:
Depends on:
Blocks: 88231
  Show dependency treegraph
 
Reported: 2012-07-03 21:11 PDT by Philip Rogers
Modified: 2012-07-08 20:57 PDT (History)
3 users (show)

See Also:


Attachments
First pass (18.13 KB, patch)
2012-07-03 21:23 PDT, Philip Rogers
zimmermann: review+
Details | Formatted Diff | Diff
Update after reviewer comments (18.65 KB, patch)
2012-07-06 16:39 PDT, Philip Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2012-07-03 21:11:18 PDT
The interaction between RenderSVGShape and {RenderSVGEllipse, RenderSVGRect} is unnecessarily confusing.

(part of https://bugs.webkit.org/show_bug.cgi?id=88231)
Comment 1 Philip Rogers 2012-07-03 21:23:46 PDT
Created attachment 150712 [details]
First pass
Comment 2 Nikolas Zimmermann 2012-07-05 23:15:59 PDT
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?
Comment 3 Philip Rogers 2012-07-06 16:39:46 PDT
Created attachment 151122 [details]
Update after reviewer comments

@Niko, do you mind taking one more look?
Comment 4 Philip Rogers 2012-07-06 17:09:45 PDT
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.
Comment 5 Nikolas Zimmermann 2012-07-08 15:10:08 PDT
Comment on attachment 151122 [details]
Update after reviewer comments

r=me.
Comment 6 Philip Rogers 2012-07-08 20:03:45 PDT
Comment on attachment 151122 [details]
Update after reviewer comments

Thanks for the review! In the queue it goes
Comment 7 WebKit Review Bot 2012-07-08 20:57:21 PDT
Comment on attachment 151122 [details]
Update after reviewer comments

Clearing flags on attachment: 151122

Committed r122079: <http://trac.webkit.org/changeset/122079>
Comment 8 WebKit Review Bot 2012-07-08 20:57:27 PDT
All reviewed patches have been landed.  Closing bug.