Bug 73989 – Refactor CSSParser::parseFontFaceSrc()

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Kenichi Ishibashi]

Some reviewers suggested me to refactor CSSParser::parseFontFaceSrc() when I fixed bug 64783 because the function is somewhat hard to understand.

[Kenichi Ishibashi]

Created attachment 118192 [details]
Patch

[Kenichi Ishibashi]

Cc'ed reviewers I can see from git blame. Also Cc'ed rniwa and morritta who suggested me to refactor the code.

[Darin Adler]

Comment on attachment 118192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118192&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. Refactoring only.

What existing test covers this parsing? Even if you are just refactoring, we need to make sure we have sufficient coverage for the code being refactored. I’m particularly interested in the test cases that cover trailing commas, for example.

> Source/WebCore/css/CSSParser.cpp:4367
> -static bool isValidFormatFunction(CSSParserValue* val)
> +bool CSSParser::parseFontFaceSrcUri(CSSValueList* valueList)

The acronym is URI, not Uri. Please use the normal capitalization for acronyms rather than turning them into words.

> Source/WebCore/css/CSSParser.cpp:4369
> +    RefPtr<CSSFontFaceSrcValue> uriValue(CSSFontFaceSrcValue::create(m_styleSheet->completeURL(m_valueList->current()->string)));

You removed the FIXME that used to accompany the completeURL call. Why? Was it wrong? Or did you fix it? Or was that just a mistake?

> Source/WebCore/css/CSSParser.cpp:4385
> +    // FIXME: http://www.w3.org/TR/2011/WD-css3-fonts-20111004/ says that format() contains a comma-separated list of string,

This should be "list of strings", not "list of string".

> Source/WebCore/css/CSSParser.cpp:4386
> +    // but CSSFontFaceSrcValue can have only one format. Allowing only one format for now.

I don’t understand what “can have only one format” means.

> Source/WebCore/css/CSSParser.cpp:4445
> -    if (values->length() && !failed) {
> +    if (values->length()) {
>          addProperty(CSSPropertySrc, values.release(), m_important);
>          m_valueList->next();
>          return true;
>      }
> -
>      return false;

I think this would read better as an early exit for the failure case rathe than putting the success case inside the if.

[Kenichi Ishibashi]

Created attachment 118304 [details]
Patch V1

[Kenichi Ishibashi]

Comment on attachment 118192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118192&action=review

Thank you for the review! Addressed your comments.

>> Source/WebCore/ChangeLog:8
>> +        No new tests. Refactoring only.
> 
> What existing test covers this parsing? Even if you are just refactoring, we need to make sure we have sufficient coverage for the code being refactored. I’m particularly interested in the test cases that cover trailing commas, for example.

According to git log, css/url-format-non-string.html and css/uri-token-parsing.html were added (or modified) when some changes were landed, but I couldn't find test cases that cover traling commas. I added a test case.

>> Source/WebCore/css/CSSParser.cpp:4367
>> +bool CSSParser::parseFontFaceSrcUri(CSSValueList* valueList)
> 
> The acronym is URI, not Uri. Please use the normal capitalization for acronyms rather than turning them into words.

Done.

>> Source/WebCore/css/CSSParser.cpp:4369
>> +    RefPtr<CSSFontFaceSrcValue> uriValue(CSSFontFaceSrcValue::create(m_styleSheet->completeURL(m_valueList->current()->string)));
> 
> You removed the FIXME that used to accompany the completeURL call. Why? Was it wrong? Or did you fix it? Or was that just a mistake?

That's just a mistake. re-added the FIXME. Thank you for pointing out.

>> Source/WebCore/css/CSSParser.cpp:4385
>> +    // FIXME: http://www.w3.org/TR/2011/WD-css3-fonts-20111004/ says that format() contains a comma-separated list of string,
> 
> This should be "list of strings", not "list of string".

Done.

>> Source/WebCore/css/CSSParser.cpp:4386
>> +    // but CSSFontFaceSrcValue can have only one format. Allowing only one format for now.
> 
> I don’t understand what “can have only one format” means.

Changed the comment to "stores only one format".

>> Source/WebCore/css/CSSParser.cpp:4445
>>      return false;
> 
> I think this would read better as an early exit for the failure case rathe than putting the success case inside the if.

Done.

[WebKit Review Bot]

Comment on attachment 118304 [details]
Patch V1

Clearing flags on attachment: 118304

Committed r102313: <http://trac.webkit.org/changeset/102313>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.