[SVG] Refactoring accumulate logic for animateMotion

Source/core/svg/SVGAnimateMotionElement.cpp

Index: Source/core/svg/SVGAnimateMotionElement.cpp
diff --git a/Source/core/svg/SVGAnimateMotionElement.cpp b/Source/core/svg/SVGAnimateMotionElement.cpp
index 86fc66111512d3600d54f0a594a948a9bc31b335..861cd61aac8d243cc7d00f61e4b4b8ab223a3189 100644
--- a/Source/core/svg/SVGAnimateMotionElement.cpp
+++ b/Source/core/svg/SVGAnimateMotionElement.cpp
@@ -231,18 +231,6 @@ bool SVGAnimateMotionElement::calculateFromAndByValues(const String& fromString,
     return true;
 }
 
-void SVGAnimateMotionElement::buildTransformForProgress(AffineTransform* transform, float percentage)
-{
-    ASSERT(!m_animationPath.isEmpty());
-
-    bool ok = false;
-    float positionOnPath = m_animationPath.length() * percentage;
-    FloatPoint position = m_animationPath.pointAtLength(positionOnPath, ok);
-    if (!ok)
-        return;
-    transform->translate(position.x(), position.y());
-}
-
 void SVGAnimateMotionElement::calculateAnimatedValue(float percentage, unsigned repeatCount, SVGSMILElement*)
 {
     SVGElement* targetElement = this->targetElement();
@@ -273,19 +261,26 @@ void SVGAnimateMotionElement::calculateAnimatedValue(float percentage, unsigned
         return;
     }
 
-    buildTransformForProgress(transform, percentage);
-
-    // Handle accumulate="sum".
-    if (isAccumulated() && repeatCount) {
-        for (unsigned i = 0; i < repeatCount; ++i)
-            buildTransformForProgress(transform, 1);
-    }
+    ASSERT(!m_animationPath.isEmpty());
 
     bool ok = false;
     float positionOnPath = m_animationPath.length() * percentage;
-    float angle = m_animationPath.normalAngleAtLength(positionOnPath, ok);
+    FloatPoint position;
+    float angle;
+    ok = m_animationPath.pointAndNormalAtLength(positionOnPath, position, angle);
     if (!ok)
         return;
+
+    // Handle accumulate="sum".
+    if (isAccumulated() && repeatCount) {
+        FloatPoint positionAtEndOfDuration;
+        float angleAtEndOfDuration;
+        ok = m_animationPath.pointAndNormalAtLength(m_animationPath.length(), positionAtEndOfDuration, angleAtEndOfDuration);

[pdr., 2013/07/17 20:58:50]

angleAtEndOfDuration doesn't look like it's used. Can you just do:
ok = m_animationPath.pointAtLength(...)?

[pavane, 2013/07/17 21:32:45]

Done

+        if (ok)

[pdr., 2013/07/17 20:58:50]

I think this would be better:
if (!ok)
  return;

What do you think?

[pavane, 2013/07/17 21:32:45]

I don't want it to return here if calculation of 'positionAtEndOfDuration' is
not a success because we still have a valid 'position' calculated above and we
need to translate to that point.

Logic here is if 'positionAtEndOfDuration' is available, add it to 'position'
else just continue with existing 'position'.

Let me know if i am wrong

+            position.move(positionAtEndOfDuration.x() * repeatCount, positionAtEndOfDuration.y() * repeatCount);
+    }
+
+    transform->translate(position.x(), position.y());
     RotateMode rotateMode = this->rotateMode();
     if (rotateMode != RotateAuto && rotateMode != RotateAutoReverse)
         return;