[webkit-reviews] review denied: [Bug 46052] SVG: Remove "create" methods and use port-specific "add" counterparts : [Attachment 70431] Proposed patch v6

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Andreas Kling <kling at webkit.org>'s
request for review:
Bug 46052: SVG: Remove "create" methods and use port-specific "add"
counterparts
https://bugs.webkit.org/show_bug.cgi?id=46052

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70431&action=review

I'm not sure if we should add an ASSERT to all toClipPath as well as toPathData
if the path is empty. (not sure how platforms implemented it, but they should
check the number of segments internally). Maybe we can trust the programmer to
never call this functions with a none empty path ;-)

These are just some notes.

> WebCore/platform/graphics/Path.cpp:112
> +void Path::addRoundedRect(const FloatRect& rectangle, const FloatSize&
roundingRadii)

You should use rect instead of rectangle for the variable, at least if you
change the function name.

> WebCore/platform/graphics/Path.cpp:117
>      float x = rectangle.x();
>      float y = rectangle.y();
>      float width = rectangle.width();
>      float height = rectangle.height();

Why not FloatRect rect(rectangle) so maybe leave the name above.

> WebCore/platform/graphics/Path.cpp:119
>      float rx = roundingRadii.width();
>      float ry = roundingRadii.height();

Can't we use FloatSize radius here?

If we copy all data, we might just use
void Path::addRoundedRect(FloatRect rect, FloatSize roundingRadii) instead of
void Path::addRoundedRect(const FloatRect& rectangle, const FloatSize&
roundingRadii). What do you think?

> WebCore/platform/graphics/Path.cpp:126
> +    float dx = rx;
> +    float dy = ry;
> +    float halfWidth = width / 2;
> +    float halfHeight = height / 2;

Maybe we can use FloatSize for the 4 values as well.