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

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #131108|review?                     |review-
               Flag|                            |




--- Comment #12 from Nikolas Zimmermann <zimmermann at kde.org>  2012-03-10 00:36:00 PST ---
(From update of attachment 131108)
View in context: https://bugs.webkit.org/attachment.cgi?id=131108&action=review

r- for the build problems across the EWS, and some general issues:

> Source/WebCore/ChangeLog:8
> +        This patch introduces a special renderer for SVGCircleElements to avoid

I'm wondering why you didn't do this for SVGEllipseElement, and special case circles in the RenderSVGEllipse renderer?
Platforms could still use their fast paths as soon as we call gc->fill/strokeEllipse.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:782
> +#if !USE(CG)
> +void GraphicsContext::fillEllipse(const FloatRect& ellipse)
> +{
> +    if (paintingDisabled())

I don't like this pattern. We should rather use:
void GraphicsContext::fillEllipse(const FloatRect& ellipse)
{
    platformFillEllipse(ellipse);
}

If a platform wants to optimize ellipse, they put && !USE(MYPLATFORM) here, and then have to supply GraphicsContext::platformFillEllipse, in their platform implementation. That can then still use fillEllipseAsPath() when it encounters cases where it can't use the fast path. Sharing the fallback path through all platforms is important!

#if !USE(CG) // append && !USE(MYPLATFORM) here, if you want to optimize ellipses for your platform...
void GraphicsContext::platformFillEllipse(const FloatRect& ellipse) { fillEllipseAsPath(ellipse); }
#endif

void GraphicsContext::fillEllipseAsPath(const FloatRect& ellipse) // Make this a private method of GraphicsContext.
{
    Path path;
    path.addEllipse(...);
    fillPath(path);
}

Seems cleaner, no?

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1647
> +        // FIXME: Implement faster fill without falling back to path rendering.

This patch makes me think CGContextFillEllipseInRect will never support gradients:
https://bugs.webkit.org/attachment.cgi?id=95657&action=prettypatch

According to CGContext reference its only fills with solid color. So this FIXME belongs in CG rather then here :-)

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1651
> +        Path path;
> +        path.addEllipse(ellipse);
> +        fillPath(path);
> +        return;

This fallback path should be shared cross-platform, that's why I'm proposing the new design above.

> Source/WebCore/rendering/svg/RenderSVGCircle.cpp:40
> +: RenderSVGShape(node)

Indentation, 4 spaces please.

> Source/WebCore/rendering/svg/RenderSVGCircle.cpp:57
> +    SVGCircleElement* circle = static_cast<SVGCircleElement*>(node());
> +    ASSERT(circle);

I'd generalize this for both circle & ellipse elements.
Introduce a calculateRadiiAndCentroid() method, that either takes these values from a circle or ellipse element, you can check via hasTagName.

> Source/WebCore/rendering/svg/RenderSVGCircle.cpp:127
> +    float distanceSquared = (point - m_centroid).diagonalLengthSquared();
> +    return distanceSquared <= (m_radius * m_radius);

That's faster than "return (point - m_centroid).diagonalLength() <= radius". If this is not hot in profiles, I'd revert this.
Assuming we'l never have negative radii, this is an equivalent comparison.

> LayoutTests/ChangeLog:7
> +

You could add some more context here.

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