[webkit-reviews] review granted: [Bug 202525] requestAnimationFrame and cancelAnimationFrame should be present on DedicatedWorkerGlobalScope : [Attachment 390521] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Lord
<clord at igalia.com>'s request for review:
Bug 202525: requestAnimationFrame and cancelAnimationFrame should be present on
DedicatedWorkerGlobalScope
https://bugs.webkit.org/show_bug.cgi?id=202525

Attachment 390521: Patch

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




--- Comment #62 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 390521
  --> https://bugs.webkit.org/attachment.cgi?id=390521
Patch

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

> Source/WebCore/dom/RequestAnimationFrameCallback.h:48
> +    static double roundToMicrosecond(DOMHighResTimeStamp timestamp) { return
std::round(1000 * timestamp); }

Callers should use Performance::reduceTimeResolution() instead.

> Source/WebCore/dom/ScriptedAnimationController.cpp:144
> +    double highResNowMs =
RequestAnimationFrameCallback::roundToMicrosecond(timestamp);

Using the double type makes this ambiguous. Is it seconds or milliseconds? Use
DOMHighResTimeStamp.

> Source/WebCore/workers/WorkerAnimationController.cpp:118
> +    if (!m_workerGlobalScope.requestAnimationFrameEnabled())
> +	   return;
> +
> +    scheduleAnimationUsingTimer();

Doesn't WorkerAnimationController always use a timer?

> Source/WebCore/workers/WorkerAnimationController.cpp:127
> +    Seconds scheduleDelay = std::max(animationInterval -
Seconds(m_workerGlobalScope.performance().now() / 1000 -
m_lastAnimationFrameTimestamp), 0_s);

Is m_workerGlobalScope.performance().now() frozen for the duration of the
callbacks? Seems like scheduling while inside the callback should give you a
predictable cadence.

Shame about the manual seconds/milliseconds math.

> Source/WebCore/workers/WorkerAnimationController.cpp:134
> +    m_lastAnimationFrameTimestamp = m_workerGlobalScope.performance().now()
/ 1000;

Shame about the manual seconds/milliseconds math.

> Source/WebCore/workers/WorkerAnimationController.cpp:143
> +    double highResNowMs =
RequestAnimationFrameCallback::roundToMicrosecond(timestamp);

double -> strong type.

> Source/WebCore/workers/WorkerAnimationController.h:77
> +    double m_lastAnimationFrameTimestamp { 0 };

Unclear if this is a timestamp (MonotonicTime), time interval or what. Needs to
be strongly typed.