DrawingArea: Painting is being resumed while the view is not visible

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

From my ChangeLog:

Using requestAnimationFrame and the fullscreen API on a DrawingArea-backed window
would cause rAF to permanently suspend animations when fullscreened, because of the following:

0. JavaScript causes fullscreen transition to start.
1. Painting (and rAF) are suspended.
2. The page changes size.
    a. DrawingAreaProxyImpl::sizeDidChange() calls DrawingAreaImpl::updateBackingStoreState, which calls DrawingAreaImpl::resumePainting.
    b. DrawingAreaImpl::resumePainting resumes painting, but does *not* resume rAF, because windowIsVisible is (legitimately) false.
3. The view becomes visible, windowIsVisible is updated to true.
4. visibilityDidChange() calls resumePainting again, but this time it early exits because painting is not suspended.

Notice that because of the early exit in 4, rAF is never resumed.

I have a fix:

If DrawingAreaProxyImpl::sizeDidChange or DrawingAreaProxyImpl::deviceScaleFactorDidChange are called while the view is not visible, defer their call to update the backing store state until the view is once again made visible.

<rdar://problem/11587039>

[Tim Horton]

The radar is actually <rdar://problem/11652545>

[Tim Horton]

Created attachment 147201 [details]
patch, but needs more testing before landing

[Tim Horton]

Created attachment 147433 [details]
patch (disable suspend/resumePainting messages when doing fullscreen animation)

The old patch didn't work, because we actually depend on painting *not* being frozen to paint the image that's animated to full-screen! So, it caused the animation to look terrible.

This one is a bit more wonky/scary, but I haven't been able to break it yet. Still needs more testing.

[Jer Noble]

Looks good to me.  Informal r+.

[Dean Jackson]

Comment on attachment 147433 [details]
patch (disable suspend/resumePainting messages when doing fullscreen animation)

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

> Source/WebKit2/ChangeLog:9
> +        Don't send any SuspendPainting/ResumePainting messages during full-screen animations on Mac.

Could you add more explanation here? Why was this a problem, and what are you doing now (transitions to and from fullscreen can continue to paint, etc).

> Source/WebKit2/UIProcess/API/mac/WKView.h:47
> +@property BOOL automaticallySuspendAndResumePainting;

I don't like the name of this property. Also, it's probably a bit backwards isn't it? You are now adding the functionality to NOT automatically suspend painting. Maybe it should be just called suspendsPaintingDuringWindowAnimations or something (that's a bad name too)

[Tim Horton]

(In reply to comment #5)
> (From update of attachment 147433 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147433&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        Don't send any SuspendPainting/ResumePainting messages during full-screen animations on Mac.
> 
> Could you add more explanation here? Why was this a problem, and what are you doing now (transitions to and from fullscreen can continue to paint, etc).
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.h:47
> > +@property BOOL automaticallySuspendAndResumePainting;
> 
> I don't like the name of this property. Also, it's probably a bit backwards isn't it? You are now adding the functionality to NOT automatically suspend painting. Maybe it should be just called suspendsPaintingDuringWindowAnimations or something (that's a bad name too)

"DuringWindowAnimations" is irrelevant, WKView has no idea that's why this is being used, just that it should stop sending suspend/resume messages.

[Tim Horton]

I still don't have a good name.

[Tim Horton]

Created attachment 147676 [details]
patch, name is no better, changelog is improved

[Dean Jackson]

Comment on attachment 147676 [details]
patch, name is no better, changelog is improved

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

Great changelog.

Still don't like the name. Maybe suspendPaintingWhileWindowHostingChanges ? Marking r+ and we can work out the name on irc.

> Source/WebKit2/ChangeLog:10
> +        would cause rAF to permanently suspend animations when fullscreened, because of the following:

I would invent a word like "fullscreened" in my own patch, but in review I must strangely request something like "when the page is made fullscreen" :)

[Tim Horton]

http://trac.webkit.org/changeset/120376