WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Patch
(25.75 KB, patch)
2021-02-01 09:06 PST
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Patch
(25.80 KB, patch)
2021-02-01 10:04 PST
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Patch
(36.81 KB, patch)
2021-02-01 12:02 PST
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Patch
(36.67 KB, patch)
2021-02-01 12:51 PST
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Antoine Quint
Comment 1
2021-02-01 08:16:42 PST
Created
attachment 418877
[details]
Patch
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
Created
attachment 418885
[details]
Patch
Antoine Quint
Comment 6
2021-02-01 10:04:51 PST
Created
attachment 418892
[details]
Patch
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
Created
attachment 418908
[details]
Patch
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
Created
attachment 418913
[details]
Patch
Antoine Quint
Comment 11
2021-02-01 14:56:11 PST
Committed
r272178
: <
https://trac.webkit.org/changeset/272178
>
Radar WebKit Bug Importer
Comment 12
2021-02-01 14:57:14 PST
<
rdar://problem/73848093
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug