[webkit-reviews] review granted: [Bug 85929] regression(111639): Issue with simultaneous CSS animations : [Attachment 141827] Patch V1.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon May 14 18:28:45 PDT 2012


Dean Jackson <dino at apple.com> has granted Igor Trindade Oliveira
<igor.oliveira at webkit.org>'s request for review:
Bug 85929: regression(111639): Issue with simultaneous CSS animations
https://bugs.webkit.org/show_bug.cgi?id=85929

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=141827&action=review


> LayoutTests/animations/fill-mode-forwards.html:12
> +    function addPopAnimation () {

Space between function name and ()

> LayoutTests/animations/fill-mode-forwards.html:15
> +	   popAnimationCounter = popAnimationCounter + 1;

Just popAnimationCounter++

> LayoutTests/animations/fill-mode-forwards.html:18
> +    function removePopAnimation () {

Ditto on the space.

> LayoutTests/animations/fill-mode-forwards.html:93
> +  <div id="pulsing-square"
onwebkitanimationiteration="setTimeout(addPopAnimation(), 50);"
> +	  onwebkitanimationend="pulsingSquareEnded()"
> +	  ></div>

I think you can move the ></div> to the previous line.

> Source/WebCore/ChangeLog:10
> +	   Currently, previousTimeToNextService is just saving the previous
CompositeAnimation::timeToNextService
> +	   for AnimationControllerPrivate::updateAnimationTimerForRenderer,
however CompositeAnimation::timeToNextService
> +	   is also called and used by
AnimationControllerPrivate::updateAnimationTimer and we
> +	   are not saving the previousTimeToNextService for
AnimationControllerPrivate::updateAnimationTimer
> +	   doing AnimationController have unexpected behavior.

This last line is not correct English. I think you could probably just change
the bit after "used by AnimationControllerPrivate::updateAnimationTimer" to be
"updateAnimationTimer. Make sure we save the existing timeToNextService from
both places, updateAnimationTimerForRenderer and updateAnimationTimer." I don't
think you need the class names (e.g. AnimationControllerPrivate::) here.


More information about the webkit-reviews mailing list