Bug 78698 – Simplify CSSParser::parseFont.

• Component: New Bugs
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

Simplify CSSParser::parseFont.

[Alexis Menard (darktears)]

Created attachment 127166 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 127169 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127169 [details]
Patch

Attachment 127169 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11515784

New failing tests:
fast/inspector-support/style.html
fast/parser/style-script-head-test.html

[Alexis Menard (darktears)]

Created attachment 127190 [details]
Patch

[WebKit Review Bot]

Attachment 127190 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

Updating OpenSource
git.webkit.org[0: 17.254.20.231]: errno=Connection refused
fatal: unable to connect a socket (Connection refused)
Died at Tools/Scripts/update-webkit line 162.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

Created attachment 136429 [details]
Patch

Rebased version

[Antti Koivisto]

Comment on attachment 136429 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:4314
> +        if (!fontStyleParsed && parseValue(CSSPropertyFontStyle, important))
> +            fontStyleParsed = true;

I think we should avoid calling to generic parseValue() from parseFont(). We already know what property this is so jumping through the huge switch seems like wasted work.

Perhaps we could refactor this so that both parseValue and this can use the same helper functions.

> Source/WebCore/css/CSSParser.cpp:-4594
> -    if (m_valueList->size() != 1)
> -        return false;

Only shorthand properties can have list longer than 1, right?  You could assert for that.

[Alexis Menard (darktears)]

Created attachment 138169 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #7)
> (From update of attachment 136429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136429&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:4314
> > +        if (!fontStyleParsed && parseValue(CSSPropertyFontStyle, important))
> > +            fontStyleParsed = true;
> 
> I think we should avoid calling to generic parseValue() from parseFont(). We already know what property this is so jumping through the huge switch seems like wasted work.

Fixed in the new version.

> 
> Perhaps we could refactor this so that both parseValue and this can use the same helper functions.
> 
> > Source/WebCore/css/CSSParser.cpp:-4594
> > -    if (m_valueList->size() != 1)
> > -        return false;
> 
> Only shorthand properties can have list longer than 1, right?  You could assert for that.

As discussed we don't really want to ASSERT in case of an invalid property, we just want to skip it.

[Antti Koivisto]

Comment on attachment 138169 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 138169 [details]
Patch

Clearing flags on attachment: 138169

Committed r114895: <http://trac.webkit.org/changeset/114895>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.