Bug 124618 – [css shapes] Parse new circle shape syntax

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

[Bem Jones-Bey]

Implement the parsing of the new circle shape syntax.

[Bem Jones-Bey]

Created attachment 217454 [details]
Patch

[EFL EWS Bot]

Comment on attachment 217454 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 217454 [details]
Patch

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

[Bem Jones-Bey]

The build failures are all because ASSERT_UNREACHED() doesn't do anything in a release build (And I only did a debug build locally). I've fixed it locally by initializing the variable in question, and will upload a new patch with that change (and any potential review feedback) in a bit.

[Build Bot]

Comment on attachment 217454 [details]
Patch

Attachment 217454 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/30508009

[Antti Koivisto]

Comment on attachment 217454 [details]
Patch

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

ok though i don't really see how the new syntax is better

> Source/WebCore/css/BasicShapeFunctions.cpp:85
> +        return pool.createValue(center.length(), style);

Not really in this patch but we need to get rid of CSSValues that take style. The factoring is just totally wrong. CSSValues are the input for constructing RenderStyle!

> Source/WebCore/css/CSSBasicShapes.cpp:130
> +    char opening[] = "circle(";
> +    char at[] = "at";
> +    char separator[] = " ";
> +    StringBuilder result;
> +    // Compute the required capacity in advance to reduce allocations.
> +    result.reserveCapacity((sizeof(opening) - 1) + (3 * (sizeof(separator) - 1)) + 1 + radius.length() + sizeof(at) + centerX.length() + centerY.length());

There are probably some more modern ways to do this though I don't know what the recommended way is currently.

[kov's GTK+ EWS bot]

Comment on attachment 217454 [details]
Patch

Attachment 217454 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/30108016

[Bem Jones-Bey]

Created attachment 217460 [details]
Updated Patch

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 217460 [details]
Updated Patch

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

> Source/WebCore/css/CSSBasicShapes.cpp:177
> +        m_centerY ? m_centerY->serializeResolvingVariables(variables) : String())

, (comma) instead of the last closing parenthesis.

[EFL EWS Bot]

Comment on attachment 217460 [details]
Updated Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 217460 [details]
Updated Patch

Attachment 217460 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/30658011

[Bem Jones-Bey]

(In reply to comment #9)
> (From update of attachment 217460 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217460&action=review
> 
> > Source/WebCore/css/CSSBasicShapes.cpp:177
> > +        m_centerY ? m_centerY->serializeResolvingVariables(variables) : String())
> 
> , (comma) instead of the last closing parenthesis.

Thanks. I was about to ask why it compiled locally, then I realized that CSS_VARIABLES is turned off on the Mac port.

[Bem Jones-Bey]

Created attachment 217462 [details]
Patch

Third time's the charm?

[EFL EWS Bot]

Comment on attachment 217462 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 217462 [details]
Patch

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

[Bem Jones-Bey]

Created attachment 217471 [details]
Patch

Different warning settings on different ports make me a sad panda. Hopefully this one will pass all the EWS.

[WebKit Commit Bot]

Comment on attachment 217471 [details]
Patch

Clearing flags on attachment: 217471

Committed r159585: <http://trac.webkit.org/changeset/159585>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 217471 [details]
Patch

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

> Source/WebCore/css/CSSBasicShapes.cpp:130
> +    result.reserveCapacity((sizeof(opening) - 1) + (3 * (sizeof(separator) - 1)) + 1 + radius.length() + sizeof(at) + centerX.length() + centerY.length());

I’m not sure this optimization actually works.

The sizeof(at) part seems wrong.

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

This needs to use appendLiteral.

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

This needs to use appendLiteral.

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

This needs to use appendLiteral.

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

This needs to use appendLiteral.

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

This needs to use appendLiteral.

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

This needs to use appendLiteral.

> Source/WebCore/css/CSSBasicShapes.cpp:145
> +    if (box.length()) {

This should be !box.isEmpty()

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

This needs to use appendLiteral.

[Bem Jones-Bey]

(In reply to comment #19)
> (From update of attachment 217471 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217471&action=review
> 
> > Source/WebCore/css/CSSBasicShapes.cpp:130
> > +    result.reserveCapacity((sizeof(opening) - 1) + (3 * (sizeof(separator) - 1)) + 1 + radius.length() + sizeof(at) + centerX.length() + centerY.length());
> 
> I’m not sure this optimization actually works.
> 
> The sizeof(at) part seems wrong.
> 
> > Source/WebCore/css/CSSBasicShapes.cpp:131
> > +    result.append(opening);
> 
> This needs to use appendLiteral.
> 
> > Source/WebCore/css/CSSBasicShapes.cpp:137
> > +            result.append(separator);
> 
> This needs to use appendLiteral.
> 
> > Source/WebCore/css/CSSBasicShapes.cpp:138
> > +        result.append(at);
> 
> This needs to use appendLiteral.
> 
> > Source/WebCore/css/CSSBasicShapes.cpp:139
> > +        result.append(separator);
> 
> This needs to use appendLiteral.
> 
> > Source/WebCore/css/CSSBasicShapes.cpp:141
> > +        result.append(separator);
> 
> This needs to use appendLiteral.
> 
> > Source/WebCore/css/CSSBasicShapes.cpp:144
> > +    result.append(")");
> 
> This needs to use appendLiteral.
> 
> > Source/WebCore/css/CSSBasicShapes.cpp:145
> > +    if (box.length()) {
> 
> This should be !box.isEmpty()
> 
> > Source/WebCore/css/CSSBasicShapes.cpp:146
> > +        result.append(separator);
> 
> This needs to use appendLiteral.

I'm going to create a patch to implement the layout behavior. I'll make these changes in that patch.