[webkit-reviews] review denied: [Bug 25108] Animation timer can keep firing after accelerated transitions finish : [Attachment 29394] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 10 13:44:01 PDT 2009

Darin Adler <darin at apple.com> has denied Chris Marrin <cmarrin at apple.com>'s
request for review:
Bug 25108: Animation timer can keep firing after accelerated transitions finish

Attachment 29394: Patch

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // We can now safely remove any animations or transitions that are
> +    // We can't remove them any earlier because we might get a false restart
> +    // a transition. This can happen because we have not yet set the final
> +    // value until we call the rendering dispatcher. So this can make the
> +    // style slightly different from the desired final style (because our
> +    // animation step was, say 0.9999 or something). And we need to remove
> +    // here because if there are no more animations running we'll never get
> +    // into the animation code to clean them up.
> +    RenderObjectAnimationMap::const_iterator animationsEnd =
> +    for (RenderObjectAnimationMap::const_iterator it =
m_compositeAnimations.begin(); it != animationsEnd; ++it) {
> +	   RefPtr<CompositeAnimation> compAnim = it->second;
> +	   compAnim->cleanupFinishedAnimations();
> +    }

Is there a guarantee that the cleanupFinishedAnimations function and the
functions it calls won't result in a change to m_compositeAnimations? If not,
then we'll need to copy the animations out of the map before iterating them,
because you can't safely iterate a hash table if it might change.

This unnecessarily thrashes the reference count of the CompositeAnimation
object. The local variable should be a raw pointer, or be eliminated entirely.

review- because of these two issues

More information about the webkit-reviews mailing list