[webkit-reviews] review granted: [Bug 107231] CSSParser::parseFontFamily should allow the keyword "default" as part of a font name : [Attachment 196627] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Thiago Marcos P. Santos
<tmpsantos at gmail.com>'s request for review:
Bug 107231: CSSParser::parseFontFamily should allow the keyword "default" as
part of a font name
https://bugs.webkit.org/show_bug.cgi?id=107231

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=196627&action=review


> Source/WebCore/ChangeLog:11
> +	   This matches the behavior of Firefox and IE.

Does this also match the specification? If so, please add an URL to the
specific version of the spec. you referenced here.

>>> Source/WebCore/css/CSSParser.cpp:5425
>>> +		     value = nextValue;
>> 
>> I don’t understand this code. Since nextValue is the same as
m_valueList->next(), why does one branch use one and one branch use the other?
> 
> It is not the same thing. m_valueList->next() gets you a different value
every time you call it because it increments the pointer to the current item.
> 
> CSSParserValue* next() { ++m_current; return current(); }
> 
> It was also tricky to me when I saw it for the first time in other parts of
the code.

We really need to rewrite this function. The code is incomphrensible as is.