Bug 63246

Summary:

Convert SVGColor to SVGAnimatorFactory concept

Product:

WebKit

Reporter:

Dirk Schulze <krit>

Component:

SVG

Assignee:

Dirk Schulze <krit>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

webkit.review.bot, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

41761

Attachments:

Description	Flags
Patch	none
Patch	zimmermann: review+

Description Dirk Schulze 2011-06-23 04:57:01 PDT

Convert SVGColor to SVGAnimatorFactory concept.

Comment 1 Dirk Schulze 2011-06-23 06:52:00 PDT

Created attachment 98344 [details]
Patch

Comment 2 WebKit Review Bot 2011-06-23 06:55:25 PDT

Attachment 98344 [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

Last 3072 characters of output:
tedAngle.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedAngle.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedTypeAnimator.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedTypeAnimator.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedTypeAnimator.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.cpp:76:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.cpp:76:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.cpp:76:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedNumber.h:54:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedNumber.h:54:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedNumber.h:54:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedRect.h:52:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedRect.h:52:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedRect.h:52:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedAngle.cpp:77:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedAngle.cpp:77:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedAngle.cpp:77:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedPointList.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedPointList.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedPointList.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Total errors found: 47 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 3 Dirk Schulze 2011-06-23 06:58:45 PDT

(In reply to comment #2)
> Attachment 98344 [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
> 
> Last 3072 characters of output:
> tedAngle.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedAngle.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedLength.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedLength.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedLength.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedLength.cpp:76:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedLength.cpp:76:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedLength.cpp:76:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedNumber.h:54:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedNumber.h:54:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedNumber.h:54:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedRect.h:52:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedRect.h:52:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedRect.h:52:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedAngle.cpp:77:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedAngle.cpp:77:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedAngle.cpp:77:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedPointList.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedPointList.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedPointList.h:39:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Total errors found: 47 in 26 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I don't want to change the ownership of the arguments (OwnPtr<SVGAnimatedType>&).

Comment 4 Nikolas Zimmermann 2011-06-23 07:07:38 PDT

Comment on attachment 98344 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98344&action=review

I agree the style bot shouldn't warn here - the code should be safe as-is. Great patch, still some tweaks needed IMHO to make it perfect:

> Source/WebCore/ChangeLog:12
> +        This information is accessible with the animation element. The animators store the pointer of the animation element, so that it is not

This information is already exposed by the animation element.

> Source/WebCore/svg/SVGAnimatedAngle.cpp:27
> +SVGAnimatedAngleAnimator::SVGAnimatedAngleAnimator(SVGSMILElement* animationElement, SVGElement* contextElement)

Why pass in SVGSMILElement and do ....

> Source/WebCore/svg/SVGAnimatedAngle.cpp:54
> +    SVGAnimateElement* animationElement = static_cast<SVGAnimateElement*>(m_animationElement);

.... static_casts to SVGAnimateElement in the other methods? That seems like a flaw. Just use SVGAnimateElement in first place, no?

> Source/WebCore/svg/SVGAnimatedNumber.h:46
> +    SVGAnimatedNumberAnimator(SVGSMILElement* animationElement, SVGElement*);

No need to name the parameter.

Comment 5 Dirk Schulze 2011-06-23 09:12:45 PDT

(In reply to comment #4)
> (From update of attachment 98344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98344&action=review
> 
> I agree the style bot shouldn't warn here - the code should be safe as-is. Great patch, still some tweaks needed IMHO to make it perfect:
> 
> > Source/WebCore/ChangeLog:12
> > +        This information is accessible with the animation element. The animators store the pointer of the animation element, so that it is not
> 
> This information is already exposed by the animation element.
> 
> > Source/WebCore/svg/SVGAnimatedAngle.cpp:27
> > +SVGAnimatedAngleAnimator::SVGAnimatedAngleAnimator(SVGSMILElement* animationElement, SVGElement* contextElement)
> 
> Why pass in SVGSMILElement and do ....
> 
> > Source/WebCore/svg/SVGAnimatedAngle.cpp:54
> > +    SVGAnimateElement* animationElement = static_cast<SVGAnimateElement*>(m_animationElement);
> 
> .... static_casts to SVGAnimateElement in the other methods? That seems like a flaw. Just use SVGAnimateElement in first place, no?

No, I plan to generalize the Animators more, so that they can be used for animateTransform and animateMotion as well. They would cast it to other elements. I tried to use SVGAnimationElement (the direct base class of animation elements), but got dependency problems.

Comment 6 Dirk Schulze 2011-06-23 09:57:53 PDT

(In reply to comment #5)
> No, I plan to generalize the Animators more, so that they can be used for animateTransform and animateMotion as well. They would cast it to other elements. I tried to use SVGAnimationElement (the direct base class of animation elements), but got dependency problems.

Ok, replaced SVGSMILElement by SVGAnimationElement, the common base class of SVGAnimateElement, SVGAnimateTransformElement and SVGAnimateMotionElement. In the future we can use the same code on all animation types and may move the animator itself to SVGAnimationElement instead of leaving it in every single animation element. Uploading a patch.

Comment 7 Dirk Schulze 2011-06-23 10:03:15 PDT

Created attachment 98362 [details]
Patch

Comment 8 WebKit Review Bot 2011-06-23 10:05:49 PDT

Attachment 98362 [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

Last 3072 characters of output:
tedAngle.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedAngle.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedTypeAnimator.h:40:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedTypeAnimator.h:40:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedTypeAnimator.h:40:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.cpp:78:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.cpp:78:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedLength.cpp:78:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedNumber.h:56:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedNumber.h:56:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedNumber.h:56:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedRect.h:54:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedRect.h:54:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedRect.h:54:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedAngle.cpp:79:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedAngle.cpp:79:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedAngle.cpp:79:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedPointList.h:40:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedPointList.h:40:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedPointList.h:40:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Total errors found: 47 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 9 Nikolas Zimmermann 2011-06-23 10:33:05 PDT

Comment on attachment 98362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98362&action=review

Looks great, r=me.

> Source/WebCore/ChangeLog:15
> +        Replace all SVGSMILElement references by SVGAnimationsElement - the direct base class of all animation elements.

s/direct/common/

Comment 10 Dirk Schulze 2011-06-23 10:42:39 PDT

Committed r89587: <http://trac.webkit.org/changeset/89587>