[webkit-reviews] review granted: [Bug 63270] Refactor creation of primitive values in CSSParser : [Attachment 98378] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tony Chang <tony at chromium.org>'s
request for review:
Bug 63270: Refactor creation of primitive values in CSSParser
https://bugs.webkit.org/show_bug.cgi?id=63270

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=98378&action=review

> Source/WebCore/css/CSSParser.cpp:716
> +inline PassRefPtr<CSSPrimitiveValue>
CSSParser::createPrimitiveNumericValue(CSSParserValue* value)
> +{
> +    return primitiveValueCache()->createValue(value->fValue,
static_cast<CSSPrimitiveValue::UnitTypes>(value->unit));
> +}
> +
> +inline PassRefPtr<CSSPrimitiveValue>
CSSParser::createPrimitiveStringValue(const String& value)
> +{
> +    return primitiveValueCache()->createValue(value,
CSSPrimitiveValue::CSS_STRING);
> +}

Since these functions are private to this .cpp file they should be marked
static to give them internal linkage. Even an inline function can be either
internal or external linkage, believe it or not, and external linkage is the
default. Adding static is the best practice.

I don’t understand why in the case of StringValue we pass the string, but in
the case of NumericValue we pass the CSSParserValue*.

A problem with createPrimitiveNumericValue is that it moves the assumption that
we can get at fValue and that we can cast the unit to a CSSPrimitiveValue unit
type away from the if statements that make this safe. That makes the code
riskier than it was before, even though it's tidier.

Is there a way to add an assertion to createPrimitiveNumericValue that checks
that the value is one where fValue is used, not iValue, string, or function? I
really worry about this use of the union getting farther away from the type
check that ensures it’s valid to look at that value in the union.

Is there a way to add an assertion that value->unit is one of the values that
is a CSSPrimitiveValue unit type?

I think that CSSParserValue itself should probably be turned into a class that
can guard against incorrect access and typecasting and use functions rather
than direct access to the data members, especially the ones in the union.