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

See Also:

Attachments
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
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

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 
<rdar://problem/85428464>
Comment 2 Antoine Quint 2021-11-18 08:40:43 PST 
Created attachment 444681 [details]
Patch
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]
Patch
Comment 5 Wenson Hsieh 2021-11-18 12:28:49 PST 
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.
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].