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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=25108

Attachment 29394: Patch
https://bugs.webkit.org/attachment.cgi?id=29394&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // We can now safely remove any animations or transitions that are
finished.
> +    // We can't remove them any earlier because we might get a false restart
of
> +    // a transition. This can happen because we have not yet set the final
property
> +    // value until we call the rendering dispatcher. So this can make the
current
> +    // style slightly different from the desired final style (because our
last 
> +    // animation step was, say 0.9999 or something). And we need to remove
them
> +    // here because if there are no more animations running we'll never get
back
> +    // into the animation code to clean them up.
> +    RenderObjectAnimationMap::const_iterator animationsEnd =
m_compositeAnimations.end();
> +    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