Move cursor property 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 c0a94a79eb40be3a6a8d5c377305819fd11d7fa5..84d1dd20b834d4715ee0d10d87da5ac531c74651 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -7,10 +7,12 @@
 
 #include "core/StylePropertyShorthand.h"
 #include "core/css/CSSCalculationValue.h"
+#include "core/css/CSSCursorImageValue.h"
 #include "core/css/CSSCustomIdentValue.h"
 #include "core/css/CSSFontFaceSrcValue.h"
 #include "core/css/CSSFontFeatureValue.h"
 #include "core/css/CSSFunctionValue.h"
+#include "core/css/CSSImageSetValue.h"
 #include "core/css/CSSPathValue.h"
 #include "core/css/CSSPrimitiveValueMappings.h"
 #include "core/css/CSSQuadValue.h"
@@ -1844,6 +1846,107 @@ static PassRefPtrWillBeRawPtr<CSSValue> consumeFlexBasis(CSSParserTokenRange& ra
     return consumeLengthOrPercent(range, cssParserMode, ValueRangeNonNegative);
 }
 
+PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::consumeImageSet(CSSParserTokenRange functionArgs)
+{
+    if (functionArgs.atEnd())
+        return nullptr;
+
+    RefPtrWillBeRawPtr<CSSImageSetValue> imageSet = CSSImageSetValue::create();
+
+    while (!functionArgs.atEnd()) {
+        AtomicString urlValue(consumeUrl(functionArgs));
+        if (urlValue.isNull())
+            return nullptr;
+
+        RefPtrWillBeRawPtr<CSSValue> image = createCSSImageValueWithReferrer(urlValue, completeURL(urlValue));
+        imageSet->append(image);
+
+        const CSSParserToken& token = functionArgs.consumeIncludingWhitespace();
+
+        if (token.type() != DimensionToken)
+            return nullptr;
+        ASSERT(token.unitType() == CSSPrimitiveValue::UnitType::Unknown);

[Timothy Loh, 2015/11/20 04:09:07]

This assertion doesn't look right, since we haven't actually checked the unit is
'x' until the line below.

[rwlbuis, 2015/11/20 20:00:21]

Done.

+        if (String(token.value()) != "x")
+            return nullptr;
+        double imageScaleFactor = token.numericValue();
+        if (imageScaleFactor <= 0)
+            return nullptr;
+        imageSet->append(cssValuePool().createValue(imageScaleFactor, CSSPrimitiveValue::UnitType::Number));
+
+        if (functionArgs.atEnd())
+            break;
+
+        if (!consumeCommaIncludingWhitespace(functionArgs))
+            return nullptr;
+    }
+    return imageSet.release();
+}
+
+PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::consumeCursor(CSSParserTokenRange& range)
+{
+    RefPtrWillBeRawPtr<CSSValueList> list = nullptr;
+    while (!range.atEnd()) {
+        RefPtrWillBeRawPtr<CSSValue> image = nullptr;
+        AtomicString uri(consumeUrl(range));
+        if (!uri.isNull()) {
+            image = createCSSImageValueWithReferrer(uri, completeURL(uri));
+        } else if (range.peek().type() == FunctionToken && range.peek().functionId() == CSSValueWebkitImageSet) {
+            image = consumeImageSet(consumeFunction(range));

[Timothy Loh, 2015/11/20 04:09:07]

Doesn't this mean that any invalid -webkit-image-set(..) is just dropped instead
of making us fail parsing? Maybe we should move the consumeFunction call to
consumeImageSet.

[rwlbuis, 2015/11/20 20:00:21]

I think I see what you mean. I guess we really need to return nullptr if
consumeImageSet fails. But as you said that would be a behavior change.

[Timothy Loh, 2015/12/02 23:59:10]

Err this wouldn't be a behavior change, currently we don't consume anything for
an invalid -webkit-image-set. We could make it just return nullptr if
consumeImageSet fails, since at that point we'll never succeed anyway, but I'm
not sure if that'll be good for composition in consumeImage.

+            if (!image)
+                break;
+        } else {
+            break;
+        }
+
+        double num;
+        IntPoint hotSpot(-1, -1);
+        bool hotSpotSpecified = false;
+        if (consumeNumberRaw(range, num)) {
+            hotSpot.setX(int(num));
+            if (!consumeNumberRaw(range, num))
+                return nullptr;
+            hotSpot.setY(int(num));
+            hotSpotSpecified = true;
+        }
+
+        if (!list)
+            list = CSSValueList::createCommaSeparated();
+
+        if (image)

[Timothy Loh, 2015/11/20 04:09:07]

looks like this is always true

[rwlbuis, 2015/11/20 20:00:21]

Done.

+            list->append(CSSCursorImageValue::create(image, hotSpotSpecified, hotSpot));
+
+        if (!consumeCommaIncludingWhitespace(range))
+            return nullptr;
+    }

[Timothy Loh, 2015/11/20 04:09:06]

blank line after here might be nice.

[rwlbuis, 2015/11/20 20:00:20]

Done.

+    CSSValueID id = range.peek().id();
+    if (!range.atEnd() && m_context.useCounter()) {
+        if (id == CSSValueWebkitZoomIn)
+            m_context.useCounter()->count(UseCounter::PrefixedCursorZoomIn);
+        else if (id == CSSValueWebkitZoomOut)
+            m_context.useCounter()->count(UseCounter::PrefixedCursorZoomOut);
+    }
+    if (list) {

[Timothy Loh, 2015/11/20 04:09:07]

Can we remove the copy-paste and fix the allowing random junk here? e.g.

RefPtrWillBeRawPtr<CSSValue> cursorType;
if (inQuirksMode() && id == CSSValueHand) {
    // Non-standard behavior
    cursorType = cssValuePool().createIdentifierValue(CSSValuePointer);
    range.consumeIncludingWhitespace();
} else if ((id >= CSSValueAuto && id <= CSSValueWebkitZoomOut) || id ==
CSSValueCopy || id == CSSValueNone) {
    cursorType = consumeIdent(range);
} else {
    return nullptr;
}

if (!list)
    return cursorType.release();
list->append(cursorType.release());
return list.release();

[rwlbuis, 2015/11/20 20:00:21]

Done.

+        if (range.atEnd())
+            return nullptr;
+        id = range.consumeIncludingWhitespace().id();
+        if (inQuirksMode() && id == CSSValueHand) // MSIE 5 compatibility :/
+            list->append(cssValuePool().createIdentifierValue(CSSValuePointer));
+        else if ((id >= CSSValueAuto && id <= CSSValueWebkitZoomOut) || id == CSSValueCopy || id == CSSValueNone)
+            list->append(cssValuePool().createIdentifierValue(id));
+        return list.release();

[Timothy Loh, 2015/11/20 04:09:06]

If you didn't notice, we accept "cursor: url(a), bla" with this (and the
current) code, this is the "random junk" I mentioned above.

[Timothy Loh, 2015/11/20 06:56:25]

On the other hand, maybe it's better to keep behaviour changes like this
separate? In any case, I think we should remove the copy-paste.

[rwlbuis, 2015/11/20 20:00:20]

I kept the behavior as-is but removed the copy-paste.

+    }
+    if (!range.atEnd()) {
+        id = range.consumeIncludingWhitespace().id();
+        if (inQuirksMode() && id == CSSValueHand) // MSIE 5 compatibility :/
+            return cssValuePool().createIdentifierValue(CSSValuePointer);
+        if ((id >= CSSValueAuto && id <= CSSValueWebkitZoomOut) || id == CSSValueCopy || id == CSSValueNone)
+            return cssValuePool().createIdentifierValue(id);
+        return nullptr;
+    }
+    ASSERT_NOT_REACHED();
+    return nullptr;
+}
+
 PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSPropertyID unresolvedProperty)
 {
     CSSPropertyID property = resolveCSSPropertyID(unresolvedProperty);
@@ -2009,6 +2112,8 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSProperty
     case CSSPropertyFlexGrow:
     case CSSPropertyFlexShrink:
         return consumeNumber(m_range, ValueRangeNonNegative);
+    case CSSPropertyCursor:
+        return consumeCursor(m_range);
     default:
         return nullptr;
     }