Created attachment 86898 [details] font-style animation without calcMode attribute Attahced file is the test case (simplified version of LayoutTests/W3C-SVG-1.1/animate-elem-46-t.svg). I think changes of font-style and translate should occur simultaneously as follows (column1: second, 2: translate, 3: font-style). 0.0-1.0s 0 normal 1.0-2.0s 30 italic 2.0-3.0s 60 normal 3.0- 60 normal However, the font-style animation seems incorrect. Here is the result of my investigation. 0.0-1.0s 0 normal 1.0-1.5s 30 normal 1.5-2.0s 30 italic 2.0-3.0s 60 italic 3.0- 60 normal If calcMode="discrete" is explicitly added to the <animate> element, it works well. I saw this problem on nightly builds (Win: r81845 and Mac: r81829).
(In reply to comment #0) > Created an attachment (id=86898) [details] > font-style animation without calcMode attribute > > Attahced file is the test case (simplified version of > LayoutTests/W3C-SVG-1.1/animate-elem-46-t.svg). > > I think changes of font-style and translate should occur simultaneously as follows (column1: second, 2: translate, 3: font-style). > 0.0-1.0s 0 normal > 1.0-2.0s 30 italic > 2.0-3.0s 60 normal > 3.0- 60 normal > > However, the font-style animation seems incorrect. Here is the result of my investigation. > 0.0-1.0s 0 normal > 1.0-1.5s 30 normal > 1.5-2.0s 30 italic > 2.0-3.0s 60 italic > 3.0- 60 normal > > If calcMode="discrete" is explicitly added to the <animate> element, it works well. > > I saw this problem on nightly builds (Win: r81845 and Mac: r81829). Yes, I can confirm this bug. I am working on a fix but it might take some time, since we have to fix other things first on animation. But IIRC the svg-wg decided to change this test in the meantime, since more than one viewer fail on this tests. Also some implementers doubt that this case is specified enough by the specification. If you put calcMode="discrete" on the certain subtests, the test should work.
Created attachment 97993 [details] Patch
Comment on attachment 97993 [details] Patch Attachment 97993 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8913954 New failing tests: svg/W3C-SVG-1.1/animate-elem-31-t.svg
Created attachment 97996 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 97993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97993&action=review I still have a question: > Source/WebCore/ChangeLog:11 > + > + The patch also changes behavior on String animation. The animation code doesn't handle inheritance s/on/for/ > Source/WebCore/svg/SVGAnimateElement.h:62 > - > + Unrelated but ok. > Source/WebCore/svg/SVGAnimationElement.cpp:492 > + if (attributeType == AnimatedBoolean > + || attributeType == AnimatedEnumeration > + || attributeType == AnimatedPreserveAspectRatio > + || attributeType == AnimatedString) > + calcMode = CalcModeDiscrete; How often is this called? If that's hot, you could create a static HashSet and add "static inline bool useStringAnimationForAttributeType(FooEnumType)" ?
(In reply to comment #5) > (From update of attachment 97993 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97993&action=review > > > Source/WebCore/svg/SVGAnimationElement.cpp:492 > > + if (attributeType == AnimatedBoolean > > + || attributeType == AnimatedEnumeration > > + || attributeType == AnimatedPreserveAspectRatio > > + || attributeType == AnimatedString) > > + calcMode = CalcModeDiscrete; > > How often is this called? If that's hot, you could create a static HashSet and add "static inline bool useStringAnimationForAttributeType(FooEnumType)" ? Once per animateElement.
Comment on attachment 97993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97993&action=review Overall LGTM but the HashSet thing needs to be cleared up... > Source/WebCore/ChangeLog:12 > + for strings anymore. Let CSS parser handle inheritance itself if we don't have proceeding animations. You may want to rephrase the "proceeding animations" bit.
Comment on attachment 97993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97993&action=review Overall LGTM but the HashSet thing needs to be cleared up... After talking with Dirk, this would be a small win if any and more confusing code. So only the ChangeLog entry can be a bit improved, but r=me. >> Source/WebCore/ChangeLog:12 >> + for strings anymore. Let CSS parser handle inheritance itself if we don't have proceeding animations. > > You may want to rephrase the "proceeding animations" bit. You may want to rephrase the "proceeding animations" bit.
Committed r89367: <http://trac.webkit.org/changeset/89367>
(In reply to comment #9) > Committed r89367: <http://trac.webkit.org/changeset/89367> Dirk, you did not fix the HashSet problem that Rob and me moaned about :-) Can you please fix it in a follow-up?
(In reply to comment #8) > (From update of attachment 97993 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97993&action=review > > Overall LGTM but the HashSet thing needs to be cleared up... After talking with Dirk, this would be a small win if any and more confusing code. So only the ChangeLog entry can be a bit improved, but r=me. > Moving code to a helper function is always a benefit IMHO. It makes Dirks change easier to read, as teh set of ifs is replaced with a descriptive function call. I just read that it's called once per animation. So I think it's fine to leave it as-is for now.