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).
Created attachment 139402 [details] Patch
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?
(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.
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.
Committed r115726: <http://trac.webkit.org/changeset/115726>
Reopening to attach new patch.
Created attachment 139759 [details] Follow-up patch
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.
(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.
Created attachment 139967 [details] Follow-up patch v2
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.
(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.
Created attachment 139970 [details] Follow-up patch v3
Comment on attachment 139970 [details] Follow-up patch v3 r=me
Committed r115950: <http://trac.webkit.org/changeset/115950>