Bug 124426

Created attachment 217069 [details] Initial Patch

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

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

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

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

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

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

Created attachment 217084 [details] Update for CSS Variables

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

Created attachment 217092 [details] Adding Compile Guard

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?

Created attachment 217220 [details] Incorporating Feedback

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

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

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

Comment on attachment 217220 [details] Incorporating Feedback Patch looks great, but please look at the change requests on the previous post. r+

Created attachment 217330 [details] Updated patch

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

Comment on attachment 217330 [details] Updated patch Forgot to update reviewer.

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

Created attachment 217332 [details] Adding reviewer to changelogs

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.

Comment on attachment 217333 [details] Parenthesizing or branches Clearing flags on attachment: 217333 Committed r159526: <http://trac.webkit.org/changeset/159526>

All reviewed patches have been landed. Closing bug.