Bug 233356 - [Model] add support for controlling looping animations
Summary: [Model] add support for controlling looping 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:
Blocks:
 
Reported: 2021-11-19 02:43 PST by Antoine Quint
Modified: 2021-11-19 10:22 PST (History)
8 users (show)

See Also:


Attachments
Patch (22.13 KB, patch)
2021-11-19 02:45 PST, Antoine Quint
wenson_hsieh: review+
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-19 02:43:47 PST
[Model] add support for controlling looping animations
Comment 1 Antoine Quint 2021-11-19 02:45:42 PST
Created attachment 444795 [details]
Patch
Comment 2 Antoine Quint 2021-11-19 02:45:48 PST
<rdar://problem/85428884>
Comment 3 Wenson Hsieh 2021-11-19 08:14:20 PST
Comment on attachment 444795 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:366
> +        callOnMainRunLoop([weakThis = WeakPtr { *this }, completionHandler = WTFMove(completionHandler), error = WebCore::ResourceError { WebCore::ResourceError::Type::General }] () mutable {

Nit - might be simpler as `completionHandler(makeUnexpected(WebCore::ResourceError { WebCore::ResourceError::Type::General }));`, instead of copying the error via block capture.

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:397
> +    callOnMainRunLoop([weakThis = WeakPtr { *this }, completionHandler = WTFMove(completionHandler)] () mutable {

Same question as the other patch — could we clarify why we need these `callOnMainRunLoop`s?
Comment 4 Antoine Quint 2021-11-19 09:59:15 PST
(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 444795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444795&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:366
> > +        callOnMainRunLoop([weakThis = WeakPtr { *this }, completionHandler = WTFMove(completionHandler), error = WebCore::ResourceError { WebCore::ResourceError::Type::General }] () mutable {
> 
> Nit - might be simpler as
> `completionHandler(makeUnexpected(WebCore::ResourceError {
> WebCore::ResourceError::Type::General }));`, instead of copying the error
> via block capture.

Absolutely, will fix when landing.

> > Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:397
> > +    callOnMainRunLoop([weakThis = WeakPtr { *this }, completionHandler = WTFMove(completionHandler)] () mutable {
> 
> Same question as the other patch — could we clarify why we need these
> `callOnMainRunLoop`s?

I think we only need those when using block-based APIs, will remove the use of `callOnMainRunLoop()` when landing.
Comment 5 Antoine Quint 2021-11-19 10:22:11 PST
Committed r286066 (244453@main): <https://commits.webkit.org/244453@main>