[webkit-reviews] review granted: [Bug 221209] Allow support for CAAnimationGroup : [Attachment 418908] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 221209: Allow support for CAAnimationGroup
https://bugs.webkit.org/show_bug.cgi?id=221209

Attachment 418908: Patch

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




--- Comment #9 from Dean Jackson <dino at apple.com> ---
Comment on attachment 418908
  --> https://bugs.webkit.org/attachment.cgi?id=418908
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:7
27
> +    Vector<Properties> animations;
> +    animations.reserveInitialCapacity(values.size());
> +
> +    for (auto& value : values)
> +	  
animations.uncheckedAppend(downcast<PlatformCAAnimationRemote>(value.get())->pr
operties());

Use values.map(... ?