[Webkit-unassigned] [Bug 80423] New renderer for SVGCircleElement

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=80423





--- Comment #15 from Nikolas Zimmermann <zimmermann at kde.org>  2012-03-19 08:15:34 PST ---
(From update of attachment 132285)
View in context: https://bugs.webkit.org/attachment.cgi?id=132285&action=review

Looks much nicer! Next round of comments:

> Source/WebCore/WebCore.gypi:5682
> +            'rendering/svg/RenderSVGEllipse.h',
> +            'rendering/svg/RenderSVGEllipse.cpp'

cpp before h. Please swap those lines.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:813
> +#endif
> +
> +#if !USE(CG) // append && !USE(MYPLATFORM) here to optimize stroking ellipses for your platform.

I'd merge those ifs, its unlikely a platform only wants to optimize stroking ellipses, but not filling.

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:64
> +    if (style()->svgStyle()->vectorEffect() == VE_NON_SCALING_STROKE) {
> +        RenderSVGShape::createShape();
> +        setIsPaintingFallback(true);
> +        return;
> +    }

Isn't this copied from RenderSVGRect? If so how about moving this whole if statement into a bool REnderSVGShape::fallbackToPathPaintingIfNeeded() method or something with similar naming.
if (fallbackToPathPaintingIfNeeded())
    return;

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:66
> +    calculateRadiiAndCentroid();

AndCenter?

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:76
> +        float halfStrokeWidth = this->strokeWidth() / 2;
> +        m_outerStrokeRect.inflate(halfStrokeWidth);

Just use m_outerStrokeRect.inflate(strokeWidth() / 2) here.

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:90
> +        m_centroid = FloatPoint(circle->cx().value(lengthContext), circle->cy().value(lengthContext));
> +    } else {

Early exit here, to avoid the } else { branch.

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:117
> +    if (!isPaintingFallback()) {

I'd swap the order, use the early return case for the complex code path.

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:126
> +    if (!isPaintingFallback()) {

Ditto.

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:136
> +{
> +    float halfStrokeWidth = this->strokeWidth() / 2;
> +

this-> is not needed. Newline can be removed as well

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:137
> +    FloatPoint d = FloatPoint(m_centroid.x() - point.x(), m_centroid.y() - point.y());

diameter?

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:146
> +    float x_rXOuter = d.x() / (m_radiusX + halfStrokeWidth); // x / (rX + sw/2)
> +    float y_rYOuter = d.y() / (m_radiusY + halfStrokeWidth); // y / (rY + sw/2)
> +
> +    return (x_rXInner * x_rXInner + y_rYInner * y_rYInner >= 1) && (x_rXOuter * x_rXOuter + y_rYOuter * y_rYOuter <= 1);
> +}

I'd avoid the pre computation of all these variables here. First check if the outer case is violated, then early exit. If not, check the inner ellipse, etc..

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:153
> +    FloatPoint d = FloatPoint(m_centroid.x() - point.x(), m_centroid.y() - point.y());

diameter?

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:158
> +    return (x_rX * x_rX + y_rY * y_rY <= 1);

Braces not needed.

> Source/WebCore/rendering/svg/RenderSVGEllipse.h:31
> +#include "RenderSVGPath.h"

s/Path/Shape/ ?

> Source/WebCore/rendering/svg/RenderSVGEllipse.h:58
> +    FloatPoint m_centroid;

m_center sounds better, no?

> Source/WebCore/rendering/svg/RenderSVGEllipse.h:60
> +    float m_radiusX;
> +    float m_radiusY;

Why not use a FloatSize here? FloatSize m_radii.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.