Move parseFontFaceDescriptor to CSSPropertyParser.cpp

Source/core/css/parser/CSSPropertyParser.cpp

Index: Source/core/css/parser/CSSPropertyParser.cpp
diff --git a/Source/core/css/parser/CSSPropertyParser.cpp b/Source/core/css/parser/CSSPropertyParser.cpp
index 2e6c483da002877dc8980b5b0be85f790357171b..b03689723686ef719395a9b0e00aa86e68611ddf 100644
--- a/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/Source/core/css/parser/CSSPropertyParser.cpp
@@ -7,6 +7,9 @@
 
 #include "core/StylePropertyShorthand.h"
 #include "core/css/CSSCalculationValue.h"
+#include "core/css/CSSFontFaceSrcValue.h"
+#include "core/css/CSSFontFeatureValue.h"
+#include "core/css/CSSUnicodeRangeValue.h"
 #include "core/css/CSSValuePool.h"
 #include "core/css/parser/CSSParserFastPaths.h"
 #include "core/css/parser/CSSParserValues.h"
@@ -91,6 +94,41 @@ static PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumeString(CSSParserTokenRan
     return cssValuePool().createValue(range.consumeIncludingWhitespace().value(), CSSPrimitiveValue::UnitType::String);
 }
 
+static String consumeUrl(CSSParserTokenRange& range)
+{
+    const CSSParserToken& token = range.peek();
+    if (token.type() == UrlToken) {
+        range.consumeIncludingWhitespace();
+        return token.value();
+    }
+    if (token.functionId() == CSSValueUrl) {
+        CSSParserTokenRange urlRange = range;
+        CSSParserTokenRange urlArgs = urlRange.consumeBlock();
+        const CSSParserToken& next = urlArgs.consumeIncludingWhitespace();
+        if (next.type() == BadStringToken || !urlArgs.atEnd())
+            return String();
+        ASSERT(next.type() == StringToken);
+        range = urlRange;
+        range.consumeWhitespace();
+        return next.value();
+    }
+
+    return String();
+}
+
+static CSSParserTokenRange consumeFunction(CSSParserTokenRange& range)
+{
+    ASSERT(range.peek().type() == FunctionToken);
+    CSSParserTokenRange contens = range.consumeBlock();

[Timothy Loh, 2015/09/16 03:22:28]

contens -> contents? :-)

