[webkit-reviews] review granted: [Bug 255953] Play Animation and Pause Animation action sheet items don't appear when a sibling link covers the animation : [Attachment 466120] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Tyler Wilcock
<tyler_w at apple.com>'s request for review:
Bug 255953: Play Animation and Pause Animation action sheet items don't appear
when a sibling link covers the animation
https://bugs.webkit.org/show_bug.cgi?id=255953

Attachment 466120: Patch

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




--- Comment #5 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 466120
  --> https://bugs.webkit.org/attachment.cgi?id=466120
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=466120&action=review

> Source/WebKit/Shared/ios/InteractionInformationRequest.cpp:98
> +    if (other.gatherAnimations != gatherAnimations)

Nit - shouldn't this be `other.gatherAnimations && !gatherAnimations`? In other
words, the currently cached position information is only invalid for the
incoming request in the case where we haven't gathered animations, but the
incoming request wants to gather animations.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:519
> +- (BOOL)_allowAnimationControls;

Nit - `WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA))` here (we still
specify availability annotations for SPI, since there may be internal clients
that may depend on this information).

> Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm:138
> +- (instancetype)_initWithType:(_WKActivatedElementType)type URL:(NSURL *)url
imageURL:(NSURL *)imageURL location:(const WebCore::IntPoint&)location
title:(NSString *)title ID:(NSString *)ID rect:(CGRect)rect
image:(WebKit::ShareableBitmap*)image imageMIMEType:(NSString *)imageMIMEType
isAnimatedImage:(BOOL)isAnimatedImage isAnimating:(BOOL)isAnimating
canShowAnimationControls:(BOOL)canShowAnimationControls
animationsUnderElement:(Vector<WebCore::ElementAnimationContext>)animationsUnde
rElement userInfo:(NSDictionary *)userInfo

Nit - either `Vector<WebCore::ElementAnimationContext>&&` or `const
Vector<WebCore::ElementAnimationContext>&` to avoid unnecessary copies.

> Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm:214
> +- (Vector<WebCore::ElementAnimationContext>)_animationsUnderElement

Nit - same here, `const Vector<WebCore::ElementAnimationContext>&` so that we
don't copy unless necessary.

> Source/WebKit/UIProcess/WebPageProxy.h:927
> +    void performActionOnElements(uint32_t action,
Vector<WebCore::ElementContext>);

Nit - `Vector<WebCore::ElementContext>&&`.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:586
> +void WebPageProxy::performActionOnElements(uint32_t action,
Vector<WebCore::ElementContext> elements)

Nit - either `Vector<WebCore::ElementContext>&&` here with `WTFMove(elements)`,
or `const Vector<WebCore::ElementContext>&`.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3302
> +	   if (std::optional elementContext = page.contextForElement(*element))

Nit - s/std::optional/auto/

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3303
> +	       info.animationsAtPoint.append({ *elementContext,
rendererAndImage->second.isAnimating() });

You can `WTFMove(*elementContext)` here.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3433
> +	   if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element))

Nit - not new in this patch, but I would use `RefPtr` here since
`setAllowsAnimation` isn't a trivial setter.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3436
> +	   if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element))

Ditto.