[Webkit-unassigned] [Bug 46052] SVG: Leverage platform's Path::addEllipse() in Path::createEllipse()

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

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




--- Comment #11 from Nikolas Zimmermann <zimmermann at kde.org>  2010-10-10 03:15:10 PST ---
(From update of attachment 70370)
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.

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