Bug 52982

Summary: SVG is missing to-animation support for Path
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Dirk Schulze <krit>
Status: RESOLVED FIXED    
Severity: Normal CC: zimmermann
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 41761, 36729    
Attachments:
Description Flags
Patch
none
Patch zimmermann: review+

Dirk Schulze
Reported 2011-01-23 14:32:48 PST
SVG is missing to-animation support for Path. This also causes an ASSERT because of missing m_fromPath. Have a patch for it soon.
Attachments
Patch (10.84 KB, patch)
2011-01-23 14:48 PST, Dirk Schulze
no flags
Patch (9.41 KB, patch)
2011-01-24 14:20 PST, Dirk Schulze
zimmermann: review+
Dirk Schulze
Comment 1 2011-01-23 14:48:56 PST
Dirk Schulze
Comment 2 2011-01-24 11:35:05 PST
The description in the test is wrong. I'd like fix it on landing on a positive review.
Eric Seidel (no email)
Comment 3 2011-01-24 13:10:23 PST
Comment on attachment 79875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79875&action=review > Source/WebCore/svg/SVGAnimateElement.cpp:143 > + SVGPathParserFactory* factory = SVGPathParserFactory::self(); I'm not familiar with this self() pattern. Do we use this other places in WebCore? I assume you're vending a shared instance here, but I thought we had other naming patterns for such. > Source/WebCore/svg/SVGPathParserFactory.cpp:129 > +PassOwnPtr<SVGPathByteStream> SVGPathParserFactory::copySVGPathByteStream(SVGPathByteStream* source) This doesn't seem to make sense on the factory. If anything the SVGPathByteStream shoudl have a copy() method which could take a factory pointer if necessary. > Source/WebCore/svg/SVGPathParserFactory.h:41 > + // SVGPathByteStream -> SVGPathByteStream > + PassOwnPtr<SVGPathByteStream> copySVGPathByteStream(SVGPathByteStream* source); Why does this go on the factory and not on the bytestream object itself?
Dirk Schulze
Comment 4 2011-01-24 13:21:54 PST
(In reply to comment #3) > > Source/WebCore/svg/SVGPathParserFactory.h:41 > > + // SVGPathByteStream -> SVGPathByteStream > > + PassOwnPtr<SVGPathByteStream> copySVGPathByteStream(SVGPathByteStream* source); > > Why does this go on the factory and not on the bytestream object itself? Hm, sounds reasonable.
Dirk Schulze
Comment 5 2011-01-24 14:20:32 PST
Dirk Schulze
Comment 6 2011-01-24 14:30:21 PST
(In reply to comment #3) > (From update of attachment 79875 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79875&action=review > > > Source/WebCore/svg/SVGAnimateElement.cpp:143 > > + SVGPathParserFactory* factory = SVGPathParserFactory::self(); > > I'm not familiar with this self() pattern. Do we use this other places in WebCore? I assume you're vending a shared instance here, but I thought we had other naming patterns for such. Yes this was the intention, dodn't know about a naming schema in WebKit for factories. :-/ The call is used in various places, like SVGPathElement, SVGGlyph or SVGPathSegList. > > > Source/WebCore/svg/SVGPathParserFactory.cpp:129 > > +PassOwnPtr<SVGPathByteStream> SVGPathParserFactory::copySVGPathByteStream(SVGPathByteStream* source) > > This doesn't seem to make sense on the factory. If anything the SVGPathByteStream shoudl have a copy() method which could take a factory pointer if necessary. > > > Source/WebCore/svg/SVGPathParserFactory.h:41 > > + // SVGPathByteStream -> SVGPathByteStream > > + PassOwnPtr<SVGPathByteStream> copySVGPathByteStream(SVGPathByteStream* source); > > Why does this go on the factory and not on the bytestream object itself? This is fixed in the last patch.
Nikolas Zimmermann
Comment 7 2011-01-25 01:35:41 PST
Comment on attachment 79977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79977&action=review Looks great, r=me with two comments: > Source/WebCore/svg/SVGPathByteStream.h:54 > + PassOwnPtr<SVGPathByteStream> copySVGPathByteStream() I'd name it copy(), as it's a member of SVGPathByteStream... > Source/WebCore/svg/SVGPathByteStream.h:58 > + OwnPtr<SVGPathByteStream> copy = SVGPathByteStream::create(); > + copy->m_data = m_data; > + return copy.release(); PassOwnPtr<SVGPathByteStream> copy() { return SVGPathByteStream::create(m_data); } Then add a private SVGPathByteStream constructor, taking a Data& reference: private: SVGPathByteStream(Data& data) : m_data(data) { } That's much cleaner and avoids a OwnPtr -> PassOwnPtr conversion.
Dirk Schulze
Comment 8 2011-01-25 04:19:17 PST
(In reply to comment #7) > (From update of attachment 79977 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79977&action=review > > Looks great, r=me with two comments: > > > Source/WebCore/svg/SVGPathByteStream.h:54 > > + PassOwnPtr<SVGPathByteStream> copySVGPathByteStream() > > I'd name it copy(), as it's a member of SVGPathByteStream... > > > Source/WebCore/svg/SVGPathByteStream.h:58 > > + OwnPtr<SVGPathByteStream> copy = SVGPathByteStream::create(); > > + copy->m_data = m_data; > > + return copy.release(); > > PassOwnPtr<SVGPathByteStream> copy() > { > return SVGPathByteStream::create(m_data); > } > > Then add a private SVGPathByteStream constructor, taking a Data& reference: > private: > SVGPathByteStream(Data& data) > : m_data(data) > { > } > > That's much cleaner and avoids a OwnPtr -> PassOwnPtr conversion. I take PassOwnPtr<SVGPathByteStream> copy() { return adoptPtr(new SVGPathByteStream(m_data)); } to avoid a second private create function.
Dirk Schulze
Comment 9 2011-01-25 05:46:58 PST
Note You need to log in before you can comment on or make changes to this bug.