[webkit-reviews] review granted: [Bug 186507] [Web Animations] Make WPT test at timing-model/timelines/document-timelines.html pass reliably : [Attachment 343908] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 186507: [Web Animations] Make WPT test at
timing-model/timelines/document-timelines.html pass reliably
https://bugs.webkit.org/show_bug.cgi?id=186507

Attachment 343908: Patch

https://bugs.webkit.org/attachment.cgi?id=343908&action=review




--- Comment #5 from Dean Jackson <dino at apple.com> ---
Comment on attachment 343908
  --> https://bugs.webkit.org/attachment.cgi?id=343908
Patch

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

> Source/WebCore/animation/DocumentTimeline.cpp:159
> +	   // animations, so we schedule the invalidation task and registere a
whenIdle callback on the VM, which will

typo register

> Source/WebCore/animation/DocumentTimeline.cpp:164
> +	   RefPtr<DocumentTimeline> self;
> +	   m_waitingOnVMIdle = true;
> +	   m_document->vm().whenIdle([self, this]() {

You never set or use self.

> Source/WebCore/animation/DocumentTimeline.cpp:215
> +    // We want to make sure we only clear the cached currenr time if we're
not currently running

typo current

> Source/WebCore/animation/DocumentTimeline.cpp:219
> +    if (!m_waitingOnVMIdle && !m_invalidationTaskQueue.hasPendingTasks())

Are you sure you'll always have an empty invalidationTaskQueue when the
whenIdle callback happens? Or I guess it could fire, but there still be a task
to run, which will itself call maybeClearCachedCurrentTime.