RESOLVED FIXED 88940
DrawingArea: Painting is being resumed while the view is not visible
https://bugs.webkit.org/show_bug.cgi?id=88940
Summary DrawingArea: Painting is being resumed while the view is not visible
Tim Horton
Reported 2012-06-12 17:22:25 PDT
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>
Attachments
patch, but needs more testing before landing (5.16 KB, patch)
2012-06-12 17:45 PDT, Tim Horton
no flags
patch (disable suspend/resumePainting messages when doing fullscreen animation) (7.72 KB, patch)
2012-06-13 15:53 PDT, Tim Horton
dino: review-
patch, name is no better, changelog is improved (9.15 KB, patch)
2012-06-14 16:20 PDT, Tim Horton
dino: review+
Tim Horton
Comment 1 2012-06-12 17:32:53 PDT
The radar is actually <rdar://problem/11652545>
Tim Horton
Comment 2 2012-06-12 17:45:24 PDT
Created attachment 147201 [details] patch, but needs more testing before landing
Tim Horton
Comment 3 2012-06-13 15:53:42 PDT
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
Comment 4 2012-06-14 13:39:20 PDT
Looks good to me. Informal r+.
Dean Jackson
Comment 5 2012-06-14 13:48:46 PDT
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
Comment 6 2012-06-14 15:18:43 PDT
(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
Comment 7 2012-06-14 15:18:53 PDT
I still don't have a good name.
Tim Horton
Comment 8 2012-06-14 16:20:11 PDT
Created attachment 147676 [details] patch, name is no better, changelog is improved
Dean Jackson
Comment 9 2012-06-14 16:32:32 PDT
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
Comment 10 2012-06-14 17:04:26 PDT
Note You need to log in before you can comment on or make changes to this bug.