[webkit-reviews] review granted: [Bug 223847] Plumb DisplayUpdate through the display refresh monitors : [Attachment 424498] 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 223847: Plumb DisplayUpdate through the display refresh monitors
https://bugs.webkit.org/show_bug.cgi?id=223847

Attachment 424498: Patch

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




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

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

> Source/WebCore/ChangeLog:15
> +	   that's that's necessary for downstream clients.

that's that's -> that's

> Source/WebKit/ChangeLog:15
> +	   the highest possible frequency; at various stages of propagation the
rate might be halved if
> +	   that's that's necessary for downstream clients.

that's that's -> that's

> Source/WebKit/ChangeLog:18
> +	   To make this frequency splitting logic simple, this patch introduces
DisplayUpdate, which
> +	   represents the fractional second for the current update. The
numerator is the frame index,

The first sentence here is a bit confusing here: "...DisplayUpdate, which
represents the fractional second for the current update". It seems more like
"DisplayUpdate represents an update of the display, and contains data about it
in the form of ...", is that correct?

> Source/WebCore/platform/graphics/DisplayUpdate.h:46
> +    void didUpdate()
> +    {
> +	   updateIndex = (updateIndex + 1) % updatesPerSecond;
> +    }

I think this would be a bit more easy to understand if this returned a new
DisplayUpdate and was called something like nextUpdate(). That way we could
treat these as immutable values, which is a bit easier to think about.

> Source/WebCore/platform/graphics/ios/DisplayRefreshMonitorIOS.mm:40
> +constexpr WebCore::FramesPerSecond DisplayLinkFramesPerSecond = 60;

Should this be named something like preferredDisplayLinkFramesPerSecond or
maximumDisplayLinkFramesPerSecond? It's a bit sad this is hard coded here and
not something more configurable per-page.

> Source/WebCore/platform/graphics/mac/LegacyDisplayRefreshMonitorMac.h:46
> +    const DisplayUpdate& currentUpdate() const { return m_currentUpdate; }

I think this could just return DisplayUpdate, rather than a reference, give how
small it is.