Summary: | Allow support for CAAnimationGroup | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||||||
Component: | Animations | Assignee: | 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
Antoine Quint
2021-02-01 08:13:14 PST
Created attachment 418877 [details]
Patch
This is required work towards bug 219894. 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 (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. Created attachment 418885 [details]
Patch
Created attachment 418892 [details]
Patch
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 * Created attachment 418908 [details]
Patch
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(... ? Created attachment 418913 [details]
Patch
Committed r272178: <https://trac.webkit.org/changeset/272178> |