Bug 63789 - SVGAnimatedType should support SVGAnimatedInteger animation
Summary: SVGAnimatedType should support SVGAnimatedInteger animation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
Depends on:
Blocks: 41761
  Show dependency treegraph
 
Reported: 2011-07-01 00:37 PDT by Dirk Schulze
Modified: 2011-07-01 01:38 PDT (History)
2 users (show)

See Also:


Attachments
Patch (27.05 KB, patch)
2011-07-01 00:51 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (27.03 KB, patch)
2011-07-01 01:17 PDT, Dirk Schulze
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2011-07-01 00:37:08 PDT
SVGAnimatedType should support SVGAnimatedInteger animation
Comment 1 Dirk Schulze 2011-07-01 00:51:33 PDT
Created attachment 99441 [details]
Patch
Comment 2 Dirk Schulze 2011-07-01 01:17:47 PDT
Created attachment 99444 [details]
Patch
Comment 3 WebKit Review Bot 2011-07-01 01:20:14 PDT
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.
Comment 4 Nikolas Zimmermann 2011-07-01 01:25:30 PDT
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?
Comment 5 Dirk Schulze 2011-07-01 01:32:43 PDT
(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.
Comment 6 Nikolas Zimmermann 2011-07-01 01:35:08 PDT
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.
Comment 7 Dirk Schulze 2011-07-01 01:38:11 PDT
Committed r90218: <http://trac.webkit.org/changeset/90218>