Move background related longhands into CSSPropertyParser

third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp

Index: third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
diff --git a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
index fc1fdc76d5aa2c249f6fd463546115b33176e16b..fb53dfeafdb8c560437fe8c3560fb15adf7b4585 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -3392,6 +3392,146 @@ static PassRefPtrWillBeRawPtr<CSSValue> consumeImageOrientation(CSSParserTokenRa
     return nullptr;
 }
 
+static PassRefPtrWillBeRawPtr<CSSValue> consumeBackgroundClip(CSSParserTokenRange& range)
+{
+    return consumeIdent<CSSValueBorderBox, CSSValuePaddingBox, CSSValueContentBox>(range);
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeBackgroundBlendMode(CSSParserTokenRange& range)
+{
+    if (identMatches<CSSValueNormal, CSSValueOverlay>(range.peek().id()))

[Timothy Loh, 2016/02/19 04:11:13]

I think it'd be clearer to not use these helpers here, e.g.

CSSValueKeyword id = range.peek().id();
if (id == CSSValueNormal || id == CSSValueOverlay || (CSSValueMultiply <= id &&
id <= CSSValueLuminosity)
    return consumeIdent(range);
return nullptr;

[rwlbuis, 2016/02/21 18:22:08]

Done.

+        return consumeIdent(range);
+    return consumeIdentRange(range, CSSValueMultiply, CSSValueLuminosity);
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeBackgroundAttachment(CSSParserTokenRange& range)
+{
+    return consumeIdent<CSSValueScroll, CSSValueFixed, CSSValueLocal>(range);
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeBackgroundOrigin(CSSParserTokenRange& range)

[Timothy Loh, 2016/02/19 04:11:13]

Can we merge this and consumeBackgroundClip and name it consumeBox or
consumeBackgroundBox? The spec calls these a <box> type.

https://drafts.csswg.org/css-backgrounds-3/#the-background-clip

[rwlbuis, 2016/02/21 18:22:08]

Done.

+{
+    return consumeIdent<CSSValueBorderBox, CSSValuePaddingBox, CSSValueContentBox>(range);
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeBackgroundComposite(CSSParserTokenRange& range)
+{
+    return consumeIdentRange(range, CSSValueClear, CSSValuePlusLighter);
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeMaskSourceType(CSSParserTokenRange& range)
+{
+    ASSERT(RuntimeEnabledFeatures::cssMaskSourceTypeEnabled());
+    return consumeIdent<CSSValueAuto, CSSValueAlpha, CSSValueLuminance>(range);
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeClipOrOrigin(CSSPropertyID property, CSSParserTokenRange& range, const CSSParserContext& context)

[Timothy Loh, 2016/02/19 04:11:13]

consumePrefixedBackgroundBox?

[rwlbuis, 2016/02/21 18:22:08]

Done.

+{
+    // CSSValueBorder, CSSValuePadding and CSSValueContent are deprecated and do not apply to the version of the property that has the -webkit- prefix removed.

[Timothy Loh, 2016/02/19 04:11:13]

Maybe better to talk about the actual values and not enum values, e.g.

// The values 'border', 'padding' and 'content' ...

[rwlbuis, 2016/02/21 18:22:08]

Done.

+    if (RefPtrWillBeRawPtr<CSSValue> value = consumeIdentRange(range, CSSValueBorder, CSSValuePaddingBox))
+        return value.release();
+    if ((property == CSSPropertyWebkitBackgroundClip || property == CSSPropertyWebkitMaskClip) && range.peek().id() == CSSValueText)
+        return consumeIdent(range);
+    return nullptr;
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeBackgroundSize(CSSPropertyID unresolvedProperty, CSSParserTokenRange& range, CSSParserMode mode)
+{
+    if (identMatches<CSSValueContain, CSSValueCover>(range.peek().id()))
+        return consumeIdent(range);
+
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> horizontal = consumeIdent<CSSValueAuto>(range);
+    if (!horizontal)
+        horizontal = consumeLengthOrPercent(range, mode, ValueRangeAll, UnitlessQuirk::Forbid);
+
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> vertical = nullptr;
+    if (!range.atEnd()) {
+        if (identMatches<CSSValueAuto>(range.peek().id())) // `auto' is the default

[Timothy Loh, 2016/02/19 04:11:13]

Why not just range.peek().id() == CSSValueAuto?

[rwlbuis, 2016/02/21 18:22:08]

Done.

+            range.consumeIncludingWhitespace();
+        else
+            vertical = consumeLengthOrPercent(range, mode, ValueRangeAll, UnitlessQuirk::Forbid);
+    } else if (unresolvedProperty == CSSPropertyAliasWebkitBackgroundSize) {
+        // For backwards compatibility we set the second value to the first if it is omitted.

[Timothy Loh, 2016/02/19 04:11:13]

I don't think this needs to talk about masks here. I'd just write e.g. 

// Legacy syntax: "-webkit-background-size: 10px" is equivalent to
"background-size: 10px 10px"

[rwlbuis, 2016/02/21 18:22:08]

Done.

+        // We only need to do this for -webkit-background-size. It should be safe to let masks match
+        // the real property.
+        vertical = horizontal;
+    }
+    if (!vertical)
+        return horizontal;
+    return CSSValuePair::create(horizontal.release(), vertical.release(), CSSValuePair::KeepIdenticalValues);
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeBackgroundComponent(CSSPropertyID property, CSSParserTokenRange& range, const CSSParserContext& context)

[Timothy Loh, 2016/02/19 04:11:13]

property -> unresolvedProperty

[rwlbuis, 2016/02/21 18:22:08]

Done.

+{
+    switch (property) {
+    case CSSPropertyBackgroundClip:
+        return consumeBackgroundClip(range);
+    case CSSPropertyBackgroundBlendMode:
+        return consumeBackgroundBlendMode(range);
+    case CSSPropertyBackgroundAttachment:
+        return consumeBackgroundAttachment(range);
+    case CSSPropertyBackgroundOrigin:
+        return consumeBackgroundOrigin(range);
+    case CSSPropertyWebkitBackgroundComposite:
+    case CSSPropertyWebkitMaskComposite:
+        return consumeBackgroundComposite(range);
+    case CSSPropertyMaskSourceType:
+        return consumeMaskSourceType(range);
+    case CSSPropertyWebkitBackgroundClip:
+    case CSSPropertyWebkitBackgroundOrigin:
+    case CSSPropertyWebkitMaskClip:
+    case CSSPropertyWebkitMaskOrigin:
+        return consumeClipOrOrigin(property, range, context);
+    case CSSPropertyBackgroundImage:
+    case CSSPropertyWebkitMaskImage:
+        return consumeImage(range, context);
+    case CSSPropertyBackgroundPositionX:
+    case CSSPropertyWebkitMaskPositionX:
+        return consumePositionX(range, context.mode());
+    case CSSPropertyBackgroundPositionY:
+    case CSSPropertyWebkitMaskPositionY:
+        return consumePositionY(range, context.mode());
+    case CSSPropertyBackgroundSize:
+    case CSSPropertyAliasWebkitBackgroundSize:
+    case CSSPropertyWebkitMaskSize:
+        return consumeBackgroundSize(property, range, context.mode());
+    case CSSPropertyBackgroundColor:
+        return consumeColor(range, context.mode());
+    default:
+        break;
+    };
+    return nullptr;
+}
+
+static void addBackgroundValue(RefPtrWillBeRawPtr<CSSValue>& lvalue, PassRefPtrWillBeRawPtr<CSSValue> rvalue)

[Timothy Loh, 2016/02/19 04:11:13]

I think it'd help to write a comment like
// To conserve memory we don't actually wrap a single value in a list

Also maybe better names are list and value? Not sure about list, but I think
it's an improvement.

[rwlbuis, 2016/02/21 18:22:07]

Done.
Right, list is not completely accurate, but probably good enough.

+{
+    if (lvalue) {
+        if (lvalue->isBaseValueList()) {

[Timothy Loh, 2016/02/19 04:11:13]

How about

if (!list->isBaseValueList()) {
    RefPtrWillBeRawPtr<CSSValueList> firstValue = list.release();
    list = CSSValueList::createCommaSeparated();
    list->append(firstValue.release());
}
list->append(value);

[rwlbuis, 2016/02/21 18:22:07]

Done, except that I have to cast to CSSValueList :)

+            toCSSValueList(lvalue.get())->append(rvalue);
+        } else {
+            PassRefPtrWillBeRawPtr<CSSValue> oldlValue(lvalue.release());
+            PassRefPtrWillBeRawPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
+            list->append(oldlValue);
+            list->append(rvalue);
+            lvalue = list;
+        }
+    } else {
+        lvalue = rvalue;
+    }
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeCommaSeparatedBackgroundComponent(CSSPropertyID property, CSSParserTokenRange& range, const CSSParserContext& context)
+{
+    RefPtrWillBeRawPtr<CSSValue> ret = nullptr;

[Timothy Loh, 2016/02/19 04:11:13]

ret -> result

[rwlbuis, 2016/02/21 18:22:07]

Done.

+    do {
+        RefPtrWillBeRawPtr<CSSValue> value = consumeBackgroundComponent(property, range, context);
+        if (!value)
+            return nullptr;
+        addBackgroundValue(ret, value);
+    } while (consumeCommaIncludingWhitespace(range));
+    return ret.release();
+}
+
 PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSPropertyID unresolvedProperty)
 {
     CSSPropertyID property = resolveCSSPropertyID(unresolvedProperty);
@@ -3555,6 +3695,7 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSProperty
     case CSSPropertyWebkitColumnRuleColor:
         return consumeColor(m_range, m_context.mode());
     case CSSPropertyColor:
+    case CSSPropertyBackgroundColor:
         return consumeColor(m_range, m_context.mode(), inQuirksMode());
     case CSSPropertyWebkitBorderStartWidth:
     case CSSPropertyWebkitBorderEndWidth:
@@ -3724,6 +3865,29 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSProperty
     case CSSPropertyImageOrientation:
         ASSERT(RuntimeEnabledFeatures::imageOrientationEnabled());
         return consumeImageOrientation(m_range, m_context.mode());
+    case CSSPropertyBackgroundClip:

[Timothy Loh, 2016/02/19 04:11:13]

Probably looks better sorted ;)

[rwlbuis, 2016/02/21 18:22:08]

Done.

+    case CSSPropertyBackgroundBlendMode:
+    case CSSPropertyBackgroundAttachment:
+    case CSSPropertyBackgroundOrigin:
+    case CSSPropertyWebkitBackgroundComposite:
+    case CSSPropertyWebkitMaskComposite:
+    case CSSPropertyMaskSourceType:
+    case CSSPropertyWebkitBackgroundClip:
+    case CSSPropertyWebkitBackgroundOrigin:
+    case CSSPropertyWebkitMaskClip:
+    case CSSPropertyWebkitMaskOrigin:
+    case CSSPropertyBackgroundImage:
+    case CSSPropertyWebkitMaskImage:
+    case CSSPropertyBackgroundPositionX:
+    case CSSPropertyWebkitMaskPositionX:
+    case CSSPropertyBackgroundPositionY:
+    case CSSPropertyWebkitMaskPositionY:
+    case CSSPropertyBackgroundSize:
+    case CSSPropertyWebkitMaskSize:
+        return consumeCommaSeparatedBackgroundComponent(unresolvedProperty, m_range, m_context);
+    case CSSPropertyWebkitMaskRepeatX:
+    case CSSPropertyWebkitMaskRepeatY:
+        return nullptr;
     default:
         CSSParserValueList valueList(m_range);
         if (valueList.size()) {