Bug 63270 – Refactor creation of primitive values in CSSParser

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

Refactor creation of primitive values in CSSParser

[Tony Chang]

Created attachment 98378 [details]
Patch

[Tony Chang]

This is a follow up from bug 62050.

[Darin Adler]

Comment on attachment 98378 [details]
Patch

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.

[Eric Seidel (no email)]

Comment on attachment 98378 [details]
Patch

Wow.  That's soooo much better than the code was before.

[Tony Chang]

Created attachment 98419 [details]
Patch

[Tony Chang]

(In reply to comment #3)
> (From update of attachment 98378 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98378&action=review
> 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.

They're member functions because they use primitiveValueCache().  We could pass this in as a param, but it would make the calls much less convenient.  Do you think it's worth it to get internal linkage?

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

Good point. I changed createPrimitiveStringValue to take a 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.

I looked but couldn't seem to find a way to do this verification.

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

Done.

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

Sounds like a good idea, but not something I plan on tackling in the near future.

[Darin Adler]

(In reply to comment #6)
> They're member functions because they use primitiveValueCache().

Oops, I missed that.

> Do you think it's worth it to get internal linkage?

No.

> > 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.
> 
> I looked but couldn't seem to find a way to do this verification.

We at least need some kind of comment. These are not safe functions to call without proper checking of the ID.

I suspect you could do the verification if you had a subset of the logic in CSSParserValue::createCSSValue.

[Tony Chang]

(In reply to comment #7)
> (In reply to comment #6)
> > > 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.
> > 
> > I looked but couldn't seem to find a way to do this verification.
> 
> We at least need some kind of comment. These are not safe functions to call without proper checking of the ID.
> 
> I suspect you could do the verification if you had a subset of the logic in CSSParserValue::createCSSValue.

Looking in CSSParserValue::createCSSValue, it looks like fValue should be valid in the same cases that |unit| is a valid numeric value.  That is, the assert I added in the last patch should also ensure that fValue is valid.

[Darin Adler]

(In reply to comment #8)
> Looking in CSSParserValue::createCSSValue, it looks like fValue should be valid in the same cases that |unit| is a valid numeric value.  That is, the assert I added in the last patch should also ensure that fValue is valid.

OK. Can we do a similar assert for strings?

[Darin Adler]

(In reply to comment #9)
> (In reply to comment #8)
> > Looking in CSSParserValue::createCSSValue, it looks like fValue should be valid in the same cases that |unit| is a valid numeric value.  That is, the assert I added in the last patch should also ensure that fValue is valid.
> 
> OK. Can we do a similar assert for strings?

Looks like you did!

[Tony Chang]

Committed r89680: <http://trac.webkit.org/changeset/89680>