[webkit-reviews] review denied: [Bug 29993] Zooming while a CSS animation is running does not recompute endpoints : [Attachment 110929] This contain the patch and the layout test for the issue .

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 13 21:17:14 PDT 2011


Dean Jackson <dino at apple.com> has denied Dean Jackson <dino at apple.com>'s
request for review:
Bug 29993: Zooming while a CSS animation is running does not recompute
endpoints
https://bugs.webkit.org/show_bug.cgi?id=29993

Attachment 110929: This contain the patch and the layout test for the issue .
https://bugs.webkit.org/attachment.cgi?id=110929&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
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.


More information about the webkit-reviews mailing list