Bug 198005

Summary: REGRESSION (r245170): gmail.com inbox table header flickers
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CompositingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: koivisto, simon.fraser, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Testcase
none
Patch koivisto: review+

Description Simon Fraser (smfr) 2019-05-17 15:11:14 PDT
After r245471 there's still a bug where the gmail header above the inbox (where it says "Primary") flickers.
Comment 1 Simon Fraser (smfr) 2019-05-17 15:11:27 PDT
<rdar://problem/50907718>
Comment 2 Simon Fraser (smfr) 2019-05-17 15:36:58 PDT
Created attachment 370164 [details]
Testcase
Comment 3 Simon Fraser (smfr) 2019-05-17 15:38:18 PDT
I think the issue is that the layer for "header" uses "paints into composited ancestor" and paints via the root, but that ancestor is obscured behind an intermediate layer (the backdrop).

S--------C-c- -- ------ ------ 0x57ef772b0 (0,0) width=875 height=835 (layerID 16) {sc 2} RenderView
S----------c- -- ------ ------   + 0x57ef77408 (0,0) width=875 height=328  <html>
S-------XC--- -- ------ ------     + 0x57ef77560 (10,10) width=550 height=350 (layerID 21) {vc 3}  <div> class='backdrop'
---------C--- -- ------ ------     + 0x57ef776b8 (20,20) width=528 height=328 (layerID 22)  <div> class='container'
S-O------Cac- -- ------ ------     + 0x57ef77810 (14,14) width=504 height=304 (layerID 28)  <div> class='header'
------------- -- ------ ------       + 0x57ef77968 (12,12) width=200 height=100  <div> class='target'
S--A-----C--- -- ------ ------       + 0x57ef77ac0 (252,122) width=220 height=120 (layerID 27)  <div> class='animating changed'
Comment 4 Simon Fraser (smfr) 2019-05-17 15:38:45 PDT
Odd that we haven't seen this before, since the "paints into composited ancestor" logic has never consulted overlap.
Comment 5 Simon Fraser (smfr) 2019-05-17 16:27:03 PDT
OIC. We used to leave an "overlap" indirect composited reason on the layer which prevented painting into an ancestor, but now with backing sharing that gets cleared. Subtle.
Comment 6 Simon Fraser (smfr) 2019-05-17 17:03:27 PDT
Created attachment 370171 [details]
Patch
Comment 7 Antti Koivisto 2019-05-17 17:12:42 PDT
Comment on attachment 370171 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        Mak some logging changes to help diagnose things like this.

Typo

> Source/WebCore/rendering/RenderLayerCompositor.cpp:935
> +        if (layerPaintsIntoProvidedBacking) {
> +            layerPaintsIntoProvidedBacking = false;
> +            // layerPaintsIntoProvidedBacking was only true for layers that would otherwise composite because of overlap. If we can
> +            // no longer share, put this this indirect reason back on the layer so that requiresOwnBackingStore() sees it.
> +            layer.setIndirectCompositingReason(IndirectCompositingReason::Overlap);

It is bit non-obvious that layerPaintsIntoProvidedBacking implies IndirectCompositingReason::Overlap. Maybe rename it to layerPaintsIntoProvidedBackingDueToOverlap or similar to avoid future mistakes?
Comment 8 Simon Fraser (smfr) 2019-05-17 18:29:58 PDT
(In reply to Antti Koivisto from comment #7)
> Comment on attachment 370171 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370171&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:935
> > +        if (layerPaintsIntoProvidedBacking) {
> > +            layerPaintsIntoProvidedBacking = false;
> > +            // layerPaintsIntoProvidedBacking was only true for layers that would otherwise composite because of overlap. If we can
> > +            // no longer share, put this this indirect reason back on the layer so that requiresOwnBackingStore() sees it.
> > +            layer.setIndirectCompositingReason(IndirectCompositingReason::Overlap);
> 
> It is bit non-obvious that layerPaintsIntoProvidedBacking implies
> IndirectCompositingReason::Overlap. Maybe rename it to
> layerPaintsIntoProvidedBackingDueToOverlap or similar to avoid future
> mistakes?

I think making "layer needs to overlap" not an IndirectCompositingReason, but a bit on the layer would have helped avoid this, but that can happen later.
Comment 9 Simon Fraser (smfr) 2019-05-17 18:32:29 PDT
https://trac.webkit.org/r245490