Bug 20119 - Execute CSS Animations
Summary: Execute CSS Animations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 19028
  Show dependency treegraph
 
Reported: 2008-07-20 13:24 PDT by Dean Jackson
Modified: 2008-08-08 13:44 PDT (History)
1 user (show)

See Also:


Attachments
implements CSS animations (and also does some editorial cleanup) (36.79 KB, patch)
2008-07-20 13:26 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Updated patch (42.25 KB, patch)
2008-07-22 16:40 PDT, Dean Jackson
eric: review-
Details | Formatted Diff | Diff
Tests simple repeated animation (1.18 KB, text/html)
2008-07-22 18:17 PDT, Dean Jackson
no flags Details
Tests animation duration (and correct parsing) (1.65 KB, text/html)
2008-07-22 18:17 PDT, Dean Jackson
no flags Details
Tests that "none" animation works (5.89 KB, text/html)
2008-07-22 18:18 PDT, Dean Jackson
no flags Details
Tests animation timing functions (928 bytes, text/html)
2008-07-22 18:19 PDT, Dean Jackson
no flags Details
Updated patch (43.33 KB, patch)
2008-08-07 15:04 PDT, Dean Jackson
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2008-07-20 13:24:44 PDT
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.
Comment 1 Dean Jackson 2008-07-20 13:26:06 PDT
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.
Comment 2 Dean Jackson 2008-07-22 16:40:22 PDT
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.
Comment 3 Dean Jackson 2008-07-22 18:17:11 PDT
Created attachment 22438 [details]
Tests simple repeated animation
Comment 4 Dean Jackson 2008-07-22 18:17:45 PDT
Created attachment 22439 [details]
Tests animation duration (and correct parsing)
Comment 5 Dean Jackson 2008-07-22 18:18:19 PDT
Created attachment 22440 [details]
Tests that "none" animation works
Comment 6 Dean Jackson 2008-07-22 18:19:01 PDT
Created attachment 22441 [details]
Tests animation timing functions
Comment 7 Dave Hyatt 2008-08-04 11:54:55 PDT
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);
Comment 8 Dave Hyatt 2008-08-04 11:57:13 PDT
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.
Comment 9 Eric Seidel (no email) 2008-08-04 12:02:04 PDT
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?
Comment 10 Eric Seidel (no email) 2008-08-04 12:02:41 PDT
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.
Comment 11 Dean Jackson 2008-08-07 15:04:46 PDT
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.
Comment 12 Dean Jackson 2008-08-07 15:44:11 PDT
> 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!!

Comment 13 Dave Hyatt 2008-08-08 13:04:17 PDT
Comment on attachment 22700 [details]
Updated patch

r=me
Comment 14 Dean Jackson 2008-08-08 13:44:50 PDT
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