[webkit-reviews] review denied: [Bug 52982] SVG is missing to-animation support for Path : [Attachment 79875] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dirk Schulze <krit at webkit.org>'s
request for review:
Bug 52982: SVG is missing to-animation support for Path
https://bugs.webkit.org/show_bug.cgi?id=52982

Attachment 79875: Patch
https://bugs.webkit.org/attachment.cgi?id=79875&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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?