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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 390248: Patch

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




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

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

> Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:106
> +    if (UNLIKELY(!m_workerAnimationController))

We don't use UNLIKELY unless there's proven performance benefit.

> Source/WebCore/workers/WorkerAnimationController.cpp:57
> +    callOnMainThread([this, protectedThis = makeRef(*this), displayID] ()
mutable {
> +	   this->windowScreenDidChange(displayID);
> +    });

I think it's preferable to just pass protectedThis and use protectedThis inside
the function.

> Source/WebCore/workers/WorkerAnimationController.cpp:69
> +    callOnMainThreadAndWait([this] () mutable {
> +	   this->tryUnregister();
> +    });

This makes me squirm. You're blocking the destructor on a wait, passing a
"this" that is half-destructed. What if the code under tryUnregister() tries to
retain() |this|?

Please figure out a way to unregister before the destructor.

> Source/WebCore/workers/WorkerAnimationController.cpp:129
> +RefPtr<DisplayRefreshMonitor>
WorkerAnimationController::createDisplayRefreshMonitor(PlatformDisplayID
displayID) const
> +{
> +    ASSERT(isMainThread());
> +    return
DisplayRefreshMonitor::createDefaultDisplayRefreshMonitor(displayID);
> +}
> +
> +void WorkerAnimationController::windowScreenDidChange(PlatformDisplayID
displayID)
> +{
> +    ASSERT(isMainThread());
> +   
DisplayRefreshMonitorManager::sharedManager().windowScreenDidChange(displayID,
*this);
> +}

Thinking about it more, it seems bad for every DedicatedWorkerGlobalScope to
have a WorkerAnimationController which has to deal with
DisplayRefreshMonitorManager stuff. I think a main-thread object should be
responsive for driving rAF in all workers (maybe this is just
ScriptedAnimationController).

Is there an expectation that rAF in a worker will fire when the main thread is
busy?

> Source/WebCore/workers/WorkerAnimationController.cpp:160
> +int
WorkerAnimationController::requestAnimationFrame(Ref<RequestAnimationFrameCallb
ack>&& callback)

CallbackId

> Source/WebCore/workers/WorkerAnimationController.cpp:166
> +    CallbackId callbackId = ++m_nextAnimationCallbackId;
> +    callback->m_firedOrCancelled = false;
> +    callback->m_id = callbackId;
> +    m_animationCallbacks.append(WTFMove(callback));

What's the policy on overlap of CallbackIds from main thread rAF and worker
rAF? Currently they can overlap, so a worker could pass one back to the main
thread and cancel the wrong rAF, which doesn't seem ideal.

> Source/WebCore/workers/WorkerAnimationController.cpp:223
> +    DOMHighResTimeStamp highResNowMs = std::round(1000 * timestamp);

This code needs to be shared so we don't forget to fix it sometime in future.