[webkit-reviews] review granted: [Bug 233319] [Model] add support for pausing and resuming animations : [Attachment 444708] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 18 12:28:49 PST 2021

Wenson Hsieh <wenson_hsieh at apple.com> has granted Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 233319: [Model] add support for pausing and resuming animations

Attachment 444708: Patch


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

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

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:398
> +// MARK: â Animations support.

The review tool isn't displaying this correctly :P
I assume it's an emdash?

> Source/WebCore/Modules/model-element/scenekit/SceneKitModelPlayer.mm:103
> +void SceneKitModelPlayer::setAnimationIsPlaying(bool,

Nit - I don't think the `bool&&` rvalue reference is necessary. Just `bool` as
the completion handler argument should be fine.

It's also slightly ambiguous what the inner `bool` represents (it seems like it
indicates whether or not the call was successful?). I would either name it
`bool success` in the method signature, or use a named `enum class` here.

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:321
> +    if (!preview || !previewHasAnimationSupport(preview)) {

Nit - it seems like this can just be `!previewHasAnimationSupport(preview)`,
since `previewHasAnimationSupport` will return false if `preview` is nil

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:330
> +	   callOnMainRunLoop([error, weakThis = WTFMove(weakThis),
completionHandler = WTFMove(completionHandler)] () mutable {

Is this called off of a background thread? If so, let's double check that we
don't hit any debug assertions in WeakPtr here.

> Source/WebKit/UIProcess/WebPageProxy.cpp:10846
> +void WebPageProxy::modelElementIsPlayingAnimation(ModelIdentifier
modelIdentifier, CompletionHandler<void(Expected<bool,
WebCore::ResourceError>)>&& completionHandler)

Nit - you can omit the WebCore:: here.

> Source/WebKit/WebProcess/Model/ARKitInlinePreviewModelPlayer.mm:146
> +	   completionHandler(WTFMove(success));

Nit - just passing `success` should be okay here.

More information about the webkit-reviews mailing list