[Webkit-unassigned] [Bug 29993] Zooming while a CSS animation is running does not recompute endpoints

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=29993


Dean Jackson <dino at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #110929|review?                     |review-
               Flag|                            |




--- Comment #17 from Dean Jackson <dino at apple.com>  2011-10-13 21:17:15 PST ---
(From update of attachment 110929)
View in context: https://bugs.webkit.org/attachment.cgi?id=110929&action=review

> LayoutTests/ChangeLog:3
> +        The animation is not affected with change in zoom factor

Why not use the bug title here? "Zooming while a CSS animation is running does not recompute endpoints"

> LayoutTests/animations/animation-Zoom-test.html:52
> +      box.addEventListener( 'webkitAnimationEnd', function() {

space between ( and '

> LayoutTests/animations/animation-Zoom-test.html:53
> +        var boxelement = document.getElementById("box");

Above you use boxElement (camel case). You should probably do that through this entire test.

> LayoutTests/animations/animation-Zoom-test.html:55
> +        var y1 = webkitConvertPointFromNodeToPage(boxelement, new WebKitPoint(0,0)).y;

You never use y1 again. And why not just have the var be a point then use .x and .y later?

> LayoutTests/animations/animation-Zoom-test.html:57
> +        var width = (window.getComputedStyle(containerelement,null).getPropertyValue("width") ) ;

You can remove the parentheses. And add a space after the comma.

> LayoutTests/animations/animation-Zoom-test.html:62
> +          resultString += "PASS - " + "The Animation Length resizes with the new zoom factor applied."  ;

4 spaces for indentation (this applies all through the file)

> LayoutTests/animations/animation-Zoom-test.html:64
> +          resultString += "FAIL - Expected to animate till  "  +out + "px" + " but stopped at "+ width + "" ;

Weird spacing.

> Source/WebCore/ChangeLog:3
> +        The animation is not affected with change in zoom factor

Again, might as well use the bug title.

> Source/WebCore/ChangeLog:8
> +        The KeyframeList(m_keyframe) is created in the in the constructor of KeyframeAnimation 

in the in the

> Source/WebCore/ChangeLog:9
> +        class and is never updated even if the  the zoom factor changes.

the space space the

> Source/WebCore/ChangeLog:12
> +        calculate and update m_keyframe.

This isn't quite right. We never touch m_keyframe, we edit m_keyframeAnimations.

> Source/WebCore/page/animation/CompositeAnimation.cpp:213
> +        } else {

I haven't followed this through completely, but don't you still want to run the above step even if the zoom has changed, after you do the code below? Otherwise a completed (postActive) animation will use an extra step.

> Source/WebCore/page/animation/CompositeAnimation.cpp:223
> +                keyframeAnim->keyFrameAnimationStyle(renderer, targetStyle);

Inconsistent case here: keyframeAnim and keyFrameAnimationStyle. The method should use keyframe (lower case f)

> Source/WebCore/page/animation/CompositeAnimation.cpp:227
> +                    m_keyframeAnimationOrderMap.append(keyframeAnim->name().impl());

Don't you already have the name as a local variable: animationName? (same above)

> Source/WebCore/page/animation/KeyframeAnimation.h:57
> +    void keyFrameAnimationStyle(RenderObject* renderer, RenderStyle* unanimatedStyle);

keyframe not keyFrame

Also, I think the name is misleading. This method doesn't return the keyframeAnimationStyle.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.