Bug 63336

Created attachment 98512 [details] SVGAnimatedPathAnimator

Created attachment 98592 [details] Patch v2

Dirk have a look at "https://bugs.webkit.org/attachment.cgi?oldid=98512&action=interdiff&newid=98592&headers=1"

Created attachment 98595 [details] Patch v3

Attachment 98595 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/svg/SVGAnimatedPath.cpp:42: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:42: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:72: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:72: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:77: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:77: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:77: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:38: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:38: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:39: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:39: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Total errors found: 14 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 98595 [details] Patch v3 Attachment 98595 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8933881

Comment on attachment 98595 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=98595&action=review LGTM in general, nice to see lots of code removed here and one general concept being used. I'll leave it to kri for the real review since he is more familiar with this code. > Source/WebCore/svg/SVGAnimatedPath.cpp:98 > + ASSERT(percentage >= 0); Is that ASSERT useful? AFAICS percentage must be zero? > Source/WebCore/svg/SVGAnimatedPath.cpp:114 > + if ((animationMode == FromToAnimation && percentage > 0.5) || animationMode == ToAnimation || percentage == 1) But percentage == 1 already made an early exit above?

Comment on attachment 98595 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=98595&action=review You forgot to edit WebCore.gyp. And please remove the unnecessary headers in SVGAnimateElement.h/.cpp. Patch looks great btw :-) r- because of the build failure and the includes. > Source/WebCore/svg/SVGAnimatedPath.cpp:39 > +PassOwnPtr<SVGAnimatedType> SVGAnimatedPathAnimator::constructFromString(const String& string) > +{ > + OwnPtr<SVGPathByteStream> byteStream = SVGPathByteStream::create(); > + SVGPathParserFactory::self()->buildSVGPathByteStreamFromString(string, byteStream, UnalteredParsing); > + return SVGAnimatedType::createPath(byteStream.leakPtr()); Don't you think it might be more secure to release the OwnPtr<SVGPathByteStream> here and pass an PassOwnPtr? SVGAnimatedType::createPath might be used on other places in the future. We should make sure that the caller does not forget, that it hasn't the ownership anymore.

(In reply to comment #7) > (From update of attachment 98595 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98595&action=review > > LGTM in general, nice to see lots of code removed here and one general concept being used. I'll leave it to kri for the real review since he is more familiar with this code. > > > Source/WebCore/svg/SVGAnimatedPath.cpp:98 > > + ASSERT(percentage >= 0); > > Is that ASSERT useful? AFAICS percentage must be zero? Good point! this gets already asserted in SVGAnimateElement.cpp > > > Source/WebCore/svg/SVGAnimatedPath.cpp:114 > > + if ((animationMode == FromToAnimation && percentage > 0.5) || animationMode == ToAnimation || percentage == 1) > > But percentage == 1 already made an early exit above? Second good point! :-)

(In reply to comment #8) > (From update of attachment 98595 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98595&action=review > > You forgot to edit WebCore.gyp. And please remove the unnecessary headers in SVGAnimateElement.h/.cpp. Patch looks great btw :-) r- because of the build failure and the includes. Fixed :-) > > > Source/WebCore/svg/SVGAnimatedPath.cpp:39 > > +PassOwnPtr<SVGAnimatedType> SVGAnimatedPathAnimator::constructFromString(const String& string) > > +{ > > + OwnPtr<SVGPathByteStream> byteStream = SVGPathByteStream::create(); > > + SVGPathParserFactory::self()->buildSVGPathByteStreamFromString(string, byteStream, UnalteredParsing); > > + return SVGAnimatedType::createPath(byteStream.leakPtr()); > > Don't you think it might be more secure to release the OwnPtr<SVGPathByteStream> here and pass an PassOwnPtr? SVGAnimatedType::createPath might be used on other places in the future. We should make sure that the caller does not forget, that it hasn't the ownership anymore. Okay.

Created attachment 98605 [details] Patch v4 Fixed Robs / Dirks comments.

Attachment 98605 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/svg/SVGAnimatedPath.cpp:48: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:48: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:72: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:72: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:77: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:77: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.cpp:77: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:38: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:38: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:39: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:39: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WebCore/svg/SVGAnimatedPath.h:41: The parameter type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Total errors found: 14 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 98605 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=98605&action=review r=me with comments. > Source/WebCore/svg/SVGAnimateElement.h:74 > You forgot to remove #include "SVGPathByteStream.h" > Source/WebCore/svg/SVGAnimatedPath.cpp:121 > + // No paced animations for strings. Hehe, not strings but SVGPathSegLists :) Please fix the comment. Make it to a FIXME, Opera supports it.

Landed in r89749.