Bug 233319 – [Model] add support for pausing and resuming animations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

[Model] add support for pausing and resuming animations

[Antoine Quint]

<rdar://problem/85428464>

[Antoine Quint]

Created attachment 444681 [details]
Patch

[Antoine Quint]

It is expected that this patch will fail to apply until bug 233265 lands.

[Antoine Quint]

Created attachment 444708 [details]
Patch

[Wenson Hsieh]

Comment on attachment 444708 [details]
Patch

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, CompletionHandler<void(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 anyways.

> 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.

[Antoine Quint]

Created attachment 444725 [details]
Patch for landing

[EWS]

Committed r286048 (244435@main): <https://commits.webkit.org/244435@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444725 [details].