[webkit-reviews] review granted: [Bug 124619] [css shapes] Layout support for new circle shape syntax : [Attachment 218213] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has granted Bem Jones-Bey <bjonesbe at adobe.com>'s
request for review:
Bug 124619: [css shapes] Layout support for new circle shape syntax
https://bugs.webkit.org/show_bug.cgi?id=124619

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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.