Bug 233319 - [Model] add support for pausing and resuming animations
Summary: [Model] add support for pausing and resuming animations
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
Keywords: InRadar
Depends on: 233265
  Show dependency treegraph
Reported: 2021-11-18 08:31 PST by Antoine Quint
Modified: 2021-11-18 23:28 PST (History)
10 users (show)

See Also:

Patch (22.87 KB, patch)
2021-11-18 08:40 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (24.04 KB, patch)
2021-11-18 11:43 PST, Antoine Quint
wenson_hsieh: review+
Details | Formatted Diff | Diff
Patch for landing (27.78 KB, patch)
2021-11-18 13:28 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2021-11-18 08:31:08 PST
[Model] add support for pausing and resuming animations
Comment 1 Antoine Quint 2021-11-18 08:33:47 PST
Comment 2 Antoine Quint 2021-11-18 08:40:43 PST
Created attachment 444681 [details]
Comment 3 Antoine Quint 2021-11-18 08:41:12 PST
It is expected that this patch will fail to apply until bug 233265 lands.
Comment 4 Antoine Quint 2021-11-18 11:43:41 PST
Created attachment 444708 [details]
Comment 5 Wenson Hsieh 2021-11-18 12:28:49 PST
Comment on attachment 444708 [details]

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.
Comment 6 Antoine Quint 2021-11-18 13:28:15 PST
Created attachment 444725 [details]
Patch for landing
Comment 7 EWS 2021-11-18 23:28:01 PST
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].