Bug 186397 – Only display refresh monitors having requested display refresh callback should get notified on screen updates.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

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.

[Per Arne Vollan]

Created attachment 342180 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/40897835>

[Per Arne Vollan]

Created attachment 342300 [details]
Patch

[Brent Fulgham]

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...

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 342327 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 342327 [details]
Patch

Clearing flags on attachment: 342327

Committed r232652: <https://trac.webkit.org/changeset/232652>

[Ahmad Saleem]

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!