Bug 183826

Summary: [Web Animations] Make imported/mozilla/css-animations/test_animation-pausing.html pass reliably
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, dbates, dino, esprehn+autocc, ews-watchlist, kangil.han, realdawei, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=186484
https://bugs.webkit.org/show_bug.cgi?id=183821
https://bugs.webkit.org/show_bug.cgi?id=186997
Attachments:
Description Flags
Patch dino: review+

Description Antoine Quint 2018-03-21 08:37:23 PDT
The new tests added under imported/mozilla/css-animations and imported/mozilla/css-transitions are mostly failing reliably, flaky or timing out.
Comment 1 Radar WebKit Bug Importer 2018-06-11 02:33:21 PDT
<rdar://problem/40997412>
Comment 2 Antoine Quint 2018-06-21 13:49:00 PDT
Created attachment 343269 [details]
Patch
Comment 3 Dean Jackson 2018-06-25 02:11:02 PDT
Comment on attachment 343269 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        The second issue is that some of the sub-tests would be flaky failure because the expected clearly that animations
> +        would be resolved prior to firing a requestAnimationFrame() callback, as the HTML5 event loop mandates. But until
> +        now, both DocumentTimeline and ScriptedAnimationController would make calls to DisplayRefreshMonitorManager::scheduleAnimation()
> +        that were not coordinated and so the order in which the DocumentTimeline and ScriptedAnimationController callbacks
> +        were performed was not guaranteed.

Why isn't this a separate patch?

> Source/WebCore/animation/DocumentAnimationScheduler.h:41
> +class DocumentAnimationScheduler : public RefCounted<DocumentAnimationScheduler>
> +    , public DisplayRefreshMonitorClient {

i don't think this should be split on two lines.

> Source/WebCore/animation/DocumentTimeline.cpp:-221
> -    DisplayRefreshMonitorManager::sharedManager().scheduleAnimation(*this);

So is DisplayRefreshMonitorManager::sharedManager() used anywhere now?
Comment 4 Antoine Quint 2018-06-25 02:27:07 PDT
(In reply to Dean Jackson from comment #3)
> Comment on attachment 343269 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343269&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        The second issue is that some of the sub-tests would be flaky failure because the expected clearly that animations
> > +        would be resolved prior to firing a requestAnimationFrame() callback, as the HTML5 event loop mandates. But until
> > +        now, both DocumentTimeline and ScriptedAnimationController would make calls to DisplayRefreshMonitorManager::scheduleAnimation()
> > +        that were not coordinated and so the order in which the DocumentTimeline and ScriptedAnimationController callbacks
> > +        were performed was not guaranteed.
> 
> Why isn't this a separate patch?

This patch is addressing all remaining issues that makes this test fail. I'll land in two separate commits.

> > Source/WebCore/animation/DocumentAnimationScheduler.h:41
> > +class DocumentAnimationScheduler : public RefCounted<DocumentAnimationScheduler>
> > +    , public DisplayRefreshMonitorClient {
> 
> i don't think this should be split on two lines.

Will fix.

> > Source/WebCore/animation/DocumentTimeline.cpp:-221
> > -    DisplayRefreshMonitorManager::sharedManager().scheduleAnimation(*this);
> 
> So is DisplayRefreshMonitorManager::sharedManager() used anywhere now?

Sure, DocumentAnimationScheduler uses it.
Comment 5 Antoine Quint 2018-06-25 02:58:21 PDT
First and main part of the reviewed patch landed as part of  https://bugs.webkit.org/show_bug.cgi?id=186997.
Comment 6 Antoine Quint 2018-06-25 03:00:49 PDT
Committed r233141: <https://trac.webkit.org/changeset/233141>