Bug 63270 - Refactor creation of primitive values in CSSParser
Summary: Refactor creation of primitive values in CSSParser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-23 12:07 PDT by Tony Chang
Modified: 2011-06-24 09:35 PDT (History)
2 users (show)

See Also:


Attachments
Patch (23.10 KB, patch)
2011-06-23 12:09 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (22.68 KB, patch)
2011-06-23 15:24 PDT, Tony Chang
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2011-06-23 12:07:41 PDT
Refactor creation of primitive values in CSSParser
Comment 1 Tony Chang 2011-06-23 12:09:56 PDT
Created attachment 98378 [details]
Patch
Comment 2 Tony Chang 2011-06-23 12:10:21 PDT
This is a follow up from bug 62050.
Comment 3 Darin Adler 2011-06-23 12:30:38 PDT
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.
Comment 4 Eric Seidel (no email) 2011-06-23 12:50:08 PDT
Comment on attachment 98378 [details]
Patch

Wow.  That's soooo much better than the code was before.
Comment 5 Tony Chang 2011-06-23 15:24:04 PDT
Created attachment 98419 [details]
Patch
Comment 6 Tony Chang 2011-06-23 15:47:10 PDT
(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.
Comment 7 Darin Adler 2011-06-23 16:43:40 PDT
(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.
Comment 8 Tony Chang 2011-06-23 16:54:37 PDT
(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.
Comment 9 Darin Adler 2011-06-23 17:05:41 PDT
(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?
Comment 10 Darin Adler 2011-06-23 17:07:49 PDT
(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!
Comment 11 Tony Chang 2011-06-24 09:35:21 PDT
Committed r89680: <http://trac.webkit.org/changeset/89680>