Bug 63789

Created attachment 99441 [details] Patch

Created attachment 99444 [details] Patch

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

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

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