[webkit-reviews] review granted: [Bug 219894] Animation of "rotate" or "scale" property does not correctly account for static "translate" property : [Attachment 418931] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 219894: Animation of "rotate" or "scale" property does not correctly
account for static "translate" property
https://bugs.webkit.org/show_bug.cgi?id=219894

Attachment 418931: Patch

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




--- Comment #23 from Dean Jackson <dino at apple.com> ---
Comment on attachment 418931
  --> https://bugs.webkit.org/attachment.cgi?id=418931
Patch

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

> Source/WebCore/ChangeLog:24
> +	   Note that animations 2 and 3 are additive and thus added in the
inverse order that we expect animations to be
> +	   applied, which is a peculiarity of Core Animations introduced in
macOS 10.15, hence the build-time flag
> +	   CA_WHERE_ADDITIVE_TRANSFORMS_ARE_REVERSED.

I think you could word this better. Due to a peculiarity of Core Animation,
additive animations are applied in an inverse order.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2893
> +    // we need to have them wrapped individually in an animation group
because Core Animation sorts not only in the order in which
> +    // animations are added to a layer, but also based on their begin time.
Since a rotate animation can have an earlier begin time

... applies animations first by their begin time, and then by the order in
which they were added (for those with the same begin time).

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2901
> +    auto animationGroupBeginTime = 1_s;

Why 1_s rather than 0?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2902
> +    auto eternalDuration = std::numeric_limits<double>::max();

I think infiniteDuration is a better name.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2910
> +	   auto animationGroup =
LayerPropertyAnimation(WTFMove(caAnimationGroup), "group-" +
createCanonicalUUIDString(), property, 0, 0, 0_s);

Do you need to use a UUID? Why not an incrementing static uint64_t? I assume
the group only has to be unique on a layer.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2917
>      enum class Additive { Yes, No };

I'm definitely not a C++ expert, but I see lots of these in WebCore. I wonder
why we don't define this somewhere common and use "using Additive =
BoolClass;"?

>
LayoutTests/webanimations/relative-ordering-of-translate-and-rotate-properties-
accelerated.html:27
> +	   0% { rotate: 180deg; }
> +	   100% { rotate: 180deg; }

You should probably pick something like 30deg. It doesn't matter in this case,
but an upside down square is going to look the same as if it was the right way
up (or turned -90/90). In other words, this test would not be able to tell if
the animation is actually applied.