Bug 20119 – Execute CSS Animations

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dean Jackson]

Previous patches have added animation properties into the style system and allowed keyframe animations to be parsed in CSS, this patch does the actual animation.

[Dean Jackson]

Created attachment 22397 [details]
implements CSS animations (and also does some editorial cleanup)

I need to test this a bit more before review. I think it asserts when a new document loads. Something isn't getting cleaned up.

[Dean Jackson]

Created attachment 22437 [details]
Updated patch

This update fixes the assertion that we hit when unloading a page with running animations. I'd like feedback on the fix because I'm sure there is a better way to do it. This patch marks a document as it is being unloaded so that the style is not recalculated (the animation engine had marked the document as changed), and marks it loaded as it comes back from history. It would be better if the updateRendering is never called, but it seems to happen from multiple places.

[Dean Jackson]

Created attachment 22438 [details]
Tests simple repeated animation

[Dean Jackson]

Created attachment 22439 [details]
Tests animation duration (and correct parsing)

[Dean Jackson]

Created attachment 22440 [details]
Tests that "none" animation works

[Dean Jackson]

Created attachment 22441 [details]
Tests animation timing functions

[Dave Hyatt]

Comment on attachment 22437 [details]
Updated patch

I am concerned about the performance implications of:

 void RenderObject::setAnimatableStyle(RenderStyle* style)
 {
-    if (!isText() && m_style && style)
+    if (!isText() && style)
         style = animation()->updateAnimations(this, style);

[Dave Hyatt]

Comment on attachment 22437 [details]
Updated patch

Everything looks fine to me.  I am concerned that the line I linked above could cause a performance regression, since you've added extra work to setStyle for every single RenderObject on a page by removing that null check.

r=me, but because of the setAnimatableStyle change you will need to run the performance tests before this can be landed to verify that no regression is caused.

[Eric Seidel (no email)]

Comment on attachment 22437 [details]
Updated patch

Bleh.  I really hate the use of long macros.  I'd much rather see that code re-written to use static inlines with function pointers. You can use macros to call the static inlines if you would prefer to not type the 8 permutation of set/get/clear Prop or whatever you'd need.

Style violation:
+    if (m_frame) {
         m_frame->animation()->styleAvailable();
+    }

I would have to look at the code... but "f" seems like a bad choice in variable name:
+    if (f)
+        f->animation()->resumeAnimations(this);

Personally I prefer using "name" in cases like this... but that said, it really doens't matter either way:
+    void setName(const String& s) { m_name = s; }

+
+    if (m_animation->direction() && (i & 1))
+        t = 1 - t;
"t" is not a very clear variable name for "number of iterations"

tt could have a better name too:
+    double tt = solveCubicBezierFunction(m_animation->timingFunction().x1(),


I don't think you need the .get()s in these calls:
+        RefPtr<TransformOperation> fromOp = i < fromSize ? from[i].get() : 0;

RefPtr will corectly construct from another RefPtr just fine.  Hum... of course the ternary operator might get made at you for having different types on each side of the : so you may need to leave it as .get()

Style:
+        if (!anim->isValidAnimation()) {
+            animsChanged = true;
+        } else {
+    if ((m_keyframeAnimations.isEmpty() && !anim) ||
+        (currentStyle && currentStyle->animations() && *(currentStyle->animations()) == *anim)) {
+        return;
+    }

I would have just written out "keyframeAnim" here:
+            KeyframeAnimation* kfAnim = (numAnims < m_keyframeAnimations.size()) ? m_keyframeAnimations[numAnims] : 0;

If I was writting this out, I might have tried to make it self-documenting:

+        // Don't bother adding the animation if it has no keyframes or won't animate
+        if ((anim->duration() || anim->delay()) && anim->iterationCount() &&
+             anim->keyframeList().get() && !anim->keyframeList()->isEmpty())

bool willAnimate = (anim->duration() || anim->delay()) && anim->iterationCount();
bool hasKeyframes = anim->keyframeList() && !anim->keyframeList()->isEmpty();
if (willAnimate && hasKeyframes)
  m_keyframeAnimations.append(new KeyframeAnimation(const_cast<Transition*>(anim), renderer, index, this));

Why are you calling anim->keyframeList().get() here?  If that's returning a PassRefPtr, that's a horribly inefficient way to get a boolean answer. :)  (Since only functions which allocate return PassRefPtrs).  Generally we don't return RefPtr's from any function (since they cause ref-churn by copying and destroying the temporary.

Also, RefPtr knows how to transparently convert to a bool, so even if that function were returning a RefPtr, you wouldn't need to call .get() to convert it to a raw pointer to convert it to a bool.


There is a vector function "deleteAllValues" which will do this for you:
+    Vector<KeyframeAnimation*>::const_iterator kfend = m_keyframeAnimations.end();
+    for (Vector<KeyframeAnimation*>::const_iterator it = m_keyframeAnimations.begin(); it != kfend; ++it) {
+        KeyframeAnimation* anim = *it;
+        delete anim;
+    }

I take it this vector m_keyframeAnimations can have null values?  You check *it against null all over.  Yet I don't see (or must have just missed) where these values are ever set to null.

I don't believe it's webkit style to indicate that variables are in/out by prepending "io":
+        if (!ioAnimatedStyle)
+            ioAnimatedStyle = const_cast<RenderStyle*>(targetStyle);

Bad variable names:
+    double t = m_animation->duration() ? (elapsedTime / m_animation->duration()) : 1;
+    int i = (int) t;
+    t -= i;
+    if (m_animation->direction() && (i & 1))
+        t = 1 - t;

I assume the compiler is smart enough to convert the 1 to a double instead of converting the division result to an int:
+    double t = m_animation->duration() ? (elapsedTime / m_animation->duration()) : 1;


In general I think we try and make double constants doubles (using 1.0 and 0.0, etc.) and floats floats, and ints ints.


If KeyframeAnimation is refcounted, it should be using the new create() syntax. instead of direct calls to "new".

This is probably just a call to:
dispatchGenericEvent:

+bool KeyframeAnimation::sendAnimationEvent(const AtomicString& inEventType, double inElapsedTime)
+{
+    // FIXME: Event dispatch goes here
+    return false; // didn't dispatch an event

However I can understand if you're trying to limit the scope of your patch. :)

Are there test cases for these changes?

[Eric Seidel (no email)]

Comment on attachment 22437 [details]
Updated patch

Ah, there are test cases attached.  The r- was for all my other comments of course. :)  Obviously you'll need a ChangeLog eventually too.

[Dean Jackson]

Created attachment 22700 [details]
Updated patch

This is an updated patch, fairly similar to the one hyatt already reviewed. It has some fixes for Eric's review. I'll open bugs on anything I've left out of here.

[Dean Jackson]

> Bleh.  I really hate the use of long macros.  I'd much rather see that code
> re-written to use static inlines with function pointers. You can use macros to
> call the static inlines if you would prefer to not type the 8 permutation of
> set/get/clear Prop or whatever you'd need.

Yeah, we hate the macros too. We'll open a bug to remove them (and the macros for
background layers, which is the code we copied)

> 
> Style violation:
> +    if (m_frame) {
>          m_frame->animation()->styleAvailable();
> +    }

Fixed

> I would have to look at the code... but "f" seems like a bad choice in variable
> name:
> +    if (f)
> +        f->animation()->resumeAnimations(this);

This is pretty common throughout (ie. not just this function and file). In this case
it is Frame.

> > Personally I prefer using "name" in cases like this... but that said, it really
> doens't matter either way:
> +    void setName(const String& s) { m_name = s; }

I'm not sure what you mean. s/setName/name/ ?

> +
> +    if (m_animation->direction() && (i & 1))
> +        t = 1 - t;
> "t" is not a very clear variable name for "number of iterations"
> 
> tt could have a better name too:
> +    double tt = solveCubicBezierFunction(m_animation->timingFunction().x1(),

I've renamed these to something better.

> I don't think you need the .get()s in these calls:
> +        RefPtr<TransformOperation> fromOp = i < fromSize ? from[i].get() : 0;
> 
> RefPtr will corectly construct from another RefPtr just fine.  Hum... of course
> the ternary operator might get made at you for having different types on each
> side of the : so you may need to leave it as .get()

I got rid of it in the lines above, but left it in this one since the compiler
complained.

> Style:
> +        if (!anim->isValidAnimation()) {
> +            animsChanged = true;
> +        } else {
> +    if ((m_keyframeAnimations.isEmpty() && !anim) ||
> +        (currentStyle && currentStyle->animations() &&
> *(currentStyle->animations()) == *anim)) {
> +        return;
> +    }

Fixed

> 
> I would have just written out "keyframeAnim" here:
> +            KeyframeAnimation* kfAnim = (numAnims <
> m_keyframeAnimations.size()) ? m_keyframeAnimations[numAnims] : 0;

Fixed

> 
> If I was writting this out, I might have tried to make it self-documenting:
> 
> +        // Don't bother adding the animation if it has no keyframes or won't
> animate
> +        if ((anim->duration() || anim->delay()) && anim->iterationCount() &&
> +             anim->keyframeList().get() && !anim->keyframeList()->isEmpty())
> 
> bool willAnimate = (anim->duration() || anim->delay()) &&
> anim->iterationCount();
> bool hasKeyframes = anim->keyframeList() && !anim->keyframeList()->isEmpty();
> if (willAnimate && hasKeyframes)
>   m_keyframeAnimations.append(new
> KeyframeAnimation(const_cast<Transition*>(anim), renderer, index, this));

I haven't fixed this one yet, but will.

> Why are you calling anim->keyframeList().get() here?  If that's returning a
> PassRefPtr, that's a horribly inefficient way to get a boolean answer. :) 
> (Since only functions which allocate return PassRefPtrs).  Generally we don't
> return RefPtr's from any function (since they cause ref-churn by copying and
> destroying the temporary.

Again, haven't fixed yet but will.

> 
> Also, RefPtr knows how to transparently convert to a bool, so even if that
> function were returning a RefPtr, you wouldn't need to call .get() to convert
> it to a raw pointer to convert it to a bool.
> 
> 
> There is a vector function "deleteAllValues" which will do this for you:
> +    Vector<KeyframeAnimation*>::const_iterator kfend =
> m_keyframeAnimations.end();
> +    for (Vector<KeyframeAnimation*>::const_iterator it =
> m_keyframeAnimations.begin(); it != kfend; ++it) {
> +        KeyframeAnimation* anim = *it;
> +        delete anim;
> +    }
> 
> I take it this vector m_keyframeAnimations can have null values?  You check *it
> against null all over.  Yet I don't see (or must have just missed) where these
> values are ever set to null.
> 
> I don't believe it's webkit style to indicate that variables are in/out by
> prepending "io":
> +        if (!ioAnimatedStyle)
> +            ioAnimatedStyle = const_cast<RenderStyle*>(targetStyle);

Changed

> Bad variable names:
> +    double t = m_animation->duration() ? (elapsedTime /
> m_animation->duration()) : 1;
> +    int i = (int) t;
> +    t -= i;
> +    if (m_animation->direction() && (i & 1))
> +        t = 1 - t;
> 

Fixed.

> I assume the compiler is smart enough to convert the 1 to a double instead of
> converting the division result to an int:
> +    double t = m_animation->duration() ? (elapsedTime /
> m_animation->duration()) : 1;
> 
> 
> In general I think we try and make double constants doubles (using 1.0 and 0.0,
> etc.) and floats floats, and ints ints.

Fixed (although there may be other places I haven't checked yet)

> If KeyframeAnimation is refcounted, it should be using the new create() syntax.
> instead of direct calls to "new".

Will fix this.

>  This is probably just a call to:
> dispatchGenericEvent:
> 
> +bool KeyframeAnimation::sendAnimationEvent(const AtomicString& inEventType,
> double inElapsedTime)
> +{
> +    // FIXME: Event dispatch goes here
> +    return false; // didn't dispatch an event
> 
> However I can understand if you're trying to limit the scope of your patch. :)

In this case it was an intentional limitation. A subsequent bug will add the code.

Thanks Eric!!

[Dave Hyatt]

Comment on attachment 22700 [details]
Updated patch

r=me

[Dean Jackson]

Committed r35646
	A	WebCore/manual-tests/animate-duration.html
	A	WebCore/manual-tests/animate-left.html
	A	WebCore/manual-tests/animate-none.html
	M	WebCore/rendering/RenderObject.cpp
	M	WebCore/rendering/style/RenderStyle.h
	M	WebCore/dom/Document.cpp
	M	WebCore/ChangeLog
	M	WebCore/css/CSSStyleSelector.cpp
	M	WebCore/page/AnimationController.h
	M	WebCore/page/AnimationController.cpp