Refactor parsing in SVGTransformList

third_party/WebKit/Source/core/svg/SVGTransformList.cpp

 /*
  * Copyright (C) 2004, 2005, 2008 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007 Rob Buis <buis@kde.org>
  * Copyright (C) 2007 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2008 Apple Inc. All rights reserved.
  * Copyright (C) Research In Motion Limited 2012. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 28 matching lines @@
 SVGTransformList::~SVGTransformList()
 {
 }
 
 PassRefPtrWillBeRawPtr<SVGTransform> SVGTransformList::consolidate()
 {
     AffineTransform matrix;
     if (!concatenate(matrix))
         return SVGTransform::create();
 
-    RefPtrWillBeRawPtr<SVGTransform> transform = SVGTransform::create(matrix);
-    clear();
-    return appendItem(transform);
+    return initialize(SVGTransform::create(matrix));
 }
 
 bool SVGTransformList::concatenate(AffineTransform& result) const
 {
     if (isEmpty())
         return false;
 
     ConstIterator it = begin();
     ConstIterator itEnd = end();
     for (; it != itEnd; ++it)
         result *= it->matrix();
 
     return true;
 }
 
 namespace {
 
 const LChar skewXDesc[] =  {'s', 'k', 'e', 'w', 'X'};
 const LChar skewYDesc[] =  {'s', 'k', 'e', 'w', 'Y'};
 const LChar scaleDesc[] =  {'s', 'c', 'a', 'l', 'e'};
 const LChar translateDesc[] =  {'t', 'r', 'a', 'n', 's', 'l', 'a', 't', 'e'};
 const LChar rotateDesc[] =  {'r', 'o', 't', 'a', 't', 'e'};
 const LChar matrixDesc[] =  {'m', 'a', 't', 'r', 'i', 'x'};
 
 template<typename CharType>
-bool parseAndSkipTransformType(const CharType*& ptr, const CharType* end, SVGTransformType& type)
+SVGTransformType parseAndSkipTransformType(const CharType*& ptr, const CharType* end)
 {
     if (ptr >= end)
-        return false;
+        return SVG_TRANSFORM_UNKNOWN;
 
     if (*ptr == 's') {
         if (skipString(ptr, end, skewXDesc, WTF_ARRAY_LENGTH(skewXDesc)))
-            type = SVG_TRANSFORM_SKEWX;
-        else if (skipString(ptr, end, skewYDesc, WTF_ARRAY_LENGTH(skewYDesc)))
-            type = SVG_TRANSFORM_SKEWY;
-        else if (skipString(ptr, end, scaleDesc, WTF_ARRAY_LENGTH(scaleDesc)))
-            type = SVG_TRANSFORM_SCALE;
-        else
-            return false;
-    } else if (skipString(ptr, end, translateDesc, WTF_ARRAY_LENGTH(translateDesc))) {
-        type = SVG_TRANSFORM_TRANSLATE;
-    } else if (skipString(ptr, end, rotateDesc, WTF_ARRAY_LENGTH(rotateDesc))) {
-        type = SVG_TRANSFORM_ROTATE;
-    } else if (skipString(ptr, end, matrixDesc, WTF_ARRAY_LENGTH(matrixDesc))) {
-        type = SVG_TRANSFORM_MATRIX;
-    } else {
-        return false;
+            return SVG_TRANSFORM_SKEWX;
+        if (skipString(ptr, end, skewYDesc, WTF_ARRAY_LENGTH(skewYDesc)))
+            return SVG_TRANSFORM_SKEWY;
+        if (skipString(ptr, end, scaleDesc, WTF_ARRAY_LENGTH(scaleDesc)))
+            return SVG_TRANSFORM_SCALE;
+
+        return SVG_TRANSFORM_UNKNOWN;
     }
-    return true;
+    if (skipString(ptr, end, translateDesc, WTF_ARRAY_LENGTH(translateDesc)))
+        return SVG_TRANSFORM_TRANSLATE;
+    if (skipString(ptr, end, rotateDesc, WTF_ARRAY_LENGTH(rotateDesc)))
+        return SVG_TRANSFORM_ROTATE;
+    if (skipString(ptr, end, matrixDesc, WTF_ARRAY_LENGTH(matrixDesc)))
+        return SVG_TRANSFORM_MATRIX;
+
+    return SVG_TRANSFORM_UNKNOWN;
 }
 
+// These should be kept in sync with enum SVGTransformType

[f(malita), 2016/01/29 20:51:14]

Nit: in cases like this, I normally add static asserts for all the enum values;
feel free to ignore if it seems like overkill.

[fs, 2016/01/29 21:48:13]

Since these (enums) are web exposed API (and unlikely to be added to...) I don't
think it's very important (and already checked by bindings IIRC.)

That being said, having the "knowledge" locally might be "illustrative" I
suppose. I assume you mean asserts like:

static_assert(SVG_TRANSFORM_UNKNOWN == 0...);
...
static_assert(SVG_TRANSFORM_SKEWY == 6...);
static_assert(WTF_ARRAY_LENGTH(...) == SVG_...);

?

Not sure if there's anything else we could usefully assert here.

[fs, 2016/02/01 13:36:20]

Asserts added.

+const unsigned requiredValuesForType[] =  {0, 6, 1, 1, 1, 1, 1};
+const unsigned optionalValuesForType[] =  {0, 0, 1, 1, 2, 0, 0};
+const unsigned MaxTransformArguments = 6;

[f(malita), 2016/01/29 20:51:14]

Nit: kMaxTransformArguments? Or am I getting them codestyles all mixed up in my
head? :)

[fs, 2016/01/29 21:48:13]

kFoo is "where we're going" (Chromium Style). I suspect using the k-form now
would be ok... (Not 100% if current Blink style says anything specificly in this
case, but this is consistent with enum naming at least...)

[fs, 2016/02/01 13:36:20]

Changed to k-prefixed.

+
+using TransformArguments = Vector<float, MaxTransformArguments>;
+
 template<typename CharType>
-int parseTransformParamList(const CharType*& ptr, const CharType* end, float* values, int required, int optional)
+int parseTransformArgumentsForType(
+    SVGTransformType type,
+    const CharType*& ptr, const CharType* end,
+    TransformArguments& arguments)
 {
-    int parsedParams = 0;
-    int maxPossibleParams = required + optional;
+    const size_t required = requiredValuesForType[type];
+    const size_t optional = optionalValuesForType[type];
+    const size_t maxPossibleParams = required + optional;

[f(malita), 2016/01/29 20:51:14]

Nit: just in case these end up changing -

ASSERT(maxPossibleParams <= MaxTransformArguments);

[fs, 2016/02/01 13:36:20]

Done.

+    ASSERT(arguments.isEmpty());
 
     bool trailingDelimiter = false;
 
-    skipOptionalSVGSpaces(ptr, end);
-    while (parsedParams < maxPossibleParams) {
-        if (!parseNumber(ptr, end, values[parsedParams], DisallowWhitespace))
+    while (arguments.size() < maxPossibleParams) {
+        float argumentValue = 0;
+        if (!parseNumber(ptr, end, argumentValue, AllowLeadingWhitespace))
             break;
 
-        ++parsedParams;
+        arguments.append(argumentValue);
+        trailingDelimiter = false;
+
+        if (arguments.size() == maxPossibleParams)

[f(malita), 2016/01/29 20:51:14]

Aren't we breaking too early here?  It looks like we might be missing a
potential trailing delimiter - e.g. translate(1,1,) would parse with the new
version but not prev one?

(later)
never mind, I see the caller catches this case.

[fs, 2016/01/29 21:48:13]

This was born out of a wish to get better error reporting - without doing too
much surgery... (yes, this is just "a little" =)). There might a little change
in behavior for the other caller though I guess - I think that caller was more
lax already though. (Compare: "1 2 foo" and "1,2,foo" for instance - in the
<animateTransform> caller.) I think I can tighten that up a bit though (require
the trailer to be WS-only.)

[fs, 2016/02/01 13:36:20]

Tightened up SVGTransformList::create(SVGTransformType, const String&) a bit to
address this.

+            break;
 
         if (skipOptionalSVGSpaces(ptr, end) && *ptr == ',') {
             ++ptr;
-            skipOptionalSVGSpaces(ptr, end);
-
             trailingDelimiter = true;
-        } else {
-            trailingDelimiter = false;
         }
     }
 
-    if (trailingDelimiter || !(parsedParams == required || parsedParams == maxPossibleParams))
+    if (trailingDelimiter || !(arguments.size() == required || arguments.size() == maxPossibleParams))
         return -1;
 
-    return parsedParams;
+    return safeCast<int>(arguments.size());
 }
 
-// These should be kept in sync with enum SVGTransformType
-const int requiredValuesForType[] =  {0, 6, 1, 1, 1, 1, 1};
-const int optionalValuesForType[] =  {0, 0, 1, 1, 2, 0, 0};
-
-template<typename CharType>
-PassRefPtrWillBeRawPtr<SVGTransform> parseTransformOfType(unsigned type, const CharType*& ptr, const CharType* end)
+PassRefPtrWillBeRawPtr<SVGTransform> createTransformFromValues(SVGTransformType type, const TransformArguments& arguments)
 {
-    if (type == SVG_TRANSFORM_UNKNOWN)
-        return nullptr;
-
-    int valueCount = 0;
-    float values[] = {0, 0, 0, 0, 0, 0};
-    if ((valueCount = parseTransformParamList(ptr, end, values, requiredValuesForType[type], optionalValuesForType[type])) < 0) {
-        return nullptr;
-    }
-
     RefPtrWillBeRawPtr<SVGTransform> transform = SVGTransform::create();
-
     switch (type) {
     case SVG_TRANSFORM_SKEWX:
-        transform->setSkewX(values[0]);
+        transform->setSkewX(arguments[0]);
         break;
     case SVG_TRANSFORM_SKEWY:
-        transform->setSkewY(values[0]);
+        transform->setSkewY(arguments[0]);
         break;
     case SVG_TRANSFORM_SCALE:
-        if (valueCount == 1) // Spec: if only one param given, assume uniform scaling
-            transform->setScale(values[0], values[0]);
+        // Spec: if only one param given, assume uniform scaling.
+        if (arguments.size() == 1)
+            transform->setScale(arguments[0], arguments[0]);
         else
-            transform->setScale(values[0], values[1]);
+            transform->setScale(arguments[0], arguments[1]);
         break;
     case SVG_TRANSFORM_TRANSLATE:
-        if (valueCount == 1) // Spec: if only one param given, assume 2nd param to be 0
-            transform->setTranslate(values[0], 0);
+        // Spec: if only one param given, assume 2nd param to be 0.
+        if (arguments.size() == 1)
+            transform->setTranslate(arguments[0], 0);
         else
-            transform->setTranslate(values[0], values[1]);
+            transform->setTranslate(arguments[0], arguments[1]);
         break;
     case SVG_TRANSFORM_ROTATE:
-        if (valueCount == 1)
-            transform->setRotate(values[0], 0, 0);
+        if (arguments.size() == 1)
+            transform->setRotate(arguments[0], 0, 0);
         else
-            transform->setRotate(values[0], values[1], values[2]);
+            transform->setRotate(arguments[0], arguments[1], arguments[2]);
         break;
     case SVG_TRANSFORM_MATRIX:
-        transform->setMatrix(AffineTransform(values[0], values[1], values[2], values[3], values[4], values[5]));
+        transform->setMatrix(AffineTransform(arguments[0], arguments[1], arguments[2], arguments[3], arguments[4], arguments[5]));
+        break;
+    case SVG_TRANSFORM_UNKNOWN:
+        ASSERT_NOT_REACHED();
         break;
     }
-
     return transform.release();
 }
 
 } // namespace
 
 template<typename CharType>
 bool SVGTransformList::parseInternal(const CharType*& ptr, const CharType* end)
 {
     clear();
 
     bool delimParsed = false;
-    while (ptr < end) {
+    while (skipOptionalSVGSpaces(ptr, end)) {
         delimParsed = false;
-        SVGTransformType transformType = SVG_TRANSFORM_UNKNOWN;
-        skipOptionalSVGSpaces(ptr, end);
 
-        if (!parseAndSkipTransformType(ptr, end, transformType))
+        SVGTransformType transformType = parseAndSkipTransformType(ptr, end);
+        if (transformType == SVG_TRANSFORM_UNKNOWN)
             return false;
 
         if (!skipOptionalSVGSpaces(ptr, end) || *ptr != '(')
             return false;
         ptr++;
 
-        RefPtrWillBeRawPtr<SVGTransform> transform = parseTransformOfType(transformType, ptr, end);
-        if (!transform)
+        TransformArguments arguments;
+        int valueCount = parseTransformArgumentsForType(transformType, ptr, end, arguments);
+        if (valueCount < 0)
             return false;

[f(malita), 2016/01/29 20:51:14]

Nit:

ASSERT(valueCount >= requiredValuesForType[transformType]);

[fs, 2016/02/01 13:36:20]

Done.

 
         if (!skipOptionalSVGSpaces(ptr, end) || *ptr != ')')
             return false;
         ptr++;
 
-        append(transform.release());
+        append(createTransformFromValues(transformType, arguments));
 
-        skipOptionalSVGSpaces(ptr, end);
-        if (ptr < end && *ptr == ',') {
+        if (skipOptionalSVGSpaces(ptr, end) && *ptr == ',') {
+            ++ptr;
             delimParsed = true;
-            ++ptr;
-            skipOptionalSVGSpaces(ptr, end);
         }
     }
-
     return !delimParsed;
 }
 
 bool SVGTransformList::parse(const UChar*& ptr, const UChar* end)
 {
     return parseInternal(ptr, end);
 }
 
 bool SVGTransformList::parse(const LChar*& ptr, const LChar* end)
 {
     return parseInternal(ptr, end);
 }
 
 SVGTransformType parseTransformType(const String& string)
 {
     if (string.isEmpty())
         return SVG_TRANSFORM_UNKNOWN;
-    SVGTransformType type = SVG_TRANSFORM_UNKNOWN;
     if (string.is8Bit()) {
         const LChar* ptr = string.characters8();
         const LChar* end = ptr + string.length();
-        parseAndSkipTransformType(ptr, end, type);
-    } else {
-        const UChar* ptr = string.characters16();
-        const UChar* end = ptr + string.length();
-        parseAndSkipTransformType(ptr, end, type);
+        return parseAndSkipTransformType(ptr, end);
     }
-    return type;
+    const UChar* ptr = string.characters16();
+    const UChar* end = ptr + string.length();
+    return parseAndSkipTransformType(ptr, end);
 }
 
 String SVGTransformList::valueAsString() const
 {
     StringBuilder builder;
 
     ConstIterator it = begin();
     ConstIterator itEnd = end();
     while (it != itEnd) {
         builder.append(it->valueAsString());
@@ skipping 32 matching lines @@
 }
 
 PassRefPtrWillBeRawPtr<SVGPropertyBase> SVGTransformList::cloneForAnimation(const String& value) const
 {
     ASSERT(RuntimeEnabledFeatures::webAnimationsSVGEnabled());
     return SVGListPropertyHelper::cloneForAnimation(value);
 }
 
 PassRefPtrWillBeRawPtr<SVGTransformList> SVGTransformList::create(SVGTransformType transformType, const String& value)
 {
-    RefPtrWillBeRawPtr<SVGTransform> transform = nullptr;
+    TransformArguments arguments;
+    int valueCount = -1;
     if (value.isEmpty()) {
     } else if (value.is8Bit()) {
         const LChar* ptr = value.characters8();
         const LChar* end = ptr + value.length();
-        transform = parseTransformOfType(transformType, ptr, end);
+        valueCount = parseTransformArgumentsForType(transformType, ptr, end, arguments);
     } else {
         const UChar* ptr = value.characters16();
         const UChar* end = ptr + value.length();
-        transform = parseTransformOfType(transformType, ptr, end);
+        valueCount = parseTransformArgumentsForType(transformType, ptr, end, arguments);
     }
 
     RefPtrWillBeRawPtr<SVGTransformList> svgTransformList = SVGTransformList::create();
-    if (transform)
-        svgTransformList->append(transform);
+    if (valueCount > 0)
+        svgTransformList->append(createTransformFromValues(transformType, arguments));
     return svgTransformList.release();
 }
 
 void SVGTransformList::add(PassRefPtrWillBeRawPtr<SVGPropertyBase> other, SVGElement* contextElement)
 {
     if (isEmpty())
         return;
 
     RefPtrWillBeRawPtr<SVGTransformList> otherList = toSVGTransformList(other);
     if (length() != otherList->length())
         return;
 
     ASSERT(length() == 1);
     RefPtrWillBeRawPtr<SVGTransform> fromTransform = at(0);
     RefPtrWillBeRawPtr<SVGTransform> toTransform = otherList->at(0);
 
     ASSERT(fromTransform->transformType() == toTransform->transformType());
-    clear();
-    append(SVGTransformDistance::addSVGTransforms(fromTransform, toTransform));
+    initialize(SVGTransformDistance::addSVGTransforms(fromTransform, toTransform));
 }
 
 void SVGTransformList::calculateAnimatedValue(SVGAnimationElement* animationElement, float percentage, unsigned repeatCount, PassRefPtrWillBeRawPtr<SVGPropertyBase> fromValue, PassRefPtrWillBeRawPtr<SVGPropertyBase> toValue, PassRefPtrWillBeRawPtr<SVGPropertyBase> toAtEndOfDurationValue, SVGElement* contextElement)
 {
     ASSERT(animationElement);
     bool isToAnimation = animationElement->animationMode() == ToAnimation;
 
     // Spec: To animations provide specific functionality to get a smooth change from the underlying value to the
     // 'to' attribute value, which conflicts mathematically with the requirement for additive transform animations
     // to be post-multiplied. As a consequence, in SVG 1.1 the behavior of to animations for 'animateTransform' is undefined
@@ skipping 41 matching lines @@
     if (at(0)->transformType() == toList->at(0)->transformType())
         return -1;
 
     // Spec: http://www.w3.org/TR/SVG/animate.html#complexDistances
     // Paced animations assume a notion of distance between the various animation values defined by the 'to', 'from', 'by' and 'values' attributes.
     // Distance is defined only for scalar types (such as <length>), colors and the subset of transformation types that are supported by 'animateTransform'.
     return SVGTransformDistance(at(0), toList->at(0)).distance();
 }
 
 } // namespace blink