[webkit-reviews] review granted: [Bug 223912] Allow non-60fps display updates to be driven by DisplayRefreshMonitor : [Attachment 424616] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 223912: Allow non-60fps display updates to be driven by
DisplayRefreshMonitor
https://bugs.webkit.org/show_bug.cgi?id=223912

Attachment 424616: Patch

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




--- Comment #3 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 424616
  --> https://bugs.webkit.org/attachment.cgi?id=424616
Patch

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

Love the tests!

> Source/WebCore/ChangeLog:21
> +	   Also add preferredFramesPerSecond() to use here, and have both it
and

I'm unclear what "here" is in this context.

> Source/WebCore/page/Page.cpp:1201
>      renderingUpdateScheduler().windowScreenDidChange(displayID);
> +    renderingUpdateScheduler().adjustRenderingUpdateFrequency();

I would either pull renderingUpdateScheduler into a local (since the getter
does work) or just have windowScreenDidChange() call
adjustRenderingUpdateFrequency(),

> Source/WebCore/page/Page.h:429
> +    // This can return nullopt if throttling reasons result in a frequency
less than one, in which case
> +    // preferredRenderingUpdateInterval provides the frequency.
> +    Optional<FramesPerSecond> preferredRenderingUpdateFramesPerSecond()
const;

This seems a little silly that two calls are needed. In the future, we should
we consider making FramesPerSecond be a struct containing a ratio that can
represent frequencies less than one, or make this return a
Variant<FramesPerSecond, Seconds>.

> Source/WebCore/platform/Logging.cpp:95
> +    LogRequestAnimationFrame.state = WTFLogChannelState::On;
> +    LogDisplayLink.state = WTFLogChannelState::On;

Did you mean to leave these in?

> Source/WebCore/platform/graphics/AnimationFrameRate.cpp:33
> +static constexpr OptionSet<ThrottlingReason> halfSpeedThrottlingReasons = {
ThrottlingReason::LowPowerMode,
ThrottlingReason::NonInteractedCrossOriginFrame, ThrottlingReason::VisuallyIdle
};

I don't think the `=` is needed here.