Bug 63246

Created attachment 98344 [details] Patch

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.

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

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

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

Created attachment 98362 [details] Patch

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 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/

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