Rework the SVGPathSource interface

Source/core/svg/SVGPathStringSource.cpp

Index: Source/core/svg/SVGPathStringSource.cpp
diff --git a/Source/core/svg/SVGPathStringSource.cpp b/Source/core/svg/SVGPathStringSource.cpp
index 8eb442ad686306fcde2583a9d6fe2a597d197903..069640ead9f61b2356281350276d4ce9c4ebcecc 100644
--- a/Source/core/svg/SVGPathStringSource.cpp
+++ b/Source/core/svg/SVGPathStringSource.cpp
@@ -19,7 +19,6 @@
  */
 
 #include "config.h"
-
 #include "core/svg/SVGPathStringSource.h"
 
 #include "core/svg/SVGParserUtilities.h"
@@ -30,6 +29,8 @@ namespace blink {
 SVGPathStringSource::SVGPathStringSource(const String& string)
     : m_string(string)
     , m_is8BitSource(string.is8Bit())
+    , m_seenError(false)
+    , m_previousCommand(PathSegUnknown)
 {
     ASSERT(!string.isEmpty());
 
@@ -40,6 +41,7 @@ SVGPathStringSource::SVGPathStringSource(const String& string)
         m_current.m_character16 = string.characters16();
         m_end.m_character16 = m_current.m_character16 + string.length();
     }
+    eatWhitespace();
 }
 
 bool SVGPathStringSource::hasMoreData() const
@@ -49,93 +51,65 @@ bool SVGPathStringSource::hasMoreData() const
     return m_current.m_character16 < m_end.m_character16;
 }
 
-bool SVGPathStringSource::moveToNextToken()
+void SVGPathStringSource::eatWhitespace()
 {
     if (m_is8BitSource)
-        return skipOptionalSVGSpaces(m_current.m_character8, m_end.m_character8);
-    return skipOptionalSVGSpaces(m_current.m_character16, m_end.m_character16);
+        skipOptionalSVGSpaces(m_current.m_character8, m_end.m_character8);
+    else
+        skipOptionalSVGSpaces(m_current.m_character16, m_end.m_character16);
 }
 
