Bug 223847 – Plumb DisplayUpdate through the display refresh monitors

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Plumb DisplayUpdate through the display refresh monitors

[Simon Fraser (smfr)]

Created attachment 424480 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 424481 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 424482 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 424483 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 424498 [details]
Patch

[Sam Weinig]

Comment on attachment 424498 [details]
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.

[Radar WebKit Bug Importer]

<rdar://problem/75958114>

[Simon Fraser (smfr)]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 424498 [details]

> > 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.

Done.

> > 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.

DisplayRefreshMonitorIOS is only used in UIWebView so I think is OK to hardcode 60.

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

Was unused, so I removed it.

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #8)
> (In reply to Sam Weinig from comment #6)
> > Comment on attachment 424498 [details]
> > > 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.
> 
> DisplayRefreshMonitorIOS is only used in UIWebView so I think is OK to
> hardcode 60.

Maybe you should Legacy- this one like you did the WebView one?

[Simon Fraser (smfr)]

Too late :P

https://trac.webkit.org/changeset/275163/webkit