[webkit-reviews] review granted: [Bug 199847] Avoid unnecessary copy of monitors under DisplayRefreshMonitorManager::displayWasUpdated() : [Attachment 374273] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 199847: Avoid unnecessary copy of monitors under
DisplayRefreshMonitorManager::displayWasUpdated()
https://bugs.webkit.org/show_bug.cgi?id=199847

Attachment 374273: Patch

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




--- Comment #5 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 374273
  --> https://bugs.webkit.org/attachment.cgi?id=374273
Patch

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

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:84
> +    auto index = findMonitorForDisplay(clientDisplayID);
> +    if (index == notFound)
>	   return;
> +    RefPtr<DisplayRefreshMonitor> monitor = m_monitors[index].monitor;
> +    if (monitor->removeClient(client)) {
> +	   if (!monitor->hasClients())
> +	       m_monitors.remove(index);

I think using this pattern is better:

if (auto* existingMonitor = monitorForDisplay(clientDisplayID)) {
    ...
}

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:128
> +    auto* monitor = monitorForDisplay(displayID);
> +    if (monitor && monitor->hasRequestedRefreshCallback())
> +	   monitor->displayLinkFired();

Ditto.

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:136
> +size_t DisplayRefreshMonitorManager::findMonitorForDisplay(PlatformDisplayID
displayID) const
> +{
> +    return m_monitors.findMatching([&](auto& monitorWrapper) {
> +	   return monitorWrapper.monitor->displayID() == displayID;
>      });
> -    for (auto& monitor : monitors) {
> -	   if (displayID == monitor->displayID() &&
monitor->hasRequestedRefreshCallback())
> -	       monitor->displayLinkFired();
> -    }
> +}

I think this function should is not needed.

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:138
> +DisplayRefreshMonitor*
DisplayRefreshMonitorManager::monitorForDisplay(PlatformDisplayID displayID)
const

monitorForDisplayID() or finMointorForDisplayID()?