[webkit-reviews] review granted: [Bug 126147] Support <box> values parsing on 'clip-path' property : [Attachment 219894] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Dirk Schulze <krit at webkit.org>'s
request for review:
Bug 126147: Support <box> values parsing on 'clip-path' property
https://bugs.webkit.org/show_bug.cgi?id=126147

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=219894&action=review


> Source/WebCore/ChangeLog:13
> +	   http://dev.w3.org/fxtf/masking/#the-clip-path

Could you refer to a specific revision of the specification you used to
implement
so that we know exactly what the reference point was should the spec change in
the future.

> Source/WebCore/css/CSSParser.cpp:5929
> +    if (!shapeFound && value->unit == CSSParserValue::Function) {
> +	   RefPtr<CSSBasicShape> shapeValue = parseBasicShape();
> +	   if (!shapeValue)
> +	       return nullptr;
> +	   list->append(cssValuePool().createValue(shapeValue.release()));
> +    } else if (shapeFound && (isBoxValue(valueId) || valueId ==
CSSValueBoundingBox)) {
> +	   list->append(parseValidPrimitive(valueId, value));
> +	   m_valueList->next();

It seems strange to repeat this code given shape and box cannot appear twice.
I would refer an approach where we have a loop and guarantee that we run it at
most twice,
and making sure that shape & box are parsed at most once.