Bug 63336 - Convert SVGPath to SVGAnimatorFactory concept
Summary: Convert SVGPath to SVGAnimatorFactory concept
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 41761
  Show dependency treegraph
 
Reported: 2011-06-24 11:03 PDT by Dirk Schulze
Modified: 2011-06-25 14:37 PDT (History)
3 users (show)

See Also:


Attachments
SVGAnimatedPathAnimator (22.33 KB, patch)
2011-06-24 11:04 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch v2 (31.71 KB, patch)
2011-06-25 10:15 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v3 (32.00 KB, patch)
2011-06-25 10:32 PDT, Nikolas Zimmermann
krit: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v4 (33.41 KB, patch)
2011-06-25 14:07 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2011-06-24 11:03:33 PDT
Convert SVGPath to SVGAnimatorFactory concept.
Comment 1 Dirk Schulze 2011-06-24 11:04:32 PDT
Created attachment 98512 [details]
SVGAnimatedPathAnimator
Comment 2 Nikolas Zimmermann 2011-06-25 10:15:10 PDT
Created attachment 98592 [details]
Patch v2
Comment 3 Nikolas Zimmermann 2011-06-25 10:29:10 PDT
Dirk have a look at "https://bugs.webkit.org/attachment.cgi?oldid=98512&action=interdiff&newid=98592&headers=1"
Comment 4 Nikolas Zimmermann 2011-06-25 10:32:39 PDT
Created attachment 98595 [details]
Patch v3
Comment 5 WebKit Review Bot 2011-06-25 10:35:57 PDT
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 6 WebKit Review Bot 2011-06-25 10:51:20 PDT
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 7 Rob Buis 2011-06-25 11:24:55 PDT
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 8 Dirk Schulze 2011-06-25 11:25:55 PDT
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.
Comment 9 Dirk Schulze 2011-06-25 11:28:00 PDT
(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! :-)
Comment 10 Nikolas Zimmermann 2011-06-25 14:07:04 PDT
(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.
Comment 11 Nikolas Zimmermann 2011-06-25 14:07:54 PDT
Created attachment 98605 [details]
Patch v4

Fixed Robs / Dirks comments.
Comment 12 WebKit Review Bot 2011-06-25 14:10:31 PDT
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 13 Dirk Schulze 2011-06-25 14:31:23 PDT
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.
Comment 14 Nikolas Zimmermann 2011-06-25 14:37:11 PDT
Landed in r89749.