[webkit-reviews] review granted: [Bug 215826] REGRESSION (r263506): scale transform transitions won't overshoot : [Attachment 407461] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 215826: REGRESSION (r263506): scale transform transitions won't overshoot
https://bugs.webkit.org/show_bug.cgi?id=215826

Attachment 407461: Patch

https://bugs.webkit.org/attachment.cgi?id=407461&action=review




--- Comment #9 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 407461
  --> https://bugs.webkit.org/attachment.cgi?id=407461
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407461&action=review

> Source/WebCore/animation/KeyframeEffect.cpp:1680
> +	   // If we are dealing with a CSS Transition and using a cubic bezier
timing function, we set up
> +	   // the Animation object to reflect this which will allow the
GraphicsLayerCA code to set the
> +	   // timing function on the single keyframe interval rather than on
the animation at large,
> +	   // working around a Core Animation issue where setting a cubic
bezier on a CAKeyframeAnimation's
> +	   // timingFunction property can lead to clipped animations should a
value be above 1.

I think this comment could be reduced to:
// Do blah to work around a limitation of Core Animation: webkit.org/b/215918

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3341
> +	   // A CSS Transition is the only scenario where Animation::property()
will have
> +	   // its mode set to SingleProperty. In this case, we don't set the
animation-wide
> +	   // timing function, instead opting to set it using the
timingFunctions (plural)
> +	   // property to work around a Core Animation issue where setting a
cubic bezier
> +	   // on a CAKeyframeAnimation's timingFunction property (singular) can
lead to
> +	   // clipped animations should a value be above 1.
> +	   // FIXME: https://bugs.webkit.org/show_bug.cgi?id=215918

I think this comment could be reduced to:
// Do blah to work around a limitation of Core Animation: webkit.org/b/215918

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3357
> +	       // A CSS Transition is the only scenario where
Animation::property() will have
> +	       // its mode set to SingleProperty. In this case, we chose not to
set set the
> +	       // animation-wide timing function, so we set it on the single
keyframe interval
> +	       // to work around a Core Animation issue where setting a cubic
bezier on a
> +	       // CAKeyframeAnimation's timingFunction property (singular) can
lead to clipped
> +	       // animations should a value be above 1.
> +	       // FIXME: https://bugs.webkit.org/show_bug.cgi?id=215918

I think this comment could be reduced to:
// Do blah to work around a limitation of Core Animation: webkit.org/b/215918

>
LayoutTests/webanimations/accelerated-css-transition-with-easing-y-axis-above-1
.html:40
> +    if (!UIHelper.isWebKit2())
> +	   await new Promise(requestAnimationFrame);

A bit weird. Can you just await UIHelper.renderingUpdate()?