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

[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 70238: Proposed patch v2
https://bugs.webkit.org/attachment.cgi?id=70238&action=review

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

Good first try, looking forward to the next version.

> WebCore/platform/graphics/Path.cpp:121
>      if (width <= 0.0f || height <= 0.0f)

Please use if (width <= 0 || height <= 0).
If there are more cases of ".f" postfixes in this file, please kill them.

> WebCore/platform/graphics/Path.cpp:133
>      if (dy > height * 0.5f)
>	   dy = height * 0.5f;

s/0.5f/0.5/

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

QUARTER has an evil name, please rename it to something sensible, and use the
"g" prefix.

> WebCore/platform/graphics/Path.cpp:164
>      if (width <= 0.0 || height <= 0.0)

if (width <= 0 || height <= 0)

> WebCore/platform/graphics/Path.cpp:204
> +#if 0

No #if 0 code please remove it alltogether, or is there a difference between
addRect and addRectangle?

> WebCore/platform/graphics/Path.h:147
> +	   void addRoundedRectangle(const FloatRect&, const FloatSize&
roundingRadii);

Can you move them below the existing add methods?

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:706
> +    for (unsigned i = 0; i < rectCount; i++) {

While you're at it s/i++/++i/

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1282
> +    {

Why do you wrap this in braces?

> WebCore/rendering/RenderBoxModelObject.cpp:1756
> +		   Path path;

Declare the Path path;	outside of hasBorderRadius and...

> WebCore/rendering/RenderBoxModelObject.cpp:1767
> +		   Path path;

... reuse it hear (with a path.clear() before)...

> WebCore/rendering/RenderBoxModelObject.cpp:1776
> +		   Path path;

...here...

> WebCore/rendering/RenderBoxModelObject.cpp:1780
> +		   Path path;

...and here...

> WebCore/svg/SVGCircleElement.cpp:131
> +    path.addEllipse(FloatRect(cx().value(this) - radius,

I wouldn't wrap the lines here.

> WebCore/svg/SVGEllipseElement.cpp:135
> +    float radiusX = rx().value(this);

Micro optimization: immediately check if (radiusX <= 0) return;.
No need to call ry().value() in that case.

> WebCore/svg/SVGEllipseElement.cpp:141
> +    path.addEllipse(FloatRect(cx().value(this) - radiusX,

I wouldn't wrap the lines here.

> WebCore/svg/SVGRectElement.cpp:154
>      FloatRect rect(x().value(this), y().value(this), width().value(this),
height().value(this));
>  
> +    if (rect.width() <= 0 || rect.height() <= 0)

This is flawed, please use:
float widthValue = width().value(this);
if (widthValue <= 0)
    return;

float heightValue = height().value(this);
if (heightValue <= 0)
    return;

float xValue = x().value(this);
float yValue = y().value(this);

> WebCore/svg/SVGRectElement.cpp:160
>	   float _rx = hasRx ? rx().value(this) : ry().value(this);

if (hasRx || hasRy) {
    float rxValue = rx().value(this);
    float ryValue = ry().value(this);
    if (!hasRx)
	rxValue = ryValue;
    else if (!hasRy)
	ryValue = rxValue;
....
}
This should be sightly faster.

> WebCore/svg/SVGStyledTransformableElement.cpp:110
>  Path SVGStyledTransformableElement::toClipPath() const

toClipPath should also take a Path& parameter.

> WebCore/svg/SVGStyledTransformableElement.h:50
> +    virtual void toPathData(Path&) const {}

Please use { } instead of {}.