[webkit-reviews] review granted: [Bug 223648] Add DisplayRefreshMonitorFactory : [Attachment 424045] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Mar 23 12:25:06 PDT 2021


Chris Dumez <cdumez at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 223648: Add DisplayRefreshMonitorFactory
https://bugs.webkit.org/show_bug.cgi?id=223648

Attachment 424045: Patch

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




--- Comment #2 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 424045
  --> https://bugs.webkit.org/attachment.cgi?id=424045
Patch

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

r=me

> Source/WebCore/loader/EmptyClients.cpp:147
> +class EmptyDisplayRefreshMonitorFactory : public
DisplayRefreshMonitorFactory {

Can the class be marked as final?

> Source/WebCore/page/RenderingUpdateScheduler.h:37
> +class RenderingUpdateScheduler : public DisplayRefreshMonitorClient {

Can this class be marked as final?

> Source/WebCore/platform/graphics/GraphicsLayerUpdater.h:43
> +class GraphicsLayerUpdater : public DisplayRefreshMonitorClient {

Can this class be marked as final?

> Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp:34
> +

I don't think we usually have this empty line in between

> Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp:44
> +{

ASSERT(isMainRunLoop()); ? since I assume DisplayLinkObserverID::generate() is
not thread safe.

> Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp:49
> +   
WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessPro
xy::StopDisplayLink(m_observerID, displayID()), 0);

Is there a chance requestRefreshCallback() has not been called yet? If so, is
there a way to early return to avoid the IPC is that case?

> Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp:61
> +	       this->m_firstCallbackInCurrentRunloop = true;

this-> should not be needed.

> Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.h:36
> +class DisplayRefreshMonitorMac : public WebCore::DisplayRefreshMonitor {

Can this class be marked as final?

> Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.h:45
> +    void displayLinkFired() override;

Can this be final?

> Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.h:46
> +    bool requestRefreshCallback() override;

ditto

> Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.h:51
> +    bool hasRequestedRefreshCallback() const override { return
m_hasSentMessage; }

ditto.


More information about the webkit-reviews mailing list