Bug 19384 - Implement path morphing for SVG animation
Summary: Implement path morphing for SVG animation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://www.w3.org/TR/SVG/paths.html#P...
Keywords:
Depends on:
Blocks:
 
Reported: 2008-06-03 18:29 PDT by Antti Koivisto
Modified: 2008-06-03 20:01 PDT (History)
0 users

See Also:


Attachments
patch (16.03 KB, patch)
2008-06-03 18:33 PDT, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2008-06-03 18:29:14 PDT
"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."
Comment 1 Antti Koivisto 2008-06-03 18:33:39 PDT
Created attachment 21488 [details]
patch
Comment 2 Darin Adler 2008-06-03 19:06:04 PDT
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.
Comment 3 Antti Koivisto 2008-06-03 19:37:16 PDT
(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.
Comment 4 Antti Koivisto 2008-06-03 19:59:16 PDT
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.
Comment 5 Antti Koivisto 2008-06-03 20:01:35 PDT
 >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.