Bug 63789 – SVGAnimatedType should support SVGAnimatedInteger animation

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

SVGAnimatedType should support SVGAnimatedInteger animation

[Dirk Schulze]

Created attachment 99441 [details]
Patch

[Dirk Schulze]

Created attachment 99444 [details]
Patch

[WebKit Review Bot]

Attachment 99444 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/svg/SVGAnimatedInteger.h:55:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.h:55:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.h:56:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.h:56:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.h:58:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.h:58:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.h:58:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.cpp:44:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.cpp:44:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.cpp:55:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.cpp:55:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.cpp:69:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.cpp:69:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Source/WebCore/svg/SVGAnimatedInteger.cpp:69:  The parameter type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Total errors found: 14 in 18 files


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

[Nikolas Zimmermann]

Comment on attachment 99441 [details]
Patch

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

Looks good to me.

> Source/WebCore/svg/SVGAnimatedInteger.cpp:40
> +    bool ok;
> +    animtedType->integer() = string.toIntStrict(&ok);

Hm, we have to revisit error handling from methods like constructFromSTring at some point.

> Source/WebCore/svg/SVGAnimatedInteger.cpp:84
> +    animatedInt = roundf(result);

Don't you have to cast to int, as the return value is a float here?
Do any of the specs (svg/smil) say sth. about rounding? whether round/ceil or floor is desired?

> Source/WebCore/svg/SVGAnimatedInteger.cpp:90
> +    int from = fromString.toIntStrict();

Oh, toIntStrict does not _need_ a bool& ok param... so why don't you leave it out in constructFromString?

[Dirk Schulze]

(In reply to comment #4)
> (From update of attachment 99441 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99441&action=review
> 
> Looks good to me.
> 
> > Source/WebCore/svg/SVGAnimatedInteger.cpp:40
> > +    bool ok;
> > +    animtedType->integer() = string.toIntStrict(&ok);
> 
> Hm, we have to revisit error handling from methods like constructFromSTring at some point.
I definitely plan it for the future.

> 
> > Source/WebCore/svg/SVGAnimatedInteger.cpp:84
> > +    animatedInt = roundf(result);
Can add it. It compiles right now
> 
> Don't you have to cast to int, as the return value is a float here?
> Do any of the specs (svg/smil) say sth. about rounding? whether round/ceil or floor is desired?
That's the problem. There is no description about that, we could use discrete animation here, but rounding to the nearest value makes more sense (is more useful for the webdeveloper) IMHO.

> 
> > Source/WebCore/svg/SVGAnimatedInteger.cpp:90
> > +    int from = fromString.toIntStrict();
> 
> Oh, toIntStrict does not _need_ a bool& ok param... so why don't you leave it out in constructFromString?
No it doesn't need it, it was a preparation for the error handling in the future. I'll remove it for now.

[Nikolas Zimmermann]

Comment on attachment 99444 [details]
Patch

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

r=me.

> Source/WebCore/svg/SVGAnimatedInteger.cpp:40
> +    bool ok;
> +    animtedType->integer() = string.toIntStrict(&ok);

Remove the ok.

> Source/WebCore/svg/SVGAnimatedInteger.cpp:84
> +    animatedInt = roundf(result);

static_cast<int>(..)

> LayoutTests/svg/animations/script-tests/svginteger-animation-1.js:39
> +	shouldBe("feConvolveMatrix.targetX.animVal", "0");

would be great if you could add the commented out baseVal statement, so I have an easier job later:
// shouldBe("feConvolveMatrix.targetX.baseVal", "0")
...

> LayoutTests/svg/animations/script-tests/svginteger-animation-1.js:43
> +	shouldBe("feConvolveMatrix.targetX.animVal", "1");

Ditto.

> LayoutTests/svg/animations/script-tests/svginteger-animation-1.js:47
> +	shouldBe("feConvolveMatrix.targetX.animVal", "2");

Ditto.

[Dirk Schulze]

Committed r90218: <http://trac.webkit.org/changeset/90218>