Add parseFontFaceDescriptor for @font-face handling

Add parseFontFaceDescriptor for @font-face handling

Add parseFontFaceDescriptor for @font-face handling, similar to
parseViewportProperty. Change FontFace to make it
end up in this new code path too.

With this change using initial/inherit/unset on properties
in @font-face is not possible anymore and all properties
not handled in parseFontFaceProperty are discarded.

This also addresses bug 408484, i.e. using !important
in @font-face causes the property to be dropped.

Behavior matches FireFox.

BUG=404023, 408484
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194903

[rwlbuis, 2015-04-29 22:34:33]

rob.buis@samsung.com changed reviewers:
+ timloh@chromium.org

[rwlbuis, 2015-04-29 22:34:33]

PTAL. V2 is slightly different but requires
https://codereview.chromium.org/1116593003/

I wonder if we can actually get parseFontFaceProperty
out of CSSPropertyParser::parseValue since each time we need
it we are consuming a @font-face rule...

[Timothy Loh, 2015-05-01 04:32:16]

https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:423: if (m_ruleType ==
StyleRule::Viewport) {
This reads weird, and also probably breaks @viewport with the Bison parser. Yes
there have probably been changes that affect it, but we should avoid breaking it
if we can. Can't we just add an else after this if?

https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:7660: bool
CSSPropertyParser::parseFontFaceProperty(CSSPropertyID propId, bool important)
I think this would be cleaner as

PassRefPtrWillBeRawPtr<CSSValue>
CSSPropertyParser::parseFontFaceDescriptorInternal(CSSPropertyID) {
   ..
}

PassRefPtrWillBeRawPtr<CSSValue>
CSSPropertyParser::parseFontFaceDescriptor(CSSPropertyID property) {
    RefPtrWillBeRawPtr<CSSValue> result =
parseFontFaceDescriptorInternal(property);
    if (!result || m_valueList->next())
        return nullptr;
    return result;
}

Also, might as well fix crbug.com/408484 in the same patch.

https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:7671: // TODO: check there is only
one family-name
TODO(rwlbuis)?

https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:7697: default:
FontStretch?

https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:7707: if (!m_valueList->current()
|| inShorthand()) {
No shorthands in here, right?

[rwlbuis, 2015-05-01 22:00:53]

On 2015/04/29 22:34:33, rwlbuis wrote:
> PTAL. V2 is slightly different but requires
> https://codereview.chromium.org/1116593003/
> 
> I wonder if we can actually get parseFontFaceProperty
> out of CSSPropertyParser::parseValue since each time we need
> it we are consuming a @font-face rule...

Answering myself : this seems a bit too hard for now.

[rwlbuis, 2015-05-01 22:04:23]

On 2015/05/01 04:32:16, Timothy Loh wrote:
>
https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:423: if (m_ruleType ==
> StyleRule::Viewport) {
> This reads weird, and also probably breaks @viewport with the Bison parser.
Yes
> there have probably been changes that affect it, but we should avoid breaking
it
> if we can. Can't we just add an else after this if?

Right, that is safer. Fixed.

>
https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:7660: bool
> CSSPropertyParser::parseFontFaceProperty(CSSPropertyID propId, bool important)
> I think this would be cleaner as
> 
> PassRefPtrWillBeRawPtr<CSSValue>
> CSSPropertyParser::parseFontFaceDescriptorInternal(CSSPropertyID) {
>    ..
> }
> 
> PassRefPtrWillBeRawPtr<CSSValue>
> CSSPropertyParser::parseFontFaceDescriptor(CSSPropertyID property) {
>     RefPtrWillBeRawPtr<CSSValue> result =
> parseFontFaceDescriptorInternal(property);
>     if (!result || m_valueList->next())
>         return nullptr;
>     return result;
> }

A good idea and I started to do that. However it would also mean making helper
methods like parseFontWeight return a CSSValue. And making that work seems hard
in this case:
        } else if (!fontWeightParsed && parseFontWeight(important)) {
            fontWeightParsed = true;

So I stuck to the old approach. Note that at least valid_primitive helper is
gone though.

> Also, might as well fix crbug.com/408484 in the same patch.

Done.

>
https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:7671: // TODO: check there is
only
> one family-name
> TODO(rwlbuis)?

Done.

>
https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:7697: default:
> FontStretch?

Added.

>
https://codereview.chromium.org/1103273010/diff/20001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:7707: if (!m_valueList->current()
> || inShorthand()) {
> No shorthands in here, right?

Correct, I removed it.

PTAL:)

[Timothy Loh, 2015-05-04 02:05:18]

Couple of last comments and then we're good to go.

https://codereview.chromium.org/1103273010/diff/60001/Source/core/css/FontFac...
File Source/core/css/FontFace.cpp (left):

https://codereview.chromium.org/1103273010/diff/60001/Source/core/css/FontFac...
Source/core/css/FontFace.cpp:68: return CSSParser::parseSingleValue(propertyID,
s, context);
I think this would look better if we exposed parseFontFaceDescriptor on
CSSParser.

https://codereview.chromium.org/1103273010/diff/60001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1103273010/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:7678: parsedValue =
parseValidPrimitive(id, value);
Maybe better to just createIdentifierValue. Not really a fan of
parseValidPrimitive.

[rwlbuis, 2015-05-04 20:07:13]

On 2015/05/04 02:05:18, Timothy Loh wrote:
> Couple of last comments and then we're good to go.
> 
>
https://codereview.chromium.org/1103273010/diff/60001/Source/core/css/FontFac...
> File Source/core/css/FontFace.cpp (left):
> 
>
https://codereview.chromium.org/1103273010/diff/60001/Source/core/css/FontFac...
> Source/core/css/FontFace.cpp:68: return
CSSParser::parseSingleValue(propertyID,
> s, context);
> I think this would look better if we exposed parseFontFaceDescriptor on
> CSSParser.

Right, much cleaner, fixed.

>
https://codereview.chromium.org/1103273010/diff/60001/Source/core/css/parser/...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1103273010/diff/60001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:7678: parsedValue =
> parseValidPrimitive(id, value);
> Maybe better to just createIdentifierValue. Not really a fan of
> parseValidPrimitive.

Sure, fixed.

PTAL:)

[Timothy Loh, 2015-05-04 23:57:46]

lgtm

[rwlbuis, 2015-05-05 00:42:50]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2015-05-05 00:43:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1103273010/80001

[commit-bot: I haz the power, 2015-05-05 03:46:41]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194903