Summary: | Implement path morphing for SVG animation | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Mac | ||||||
OS: | OS X 10.5 | ||||||
URL: | http://www.w3.org/TR/SVG/paths.html#PathElement | ||||||
Attachments: |
|
Description
Antti Koivisto
2008-06-03 18:29:14 PDT
Created attachment 21488 [details]
patch
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.
(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. 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. >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.
|