Bug 63296 – Convert AnimatedString to SVGAnimatorFactory concept

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Convert AnimatedString to SVGAnimatorFactory concept.

[Dirk Schulze]

Created attachment 98420 [details]
Patch

[Dirk Schulze]

Created attachment 98422 [details]
Patch

[WebKit Review Bot]

Attachment 98420 [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/SVGAnimatedString.cpp:41:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:41:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatorFactory.h:57:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 15 in 13 files


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

[WebKit Review Bot]

Attachment 98422 [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/SVGAnimatedString.cpp:41:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:41:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Total errors found: 14 in 13 files


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

[Dirk Schulze]

(In reply to comment #4)
> Attachment 98422 [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/SVGAnimatedString.cpp:41:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.cpp:41:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.cpp:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.cpp:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.h:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.h:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
> Total errors found: 14 in 13 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Again, this is a false  alert.

A note to this change:
356             valueToApply = m_animatedString;
 352            valueToApply = String();

This is called if we can't parse the string. It is irrelevant if we pass an empty string or a invalid string. Results in the same empty path. Changing it to an empty string avoids the need of m_animatedString.

[Rob Buis]

Comment on attachment 98422 [details]
Patch

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

r- since I have some questions, also double check those PassOwnPtr suggestions.

> Source/WebCore/svg/SVGAnimateElement.cpp:352
> +            valueToApply = String();

I don't get this one? Was m_animatedString always empty String for AnimatedPath case before?

> Source/WebCore/svg/SVGAnimatedString.cpp:38
> +    return animtedType.release();

animted?

> Source/WebCore/svg/SVGAnimatorFactory.h:-58
> -            return adoptPtr(new SVGAnimatedLengthAnimator(animationElement, contextElement));

Why is this one removed?

[Dirk Schulze]

(In reply to comment #6)
> (From update of attachment 98422 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98422&action=review
> 
> r- since I have some questions, also double check those PassOwnPtr suggestions.
I don't want to change the ownership. The animationelement should still have the ownership. And I do not pass an OwnPtr<> but a OwnPtr<>&.

> 
> > Source/WebCore/svg/SVGAnimateElement.cpp:352
> > +            valueToApply = String();
> 
> I don't get this one? Was m_animatedString always empty String for AnimatedPath case before?
Seems you did not read my comment at https://bugs.webkit.org/show_bug.cgi?id=63296#c5 . This is a valid question, but thats why I answered it before the review. If we don't have an Bytestream there, it means we couldn't parse the path. If we couldn't parse the path before, we won't get it parsed in SVGPathElement. Both use the same parsing code. So I can omit this completely and put an empty String there instead. This way we don't need m_animatedString anymore.

> 
> > Source/WebCore/svg/SVGAnimatedString.cpp:38
> > +    return animtedType.release();
> 
> animted?
Typo fixed.

> 
> > Source/WebCore/svg/SVGAnimatorFactory.h:-58
> > -            return adoptPtr(new SVGAnimatedLengthAnimator(animationElement, contextElement));
> 
> Why is this one removed?
I did not removed it, I replaced it by StringAnimator. If you take a look one line before, you see a ASSERT_NOT_REACHED. If we get there, we are in an invalid state. So String animation for release builds makes much mire sense than length animation. Since it is clearly a failure to get there.

[Dirk Schulze]

Created attachment 98465 [details]
Patch

[WebKit Review Bot]

Attachment 98465 [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/SVGAnimatedString.cpp:41:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:41:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.cpp:61:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:50:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:51:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedString.h:53:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Total errors found: 14 in 13 files


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

[Nikolas Zimmermann]

Comment on attachment 98465 [details]
Patch

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

Looks good to me, I've discussed on IRC with Dirk, he actually already explained both my and Robs question in an earlier comment. r=me.

> Source/WebCore/svg/SVGAnimateElement.cpp:352
> -            valueToApply = m_animatedString;
> +            valueToApply = String();

Dirk explained this in an earlier comment, it's safe and can be used to avoid m_animatedString completely.

> Source/WebCore/svg/SVGAnimatorFactory.h:61
> -            return adoptPtr(new SVGAnimatedLengthAnimator(animationElement, contextElement));
> +            return adoptPtr(new SVGAnimatedStringAnimator(animationElement, contextElement));

To avoid the confusion use "return nullptr" here. It doesn't matter what's returned here, the code is never reached. But it's certainly better to return 0 than some other value.

[Dirk Schulze]

Committed r89661: <http://trac.webkit.org/changeset/89661>