Bug 221209 – Allow support for CAAnimationGroup

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

Allow support for CAAnimationGroup

[Antoine Quint]

Created attachment 418877 [details]
Patch

[Antoine Quint]

This is required work towards bug 219894.

[Simon Fraser (smfr)]

Comment on attachment 418877 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:244
> +    if ([static_cast<CAAnimation*>(m_animation.get()) isKindOfClass:[CAPropertyAnimation class]])
> +        return [static_cast<CAPropertyAnimation*>(m_animation.get()) keyPath];

Instead of isKindOfClass: with its Obj-C overhead, can we instead just check m_type?

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:564
> +    for (auto& animation : value) {
> +        RefPtr<PlatformCAAnimation> animation = value[i];

Seems wrong

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 418877 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418877&action=review
> 
> > Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:244
> > +    if ([static_cast<CAAnimation*>(m_animation.get()) isKindOfClass:[CAPropertyAnimation class]])
> > +        return [static_cast<CAPropertyAnimation*>(m_animation.get()) keyPath];
> 
> Instead of isKindOfClass: with its Obj-C overhead, can we instead just check
> m_type?

I was following the approach taken with other methods such as PlatformCAAnimationCocoa::isAdditive(). Isn't it safer to use isKindOfClass here before casting? I suppose we could use the type for the test and have an ASSERT for the isKindOfClass test?

> > Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:564
> > +    for (auto& animation : value) {
> > +        RefPtr<PlatformCAAnimation> animation = value[i];
> 
> Seems wrong

Oops! The bots won't be happy about that :) That was a refactor error after integrating some early review feedback provided in bug 219894.

[Antoine Quint]

Created attachment 418885 [details]
Patch

[Antoine Quint]

Created attachment 418892 [details]
Patch

[Sam Weinig]

Comment on attachment 418892 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.h:35
> +OBJC_CLASS CAAnimationGroup;

This doesn't seemed used in the header, so you can probably leave it out.

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:191
> +    else if ([static_cast<CAAnimation*>(animation) isKindOfClass:[CAAnimationGroup class]])

CAAnimation* -> CAAnimation *

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:243
> +    if (animationType() == Group) {

I think this should probably be `animationType() != Group`?

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:244
> +        ASSERT([static_cast<CAAnimation*>(m_animation.get()) isKindOfClass:[CAPropertyAnimation class]]);

CAAnimation* -> CAAnimation *

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:245
> +        return [static_cast<CAPropertyAnimation*>(m_animation.get()) keyPath];

CAPropertyAnimation* -> CAPropertyAnimation *

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:367
> +    if ([static_cast<CAAnimation*>(m_animation.get()) isKindOfClass:[CAPropertyAnimation class]])

Why use isKindOfClass here (and below) rather than animationType() as you did in keyPath?

CAPropertyAnimation* -> CAPropertyAnimation *

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:368
> +        return [static_cast<CAPropertyAnimation*>(m_animation.get()) isAdditive];

CAPropertyAnimation* -> CAPropertyAnimation *

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:374
> +    if ([static_cast<CAAnimation*>(m_animation.get()) isKindOfClass:[CAPropertyAnimation class]])

CAPropertyAnimation* -> CAPropertyAnimation *

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:375
> +        return [static_cast<CAPropertyAnimation*>(m_animation.get()) setAdditive:value];

CAPropertyAnimation* -> CAPropertyAnimation *

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:380
> +    if ([static_cast<CAAnimation*>(m_animation.get()) isKindOfClass:[CAPropertyAnimation class]])

CAPropertyAnimation* -> CAPropertyAnimation *

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:381
> +        return fromCAValueFunctionType([[static_cast<CAPropertyAnimation*>(m_animation.get()) valueFunction] name]);

CAPropertyAnimation* -> CAPropertyAnimation *

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:387
> +    if ([static_cast<CAAnimation*>(m_animation.get()) isKindOfClass:[CAPropertyAnimation class]])

CAAnimation* -> CAAnimation *

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:388
> +        return [static_cast<CAPropertyAnimation*>(m_animation.get()) setValueFunction:[CAValueFunction functionWithName:toCAValueFunctionType(value)]];

CAPropertyAnimation* -> CAPropertyAnimation *

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:568
> +    [static_cast<CAAnimationGroup*>(m_animation.get()) setAnimations:createNSArray(value, [&] (auto& animation) -> CAAnimation * {

Why is it safe to cast to CAAnimationGroup here?

Need space after CAAnimationGroup.

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:578
> +    auto other = static_cast<CAAnimationGroup*>(downcast<PlatformCAAnimationCocoa>(value).m_animation.get());
> +    [static_cast<CAAnimationGroup*>(m_animation.get()) setAnimations:[other animations]];

Should be a space after CAAnimationGroup:

    auto other = static_cast<CAAnimationGroup *>(downcast<PlatformCAAnimationCocoa>(value).m_animation.get());
    [static_cast<CAAnimationGroup *>(m_animation.get()) setAnimations:[other animations]];

Why are these casts without any checking safe to do here?

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm:514
> +    CAAnimation* propertyAnimation = static_cast<CAAnimation*>(downcast<PlatformCAAnimationCocoa>(animation).platformAnimation());

This was wrong before as well, but the * should be on the other side when attached to an objective-c type. So this should be:

CAAnimation *propertyAnimation = static_cast<CAAnimation *>(downcast<PlatformCAAnimationCocoa>(animation).platformAnimation());

This same issue is quite prevalent in this code.

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm:532
> +    CAAnimation* propertyAnimation = static_cast<CAAnimation*>([m_layer animationForKey:key]);

CAAnimation* -> CAAnimation *

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCAAnimationRemote.mm:850
> +        [(CAPropertyAnimation*)caAnimation setAdditive:properties.additive];

CAPropertyAnimation* -> CAPropertyAnimation *

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCAAnimationRemote.mm:852
> +            [(CAPropertyAnimation*)caAnimation setValueFunction:[CAValueFunction functionWithName:toCAValueFunctionType(properties.valueFunction)]];

CAPropertyAnimation* -> CAPropertyAnimation *

[Antoine Quint]

Created attachment 418908 [details]
Patch

[Dean Jackson]

Comment on attachment 418908 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:184
> +    if ([static_cast<CAAnimation *>(animation) isKindOfClass:[CABasicAnimation class]]) {

Why not make a local variable that is the result of static_cast<CAAnimation *>(animation) and use that in the 5 places below?

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:247
> +    if (animationType() == Group)
> +        return emptyString();
> +
> +    ASSERT([static_cast<CAAnimation *>(m_animation.get()) isKindOfClass:[CAPropertyAnimation class]]);
> +    return [static_cast<CAPropertyAnimation *>(m_animation.get()) keyPath];

Should you do this (and the versions below) the other way around?

auto animation = static_cast<CAAnimation *>(m_animation.get());
if ([animation isKindOfClass:whatever])
  return [animation keyPath];

return emptyString();

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCAAnimationRemote.mm:727
> +    Vector<Properties> animations;
> +    animations.reserveInitialCapacity(values.size());
> +
> +    for (auto& value : values)
> +        animations.uncheckedAppend(downcast<PlatformCAAnimationRemote>(value.get())->properties());

Use values.map(... ?

[Antoine Quint]

Created attachment 418913 [details]
Patch

[Antoine Quint]

Committed r272178: <https://trac.webkit.org/changeset/272178>

[Radar WebKit Bug Importer]

<rdar://problem/73848093>