Bug 63246 – Convert SVGColor to SVGAnimatorFactory concept

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Convert SVGColor to SVGAnimatorFactory concept.

[Dirk Schulze]

Created attachment 98344 [details]
Patch

[WebKit Review Bot]

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.

[Dirk Schulze]

(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>&).

[Nikolas Zimmermann]

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.

[Dirk Schulze]

(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.

[Dirk Schulze]

(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.

[Dirk Schulze]

Created attachment 98362 [details]
Patch

[WebKit Review Bot]

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.

[Nikolas Zimmermann]

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/

[Dirk Schulze]

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