63246 – Convert SVGColor to SVGAnimatorFactory concept

Bug 63246 - Convert SVGColor to SVGAnimatorFactory concept

Summary: Convert SVGColor to SVGAnimatorFactory concept

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	SVG (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Dirk Schulze

URL:
Keywords:

Depends on:
Blocks:	41761
	Show dependency tree / graph

Reported:	2011-06-23 04:57 PDT by Dirk Schulze
Modified:	2011-06-23 10:42 PDT (History)
CC List:	2 users (show)

See Also:

Attachments
Patch (70.12 KB, patch) 2011-06-23 06:52 PDT, Dirk Schulze	no flags	Details \| Formatted Diff \| Diff
Patch (73.55 KB, patch) 2011-06-23 10:03 PDT, Dirk Schulze	zimmermann: review+	Details \| Formatted Diff \| Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

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>