Created attachment 55795 [details] manual test The timing functions within animations are not applying correctly. See the testcase - I'm noticing a few errors: - the tf rule on a 0%/from keyframe is applying to the whole animation, if no other keyframes specify tfs - the tf rule on a 0%/from keyframe is NOT applying to that keyframe when the animation is iterating backwards
<rdar://problem/7966984>
Just making notes to myself here. KeyframeAnimation::getKeyframeAnimationInterval() const TimingFunction* timingFunction = 0; if (fromStyle->animations() && fromStyle->animations()->size() > 0) { This means that: eg. 0% { animation-timing-function: linear; } 20% { animation-timing-function: ease-in; } and you are currently at 10%, going forwards, then it should use the linear function (sort-of opposite of what transitions do) and when you are at the same position going backwards, the same timing function (linear) applies. Even though technically the from and to have swapped, it's just that the animation is playing backwards, not that different rules apply.
This means part of my test case is wrong
The problem is that when we clone the RenderStyle for each keyframe, the AnimationList is new, but it still points to the same animation elements. This means that when a later keyframe sets the timing function, it overwrites the timing function for all previous keyframes. I've attached two samples which show this in action. Also, here is a patch to fix it. My answer was to make sure that there was a real copy done on AnimationList, rather than a shallow one. I'm not sure this is the best way - I couldn't find other places that trigger this code (only when applyDeclarations is called from within CSSStyleSelector::keyframeStylesForAnimation), so this isn't a review request. If it looks ok, I'll write a real regression test.
Created attachment 56753 [details] example with HW anim
Created attachment 56754 [details] Example with SW anim
Created attachment 56755 [details] patch
Comment on attachment 56755 [details] patch > class AnimationList : public FastAllocBase { > public: > + > + AnimationList() {} Not sure if you need that. Otherwise I think this patch is basically correct. I do wonder if m_animations should be a DataRef in StyleRareNonInheritedData though.
> > class AnimationList : public FastAllocBase { > > public: > > + > > + AnimationList() {} > Not sure if you need that. Yeah, I did. There was somewhere (can't remember) that used the default constructor. Once I added the copy constructor the compile failed because it wasn't generating the parameterless version.
Dean, you should post this for review someday.
Created attachment 57977 [details] Testcase
Created attachment 57978 [details] Testcase that should work, but doesn't For some reason this testcase doesn't work with or without the fix. Weird.
Created attachment 58531 [details] Patch
I was not able to reproduce the issue with the transform testcase.
Comment on attachment 58531 [details] Patch > + AnimationList() {} > + AnimationList(const AnimationList& o); We normally put a space between the braces in "{}". There’s no need for the argument name "o". (In the definition we prefer names to letters for arguments, so there other would be better then o.) It’s best practice to also defined an assignment operator if you define a copy constructor. There are three ways you can deal with this: 1) Declare a private assignment operator and do not define it. 2) Use Noncopyable to do the above, but this will require a change to the copy constructor. 3) Define a public assignment operator that uses swap (see <http://www.gotw.ca/gotw/059.htm>). AnimationList& operator=(const AnimationList& other) { AnimationList copy(other); swap(copy); return *this; } If you do (3) we also need to write the swap function, but that is typically simple. I suggest doing either (1) or (3) rather than checking this in as-is.
http://trac.webkit.org/changeset/61068