Bug 124426 – [CSS Shapes] Parse [<box> || <shape>] values

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

[Bear Travis]

A shape value can be accompanied by an optional box value, which modifies the coordinate box the shape uses to position and size itself.
http://dev.w3.org/csswg/css-shapes/#shape-outside-property

[Bear Travis]

Created attachment 217069 [details]
Initial Patch

[EFL EWS Bot]

Comment on attachment 217069 [details]
Initial Patch

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

[Build Bot]

Comment on attachment 217069 [details]
Initial Patch

Attachment 217069 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/24298001

[EFL EWS Bot]

Comment on attachment 217069 [details]
Initial Patch

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

[Build Bot]

Comment on attachment 217069 [details]
Initial Patch

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

New failing tests:
css3/masking/clip-path-animation.html
fast/shapes/shape-inside/shape-inside-animation.html
fast/shapes/parsing/parsing-shape-lengths.html
fast/shapes/css-shapes-enabled.html
fast/shapes/shape-inside/shape-inside-calc-crash.html
fast/shapes/shape-outside-floats/shape-outside-animation.html
fast/shapes/parsing/parsing-shape-outside.html
fast/shapes/parsing/parsing-shape-inside.html
fast/masking/parsing-clip-path-shape.html

[Build Bot]

Created attachment 217078 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[kov's GTK+ EWS bot]

Comment on attachment 217069 [details]
Initial Patch

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

[Bear Travis]

Created attachment 217084 [details]
Update for CSS Variables

[Build Bot]

Comment on attachment 217084 [details]
Update for CSS Variables

Attachment 217084 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/23608223

[Bear Travis]

Created attachment 217092 [details]
Adding Compile Guard

[Dirk Schulze]

Comment on attachment 217092 [details]
Adding Compile Guard

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

Looks great already. Some snippets. r- because of need for more negative tests.

> Source/WebCore/css/BasicShapeFunctions.cpp:44
> +    switch (box) {
> +    case BasicShape::ContentBox: return cssValuePool().createIdentifierValue(CSSValueContentBox);

indentation looks wrong, but the bots didn't fire. Did it change recently?

> Source/WebCore/css/BasicShapeFunctions.cpp:60
> +    switch (value->getValueID()) {
> +    case CSSValueContentBox: return BasicShape::ContentBox;

Ditto.

> Source/WebCore/css/CSSBasicShapes.cpp:128
> +    result.reserveCapacity(sizeof(opening) + 1 + 2 * sizeof(separator) + x.length() + y.length() + radius.length() + (box.length() ? box.length() + 1 : 0));

weird. In the past you didn't need to reserve space upfront, even if it might save some time though.

> Source/WebCore/css/CSSBasicShapes.cpp:174
> +        || (m_box && m_box->hasVariableReference());

the braces seem to be unnecessary.

> Source/WebCore/css/CSSParser.cpp:5682
> +static inline bool isBoxValue(CSSValueID valueId)

isn't static enough?

> Source/WebCore/css/CSSParser.cpp:5696
> +PassRefPtr<CSSValue> CSSParser::parseShapeProperty(CSSPropertyID propId)

This looks much more complicated than the original code... just because of a new box value?

> Source/WebCore/css/CSSParser.cpp:5707
> +        || (valueId == CSSValueOutsideShape && propId == CSSPropertyWebkitShapeInside)) {

braces again.

> Source/WebCore/css/CSSParser.cpp:5735
> +            shapeValue = parseBasicShape();
> +        } else if (shapeValue && isBoxValue(valueId)) {
> +            keywordValue = parseValidPrimitive(valueId, value);
> +            m_valueList->next();

Is parseBasicShape() calling next() on valueList already?

> Source/WebCore/css/CSSParser.cpp:5771
>      m_valueList->next();

ok, it is.

> LayoutTests/ChangeLog:10
> +        Test that <box> <shape> and <shape> <box> values are both supported and successfully parsed.
> +        Currently, we order the parsed result as <shape> <box> when the value is output through
> +        the CSSOM.

Could you add some more negative tests like adding box value to <url> and so please?

[Bear Travis]

Created attachment 217220 [details]
Incorporating Feedback

[Bear Travis]

(In reply to comment #11)

Thanks for the feedback! I've worked to incorporate your feedback into the updated patch.

> > Source/WebCore/css/BasicShapeFunctions.cpp:44
> > +    switch (box) {
> > +    case BasicShape::ContentBox: return cssValuePool().createIdentifierValue(CSSValueContentBox);
> 
> indentation looks wrong, but the bots didn't fire. Did it change recently?

Indentation of the case labels is correct, but the case statements have all been inlined, which may be why this looked odd. In the updated patch, each case statement is on its own line, so it may now look more like a typical switch statement.
http://www.webkit.org/coding/coding-style.html#indentation-case-label 

> > Source/WebCore/css/CSSBasicShapes.cpp:128
> > +    result.reserveCapacity(sizeof(opening) + 1 + 2 * sizeof(separator) + x.length() + y.length() + radius.length() + (box.length() ? box.length() + 1 : 0));
> 
> weird. In the past you didn't need to reserve space upfront, even if it might save some time though.

You don't need to here, either. It was an optimization present in the previous code, and I just added the logic for the new box argument. I can remove the optimization if it seems like we'd be better off without it.

> > Source/WebCore/css/CSSBasicShapes.cpp:174
> > +        || (m_box && m_box->hasVariableReference());
> 
> the braces seem to be unnecessary.

I added these for clarity, though the code does not technically require them. In this case, it also matches the code style above it, and so I have left it in place. There doesn't appear to be a rule for it in the style guidelines, but there is precedent in the examples they give:
http://www.webkit.org/coding/coding-style.html#indentation-wrap-bool-op
Personally, I feel like they make the code more readable and have therefore left them in. If you feel strongly about them, though, I will remove them.

> > Source/WebCore/css/CSSParser.cpp:5682
> > +static inline bool isBoxValue(CSSValueID valueId)
> 
> isn't static enough?

I've removed 'inline' from the definition here. The compiler can inline it if it wants to.

> > Source/WebCore/css/CSSParser.cpp:5696
> > +PassRefPtr<CSSValue> CSSParser::parseShapeProperty(CSSPropertyID propId)
> 
> This looks much more complicated than the original code... just because of a new box value?

Yes. The new syntax is roughly an unordered shorthand, and therefore you may now have:
<box>
<box> <shape>
<shape>
<shape> <box>
The code is trickier because you may see <box> or <shape> and then need to determine if it is followed by a valid modifier. This seemed like the cleanest factoring to me.

> > Source/WebCore/css/CSSParser.cpp:5735
> > +            shapeValue = parseBasicShape();
> > +        } else if (shapeValue && isBoxValue(valueId)) {
> > +            keywordValue = parseValidPrimitive(valueId, value);
> > +            m_valueList->next();
> 
> Is parseBasicShape() calling next() on valueList already?
> ok, it is.

Definitely not my favorite aspect that some parse functions advance the parser, while some do not.

> Could you add some more negative tests like adding box value to <url> and so please?

Done.

[Bear Travis]

(In reply to comment #11)
> > Source/WebCore/css/CSSBasicShapes.cpp:128
> > +    result.reserveCapacity(sizeof(opening) + 1 + 2 * sizeof(separator) + x.length() + y.length() + radius.length() + (box.length() ? box.length() + 1 : 0));
> 
> weird. In the past you didn't need to reserve space upfront, even if it might save some time though.

Just saw this again in context. I broke the ellipse and circle string methods out into longer versions because I felt the one line version would have been a little much. I followed the template for rectangle and polygon, which reserve the appropriate capacity as an optimization. This doesn't have to, I was just using the methods that were previously written as a template.

Do you think that reserving this capacity up front helps or maybe isn't worth it?

[Dirk Schulze]

(In reply to comment #13)

> 
> > > Source/WebCore/css/CSSBasicShapes.cpp:128
> > > +    result.reserveCapacity(sizeof(opening) + 1 + 2 * sizeof(separator) + x.length() + y.length() + radius.length() + (box.length() ? box.length() + 1 : 0));
> > 
> > weird. In the past you didn't need to reserve space upfront, even if it might save some time though.
> 
> You don't need to here, either. It was an optimization present in the previous code, and I just added the logic for the new box argument. I can remove the optimization if it seems like we'd be better off without it.

Well, do we have performance tests showing that this is really more efficient? Since all functions are static and still needs to be calculated on every call, it does not need to be a big win but complicates the source code. Can you add create a simple perf test and bring this discussion up on webkit-dev? I am ok with leaving it in for now.

> 
> > > Source/WebCore/css/CSSBasicShapes.cpp:174
> > > +        || (m_box && m_box->hasVariableReference());
> > 
> > the braces seem to be unnecessary.
> 
> I added these for clarity, though the code does not technically require them. In this case, it also matches the code style above it, and so I have left it in place. There doesn't appear to be a rule for it in the style guidelines, but there is precedent in the examples they give:
> http://www.webkit.org/coding/coding-style.html#indentation-wrap-bool-op
> Personally, I feel like they make the code more readable and have therefore left them in. If you feel strongly about them, though, I will remove them.

We have these guidelines for consistent code across WebKit. I rather think that the line break styling already create enough readability and would be in favor for removing them.

> 
> > > Source/WebCore/css/CSSParser.cpp:5696
> > > +PassRefPtr<CSSValue> CSSParser::parseShapeProperty(CSSPropertyID propId)
> > 
> > This looks much more complicated than the original code... just because of a new box value?
> 
> Yes. The new syntax is roughly an unordered shorthand, and therefore you may now have:
> <box>
> <box> <shape>
> <shape>
> <shape> <box>
> The code is trickier because you may see <box> or <shape> and then need to determine if it is followed by a valid modifier. This seemed like the cleanest factoring to me.

Yes, I read the spec again after I wrote the above lines. Fine with me.

[Dirk Schulze]

Comment on attachment 217220 [details]
Incorporating Feedback

Patch looks great, but please look at the change requests on the previous post. r+

[Bear Travis]

Created attachment 217330 [details]
Updated patch

[Bear Travis]

(In reply to comment #15)
 
> Well, do we have performance tests showing that this is really more efficient? Since all functions are static and still needs to be calculated on every call, it does not need to be a big win but complicates the source code. Can you add create a simple perf test and bring this discussion up on webkit-dev? I am ok with leaving it in for now.

I'm removing the reserve capacity code for the new string methods and will save performance experimentation for a subsequent patch. (Bug 124604)

> We have these guidelines for consistent code across WebKit. I rather think that the line break styling already create enough readability and would be in favor for removing them.

I've gone ahead and removed the instances of these introduced in this code.

[Bear Travis]

Comment on attachment 217330 [details]
Updated patch

Forgot to update reviewer.

[EFL EWS Bot]

Comment on attachment 217330 [details]
Updated patch

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

[Bear Travis]

Created attachment 217332 [details]
Adding reviewer to changelogs

[Bear Travis]

Created attachment 217333 [details]
Parenthesizing or branches

Turns out EFL enforces a warning-as-error check on parentheses around && conditions inside an || condition, so I'm adding these back in.

[WebKit Commit Bot]

Comment on attachment 217333 [details]
Parenthesizing or branches

Clearing flags on attachment: 217333

Committed r159526: <http://trac.webkit.org/changeset/159526>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.