[webkit-reviews] review denied: [Bug 124426] [CSS Shapes] Parse [<box> || <shape>] values : [Attachment 217092] Adding Compile Guard

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Bear Travis <betravis at adobe.com>'s
request for review:
Bug 124426: [CSS Shapes] Parse [<box> || <shape>] values
https://bugs.webkit.org/show_bug.cgi?id=124426

Attachment 217092: Adding Compile Guard
https://bugs.webkit.org/attachment.cgi?id=217092&action=review

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