Bug 124620 – [css shapes] Parse new ellipse shape syntax

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Bem Jones-Bey]

Support parsing the new ellipse shape syntax

[Rob Buis]

(In reply to comment #0)
> Support parsing the new ellipse shape syntax

Hi Bem, I am interested in taking this bug, I hope you don't mind? My patch is far along.

[Rob Buis]

Created attachment 218124 [details]
Patch

[Dirk Schulze]

Comment on attachment 218124 [details]
Patch

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.

[Rob Buis]

Comment on attachment 218124 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218124&action=review

>> 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?

Here I would just like to do the same thing as before my patch, i.e. not change existing behavior for circle.

>> Source/WebCore/css/BasicShapeFunctions.cpp:250
>> +    if (radius) {
> 
> could you make an early return instead?
> 
> if (!radius)
>     return BasicShapeRadius(BasicShapeRadius::ClosestSide);

Fixed.

>> Source/WebCore/css/BasicShapeFunctions.cpp:259
>> +                break;
> 
> This would be a return as well, but still more readable IMO.

Fixed.

>> 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.

Probably correct, removed.

>> 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.

Kept as is.

>> 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;

Fixed.

>> Source/WebCore/css/CSSParser.cpp:5598
>> +            if (centerX && centerY) {
> 
> This could be an early return:
> 
> if (!centerX || !centerY)
>     return 0;

Fixed.

>> 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

Fixed.

>> 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)

Fixed.

>> 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.

You are right, I made a new bug for this.

>> Source/WebCore/rendering/style/BasicShapes.cpp:204
>> +    ));
> 
> braces in the same line.

Fixed.

>> 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.

Ok :)

>> 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.

Fixed, and also the other occurrences.

[Rob Buis]

Created attachment 218185 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 218185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218185&action=review

Hey Rob, I just got back from vacation. Thanks for putting together this patch. I have a few comments myself:

> Source/WebCore/css/BasicShapeFunctions.cpp:103
> +static PassRefPtr<CSSPrimitiveValue> BasicShapeRadiusToCSSValue(const RenderStyle* style, CSSValuePool& pool, const BasicShapeRadius& radius)

This should start with a lower case letter: basicShapeRadiusToCSSValue

> Source/WebCore/css/BasicShapeFunctions.cpp:248
> +static BasicShapeRadius CSSValueToBasicShapeRadius(const RenderStyle* style, const RenderStyle* rootStyle, PassRefPtr<CSSPrimitiveValue> radius)

This should start with a lower case letter: cssValueToBasicShapeRadius.

[Bem Jones-Bey]

(In reply to comment #4)
> (From update of attachment 218124 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218124&action=review
> 
> >> 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?
> 
> Here I would just like to do the same thing as before my patch, i.e. not change existing behavior for circle.

In this case, I'd agree with Dirk and argue for removing that return; I personally think it's better to have the compiler complain if the return is missing rather than have the code quietly do something that isn't correct. (If you do need to add the return 0 to silence the compilet, I'd put an ASSERT_UNREACHED()) before it.

[Bem Jones-Bey]

Comment on attachment 218185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218185&action=review

Missed a couple comments on the first look.

> Source/WebCore/css/CSSBasicShapes.cpp:190
> +    result.append(opening);

You should use appendLiteral here.

> Source/WebCore/css/CSSBasicShapes.cpp:198
> +            result.append(separator);

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:205
> +            result.append(separator);

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:206
> +        result.append(at);

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:207
> +        result.append(separator);

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:209
> +        result.append(separator);

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:212
> +    result.append(")");

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:214
> +        result.append(separator);

Ditto.

[Bem Jones-Bey]

Comment on attachment 218185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218185&action=review

Missed a couple comments on the first look.

> Source/WebCore/css/CSSBasicShapes.cpp:190
> +    result.append(opening);

You should use appendLiteral here.

> Source/WebCore/css/CSSBasicShapes.cpp:198
> +            result.append(separator);

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:205
> +            result.append(separator);

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:206
> +        result.append(at);

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:207
> +        result.append(separator);

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:209
> +        result.append(separator);

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:212
> +    result.append(")");

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:214
> +        result.append(separator);

Ditto.

[Rob Buis]

Created attachment 218195 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 218195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218195&action=review

lgtm, just a nit on a comment.

> Source/WebCore/css/CSSParser.cpp:5569
> +    // ellipse(radius at <position>

Strictly speaking, this should be:
// ellipse(radiusX radiusY)
// ellipse(radiusX radiusY at <position>)

[Dirk Schulze]

Comment on attachment 218195 [details]
Patch

Wit the blessing of Bem: r=me :) But please fix the nits that Bem pointed out.

[Bem Jones-Bey]

(In reply to comment #12)
> (From update of attachment 218195 [details])
> Wit the blessing of Bem: r=me :) But please fix the nits that Bem pointed out.

I told him that I didn't mind if he fixed them in a future patch, so I'm happy if he lands this one as is.

[Rob Buis]

Created attachment 218203 [details]
Patch

[Rob Buis]

Created attachment 218205 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 218205 [details]
Patch

Clearing flags on attachment: 218205

Committed r159954: <http://trac.webkit.org/changeset/159954>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.