Bug 73989 - Refactor CSSParser::parseFontFaceSrc()
Summary: Refactor CSSParser::parseFontFaceSrc()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenichi Ishibashi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-07 01:14 PST by Kenichi Ishibashi
Modified: 2011-12-07 21:38 PST (History)
8 users (show)

See Also:


Attachments
Patch (8.24 KB, patch)
2011-12-07 02:21 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V1 (14.27 KB, patch)
2011-12-07 18:34 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2011-12-07 01:14:16 PST
Some reviewers suggested me to refactor CSSParser::parseFontFaceSrc() when I fixed bug 64783 because the function is somewhat hard to understand.
Comment 1 Kenichi Ishibashi 2011-12-07 02:21:56 PST
Created attachment 118192 [details]
Patch
Comment 2 Kenichi Ishibashi 2011-12-07 02:46:59 PST
Cc'ed reviewers I can see from git blame. Also Cc'ed rniwa and morritta who suggested me to refactor the code.
Comment 3 Darin Adler 2011-12-07 09:19:23 PST
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.
Comment 4 Kenichi Ishibashi 2011-12-07 18:34:30 PST
Created attachment 118304 [details]
Patch V1
Comment 5 Kenichi Ishibashi 2011-12-07 18:36:17 PST
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.
Comment 6 WebKit Review Bot 2011-12-07 21:38:45 PST
Comment on attachment 118304 [details]
Patch V1

Clearing flags on attachment: 118304

Committed r102313: <http://trac.webkit.org/changeset/102313>
Comment 7 WebKit Review Bot 2011-12-07 21:38:50 PST
All reviewed patches have been landed.  Closing bug.