[webkit-reviews] review granted: [Bug 204713] Throttling requestAnimationFrame should be controlled by RenderingUpdateScheduler : [Attachment 385114] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 204713: Throttling requestAnimationFrame should be controlled by
RenderingUpdateScheduler
https://bugs.webkit.org/show_bug.cgi?id=204713

Attachment 385114: Patch

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




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

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

> Source/WebCore/dom/ScriptedAnimationController.h:66
>      WEBCORE_EXPORT bool isThrottled() const;

Does anyone still call isThrottled()?

> Source/WebCore/page/RenderingUpdateScheduler.cpp:45
> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS)

PLATFORM(IOS_FAMILY)

> Source/WebCore/page/RenderingUpdateScheduler.cpp:49
> +    if (interval < 1_s)

Why is 1_s special? Doesn't DisplayRefreshMonitorManager handle any refresh
rate?

> Source/WebCore/page/RenderingUpdateScheduler.cpp:56
> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS)

PLATFORM(IOS_FAMILY)

> Source/WebCore/page/RenderingUpdateScheduler.cpp:81
> +    if (interval < 1_s) {

Same weird cutoff.

> Source/WebCore/page/RenderingUpdateScheduler.cpp:82
> +#if PLATFORM(IOS)

PLATFORM(IOS_FAMILY)

> Source/WebCore/page/RenderingUpdateScheduler.h:57
> +#if PLATFORM(IOS)

PLATFORM(IOS_FAMILY)

> Source/WebCore/page/RenderingUpdateScheduler.h:74
> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS)

PLATFORM(IOS_FAMILY)

> Source/WebCore/page/RenderingUpdateScheduler.h:75
> +    std::once_flag m_createdMonitor;

These are always static.

> Source/WebCore/platform/graphics/AnimationFrameRate.h:48
> +constexpr const FramesPerSecond ZeroFramesPerSecond = 60;

This is very confusing.

> Source/WebCore/platform/graphics/AnimationFrameRate.h:50
> +constexpr const FramesPerSecond MaxFramesPerSecond = 60;

Maybe FullSpeedFramesPerSecond.

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:60
> -    LOG(RequestAnimationFrame,
"DisplayRefreshMonitorManager::createMonitorForClient() - created monitor %p",
monitor.get());
> +    LOG(RequestAnimationFrame,
"DisplayRefreshMonitorManager::monitorForClient() - created monitor %p",
monitor.get());

This is createMonitorForClient