+    range.consumeWhitespace();
+    return contens;
+}
+
+static inline bool isComma(const CSSParserToken& value)
+{
+    return value.type() == CommaToken;
+}
+
 // Methods for consuming non-shorthand properties starts here.
 static PassRefPtrWillBeRawPtr<CSSValue> consumeWillChange(CSSParserTokenRange& range)
 {
@@ -142,6 +180,105 @@ static PassRefPtrWillBeRawPtr<CSSValue> consumeWillChange(CSSParserTokenRange& r
     return values.release();
 }
 
+static PassRefPtrWillBeRawPtr<CSSValue> consumeFontVariantLigatures(CSSParserTokenRange& range)

[Timothy Loh, 2015/09/16 03:22:28]

Can we move the properties which are not @font-face descriptors in a separate
patch? It'll be much easier to review these if the diffs are smaller :)

+{
+    if (range.peek().id() == CSSValueNormal)
+        return consumeIdent(range);
+    RefPtrWillBeRawPtr<CSSValueList> ligatureValues = CSSValueList::createSpaceSeparated();
+    bool sawCommonLigaturesValue = false;
+    bool sawDiscretionaryLigaturesValue = false;
+    bool sawHistoricalLigaturesValue = false;
+    bool sawContextualLigaturesValue = false;
+    do {
+        CSSValueID id = range.peek().id();
+        switch (id) {
+        case CSSValueNoCommonLigatures:
+        case CSSValueCommonLigatures:
+            if (sawCommonLigaturesValue)
+                return nullptr;
+            sawCommonLigaturesValue = true;
+            break;
+        case CSSValueNoDiscretionaryLigatures:
+        case CSSValueDiscretionaryLigatures:
+            if (sawDiscretionaryLigaturesValue)
+                return nullptr;
+            sawDiscretionaryLigaturesValue = true;
+            break;
+        case CSSValueNoHistoricalLigatures:
+        case CSSValueHistoricalLigatures:
+            if (sawHistoricalLigaturesValue)
+                return nullptr;
+            sawHistoricalLigaturesValue = true;
+            break;
+        case CSSValueNoContextual:
+        case CSSValueContextual:
+            if (sawContextualLigaturesValue)
+                return nullptr;
+            sawContextualLigaturesValue = true;
+            break;
+        default:
+            return nullptr;
+        }
+        ligatureValues->append(consumeIdent(range));
+    } while (!range.atEnd());
+
+    if (!ligatureValues->length())

[Timothy Loh, 2015/09/16 03:22:28]

The list will never be empty here

+        return nullptr;
+
+    return ligatureValues.release();
+}
+
+static bool consumeFontFeatureTag(CSSParserTokenRange& range, CSSValueList* settings)

[Timothy Loh, 2015/09/16 03:22:28]

should probably just return a CSSFontFeatureValue and not take a CSSValueList

+{
+    // Feature tag name consists of 4-letter characters.
+    static const unsigned tagNameLength = 4;
+
+    CSSParserToken token = range.peek();

[Timothy Loh, 2015/09/16 03:22:28]

const CSSParserToken&. Also maybe easier if this is just
consumeIncludingWhitespace?

+    // Feature tag name comes first
+    if (token.type() != StringToken)
+        return false;
+    if (token.value().length() != tagNameLength)
+        return false;
+    AtomicString tag = token.value();
+    for (unsigned i = 0; i < tagNameLength; ++i) {
+        // Limits the range of characters to 0x20-0x7E, following the tag name rules defiend in the OpenType specification.
+        UChar character = tag[i];
+        if (character < 0x20 || character > 0x7E)
+            return false;
+    }
+
+    int tagValue = 1;
+    // Feature tag values could follow: <integer> | on | off
+    range.consumeIncludingWhitespace();
+    if (!range.atEnd()) {

[Timothy Loh, 2015/09/16 03:22:28]

don't need this check, peek() will just return EOFToken

+        if (range.peek().type() == NumberToken && range.peek().numericValueType() == IntegerValueType && range.peek().numericValue() >= 0) {
+            tagValue = clampTo<int>(range.peek().numericValue());

[Timothy Loh, 2015/09/16 03:22:28]

range.consumeIncludingWhitespace().numericValue()?

+            if (tagValue < 0)
+                return false;

[Timothy Loh, 2015/09/16 03:22:28]

does this ever get hit?

+            range.consumeIncludingWhitespace();
+        } else if (range.peek().id() == CSSValueOn || range.peek().id() == CSSValueOff) {
+            tagValue = range.peek().id() == CSSValueOn;

[Timothy Loh, 2015/09/16 03:22:28]

range.consumeIncludingWhitespace().id()

+            range.consumeIncludingWhitespace();
+        }
+    }
+    settings->append(CSSFontFeatureValue::create(tag, tagValue));
+    return true;
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeFontFeatureSettings(CSSParserTokenRange& range)

[Timothy Loh, 2015/09/16 03:22:28]

this one too, separate patch

+{
+    if (range.peek().id() == CSSValueNormal)
+        return consumeIdent(range);
+    RefPtrWillBeRawPtr<CSSValueList> settings = CSSValueList::createCommaSeparated();
+    do {
+        if (!consumeFontFeatureTag(range, settings.get()))
+            return nullptr;
+        if (range.atEnd())

[Timothy Loh, 2015/09/16 03:22:28]

I think just return the value outside the loop and not have this check.

+            return settings.release();
+    } while (consumeCommaIncludingWhitespace(range));
+    return nullptr;
+}
+
 static PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumePage(CSSParserTokenRange& range)
 {
     if (range.peek().id() == CSSValueAuto)
@@ -173,6 +310,52 @@ static PassRefPtrWillBeRawPtr<CSSValue> consumeWebkitHighlight(CSSParserTokenRan
     return consumeString(range);
 }
 
+// normal | small-caps | inherit

[Timothy Loh, 2015/09/16 03:22:28]

Comment is wrong, let's just drop these (as well as elsewhere, most of them are
wrong..)

+PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::consumeFontVariant()

[Timothy Loh, 2015/09/16 03:22:28]

static for consistency?

+{
+    RefPtrWillBeRawPtr<CSSValueList> values = nullptr;

[Timothy Loh, 2015/09/16 03:22:28]

Can we rewrite this function so it isn't super complicated? Also would be nice
to comment that "all" is no longer in the spec and having lists of
normal/small-caps is also no longer valid.

+    bool expectComma = false;
+    while (!m_range.atEnd()) {
+        RefPtrWillBeRawPtr<CSSPrimitiveValue> parsedValue = nullptr;
+        if (!expectComma) {
+            expectComma = true;
+            if (m_range.peek().id() == CSSValueNormal || m_range.peek().id() == CSSValueSmallCaps) {
+                parsedValue = consumeIdent(m_range);
+            } else if (m_range.peek().id() == CSSValueAll && !values) {
+                // FIXME: CSSPropertyParser::parseFontVariant() implements
+                // the old css3 draft:
+                // http://www.w3.org/TR/2002/WD-css3-webfonts-20020802/#font-variant
+                // 'all' is only allowed in @font-face and with no other values. Make a value list to
+                // indicate that we are in the @font-face case.
+                values = CSSValueList::createCommaSeparated();
+                parsedValue = consumeIdent(m_range);
+            }
+        } else if (consumeCommaIncludingWhitespace(m_range)) {
+            expectComma = false;
+            continue;
+        }
+
+        if (!parsedValue)
+            return nullptr;
+
+        if (isComma(m_range.peek()))
+            values = CSSValueList::createCommaSeparated();
+
+        if (values)
+            values->append(parsedValue.release());
+        else
+            return parsedValue.release();
+    }
+
+    if (values && values->length()) {
+        if (m_ruleType != StyleRule::FontFace)
+            return nullptr;
+        return values.release();
+    }
+
+    return nullptr;
+}
+
 PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSPropertyID propId)
 {
     m_range.consumeWhitespace();
@@ -185,11 +368,246 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSProperty
         return consumeQuotes(m_range);
     case CSSPropertyWebkitHighlight:
         return consumeWebkitHighlight(m_range);
+    case CSSPropertyFontVariantLigatures:
+        return consumeFontVariantLigatures(m_range);
+    case CSSPropertyWebkitFontFeatureSettings:
+        return consumeFontFeatureSettings(m_range);
+    case CSSPropertyFontVariant:
+        return consumeFontVariant();
+    case CSSPropertyFontFamily:
+        // [[ <family-name> | <generic-family> ],]* [<family-name> | <generic-family>] | inherit
+        return consumeFontFamily();
     default:
         return nullptr;
     }
 }
 