-template <typename CharacterType>
-static bool parseSVGSegmentTypeHelper(const CharacterType*& current, SVGPathSegType& pathSegType)
+static SVGPathSegType parseSVGSegmentTypeHelper(unsigned lookahead)
 {
-    switch (*(current++)) {
+    switch (lookahead) {
     case 'Z':
     case 'z':
-        pathSegType = PathSegClosePath;
-        break;
+        return PathSegClosePath;
     case 'M':
-        pathSegType = PathSegMoveToAbs;
-        break;
+        return PathSegMoveToAbs;
     case 'm':
-        pathSegType = PathSegMoveToRel;
-        break;
+        return PathSegMoveToRel;
     case 'L':
-        pathSegType = PathSegLineToAbs;
-        break;
+        return PathSegLineToAbs;
     case 'l':
-        pathSegType = PathSegLineToRel;
-        break;
+        return PathSegLineToRel;
     case 'C':
-        pathSegType = PathSegCurveToCubicAbs;
-        break;
+        return PathSegCurveToCubicAbs;
     case 'c':
-        pathSegType = PathSegCurveToCubicRel;
-        break;
+        return PathSegCurveToCubicRel;
     case 'Q':
-        pathSegType = PathSegCurveToQuadraticAbs;
-        break;
+        return PathSegCurveToQuadraticAbs;
     case 'q':
-        pathSegType = PathSegCurveToQuadraticRel;
-        break;
+        return PathSegCurveToQuadraticRel;
     case 'A':
-        pathSegType = PathSegArcAbs;
-        break;
+        return PathSegArcAbs;
     case 'a':
-        pathSegType = PathSegArcRel;
-        break;
+        return PathSegArcRel;
     case 'H':
-        pathSegType = PathSegLineToHorizontalAbs;
-        break;
+        return PathSegLineToHorizontalAbs;
     case 'h':
-        pathSegType = PathSegLineToHorizontalRel;
-        break;
+        return PathSegLineToHorizontalRel;
     case 'V':
-        pathSegType = PathSegLineToVerticalAbs;
-        break;
+        return PathSegLineToVerticalAbs;
     case 'v':
-        pathSegType = PathSegLineToVerticalRel;
-        break;
+        return PathSegLineToVerticalRel;
     case 'S':
-        pathSegType = PathSegCurveToCubicSmoothAbs;
-        break;
+        return PathSegCurveToCubicSmoothAbs;
     case 's':
-        pathSegType = PathSegCurveToCubicSmoothRel;
-        break;
+        return PathSegCurveToCubicSmoothRel;
     case 'T':
-        pathSegType = PathSegCurveToQuadraticSmoothAbs;
-        break;
+        return PathSegCurveToQuadraticSmoothAbs;
     case 't':
-        pathSegType = PathSegCurveToQuadraticSmoothRel;
-        break;
+        return PathSegCurveToQuadraticSmoothRel;
     default:
-        pathSegType = PathSegUnknown;
+        return PathSegUnknown;
     }
-    return true;
-}
-
-bool SVGPathStringSource::parseSVGSegmentType(SVGPathSegType& pathSegType)
-{
-    if (m_is8BitSource)
-        return parseSVGSegmentTypeHelper(m_current.m_character8, pathSegType);
-    return parseSVGSegmentTypeHelper(m_current.m_character16, pathSegType);
 }
 
-template <typename CharacterType>
-static bool nextCommandHelper(const CharacterType*& current, SVGPathSegType previousCommand, SVGPathSegType& nextCommand)
+static bool nextCommandHelper(unsigned lookahead, SVGPathSegType previousCommand, SVGPathSegType& nextCommand)
 {
     // Check for remaining coordinates in the current command.
-    if ((*current == '+' || *current == '-' || *current == '.' || (*current >= '0' && *current <= '9'))
+    if ((lookahead == '+' || lookahead == '-' || lookahead == '.' || (lookahead >= '0' && lookahead <= '9'))
         && previousCommand != PathSegClosePath) {
         if (previousCommand == PathSegMoveToAbs) {
             nextCommand = PathSegLineToAbs;
@@ -148,103 +122,116 @@ static bool nextCommandHelper(const CharacterType*& current, SVGPathSegType prev
         nextCommand = previousCommand;
         return true;
     }
-
     return false;
 }
 
-SVGPathSegType SVGPathStringSource::nextCommand(SVGPathSegType previousCommand)
-{
-    SVGPathSegType nextCommand;
-    if (m_is8BitSource) {
-        if (nextCommandHelper(m_current.m_character8, previousCommand, nextCommand))
-            return nextCommand;
-    } else {
-        if (nextCommandHelper(m_current.m_character16, previousCommand, nextCommand))
-            return nextCommand;
-    }
-
-    parseSVGSegmentType(nextCommand);
-    return nextCommand;
-}
-
-bool SVGPathStringSource::parseMoveToSegment(FloatPoint& targetPoint)
-{
-    if (m_is8BitSource)
-        return parseFloatPoint(m_current.m_character8, m_end.m_character8, targetPoint);
-    return parseFloatPoint(m_current.m_character16, m_end.m_character16, targetPoint);
-}
-
-bool SVGPathStringSource::parseLineToSegment(FloatPoint& targetPoint)
+float SVGPathStringSource::parseNumberWithError()
 {
+    float numberValue = 0;
     if (m_is8BitSource)
-        return parseFloatPoint(m_current.m_character8, m_end.m_character8, targetPoint);
-    return parseFloatPoint(m_current.m_character16, m_end.m_character16, targetPoint);
+        m_seenError |= !parseNumber(m_current.m_character8, m_end.m_character8, numberValue);
+    else
+        m_seenError |= !parseNumber(m_current.m_character16, m_end.m_character16, numberValue);
+    return numberValue;
 }
 
-bool SVGPathStringSource::parseLineToHorizontalSegment(float& x)
+bool SVGPathStringSource::parseArcFlagWithError()
 {
+    bool flagValue = false;
     if (m_is8BitSource)
-        return parseNumber(m_current.m_character8, m_end.m_character8, x);
-    return parseNumber(m_current.m_character16, m_end.m_character16, x);
+        m_seenError |= !parseArcFlag(m_current.m_character8, m_end.m_character8, flagValue);
+    else
+        m_seenError |= !parseArcFlag(m_current.m_character16, m_end.m_character16, flagValue);
+    return flagValue;
 }
 
-bool SVGPathStringSource::parseLineToVerticalSegment(float& y)
+SVGPathSegType SVGPathStringSource::peekSegmentType()
 {
-    if (m_is8BitSource)
-        return parseNumber(m_current.m_character8, m_end.m_character8, y);
-    return parseNumber(m_current.m_character16, m_end.m_character16, y);
+    ASSERT(hasMoreData());
+    // This won't work in all cases because of the state required to "detect" implicit commands.
+    unsigned lookahead = m_is8BitSource ? *m_current.m_character8 : *m_current.m_character16;
+    return parseSVGSegmentTypeHelper(lookahead);
 }
 
-bool SVGPathStringSource::parseCurveToCubicSegment(FloatPoint& point1, FloatPoint& point2, FloatPoint& targetPoint)
+PathSegmentData SVGPathStringSource::parseSegment()
 {
-    if (m_is8BitSource)
-        return parseFloatPoint3(m_current.m_character8, m_end.m_character8, point1, point2, targetPoint);
-    return parseFloatPoint3(m_current.m_character16, m_end.m_character16, point1, point2, targetPoint);
-}
-
-bool SVGPathStringSource::parseCurveToCubicSmoothSegment(FloatPoint& point1, FloatPoint& targetPoint)
-{
-    if (m_is8BitSource)
-        return parseFloatPoint2(m_current.m_character8, m_end.m_character8, point1, targetPoint);
-    return parseFloatPoint2(m_current.m_character16, m_end.m_character16, point1, targetPoint);
-}
-
-bool SVGPathStringSource::parseCurveToQuadraticSegment(FloatPoint& point2, FloatPoint& targetPoint)
-{
-    if (m_is8BitSource)
-        return parseFloatPoint2(m_current.m_character8, m_end.m_character8, point2, targetPoint);
-    return parseFloatPoint2(m_current.m_character16, m_end.m_character16, point2, targetPoint);
-}
-
-bool SVGPathStringSource::parseCurveToQuadraticSmoothSegment(FloatPoint& targetPoint)
-{
-    if (m_is8BitSource)
-        return parseFloatPoint(m_current.m_character8, m_end.m_character8, targetPoint);
-    return parseFloatPoint(m_current.m_character16, m_end.m_character16, targetPoint);
-}
+    ASSERT(hasMoreData());
+    PathSegmentData segment;
+    unsigned lookahead = m_is8BitSource ? *m_current.m_character8 : *m_current.m_character16;
+    SVGPathSegType command = parseSVGSegmentTypeHelper(lookahead);
+    if (command == PathSegUnknown) {
+        // Possibly an implicit command. Not allowed if this is the first command.
+        if (m_previousCommand == PathSegUnknown)
+            return segment;
+        if (!nextCommandHelper(lookahead, m_previousCommand, command))
+            return segment;
+    } else {
+        // Valid explicit command.
+        if (m_is8BitSource)
+            m_current.m_character8++;
+        else
+            m_current.m_character16++;
+    }
 
-template <typename CharacterType>
-static bool parseArcToSegmentHelper(const CharacterType*& current, const CharacterType* end, float& rx, float& ry, float& angle, bool& largeArc, bool& sweep, FloatPoint& targetPoint)
-{
-    float toX;
-    float toY;
-    if (!parseNumber(current, end, rx)
-        || !parseNumber(current, end, ry)
-        || !parseNumber(current, end, angle)
-        || !parseArcFlag(current, end, largeArc)
-        || !parseArcFlag(current, end, sweep)
-        || !parseNumber(current, end, toX)
-        || !parseNumber(current, end, toY))
-        return false;
-    targetPoint = FloatPoint(toX, toY);
-    return true;
-}
+    segment.command = m_previousCommand = command;
+
+    ASSERT(!m_seenError);

[Stephen Chennney, 2015/03/20 19:40:55]

This concerned me because I think of a command that we can't parse as an error
we should somehow report. But I know we don't do this right now (I looked into
it at some point in the past) so I'm OK with moving forward as is.

[fs, 2015/03/23 11:22:09]

Yes, error reporting here is not great (or readable for large input). I have
some ideas for how that could be improved - now I just need to find that
mythical creature known as "time", so I can try it out =)

+
+    switch (segment.command) {
+    case PathSegCurveToCubicRel:
+    case PathSegCurveToCubicAbs:
+        segment.point1.setX(parseNumberWithError());
+        segment.point1.setY(parseNumberWithError());
+        /* fall through */
+    case PathSegCurveToCubicSmoothRel:
+    case PathSegCurveToCubicSmoothAbs:
+        segment.point2.setX(parseNumberWithError());
+        segment.point2.setY(parseNumberWithError());
+        /* fall through */
+    case PathSegMoveToRel:
+    case PathSegMoveToAbs:
+    case PathSegLineToRel:
+    case PathSegLineToAbs:
+    case PathSegCurveToQuadraticSmoothRel:
+    case PathSegCurveToQuadraticSmoothAbs:
+        segment.targetPoint.setX(parseNumberWithError());
+        segment.targetPoint.setY(parseNumberWithError());
+        break;
+    case PathSegLineToHorizontalRel:
+    case PathSegLineToHorizontalAbs:
+        segment.targetPoint.setX(parseNumberWithError());
+        break;
+    case PathSegLineToVerticalRel:
+    case PathSegLineToVerticalAbs:
+        segment.targetPoint.setY(parseNumberWithError());
+        break;
+    case PathSegClosePath:
+        eatWhitespace();
+        break;
+    case PathSegCurveToQuadraticRel:
+    case PathSegCurveToQuadraticAbs:
+        segment.point1.setX(parseNumberWithError());
+        segment.point1.setY(parseNumberWithError());
+        segment.targetPoint.setX(parseNumberWithError());
+        segment.targetPoint.setY(parseNumberWithError());
+        break;
+    case PathSegArcRel:
+    case PathSegArcAbs:
+        segment.point1.setX(parseNumberWithError()); // rx
+        segment.point1.setY(parseNumberWithError()); // ry
+        segment.point2.setX(parseNumberWithError()); // angle
+        segment.arcLarge = parseArcFlagWithError();
+        segment.arcSweep = parseArcFlagWithError();
+        segment.targetPoint.setX(parseNumberWithError());
+        segment.targetPoint.setY(parseNumberWithError());
+        break;
+    case PathSegUnknown:
+        ASSERT_NOT_REACHED();
+    }
 
-bool SVGPathStringSource::parseArcToSegment(float& rx, float& ry, float& angle, bool& largeArc, bool& sweep, FloatPoint& targetPoint)
-{
-    if (m_is8BitSource)
-        return parseArcToSegmentHelper(m_current.m_character8, m_end.m_character8, rx, ry, angle, largeArc, sweep, targetPoint);
-    return parseArcToSegmentHelper(m_current.m_character16, m_end.m_character16, rx, ry, angle, largeArc, sweep, targetPoint);
+    if (UNLIKELY(m_seenError))
+        segment.command = PathSegUnknown;
+    return segment;
 }
 
 } // namespace blink