[SVG] Refactoring accumulate logic for animateMotion

Source/core/svg/SVGAnimateMotionElement.cpp

 /*
  * Copyright (C) 2007 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2007 Rob Buis <buis@kde.org>
  * Copyright (C) 2008 Apple Inc. 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
  * version 2 of the License, or (at your option) any later version.
  *
@@ skipping 213 matching lines @@
     m_hasToPointAtEndOfDuration = false;
     if (animationMode() == ByAnimation && !isAdditive())
         return false;
     parsePoint(fromString, m_fromPoint);
     FloatPoint byPoint;
     parsePoint(byString, byPoint);
     m_toPoint = FloatPoint(m_fromPoint.x() + byPoint.x(), m_fromPoint.y() + byPoint.y());
     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();
     if (!targetElement)
         return;
     AffineTransform* transform = targetElement->supplementalTransform();
     if (!transform)
         return;
 
     if (RenderObject* targetRenderer = targetElement->renderer())
@@ skipping 10 matching lines @@
         float animatedX = 0;
         animateAdditiveNumber(percentage, repeatCount, m_fromPoint.x(), m_toPoint.x(), toPointAtEndOfDuration.x(), animatedX);
 
         float animatedY = 0;
         animateAdditiveNumber(percentage, repeatCount, m_fromPoint.y(), m_toPoint.y(), toPointAtEndOfDuration.y(), animatedY);
 
         transform->translate(animatedX, animatedY);
         return;
     }
 
-    buildTransformForProgress(transform, percentage);
+    ASSERT(!m_animationPath.isEmpty());
+
+    bool ok = false;
+    float positionOnPath = m_animationPath.length() * percentage;
+    FloatPoint position;
+    float angle;
+    ok = m_animationPath.pointAndNormalAtLength(positionOnPath, position, angle);
+    if (!ok)
+        return;
 
     // Handle accumulate="sum".
     if (isAccumulated() && repeatCount) {
-        for (unsigned i = 0; i < repeatCount; ++i)
-            buildTransformForProgress(transform, 1);
+        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);
     }
 
-    bool ok = false;
-    float positionOnPath = m_animationPath.length() * percentage;
-    float angle = m_animationPath.normalAngleAtLength(positionOnPath, ok);
-    if (!ok)
-        return;
+    transform->translate(position.x(), position.y());
     RotateMode rotateMode = this->rotateMode();
     if (rotateMode != RotateAuto && rotateMode != RotateAutoReverse)
         return;
     if (rotateMode == RotateAutoReverse)
         angle += 180;
     transform->rotate(angle);
 }
 
 void SVGAnimateMotionElement::applyResultsToTarget()
 {
@@ skipping 40 matching lines @@
 
 void SVGAnimateMotionElement::updateAnimationMode()
 {
     if (!m_animationPath.isEmpty())
         setAnimationMode(PathAnimation);
     else
         SVGAnimationElement::updateAnimationMode();
 }
 
 }