Bug 186397

Summary: Only display refresh monitors having requested display refresh callback should get notified on screen updates.
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebCore Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: ahmad.saleem792, bfulgham, commit-queue, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
bfulgham: review+
Patch none

Description Per Arne Vollan 2018-06-07 09:20:16 PDT
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.
Comment 1 Per Arne Vollan 2018-06-07 09:34:57 PDT
Created attachment 342180 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2018-06-07 09:35:34 PDT
<rdar://problem/40897835>
Comment 3 Per Arne Vollan 2018-06-08 11:34:14 PDT
Created attachment 342300 [details]
Patch
Comment 4 Brent Fulgham 2018-06-08 11:47:12 PDT
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...
Comment 5 Per Arne Vollan 2018-06-08 11:54:25 PDT
(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!
Comment 6 Per Arne Vollan 2018-06-08 14:22:37 PDT
Created attachment 342327 [details]
Patch
Comment 7 WebKit Commit Bot 2018-06-08 16:50:19 PDT
Comment on attachment 342327 [details]
Patch

Clearing flags on attachment: 342327

Committed r232652: <https://trac.webkit.org/changeset/232652>
Comment 8 Ahmad Saleem 2022-10-09 06:06:41 PDT
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!