Avoid compositing fixed-position elements if they cannot scroll

Source/core/rendering/RenderLayerCompositor.cpp

Index: Source/core/rendering/RenderLayerCompositor.cpp
diff --git a/Source/core/rendering/RenderLayerCompositor.cpp b/Source/core/rendering/RenderLayerCompositor.cpp
index 397a5e8703c6d3b0c683f0d2c716caeb5eebb490..2ba3141a2de2890920e3ef4cc1e9dfb5cfc17dfc 100644
--- a/Source/core/rendering/RenderLayerCompositor.cpp
+++ b/Source/core/rendering/RenderLayerCompositor.cpp
@@ -1924,6 +1924,31 @@ bool RenderLayerCompositor::requiresCompositingForPosition(RenderObject* rendere
         return false;
     }
     
+    // If the fixed-position element does not have any scrollable ancestor between it and
+    // its container, then we do not need to spend compositor resources for it. Start by
+    // assuming we can opt-out (i.e. no scrollable ancestor), and refine the answer below.
+    bool noScrollableAncestor = true;

[enne (OOO), 2013/04/30 01:51:35]

style nit: bools are always easier to understand when phrased positively, i.e.
"hasScrollableAncestor".

+
+    // The FrameView has the scrollbars associated with the top level viewport, so we have to

[enne (OOO), 2013/04/30 01:51:35]

I'm not sure what you mean by "top level" here.  This could be the FrameView for
an iframe.  Do you have any tests that make sure the right thing is done when
there are multiple FrameViews in play?

+    // check the FrameView in addition to the hierarchy of ancestors.
+    if (m_renderView->frameView() && (m_renderView->frameView()->isScrollable()))
+        noScrollableAncestor = false;

[enne (OOO), 2013/04/30 01:51:35]

I don't quite understand how you can set this to false here.  What happens if
there is a scrollable div between this layer and its frame in the RenderLayer
ancestor chain, but the frame is not scrollable?

Is the implicit argument here that if the overflow div is not composited that it
will fully invalidate when scrolling, so promoting the fixed pos layer is a
waste.  If the overflow div is composited then the fixed pos layer will only get
composited due to overlap? (Although, what code prevents it from incorrectly
sharing the same backing if it does overlap?)

[shawnsingh, 2013/04/30 20:53:51]

Setting it to false means that we should still composite the fixed-position
element... It won't matter if there is any additional scrolling div in the
RenderLayer ancestor change, since we already find that the FrameView is
scrollable.

You're right - "noScrollableAncestor" should be flipped to
"hasScrollableAncestor", and that should make this clearer.

Does that clear up the confusion?
That's almost my argument, yes - but I should clarify where we want to opt-in. 
Here's the way I see it:

CASE 1: fixed-pos elements that have scrollable ancestors.  we definitely want
to composite the fixed-pos element.

CASE 2: fixed-pos elements on top of scrollable siblings.  eventually we should
rely on compositing the scroll div, and then getting overlap, just like you
said.  Until then, we won't benefit much from compositing the fixed-pos element
in the common case anyway, since the scrolling div will still have to repaint.

CASE 3: fixed-pos elements that have other non-scroll related things changing
underneath.  It seems we should also rely on overlap here, too.  If any good
reason requires us to opt-in here because the changing content underneath is not
composited, we can handle it when it comes up.

Case #1 is the one we will always opt-in.  So we will not incorrectly sharing
the same backing.  Case #2 will definitely have a different backing.  Case #3 I
feel is premature to optimize for, unless we already have use cases to worry
about.

+
+    RenderLayer* ancestor = layer->parent();

[Tom Hudson, 2013/04/29 14:50:24]

Naive question: this function assumes layer is non-NULL (not just here, but
above too)?

[shawnsingh, 2013/04/29 15:57:13]

It's a "reasonable assumption" that it will not be null, because the code paths
that call this function are iterating over lists of layers, which are not
intended to have null values when passed to this function.  But it is probably a
good idea to at least add an assertion at the beginning of this function.

Could I please do that in a separate trivial patch?  If there was a case where
layer is null, it would be a bug beyond this patch, anyway.

+    while (ancestor && noScrollableAncestor) {
+        if (ancestor->hasScrollbars())

[Ian Vollick, 2013/04/30 00:22:22]

I believe the check is frameView->containsScrollableArea(layer)

+            noScrollableAncestor = false;
+        if (ancestor->renderer() == container)

[enne (OOO), 2013/04/30 01:51:35]

I think it would be clearer to call container m_renderView here.

+            break;
+        ancestor = ancestor->parent();
+    }
+
+    if (noScrollableAncestor) {
+        if (viewportConstrainedNotCompositedReason)
+            *viewportConstrainedNotCompositedReason = RenderLayer::NotCompositedForUnscrollableAncestors;
+        return false;
+    }
+
     // Subsequent tests depend on layout. If we can't tell now, just keep things the way they are until layout is done.
     if (!m_inPostLayoutUpdate) {
         m_reevaluateCompositingAfterLayout = true;