Move background related longhands into CSSPropertyParser

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/css/parser/CSSPropertyParser.h"
 
 #include "core/StylePropertyShorthand.h"
 #include "core/css/CSSBasicShapeValues.h"
 #include "core/css/CSSBorderImage.h"
 #include "core/css/CSSCalculationValue.h"
@@ skipping 3374 matching lines @@
     if (range.peek().id() == CSSValueFromImage)
         return consumeIdent(range);
     if (range.peek().type() != NumberToken) {
         RefPtrWillBeRawPtr<CSSPrimitiveValue> angle = consumeAngle(range, cssParserMode);
         if (angle && angle->getDoubleValue() == 0)
             return angle;
     }
     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);
     if (CSSParserFastPaths::isKeywordPropertyID(property)) {
         if (!CSSParserFastPaths::isValidKeywordPropertyAndValue(property, m_range.peek().id()))
             return nullptr;
         return consumeIdent(m_range);
     }
     switch (property) {
     case CSSPropertyWillChange:
@@ skipping 143 matching lines @@
     case CSSPropertyWebkitBorderEndColor:
     case CSSPropertyWebkitBorderBeforeColor:
     case CSSPropertyWebkitBorderAfterColor:
     case CSSPropertyWebkitTextStrokeColor:
     case CSSPropertyStopColor:
     case CSSPropertyFloodColor:
     case CSSPropertyLightingColor:
     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:
     case CSSPropertyWebkitBorderBeforeWidth:
     case CSSPropertyWebkitBorderAfterWidth:
         return consumeBorderWidth(m_range, m_context.mode(), UnitlessQuirk::Forbid);
     case CSSPropertyBorderBottomColor:
     case CSSPropertyBorderLeftColor:
     case CSSPropertyBorderRightColor:
     case CSSPropertyBorderTopColor: {
@@ skipping 149 matching lines @@
     case CSSPropertyWebkitBorderImage:
         return consumeWebkitBorderImage(property, m_range, m_context);
     case CSSPropertyWebkitBoxReflect:
         return consumeReflect(m_range, m_context);
     case CSSPropertyFontSizeAdjust:
         ASSERT(RuntimeEnabledFeatures::cssFontSizeAdjustEnabled());
         return consumeFontSizeAdjust(m_range);
     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()) {
             m_valueList = &valueList;
             if (RefPtrWillBeRawPtr<CSSValue> result = legacyParseValue(unresolvedProperty)) {
                 while (!m_range.atEnd())
                     m_range.consume();
                 return result.release();
             }
         }
@@ skipping 774 matching lines @@
         m_currentShorthand = oldShorthand;
         CSSParserValueList valueList(m_range);
         if (!valueList.size())
             return false;
         m_valueList = &valueList;
         return legacyParseShorthand(unresolvedProperty, important);
     }
 }
 
 } // namespace blink