Fix Compositing of Opaque Scrolling Layers and Add Tests

third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp

Index: third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
diff --git a/third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp b/third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
index 8ae2819e6a25525fb5c6aeba17bda724df869e7d..5597030972678740f42906605d12dc42c4ce6bbc 100644
--- a/third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
+++ b/third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
@@ -236,18 +236,14 @@ void CompositingRequirementsUpdater::updateRecursive(PaintLayer* ancestorLayer,
         if (layer->isRootLayer() && compositor->rootShouldAlwaysComposite())
             reasonsToComposite |= CompositingReasonRoot;
 
+        // Add CompositingReasonOverflowScrollingTouch for layers that do not already have it but could.
+        // Note that m_compositingReasonFinder.directReasons(layer) already includes CompositingReasonOverflowScrollingTouch for
+        // anything that has layer->needsCompositedScrolling() true. That is, for cases where we explicitly decide not to have LCD
+        // text or cases where the layer will still support LCD text even if the layer is composited.
         if (reasonsToComposite && layer->scrollsOverflow() && !layer->needsCompositedScrolling()) {
-            // We will only set needsCompositedScrolling if we don't care about
-            // the LCD text hit, we may be able to switch to the compositor
-            // driven path if we're alread composited for other reasons and are
-            // therefore using grayscale AA.

[Stephen Chennney, 2016/09/07 20:38:40]

Clarified the meaning of this comment.

-            //
-            // FIXME: it should also be possible to promote if the layer can

[Stephen Chennney, 2016/09/07 20:38:40]

This FIXME is redundant in that we have patches in flight or bugs to address
every reason we can think of.

-            // still use LCD text when promoted, but detecting when the
-            // compositor can do this is tricky. Currently, the layer must be
-            // both opaque and may only have an integer translation as its
-            // transform. Both opacity and screen space transform are inherited
-            // properties, so this cannot be determined from local information.
+            // We can get here for a scroller that will be composited for some other reason and hence will already
+            // use grayscale AA text. We recheck for needsCompositedScrolling ignoring LCD to correctly add the
+            // CompositingReasonOverflowScrollingTouch reason to layers that can support it with grayscale AA text.
             layer->getScrollableArea()->updateNeedsCompositedScrolling(PaintLayerScrollableArea::IgnoreLCDText);
             if (layer->needsCompositedScrolling())
                 reasonsToComposite |= CompositingReasonOverflowScrollingTouch;
@@ -283,8 +279,10 @@ void CompositingRequirementsUpdater::updateRecursive(PaintLayer* ancestorLayer,
         for (size_t i = 0; i < unclippedDescendantsToRemove.size(); i++)
             unclippedDescendants.remove(unclippedDescendantsToRemove.at(unclippedDescendantsToRemove.size() - i - 1));
 
-        if (reasonsToComposite & CompositingReasonOutOfFlowClipping)
+        if (layer->clipParent()) {

[chrishtr, 2016/09/07 20:38:07]

What went wrong exactly that it had to move from CompositingReasonFinder?

[Stephen Chennney, 2016/09/07 20:38:40]

This check was previously in CompositingReasonsFinder, but then the code here
only applied when the preferCompositingToLCDText flag was set, and also did not
apply if the m_hasCompositedScrollingAncestor flag was set in the code block
above (where we add CompositingReasonOverflowScroll for things that will not be
painting LCD text). Now it will apply when it needs to, and only when it needs
to.

Note that the effect of this is to add CompositingReasonsAssumedOverlap to some
layers that previously may not have had it, but which it seems should have based
on the logic for preferCompositingToLCDText. It could be I am misunderstanding
and we only need this when we composite without LCD text, but that seems to
suggest we need to avoid promoting scrollers if this condition would be hit.

[flackr, 2016/09/07 20:57:06]

I'm not sure this is correct, it seems to me this reason should be applied only
if clipParent() is not a descendant of the nearest composited scroller. I.e. if
you promote a scroller, you need to promote descendants for this reason, but if
a scroller wasn't promoted then we don't need to promote these descendants.

[Stephen Chennney, 2016/09/07 21:08:19]

Maybe the logic covers that. We're only here if
currentRecursionData.m_hasCompositedScrollingAncestor is true, so we have a
composited scrolling ancestor. I'm having a great deal of trouble figuring out
the code immediately before this, but it seems to be only adding the
CompositingReasonAssumedOverlap reason under some circumstances, which maybe
covers your concerns.

We have the option of totally removing CompositingReasonOutOfFlowClipping. It
only gets added here now and is never used elsewhere, except if we dump reasons
for debug or test.

[flackr, 2016/09/07 22:38:41]

Adding it here causes a promotion of the current layer because
reasonsToComposite may have previously been CompositingReasonNone, right?

That being said since as you pointed out we only get into this case if we
already have an ancestor composited scroller it will most of the time be correct
to promote the current element. Can you add a comment to the effect of "We only
need to promote when the clipParent is not a descendant of the ancestor
scroller". I think we don't want to actually do this check because it would
require a walk but it's worth noting that we'll be promoting more often than we
may need to.

+            reasonsToComposite |= CompositingReasonOutOfFlowClipping;
             unclippedDescendants.append(layer);
+        }
     }
 
     const IntRect& absBounds = layer->clippedAbsoluteBoundingBox();