Issue repaints before and after changing the squashing assignment of a RenderLayer.

Source/core/rendering/RenderLayerRepainter.cpp

Index: Source/core/rendering/RenderLayerRepainter.cpp
diff --git a/Source/core/rendering/RenderLayerRepainter.cpp b/Source/core/rendering/RenderLayerRepainter.cpp
index 8d33c3215fcc3431e5a6d7436c70f3586f3f4ebc..f60ccc46cb455902af56a65c234a4cfcd80894e0 100644
--- a/Source/core/rendering/RenderLayerRepainter.cpp
+++ b/Source/core/rendering/RenderLayerRepainter.cpp
@@ -152,10 +152,14 @@ void RenderLayerRepainter::repaintIncludingNonCompositingDescendants()
 
 void RenderLayerRepainter::repaintIncludingNonCompositingDescendantsInternal(const RenderLayerModelObject* repaintContainer)
 {
-    m_renderer.repaintUsingContainer(repaintContainer, pixelSnappedIntRect(m_renderer.computeRepaintRect()), InvalidationLayer);

[ojan, 2014/06/03 18:41:11]

Are there cases where we call this without a repaintContainer? Presumably that's
why you change this function call?

boundsRectForRepaint concerns me somewhat as it makes it easy to accidentally
not pass a repaintContainer and get the slow, but correct result.

[chrishtr, 2014/06/03 20:47:08]

The computeRepaintRect() method is the wrong one to call. I'm in the process of
refactoring the
RenderObject code as well to make it harder/impossible to call the wrong method.

+    m_renderer.repaintUsingContainer(repaintContainer, pixelSnappedIntRect(m_renderer.boundsRectForRepaint(repaintContainer)), InvalidationLayer);
+
+    // FIXME: Repaints can be issued during style recalc at present, via RenderLayerModelObject::styleWillChange. This happens in scenarios when
+    // repaint is needed but not layout.
+    DisableCompositingQueryAsserts disabler;

[Ian Vollick, 2014/06/03 17:05:16]

This seems reasonable to me, but I wonder if it would make sense to add a
possiblyStaleCompositingState accessor for use during repainting. We want to
invalidate our old rects in terms of the old composited layer tree, right? If
that's the case, maybe we should formally allow accessing the stale state?

[chrishtr, 2014/06/03 17:14:52]

Yes.
Separating old and new is what we need to do, I agree.

[ojan, 2014/06/03 18:41:11]

This will be fixed when we stop doing repaints during style recalc, right?
Adding a staleCompositingState has appealed to me in the past, but everytime
I've thought through how it would actually work, it's led to a mess of spaghetti
code. I'm not opposed to the idea, but it's also not clearly good to me unless
we can find a clean way of writing the code. My hope is that we instead
structure all the code so that we don't need to query stale data outside of
InCompositingUpdate, where it's ok-ish as long as we're very careful.

Also, presumably you saw tests fail without this disabler? Please list one
failing test for reference in the comment. That way, if someone tries to remove
this in the future and no tests fail, they can have some confidence that the
disabler is actually no longer needed.

[chrishtr, 2014/06/03 20:47:08]

Yes, if that is actually the case. We would need to do:

1. Style recalc, set dirty bits on repaint for various things
2. Issue repaints for the dirty things with the existing compositing state
3. Update compositing state
4. Issue repaints for the dirty things with the new compositing state

With that kind of phasing, we can have more accurate asserts.
Yes. When loading Calendar. But for some reason does not happen any more. And no
layout tests crash.

 
     for (RenderLayer* curr = m_renderer.layer()->firstChild(); curr; curr = curr->nextSibling()) {
-        if (!curr->hasCompositedLayerMapping())
+        if (curr->compositingState() != PaintsIntoOwnBacking && curr->compositingState() != PaintsIntoGroupedBacking)

[Ian Vollick, 2014/06/03 17:05:16]

Do we want to include HasOwnBackingButPaintsIntoAncestor here? It seems like we
shouldn't? Here and below.

[chrishtr, 2014/06/03 17:14:52]

Don't know for sure. I don't really understand
HasOwnBackingButPaintsIntoAncestor. 

[chrishtr, 2014/06/03 18:14:32]

Ok will leave it. if it paints into the ancestor, repaints should be included
when computing
recursive repaint rects that paint into the same backing, which is the intent of
this method.

[Ian Vollick, 2014/06/03 18:24:00]

Yeah, I agree. The behavior change seems good. I've got an idea that will make
this change a bit cleaner and remove the need for the disabler. Can we add a
method, which would return true if the compositing state was neither
PaintsIntoOwnBacking nor PaintsIntoGroupedBacking? If we named it something
invalidation-specific, we could access the stale state. Maybe
RenderLayer::repaintsIntoAncestorContainer? (RenderLayer::paintsIntoAncestor
would be nicer, but doesn't provide a hint that this is intended just for
invalidation).

[chrishtr, 2014/06/03 20:47:07]

Hmm.

The case here I agree does not actually violate any chicken-egg constraints. But
somehow we need
to encapsulate that fact in the interfaces. It's not ok for random code to call
your proposed paintsIntoAncestor() for example... need to think about this some
more.

             curr->repainter().repaintIncludingNonCompositingDescendantsInternal(repaintContainer);
     }
 }
@@ -170,7 +174,7 @@ LayoutRect RenderLayerRepainter::repaintRectIncludingNonCompositingDescendants()
 
     for (RenderLayer* child = m_renderer.layer()->firstChild(); child; child = child->nextSibling()) {
         // Don't include repaint rects for composited child layers; they will paint themselves and have a different origin.
-        if (child->hasCompositedLayerMapping())
+        if (child->compositingState() == PaintsIntoOwnBacking || child->compositingState() == PaintsIntoGroupedBacking)
             continue;
 
         repaintRect.unite(child->repainter().repaintRectIncludingNonCompositingDescendants());