RESOLVED FIXED 221209
Allow support for CAAnimationGroup
https://bugs.webkit.org/show_bug.cgi?id=221209
Summary Allow support for CAAnimationGroup
Antoine Quint
Reported 2021-02-01 08:13:14 PST
Allow support for CAAnimationGroup
Attachments
Patch (25.81 KB, patch)
2021-02-01 08:16 PST, Antoine Quint
ews-feeder: commit-queue-
Patch (25.75 KB, patch)
2021-02-01 09:06 PST, Antoine Quint
no flags
Patch (25.80 KB, patch)
2021-02-01 10:04 PST, Antoine Quint
no flags
Patch (36.81 KB, patch)
2021-02-01 12:02 PST, Antoine Quint
no flags
Patch (36.67 KB, patch)
2021-02-01 12:51 PST, Antoine Quint
no flags
Antoine Quint
Comment 1 2021-02-01 08:16:42 PST
Antoine Quint
Comment 2 2021-02-01 08:17:19 PST
This is required work towards bug 219894.
Simon Fraser (smfr)
Comment 3 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
Antoine Quint
Comment 4 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.
Antoine Quint
Comment 5 2021-02-01 09:06:42 PST
Antoine Quint
Comment 6 2021-02-01 10:04:51 PST
Sam Weinig
Comment 7 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 *
Antoine Quint
Comment 8 2021-02-01 12:02:06 PST
Dean Jackson
Comment 9 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(... ?
Antoine Quint
Comment 10 2021-02-01 12:51:09 PST
Antoine Quint
Comment 11 2021-02-01 14:56:11 PST
Radar WebKit Bug Importer
Comment 12 2021-02-01 14:57:14 PST
Note You need to log in before you can comment on or make changes to this bug.