[webkit-reviews] review denied: [Bug 124620] [css shapes] Parse new ellipse shape syntax : [Attachment 218124] 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 124620: [css shapes] Parse new ellipse shape syntax
https://bugs.webkit.org/show_bug.cgi?id=124620

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

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


Overall great patch. Still some requests. r-

> Source/WebCore/css/BasicShapeFunctions.cpp:114
> +    return 0;

Is this even necessary? The compiler should be smart enough to detect that the
switch needs to run though all types, no?

> Source/WebCore/css/BasicShapeFunctions.cpp:250
> +    if (radius) {

could you make an early return instead?

if (!radius)
    return BasicShapeRadius(BasicShapeRadius::ClosestSide);

> Source/WebCore/css/BasicShapeFunctions.cpp:259
> +		   break;

This would be a return as well, but still more readable IMO.

> Source/WebCore/css/CSSBasicShapes.cpp:191
> +    result.reserveCapacity((sizeof(opening) - 1) + (4 * (sizeof(separator) -
1)) + 1 + radiusX.length() + radiusY.length() + sizeof(at) + centerX.length() +
centerY.length());

I don't think that this is particular useful. I had this discussion with Bear
before IIRC. StringBuilder should already be quite fast. This complicates the
code more.

> Source/WebCore/css/CSSParser.cpp:5574
> +    for (CSSParserValue* argument = args->current(); argument; argument =
args->next()) {

Didn't we use while loops in the past? I don't particularly care but

while(argument) {
   argument = args->next();
}
seems more readable. Guess it is a matter of taste. You can keep it.

> Source/WebCore/css/CSSParser.cpp:5593
> +	   if (argument->id == CSSValueAt) {

This could be an early return and a comment that we assume an at value now.

// At this point we expect an 'at'
if (argument->id != CSSValueAt)
    return 0;

> Source/WebCore/css/CSSParser.cpp:5598
> +	       if (centerX && centerY) {

This could be an early return:

if (!centerX || !centerY)
    return 0;

> Source/WebCore/rendering/shapes/Shape.cpp:173
> +	   // FIXME implement layout. bug 124619

Change

// FIXME implement layout. bug 124619

to

// FIXME: Layout implementation needed. See bug
https://bugs.webkit.org/show_bug.cgi?id=124619

> Source/WebCore/rendering/shapes/Shape.cpp:174
> +	   shape = createRectangleShape(FloatRect(0, 0, boxWidth, boxHeight),
FloatSize(0, 0));

s/FloatSize(0, 0)/FloatSize()/ (IIRC FloatSize defaults to 0,0)

> Source/WebCore/rendering/style/BasicShapes.cpp:186
> +    // FIXME Complete implementation of path. Bug 124619.

// FIXME Complete implementation of path. Bug 124619.

// FIXME: The implementation of path is incomplete. See
https://bugs.webkit.org/show_bug.cgi?id=124619

Though I am not sure what that actually means. We do not support creating clip
paths with this patch? So -webkit-clip-path: would support the new syntax, but
the path is not created? If so, can you state that and create a new bug for
that please? 124619 is not about paths.

> Source/WebCore/rendering/style/BasicShapes.cpp:204
> +	   radiusY * 2
> +    ));

braces in the same line.

> LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:-33
> -PASS getCSSText("-webkit-shape-inside", "circle(at 10px)") is "circle(at
10px 50%)"
> -PASS getComputedStyleValue("-webkit-shape-inside", "circle(at 10px)") is
"circle(closest-side at 10px 50%)"

Why removing these passes?

> LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:-133
> -PASS getCSSText("-webkit-shape-inside", "ellipse()") is ""
> -PASS getComputedStyleValue("-webkit-shape-inside", "ellipse()") is "auto"
> -PASS getCSSText("-webkit-shape-inside", "ellipse(10px)") is ""
> -PASS getComputedStyleValue("-webkit-shape-inside", "ellipse(10px)") is
"auto"

Ditto.

> LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt:-33
> -PASS getCSSText("-webkit-shape-outside", "circle(at 10px)") is "circle(at
10px 50%)"
> -PASS getComputedStyleValue("-webkit-shape-outside", "circle(at 10px)") is
"circle(closest-side at 10px 50%)"

Ditto.

> LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt:-131
> -PASS getCSSText("-webkit-shape-outside", "ellipse()") is ""
> -PASS getComputedStyleValue("-webkit-shape-outside", "ellipse()") is "auto"
> -PASS getCSSText("-webkit-shape-outside", "ellipse(10px)") is ""
> -PASS getComputedStyleValue("-webkit-shape-outside", "ellipse(10px)") is
"auto"

Ditto.

> LayoutTests/fast/shapes/parsing/parsing-test-utils.js:-24
> -    ["circle(at 10px)", "circle(at 10px 50%)", "circle(closest-side at 10px
50%)"],

Ah, duplication. I take the question back.

> LayoutTests/fast/shapes/parsing/parsing-test-utils.js:31
> +    "ellipse(10px, 20px, 30px, 40px)", // FIXME remove with deprecated
ellipse

Sentences please. 
// FIXME: Remove this test once we do not support the deprecated CSS Shapes
syntax anymore.