[webkit-reviews] review denied: [Bug 124621] [css shapes] layout for new ellipse syntax : [Attachment 218295] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Rob Buis <rwlbuis at gmail.com>'s
request for review:
Bug 124621: [css shapes] layout for new ellipse syntax
https://bugs.webkit.org/show_bug.cgi?id=124621

Attachment 218295: Patch
https://bugs.webkit.org/attachment.cgi?id=218295&action=review

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


Some snippets.

> Source/WebCore/ChangeLog:8
> +	   Implement support for doing layout with the new circle shape syntax,


This patch is doing ellipse, not circle.

> Source/WebCore/ChangeLog:18
> +	   (WebCore::BasicShapeEllipse::floatValueForRadiusInBox): helper
function to calculate

s/helper/Helper/ :)

> Source/WebCore/rendering/style/BasicShapes.cpp:64
> +    if (type() == BasicShape::BasicShapeEllipseType) {
> +	   const BasicShapeEllipse* thisEllipse = static_cast<const
BasicShapeEllipse*>(this);

early return here:

if (type() != BasicShape::BasicShapeEllipseType)
    return true;

> Source/WebCore/rendering/style/BasicShapes.cpp:213
> +    // If radius.type() == BasicShapeRadius::FarthestSide.

Can you assert that?
ASSERT(radius.type() == BasicShapeRadius::FarthestSide)

instead of the comment

> LayoutTests/ChangeLog:25
> +	   *
fast/shapes/shape-outside-floats/shape-outside-floats-ellipse-000-expected.txt:
Added.
> +	   *
fast/shapes/shape-outside-floats/shape-outside-floats-ellipse-000.html: Added.

I mean it shows something. But could you still try to follow the other tests
and make it a ref test please?

> LayoutTests/animations/resources/animation-test-helpers.js:222
> -	   matches =
s.match("ellipse\\((.*)\\s*,\\s*(.*)\\s*,\\s*(.*)\\,\\s*(.*)\\)");
> +	   matches =
s.match("ellipse\\((.*)\\s+(.*)\\s+at\\s+(.*)\\s+(.*)\\)");

Something that bugs me on this and Bems patch (but didn't realize it until
now): We do not test the old syntax anymore and don't know if this is still
working. We should discuss if we want to add a deprecated mode to testing as
well.

> LayoutTests/css3/masking/clip-path-ellipse.html:16
>  \ No newline at end of file

argh. Not this line again :)