Bug 221209

Summary: Allow support for CAAnimationGroup
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, graouts, sam, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 219894    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch none

Description Antoine Quint 2021-02-01 08:13:14 PST
Allow support for CAAnimationGroup
Comment 1 Antoine Quint 2021-02-01 08:16:42 PST
Created attachment 418877 [details]
Patch
Comment 2 Antoine Quint 2021-02-01 08:17:19 PST
This is required work towards bug 219894.
Comment 3 Simon Fraser (smfr) 2021-02-01 08:40:35 PST
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
Comment 4 Antoine Quint 2021-02-01 09:00:49 PST
(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.
Comment 5 Antoine Quint 2021-02-01 09:06:42 PST
Created attachment 418885 [details]
Patch
Comment 6 Antoine Quint 2021-02-01 10:04:51 PST
Created attachment 418892 [details]
Patch
Comment 7 Sam Weinig 2021-02-01 10:44:54 PST
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 *
Comment 8 Antoine Quint 2021-02-01 12:02:06 PST
Created attachment 418908 [details]
Patch
Comment 9 Dean Jackson 2021-02-01 12:29:24 PST
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(... ?
Comment 10 Antoine Quint 2021-02-01 12:51:09 PST
Created attachment 418913 [details]
Patch
Comment 11 Antoine Quint 2021-02-01 14:56:11 PST
Committed r272178: <https://trac.webkit.org/changeset/272178>
Comment 12 Radar WebKit Bug Importer 2021-02-01 14:57:14 PST
<rdar://problem/73848093>