+static PassRefPtrWillBeRawPtr<CSSValueList> consumeFontFaceUnicodeRange(CSSParserTokenRange& range)
+{
+    RefPtrWillBeRawPtr<CSSValueList> values = CSSValueList::createCommaSeparated();
+
+    do {
+        const CSSParserToken& token = range.consumeIncludingWhitespace();
+        if (token.type() != UnicodeRangeToken)
+            return nullptr;
+
+        UChar32 start = token.unicodeRangeStart();
+        UChar32 end = token.unicodeRangeEnd();
+        if (start > end)
+            return nullptr;
+        values->append(CSSUnicodeRangeValue::create(start, end));
+    } while (consumeCommaIncludingWhitespace(range));
+
+    return values.release();
+}
+
+bool CSSPropertyParser::consumeFontFaceSrcURI(CSSValueList* valueList)

[Timothy Loh, 2015/09/16 03:22:28]

return a CSSValue instead of updating a CSSValueList directly

+{
+    String url = consumeUrl(m_range);
+    if (url.isNull())
+        return false;
+    RefPtrWillBeRawPtr<CSSFontFaceSrcValue> uriValue(CSSFontFaceSrcValue::create(completeURL(url), m_context.shouldCheckContentSecurityPolicy()));
+    uriValue->setReferrer(m_context.referrer());
+
+    if (m_range.peek().functionId() != CSSValueFormat) {
+        valueList->append(uriValue.release());
+        return true;
+    }
+
+    // FIXME: http://www.w3.org/TR/2011/WD-css3-fonts-20111004/ says that format() contains a comma-separated list of strings,

[Timothy Loh, 2015/09/16 03:22:28]

Should update the URL (https://drafts.csswg.org/css-fonts). Also maybe a good
idea to make it clearer that IdentTokens are not valid as per spec here.

+    // but CSSFontFaceSrcValue stores only one format. Allowing one format for now.
+    CSSParserTokenRange args = consumeFunction(m_range);
+    if (args.peek().type() != StringToken && args.peek().type() != IdentToken)
+        return false;
+    uriValue->setFormat(args.consumeIncludingWhitespace().value());

[Timothy Loh, 2015/09/16 03:22:28]

need to check for any tokens after the string?

+    valueList->append(uriValue.release());
+    return true;
+}
+
+bool CSSPropertyParser::consumeFontFaceSrcLocal(CSSValueList* valueList)

[Timothy Loh, 2015/09/16 03:22:28]

return a CSSValue

+{
+    CSSParserTokenRange args = consumeFunction(m_range);
+    if (args.atEnd())
+        return false;
+
+    const CSSParserToken& arg = args.consumeIncludingWhitespace();

[Timothy Loh, 2015/09/16 03:22:28]

This is just <family-name>, right? We should share the logic with font-family as
much as possible (the font-family property will need to check the
<generic-family> idents but is otherwise just a simple commas separated list).
The logic for font-family below seems super convoluted to me.

+    ContentSecurityPolicyDisposition shouldCheckContentSecurityPolicy = m_context.shouldCheckContentSecurityPolicy();
+    if (arg.type() == StringToken) {
+        if (!args.atEnd())
+            return false;
+        valueList->append(CSSFontFaceSrcValue::createLocal(arg.value(), shouldCheckContentSecurityPolicy));
+    } else if (arg.type() == IdentToken) {
+        StringBuilder builder;
+        builder.append(arg.value());
+        while (!args.atEnd()) {
+            CSSParserToken localValue = args.consumeIncludingWhitespace();
+            if (localValue.type() != IdentToken)
+                return false;
+            if (!builder.isEmpty())
+                builder.append(' ');
+            builder.append(localValue.value());
+        }
+        valueList->append(CSSFontFaceSrcValue::createLocal(builder.toString(), shouldCheckContentSecurityPolicy));
+    } else {
+        return false;
+    }
+
+    return true;
+}
+
+PassRefPtrWillBeRawPtr<CSSValueList> CSSPropertyParser::consumeFontFaceSrc()
+{
+    RefPtrWillBeRawPtr<CSSValueList> values(CSSValueList::createCommaSeparated());
+
+    do {
+        const CSSParserToken& token = m_range.peek();
+        if (token.functionId() == CSSValueLocal) {
+            if (!consumeFontFaceSrcLocal(values.get()))
+                return nullptr;
+        } else if (!consumeFontFaceSrcURI(values.get())) {
+            return nullptr;
+        }
+
+        if (m_range.atEnd())
+            return values.release();
+    } while (consumeCommaIncludingWhitespace(m_range));
+    return nullptr;
+}
+
+static inline bool isCSSWideKeyword(const CSSParserToken& token)
+{
+    return token.id() == CSSValueInitial || token.id() == CSSValueInherit || token.id() == CSSValueUnset || token.id() == CSSValueDefault;

[Timothy Loh, 2015/09/16 03:22:28]

I know id() has a cache but can we pull the value into a local first? Also this
probably deserves to be higher up in the file.

+}
+
+PassRefPtrWillBeRawPtr<CSSValueList> CSSPropertyParser::consumeFontFamily()
+{
+    RefPtrWillBeRawPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
+    CSSParserToken token = m_range.consumeIncludingWhitespace();
+
+    FontFamilyValueBuilder familyBuilder(list.get());
+    bool inFamily = false;
+
+    while (token.type() != EOFToken) {
+        const CSSParserToken& nextToken = m_range.consumeIncludingWhitespace();
+        bool nextValBreaksFont = nextToken.type() == EOFToken || isComma(nextToken);
+        bool nextValIsFontName = ((nextToken.id() >= CSSValueSerif && nextToken.id() <= CSSValueWebkitBody)
+            || (nextToken.type() == StringToken || nextToken.type() == IdentToken));
+
+        if (isCSSWideKeyword(token) && !inFamily) {
+            if (nextValBreaksFont)
+                return nullptr;
+            if (nextValIsFontName)
+                token = nextToken;
+            continue;
+        }
+
+        if (token.id() >= CSSValueSerif && token.id() <= CSSValueWebkitBody) {
+            if (inFamily) {
+                familyBuilder.add(token.value());
+            } else if (nextValBreaksFont || !nextValIsFontName) {
+                list->append(cssValuePool().createIdentifierValue(token.id()));
+            } else {
+                familyBuilder.commit();
+                familyBuilder.add(token.value());
+                inFamily = true;
+            }
+        } else if (token.type() == StringToken) {
+            // Strings never share in a family name.
+            inFamily = false;
+            familyBuilder.commit();
+            list->append(cssValuePool().createFontFamilyValue(token.value()));
+        } else if (token.type() == IdentToken) {
+            if (inFamily) {
+                familyBuilder.add(token.value());
+            } else if (nextValBreaksFont || !nextValIsFontName) {
+                list->append(cssValuePool().createFontFamilyValue(token.value()));
+            } else {
+                familyBuilder.commit();
+                familyBuilder.add(token.value());
+                inFamily = true;
+            }
+        } else {
+            break;
+        }
+
+        if (nextToken.type() == EOFToken)
+            break;
+
+        if (nextValBreaksFont) {
+            token = m_range.consumeIncludingWhitespace();
+            familyBuilder.commit();
+            inFamily = false;
+        } else if (nextValIsFontName) {
+            token = nextToken;
+        } else {
+            break;
+        }
+    }
+    familyBuilder.commit();
+    if (!list->length() || (m_ruleType == StyleRule::FontFace && list->length() > 1))
+        list = nullptr;
+    return list.release();
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeFontWeight(CSSParserTokenRange& range)
+{
+    const CSSParserToken& token = range.peek();
+    if (token.id() >= CSSValueNormal && token.id() <= CSSValueLighter)
+        return consumeIdent(range);
+    if (token.type() != NumberToken)

[Timothy Loh, 2015/09/16 03:22:28]

|| token.numericValueType() != IntegerValueType?

+        return nullptr;
+    int weight = static_cast<int>(token.numericValue());
+    if ((weight % 100) || weight < 100 || weight > 900)
+        return nullptr;
+    range.consumeIncludingWhitespace();
+    return cssValuePool().createIdentifierValue(static_cast<CSSValueID>(CSSValue100 + weight / 100 - 1));
+}
+
+bool CSSPropertyParser::parseFontFaceDescriptor(CSSPropertyID propId)

[Timothy Loh, 2015/09/16 03:22:28]

I think we decided a while ago to split this into a function that returns a
CSSValue and a function which checks everything is consumed and yields the
property?

+{
+    RefPtrWillBeRawPtr<CSSValue> parsedValue = nullptr;
+
+    m_range.consumeWhitespace();
+    switch (propId) {
+    case CSSPropertyFontFamily:
+        // <family-name>
+        // TODO(rwlbuis): check there is only one family-name
+        parsedValue = consumeFontFamily();
+        break;
+    case CSSPropertySrc: // This is a list of urls or local references.
+        parsedValue = consumeFontFaceSrc();
+        break;
+    case CSSPropertyUnicodeRange:
+        parsedValue = consumeFontFaceUnicodeRange(m_range);
+        break;
+    case CSSPropertyFontWeight: // normal | bold | 100 | 200 | 300 | 400 | 500 | 600 | 700 | 800 | 900
+        parsedValue = consumeFontWeight(m_range);
+        break;
+    case CSSPropertyFontStretch:
+    case CSSPropertyFontStyle: {
+        CSSValueID id = m_range.consumeIncludingWhitespace().id();
+        if (!CSSParserFastPaths::isValidKeywordPropertyAndValue(propId, id))
+            return false;
+        addProperty(propId, cssValuePool().createIdentifierValue(id), false);

[Timothy Loh, 2015/09/16 03:22:28]

parsedValue = ?

+        return true;
+    }
+    case CSSPropertyFontVariant: // normal | small-caps | inherit
+        parsedValue = consumeFontVariant();
+        break;
+    case CSSPropertyWebkitFontFeatureSettings:
+        parsedValue = parseSingleValue(propId);
+        break;
+    default:
+        break;
+    }
+
+    if (!parsedValue || !m_range.atEnd())
+        return false;
+
+    addProperty(propId, parsedValue.release(), false);
+    return true;
+}
+
 bool CSSPropertyParser::parseShorthand(CSSPropertyID propId, bool important)
 {
     m_range.consumeWhitespace();