[webkit-reviews] review denied: [Bug 80423] New renderer for SVGCircleElement : [Attachment 131108] Updated patch to head so it should apply cleanly

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Philip Rogers
<pdr at google.com>'s request for review:
Bug 80423: New renderer for SVGCircleElement
https://bugs.webkit.org/show_bug.cgi?id=80423

Attachment 131108: Updated patch to head so it should apply cleanly
https://bugs.webkit.org/attachment.cgi?id=131108&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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.