Bug 124619 – [css shapes] Layout support for new circle shape syntax

• Component: CSS
• Status: RESOLVED FIXED

[Bem Jones-Bey]

Support new circle shape syntax in layout

[Bem Jones-Bey]

Created attachment 218213 [details]
Patch

[EFL EWS Bot]

Comment on attachment 218213 [details]
Patch

Attachment 218213 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/42298013

[Dirk Schulze]

Comment on attachment 218213 [details]
Patch

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

r=me but still with some nits.

> Source/WebCore/css/CSSParser.cpp:5460
> +    if (value->id == CSSValueClosestSide || value->id == CSSValueFarthestSide)

Is there a test for the change? I wonder why previous tests didn't detect this.

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

s/if/If/ :)

> Source/WebCore/rendering/style/BasicShapes.h:146
> +        return m_keyword == None && other.keyword() == None;

It would be great if we can investigate if it is possible to interpolate between some keywords. Can you add an FIXME with a link to a bug report please?

> Source/WebCore/rendering/style/BasicShapes.h:179
> +        return m_type == Value && other.type() == Value;

Ditto. Can be the same bug.

> LayoutTests/css3/masking/clip-path-circle-overflow-hidden.html:25
>  \ No newline at end of file

Hm, do we really want that in this file?

> LayoutTests/css3/masking/clip-path-circle-overflow.html:17
>  \ No newline at end of file

Ditto.

> LayoutTests/css3/masking/clip-path-circle-relative-overflow.html:17
>  \ No newline at end of file

Ditto.

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

Ditto.

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

Ditto.

[Bem Jones-Bey]

Comment on attachment 218213 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:5460
>> +    if (value->id == CSSValueClosestSide || value->id == CSSValueFarthestSide)
> 
> Is there a test for the change? I wonder why previous tests didn't detect this.

Because I failed to add a test for this when I wrote the parsing patch. I added a test to the parsing tests for this in this patch.

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

Heh. Applied.

>> Source/WebCore/rendering/style/BasicShapes.h:146
>> +        return m_keyword == None && other.keyword() == None;
> 
> It would be great if we can investigate if it is possible to interpolate between some keywords. Can you add an FIXME with a link to a bug report please?

Ok, will do.

>> LayoutTests/css3/masking/clip-path-circle-overflow-hidden.html:25
>>  \ No newline at end of file
> 
> Hm, do we really want that in this file?

That's not in the file, it was just mentioning that a newline was added at the end of the file when I made my change, because I edited it using Vim , and if there isn't a newline at the end of a file it edits, it adds one. (Or are you saying that it's bad that a newline got added?)

[Bem Jones-Bey]

Created attachment 218220 [details]
Updated Patch

[WebKit Commit Bot]

Comment on attachment 218220 [details]
Updated Patch

Clearing flags on attachment: 218220

Committed r159979: <http://trac.webkit.org/changeset/159979>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.