[webkit-reviews] review denied: [Bug 46052] SVG: Leverage platform's Path::addEllipse() in Path::createEllipse() : [Attachment 70370] Proposed patch v5

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Andreas Kling
<kling at webkit.org>'s request for review:
Bug 46052: SVG: Leverage platform's Path::addEllipse() in Path::createEllipse()
https://bugs.webkit.org/show_bug.cgi?id=46052

Attachment 70370: Proposed patch v5
https://bugs.webkit.org/attachment.cgi?id=70370&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70370&action=review

Almost there, still giving r- as it's not perfect yet.

> WebCore/ChangeLog:5
> +	   SVG: Leverage platform's Path::addEllipse() in Path::createEllipse()


The bug title should be changed, you're modifying more than addEllipse.

> WebCore/platform/graphics/Path.cpp:80
> +	       static const float rad2deg = 180.0 / piFloat;

Ok, this is unrelated, but please remove thie rad2deg static and use the one in
wtf/MathExtras.h

> WebCore/platform/graphics/Path.cpp:128
> +    if (dx > width * 0.5)

I think it makes sense to declare:
float halfWidth = width / 2;
if (dx > halfWidth)
   dx = halfWidth;

As width * 0.5 is used several times below., Same for height * 0.5 Just a
little optimization.

> WebCore/platform/graphics/Path.cpp:141
> +    addBezierCurveTo(FloatPoint(x + width - dx * (1 - gCircleControlPoint),
y), FloatPoint(x + width, y + dy * (1 - gCircleControlPoint)), FloatPoint(x +
width, y + dy));

If we're always using 1 - gCircleControlPoint, that should go into the static
directly. so it read FloatPoint(x + width - dx * gCircle...

> WebCore/platform/graphics/Path.cpp:161
> +void Path::addRoundedRectangle(const FloatRect& rectangle, const FloatSize&
topLeftRadius, const FloatSize& topRightRadius, const FloatSize&
bottomLeftRadius, const FloatSize& bottomRightRadius)

For consistency, this should be renamed addRoundedRect. Or rename addRect to
addRectangle.

> WebCore/platform/graphics/Path.cpp:183
> +    addLineTo(FloatPoint(x + width, y + height -
bottomRightRadius.height()));

I'd add a newline before this addLineTo line, and split the upper bezier
curveTo in two lines,
addBezierCurveTo(FloatPoint(x + width, ..., y),
				 FloatPoint(x + width, ....),
(aligning the FloatPoints

> WebCore/platform/graphics/Path.cpp:185
> +    addLineTo(FloatPoint(x + bottomLeftRadius.width(), y + height));

Ditto.

> WebCore/platform/graphics/Path.cpp:187
> +    addLineTo(FloatPoint(x, y + topLeftRadius.height()));

Ditto.

> WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:243
> +    {

Why this extra brace?

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:707
> +	   Path path;

How about declaring this before the loop, and using if (i > 0) path.clear()
here ?

> WebCore/rendering/RenderBoxModelObject.cpp:1766
> +	       path.clear();

You only need the clear, if (hasBorderRadius), avoids an unncessary call.

> WebCore/rendering/svg/SVGInlineTextBox.cpp:496
> +    if (fragment.width > 0 && thickness > 0)

I'd change this:
if (fragment.width <= 0 || thickness <= 0)
    return;


And move the float y computation etc. below this check. Early exit if there's
nothing to do.

> WebCore/svg/SVGAnimateMotionElement.cpp:118
> +	       return path;

Evil Path copying.... but not something you should change now.