RESOLVED FIXED 85158
Accumulation for values-animation is broken 
https://bugs.webkit.org/show_bug.cgi?id=85158
Summary Accumulation for values-animation is broken
Nikolas Zimmermann
Reported 2012-04-29 07:50:20 PDT
Example: <rect width="999" height="100" fill="green"/> <animate begin="0s" values="0; 30; 20" accumulate="sum" repeatCount="5" dur="2s"/> </rect> The rect should animate like this: 0.000s -> 0 0.500s -> 15 1.000s -> 30 1.500s -> 25 1.999s -> 20 2.000s -> 20 (first accumulation, starts accumulating from the last set value, here '20'). 2.500s -> 45 3.000s -> 50 3.500s -> 45 3.999s -> 40 4.000s -> 40 (second accumulation) etc. This is currently broken for values-animation. The accumulation should happen after a full cycle of the values animation ran (aka. at the end of the duration).
Attachments
Patch (74.37 KB, patch)
2012-04-29 09:14 PDT, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Follow-up patch (49.24 KB, patch)
2012-05-02 00:29 PDT, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Follow-up patch v2 (56.65 KB, patch)
2012-05-03 00:08 PDT, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Follow-up patch v3 (61.26 KB, patch)
2012-05-03 01:12 PDT, Nikolas Zimmermann 		zherczeg: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Nikolas Zimmermann
Comment 1 2012-04-29 09:14:28 PDT
Created attachment 139402 [details] Patch
Darin Adler
Comment 2 2012-04-29 10:52:33 PDT
Comment on attachment 139402 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139402&action=review > Source/WebCore/svg/SVGAnimatedColor.cpp:73 > + Color& toAtEndOfDurationColor = toAtEndOfDuration->color(); This is a curious idiom. A getter that returns a non-const reference. Do we actually modify the color through this reference?
Nikolas Zimmermann
Comment 3 2012-05-01 07:18:59 PDT
(In reply to comment #2) > (From update of attachment 139402 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139402&action=review > > > Source/WebCore/svg/SVGAnimatedColor.cpp:73 > > + Color& toAtEndOfDurationColor = toAtEndOfDuration->color(); > > This is a curious idiom. A getter that returns a non-const reference. Do we actually modify the color through this reference? Thanks Darin for the review! It's a good point - I left it as-is for consistency. A follow-up patch will introduce const getters for SVGAnimatedType to get rid of the design flaw.
Nikolas Zimmermann
Comment 4 2012-05-01 07:20:49 PDT
Comment on attachment 139402 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139402&action=review > Source/WebCore/svg/SVGAnimateElement.cpp:141 > + OwnPtr<SVGAnimatedType> toAtEndOfDurationType = m_animator->constructFromString(toAtEndOfDurationString); Note to self: We should cache the toAtEndOfDurationType to avoid constructing this type for values animations at each point in time. I'll take care of this when posting a follow-up patch to add the const getters for SVGAnimatedType.
Nikolas Zimmermann
Comment 5 2012-05-01 08:17:40 PDT
Committed r115726: <http://trac.webkit.org/changeset/115726>
Nikolas Zimmermann
Comment 6 2012-05-02 00:28:57 PDT
Reopening to attach new patch.
Nikolas Zimmermann
Comment 7 2012-05-02 00:29:18 PDT
Created attachment 139759 [details] Follow-up patch
Nikolas Zimmermann
Comment 8 2012-05-02 08:34:09 PDT
Comment on attachment 139759 [details] Follow-up patch This actually changes the behavior, we stop mutating from/to values it certain cases, I need to add a new testcase, and do some stylistic fixes.
Nikolas Zimmermann
Comment 9 2012-05-03 00:07:46 PDT
(In reply to comment #8) > (From update of attachment 139759 [details]) > This actually changes the behavior, we stop mutating from/to values it certain cases, I need to add a new testcase, and do some stylistic fixes. Ok, I was wrong, it was a bit too late yesterday - in fact I'm changing this to avoid any mutation, but it didn't cause problems in practice before, as we reset the from/to values on every animation step, this is now gone.
Nikolas Zimmermann
Comment 10 2012-05-03 00:08:19 PDT
Created attachment 139967 [details] Follow-up patch v2
Zoltan Herczeg
Comment 11 2012-05-03 00:48:40 PDT
Comment on attachment 139967 [details] Follow-up patch v2 Looks good, some comments: View in context: https://bugs.webkit.org/attachment.cgi?id=139967&action=review > Source/WebCore/svg/SVGAnimatedBoolean.cpp:84 > bool& animatedBoolean = animated->boolean(); Could we remove the reference here as well? > Source/WebCore/svg/SVGAnimatedType.cpp:244 > +const std::pair<SVGAngle, unsigned>& SVGAnimatedType::angleAndEnumeration() const > +{ > + ASSERT(m_type == AnimatedAngle); > + return *m_data.angleAndEnumeration; > +} Ok, these one liners should all be moved to the header, otherwise they would be a pure regressions.
Nikolas Zimmermann
Comment 12 2012-05-03 00:52:33 PDT
(In reply to comment #11) > (From update of attachment 139967 [details]) > Looks good, some comments: Thanks! > > Source/WebCore/svg/SVGAnimatedBoolean.cpp:84 > > bool& animatedBoolean = animated->boolean(); > Could we remove the reference here as well? Nope, all "animated*" types are mutable - they will receive the actual animation updates. > > Source/WebCore/svg/SVGAnimatedType.cpp:244 > > +const std::pair<SVGAngle, unsigned>& SVGAnimatedType::angleAndEnumeration() const > > +{ > > + ASSERT(m_type == AnimatedAngle); > > + return *m_data.angleAndEnumeration; > > +} > > Ok, these one liners should all be moved to the header, otherwise they would be a pure regressions. Ok, you're right, I'll move all of them.
Nikolas Zimmermann
Comment 13 2012-05-03 01:12:42 PDT
Created attachment 139970 [details] Follow-up patch v3
Zoltan Herczeg
Comment 14 2012-05-03 02:04:44 PDT
Comment on attachment 139970 [details] Follow-up patch v3 r=me
Nikolas Zimmermann
Comment 15 2012-05-03 02:08:53 PDT
Committed r115950: <http://trac.webkit.org/changeset/115950>
Note You need to log in before you can comment on or make changes to this bug.