Bug 223648 - Add DisplayRefreshMonitorFactory
Summary: Add DisplayRefreshMonitorFactory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-23 11:29 PDT by Simon Fraser (smfr)
Modified: 2021-09-13 05:26 PDT (History)
17 users (show)

See Also:


Attachments
Patch (49.78 KB, patch)
2021-03-23 12:06 PDT, Simon Fraser (smfr)
cdumez: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (47.75 KB, patch)
2021-03-23 13:53 PDT, Simon Fraser (smfr)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (49.79 KB, patch)
2021-03-23 14:16 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>