NEW 86314
LayerFlushScheduler should be suspended when painting is suspended (background tabs, etc.)
https://bugs.webkit.org/show_bug.cgi?id=86314
Summary LayerFlushScheduler should be suspended when painting is suspended (backgroun...
Tim Horton
Reported 2012-05-13 01:50:33 PDT
<rdar://problem/11441513> The LayerFlushScheduler still fires when a tab is in the background (and painting is supposed to be suspended). This causes painting to happen; one side-effect of this is that (expensive) animated GIF animations continue to run in the background.
Attachments
patch, maybe? (7.48 KB, patch)
2012-05-13 02:20 PDT, Tim Horton
darin: review-
patch (7.58 KB, patch)
2012-05-13 16:27 PDT, Tim Horton
no flags
patch for landing (7.60 KB, patch)
2012-05-13 17:05 PDT, Tim Horton
no flags
Tim Horton
Comment 1 2012-05-13 02:20:25 PDT
Created attachment 141603 [details] patch, maybe? There might be an obvious reason that we weren't doing this before that I'm just not seeing at 2AM.
Darin Adler
Comment 2 2012-05-13 16:14:50 PDT
Comment on attachment 141603 [details] patch, maybe? View in context: https://bugs.webkit.org/attachment.cgi?id=141603&action=review > Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:40 > - if (m_isSuspended) > + if (m_suspendCount) > return; > > - m_isSuspended = true; > + m_suspendCount++; > invalidate(); It’s fine to skip the invalidate, but seems wrong to skip the suspend count bumping. > Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:49 > - if (!m_isSuspended) > + if (!m_suspendCount) > return; > > - m_isSuspended = false; > + m_suspendCount--; > schedule(); Seems we should only schedule if the suspend count becomes zero.
Tim Horton
Comment 3 2012-05-13 16:15:41 PDT
(In reply to comment #2) > (From update of attachment 141603 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141603&action=review > > > Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:40 > > - if (m_isSuspended) > > + if (m_suspendCount) > > return; > > > > - m_isSuspended = true; > > + m_suspendCount++; > > invalidate(); > > It’s fine to skip the invalidate, but seems wrong to skip the suspend count bumping. > > > Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:49 > > - if (!m_isSuspended) > > + if (!m_suspendCount) > > return; > > > > - m_isSuspended = false; > > + m_suspendCount--; > > schedule(); > > Seems we should only schedule if the suspend count becomes zero. Oooh, quite right on both counts. Will fix.
Tim Horton
Comment 4 2012-05-13 16:27:00 PDT
WebKit Review Bot
Comment 5 2012-05-13 16:30:17 PDT
Attachment 141615 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:49: Missing space before ( in if( [whitespace/parens] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 6 2012-05-13 16:43:39 PDT
Comment on attachment 141615 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=141615&action=review > Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:45 > + if (!m_suspendCount) > return; Might want to assert or log here, since this means we did not balance our suspend and resume calls properly. >> Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:49 >> + if(!m_suspendCount) > > Missing space before ( in if( [whitespace/parens] [5] I agree with stylebot. Could even consider an unconditional call to schedule, since schedule already checks the suspend count. > Source/WebCore/platform/graphics/ca/LayerFlushScheduler.h:52 > + unsigned int m_suspendCount; Should just use the word unsigned here rather than unsigned int.
Tim Horton
Comment 7 2012-05-13 16:51:49 PDT
(In reply to comment #6) > (From update of attachment 141615 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141615&action=review > > > Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:45 > > + if (!m_suspendCount) > > return; > > Might want to assert or log here, since this means we did not balance our suspend and resume calls properly. I'll throw an assert in, sure. > >> Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp:49 > >> + if(!m_suspendCount) > > > > Missing space before ( in if( [whitespace/parens] [5] > > I agree with stylebot. Could even consider an unconditional call to schedule, since schedule already checks the suspend count. Stylebot and I fight about that one a lot :D (i.e. I fall back into old habits, and it reminds me gently) > > Source/WebCore/platform/graphics/ca/LayerFlushScheduler.h:52 > > + unsigned int m_suspendCount; > > Should just use the word unsigned here rather than unsigned int. Ok, sure. Since that's WebKit style, maybe we should teach the stylebot about it? Thanks for the review, Darin! I'm not sure I want to land this until I hear from Anders, though, just in case.
Tim Horton
Comment 8 2012-05-13 17:05:29 PDT
Created attachment 141620 [details] patch for landing (In reply to comment #6) > (From update of attachment 141615 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141615&action=review > Could even consider an unconditional call to schedule, since schedule already checks the suspend count. Our implementation does, but there's no reason it should/no guarantee another implementation would. I think it makes more sense to have that check in the caller, no? Attaching a patch for landing with andersca's OK.
Tim Horton
Comment 9 2012-05-14 15:18:20 PDT
Unfortunately the switching LayerFlushScheduler suspend/resume from bool to a count bit would bring back https://bugs.webkit.org/show_bug.cgi?id=72535, so we can't land this as-is. Very sad.
WebKit Review Bot
Comment 10 2012-07-27 05:02:35 PDT
Comment on attachment 141615 [details] patch Cleared Darin Adler's review+ from obsolete attachment 141615 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Note You need to log in before you can comment on or make changes to this bug.