Bug 223648

Summary: Add DisplayRefreshMonitorFactory
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: WebCore Misc.Assignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, cdumez, changseok, esprehn+autocc, ews-watchlist, fred.wang, glenn, gyuyoung.kim, japhet, kondapallykalyan, pdr, ryuan.choi, sabouhallawa, sergio, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=230214
Attachments:
Description Flags
Patch
cdumez: review+, ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch none

Description Simon Fraser (smfr) 2021-03-23 11:29:50 PDT
Gets createDisplayRefreshMonitor() off of DisplayRefreshMonitorClient
Comment 1 Simon Fraser (smfr) 2021-03-23 12:06:39 PDT
Created attachment 424045 [details]
Patch
Comment 2 Chris Dumez 2021-03-23 12:25:06 PDT
Comment on attachment 424045 [details]
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::WebProcessProxy::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.
Comment 3 Simon Fraser (smfr) 2021-03-23 13:53:59 PDT
Created attachment 424059 [details]
Patch
Comment 4 Simon Fraser (smfr) 2021-03-23 14:16:06 PDT
Created attachment 424064 [details]
Patch
Comment 5 EWS 2021-03-23 19:35:30 PDT
Committed r274929: <https://commits.webkit.org/r274929>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424064 [details].
Comment 6 Radar WebKit Bug Importer 2021-03-23 19:36:14 PDT
<rdar://problem/75766987>