Since all display refresh monitors in the WebContent process share a single UI process display link, we should make sure that only the monitors having requested callback, are getting notified on screen updates. This does not appear to be a problem currently, but we should safeguard the code for future code changes.
Created attachment 342180 [details] Patch
<rdar://problem/40897835>
Created attachment 342300 [details] Patch
Comment on attachment 342300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342300&action=review I think this seems like a very reasonable approach. We just need to be sure there aren't cases where we were relying on an active monitor (that has not sent a message recently) getting updates. We might find that we need to be more explicit about specifically requesting refresh callback if we encounter animation or other rendering issues. > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=186397 Radar? > Source/WebCore/ChangeLog:9 > + we should make sure that only the monitors having requested callback, are getting notified on screen No comma after "callback" please. > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:131 > + if (monitor->hasRequestedRefreshCallback()) How frequently do we have active monitors that have not requested refresh callbacks? I'm curious how much of a benefit this might be...
(In reply to Brent Fulgham from comment #4) > Comment on attachment 342300 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342300&action=review > > I think this seems like a very reasonable approach. We just need to be sure > there aren't cases where we were relying on an active monitor (that has not > sent a message recently) getting updates. > > We might find that we need to be more explicit about specifically requesting > refresh callback if we encounter animation or other rendering issues. > > > Source/WebCore/ChangeLog:4 > > + https://bugs.webkit.org/show_bug.cgi?id=186397 > > Radar? > > > Source/WebCore/ChangeLog:9 > > + we should make sure that only the monitors having requested callback, are getting notified on screen > > No comma after "callback" please. > > > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:131 > > + if (monitor->hasRequestedRefreshCallback()) > > How frequently do we have active monitors that have not requested refresh > callbacks? I'm curious how much of a benefit this might be... I think this is rare. While inspecting the code, I only found that this is only possible if the window is changing screen while a display refresh monitor is active. Thanks for reviewing!
Created attachment 342327 [details] Patch
Comment on attachment 342327 [details] Patch Clearing flags on attachment: 342327 Committed r232652: <https://trac.webkit.org/changeset/232652>
Checked via GitHub using BugID and it landed in following: https://github.com/WebKit/WebKit/commit/86295446fc35891e6e5c57909e64023fbc77380f and didn't backed out. Marking this as "RESOLVED FIXED". Thanks!