[SVG] Refactoring accumulate logic for animateMotion

[SVG] Refactoring accumulate logic for animateMotion

made some changes to simplify the logic for accumulation

R=pdr@chromium.org, schenney@chromium.org, dschulze@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154444

[pavane, 2013-07-17 09:14:39]

Please have a look at this patch

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

Just a couple minor points.

This looks good--lets just do one more round.

https://codereview.chromium.org/19500005/diff/1/Source/core/svg/SVGAnimateMot...
File Source/core/svg/SVGAnimateMotionElement.cpp (right):

https://codereview.chromium.org/19500005/diff/1/Source/core/svg/SVGAnimateMot...
Source/core/svg/SVGAnimateMotionElement.cpp:278: ok =
m_animationPath.pointAndNormalAtLength(m_animationPath.length(),
positionAtEndOfDuration, angleAtEndOfDuration);
angleAtEndOfDuration doesn't look like it's used. Can you just do:
ok = m_animationPath.pointAtLength(...)?

https://codereview.chromium.org/19500005/diff/1/Source/core/svg/SVGAnimateMot...
Source/core/svg/SVGAnimateMotionElement.cpp:279: if (ok)
I think this would be better:
if (!ok)
  return;

What do you think?

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

Please have a look once again

https://codereview.chromium.org/19500005/diff/1/Source/core/svg/SVGAnimateMot...
File Source/core/svg/SVGAnimateMotionElement.cpp (right):

https://codereview.chromium.org/19500005/diff/1/Source/core/svg/SVGAnimateMot...
Source/core/svg/SVGAnimateMotionElement.cpp:278: ok =
m_animationPath.pointAndNormalAtLength(m_animationPath.length(),
positionAtEndOfDuration, angleAtEndOfDuration);
On 2013/07/17 20:58:50, pdr wrote:
> angleAtEndOfDuration doesn't look like it's used. Can you just do:
> ok = m_animationPath.pointAtLength(...)?
Done

https://codereview.chromium.org/19500005/diff/1/Source/core/svg/SVGAnimateMot...
Source/core/svg/SVGAnimateMotionElement.cpp:279: if (ok)
On 2013/07/17 20:58:50, pdr wrote:
> I think this would be better:
> if (!ok)
>   return;
> 
> What do you think?
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

[pdr., 2013-07-17 21:36:39]

On 2013/07/17 21:32:45, pavane wrote:
> Please have a look once again
> 
>
https://codereview.chromium.org/19500005/diff/1/Source/core/svg/SVGAnimateMot...
> File Source/core/svg/SVGAnimateMotionElement.cpp (right):
> 
>
https://codereview.chromium.org/19500005/diff/1/Source/core/svg/SVGAnimateMot...
> Source/core/svg/SVGAnimateMotionElement.cpp:278: ok =
> m_animationPath.pointAndNormalAtLength(m_animationPath.length(),
> positionAtEndOfDuration, angleAtEndOfDuration);
> On 2013/07/17 20:58:50, pdr wrote:
> > angleAtEndOfDuration doesn't look like it's used. Can you just do:
> > ok = m_animationPath.pointAtLength(...)?
> Done
> 
>
https://codereview.chromium.org/19500005/diff/1/Source/core/svg/SVGAnimateMot...
> Source/core/svg/SVGAnimateMotionElement.cpp:279: if (ok)
> On 2013/07/17 20:58:50, pdr wrote:
> > I think this would be better:
> > if (!ok)
> >   return;
> > 
> > What do you think?
> 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

Makes sense to me!

LGTM.

[commit-bot: I haz the power, 2013-07-17 21:36:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavan.e@samsung.com/19500005/5002

[commit-bot: I haz the power, 2013-07-17 23:25:19]

Change committed as 154444