Bug 19384 – Implement path morphing for SVG animation

• Component: SVG
• Status: RESOLVED FIXED

[Antti Koivisto]

"Path data animation is only possible when each path data specification within an animation specification has exactly the same list of path data commands as the d attribute. If an animation is specified and the list of path data commands is not the same, then the animation specification is in error (see Error Processing). The animation engine interpolates each parameter to each path data command separately based on the attributes to the given animation element. Flags and booleans are interpolated as fractions between zero and one, with any non-zero value considered to be a value of one/true."

[Antti Koivisto]

Created attachment 21488 [details]
patch

[Darin Adler]

Comment on attachment 21488 [details]
patch

 161         m_fromPath.clear();
 162         m_toPath.clear();
 163         m_fromPath = SVGPathSegList::create(SVGNames::dAttr);

Is the call to m_fromPath.clear() needed here?

 238                 SVGPathSeg* segment = m_animatedPath->getItem(n, ec).get();

It seems unnecessarily dangerous to use a raw pointer here. If it was safe to be a raw pointer, then I think getItem's return value should be a raw pointer. I suggest using a RefPtr instead. There should be no additional ref count churn, since you start with a PassRefPtr anyway.

Can getItem fail? if so, then don't we need to check the value of "ec"?

 188         SVGPathSeg* from = fromList->getItem(n, ec).get();
 189         SVGPathSeg* to = toList->getItem(n, ec).get();

Same two comments here.

 254         result->appendItem(segment, ec);

Don't we need to check ec here? Or can this never fail?

 17 </svg>
018 \ No newline at end of file

Could you include a newline?

I'm going to say r=me, but please consider those RefPtr issues.

[Antti Koivisto]

(In reply to comment #2)
> (From update of attachment 21488 [details] [edit])
>  161         m_fromPath.clear();
>  162         m_toPath.clear();
>  163         m_fromPath = SVGPathSegList::create(SVGNames::dAttr);
> 
> Is the call to m_fromPath.clear() needed here?

No.

> Can getItem fail? if so, then don't we need to check the value of "ec"?

No, the only way for it to fail is out of bounds index.

>  254         result->appendItem(segment, ec);
> 
> Don't we need to check ec here? Or can this never fail?

This can't fail.

[Antti Koivisto]

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/platform/mac/svg/custom/animate-path-morphing-expected.txt
Adding         LayoutTests/svg/custom/animate-path-morphing.svg
Sending        WebCore/ChangeLog
Sending        WebCore/svg/SVGAnimateElement.cpp
Sending        WebCore/svg/SVGAnimateElement.h
Sending        WebCore/svg/SVGPathSegList.cpp
Sending        WebCore/svg/SVGPathSegList.h
Transmitting file data ........
Committed revision 34358.

[Antti Koivisto]

 >188         SVGPathSeg* from = fromList->getItem(n, ec).get();
 >189         SVGPathSeg* to = toList->getItem(n, ec).get();
>Same two comments here.

I didn't use RefPtrs here to keep the casts in macros simple. In practice nothing can destroy those segments inside the loop.