Bug 86314 – LayerFlushScheduler should be suspended when painting is suspended (background tabs, etc.)

• Component: WebKit2
• Status: NEW

[Tim Horton]

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

[Tim Horton]

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

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

Created attachment 141615 [details]
patch

[WebKit Review Bot]

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

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

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]

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