Move some scroll invalidations to the paint invalidation phase

Source/core/frame/FrameView.cpp

Index: Source/core/frame/FrameView.cpp
diff --git a/Source/core/frame/FrameView.cpp b/Source/core/frame/FrameView.cpp
index 5d42bdac25b2d6c954d44fa2973ecd91db2e3f19..e2c29b089c9d09a4feb69d2d53962b009fa04962 100644
--- a/Source/core/frame/FrameView.cpp
+++ b/Source/core/frame/FrameView.cpp
@@ -1224,12 +1224,18 @@ void FrameView::scrollContentsIfNeededRecursive()
     }
 }
 
-void FrameView::scrollContentsIfNeeded()
+// FIXME: If we had a flag to force invalidations in a whole subtree, we could get rid of this function.

[dsinclair, 2014/09/02 18:36:47]

crbug?

[Julien - ping for review, 2014/09/03 00:20:20]

crbug.com/410097 (probably unneeded but at least it's better to have it than
not).

+static void paintInvalidationRectIncludingNonCompositingDescendants(const RenderLayer* layer)

[dsinclair, 2014/09/02 18:36:48]

This method name doesn't seem to match up with what the method is doing. It
isn't getting a paintInvalidationRect it's setting
shouldDoFullPaintInvalidation.

setShouldDoFullPaintInvalidationIncludingNonCompositingDescendants ?

[Julien - ping for review, 2014/09/03 00:20:20]

Fine with the alternative name.

 {
-    bool didScroll = !pendingScrollDelta().isZero();
-    ScrollView::scrollContentsIfNeeded();
-    if (didScroll)
-        updateFixedElementPaintInvalidationRectsAfterScroll();
+    layer->renderer()->setShouldDoFullPaintInvalidation(true);
+
+    for (RenderLayer* child = layer->firstChild(); child; child = child->nextSibling()) {
+        // Don't include paint invalidation rects for composited child layers; they will paint themselves and have a different origin.
+        if (child->isPaintInvalidationContainer())
+            continue;
+
+        paintInvalidationRectIncludingNonCompositingDescendants(child);

[esprehn, 2014/09/02 20:23:10]

Instead of walking the whole tree recursively we should just have a bit that
does a Force down the tree through the repaint invalidation tree walk. That's
how style and compositing work and avoids this extra tree walk of all
descendants.

[Julien - ping for review, 2014/09/03 00:20:20]

The FIXME above exactly what covers this case :-)

I don't want to add this to the current patch as it's already complicated
enough. Also switching to using a new flag needs to cover
https://codereview.chromium.org/433603004 AFAICT so it adds too much complexity.

+    }
 }
 
 bool FrameView::scrollContentsFastPath(const IntSize& scrollDelta)
@@ -1242,7 +1248,6 @@ bool FrameView::scrollContentsFastPath(const IntSize& scrollDelta)
         return true;
     }
 
-    Region regionToUpdate;
     ViewportConstrainedObjectSet::const_iterator end = m_viewportConstrainedObjects->end();
     for (ViewportConstrainedObjectSet::const_iterator it = m_viewportConstrainedObjects->begin(); it != end; ++it) {
         RenderObject* renderer = *it;
@@ -1250,8 +1255,7 @@ bool FrameView::scrollContentsFastPath(const IntSize& scrollDelta)
         ASSERT(renderer->hasLayer());
         RenderLayer* layer = toRenderBoxModelObject(renderer)->layer();
 
-        CompositingState state = layer->compositingState();
-        if (state == PaintsIntoOwnBacking || state == PaintsIntoGroupedBacking)
+        if (layer->isPaintInvalidationContainer())
             continue;
 
         if (layer->subtreeIsInvisible())
@@ -1262,37 +1266,7 @@ bool FrameView::scrollContentsFastPath(const IntSize& scrollDelta)
         if (layer->hasAncestorWithFilterOutsets())
             return false;
 
-        IntRect updateRect = pixelSnappedIntRect(layer->paintInvalidator().paintInvalidationRectIncludingNonCompositingDescendants());
-
-        const RenderLayerModelObject* repaintContainer = layer->renderer()->containerForPaintInvalidation();
-        if (repaintContainer && !repaintContainer->isRenderView()) {
-            // Invalidate the old and new locations of fixed position elements that are not drawn into the RenderView.
-            updateRect.moveBy(scrollPosition());
-            IntRect previousRect = updateRect;
-            previousRect.move(scrollDelta);
-            // FIXME: Rather than uniting the rects, we should just issue both invalidations.
-            updateRect.unite(previousRect);
-            layer->renderer()->invalidatePaintUsingContainer(repaintContainer, updateRect, InvalidationScroll);
-        } else {
-            // Coalesce the paint invalidations that will be issued to the renderView.
-            updateRect = contentsToRootView(updateRect);
-            if (!updateRect.isEmpty())
-                regionToUpdate.unite(updateRect);
-        }
-    }
-
-    InspectorInstrumentation::didScroll(page());

[dsinclair, 2014/09/02 18:36:48]

Does this still need to exist somewhere?

[Julien - ping for review, 2014/09/03 00:20:20]

I would believe it does, it's a bad merge :(

-
-    // Invalidate the old and new locations of fixed position elements that are drawn into the RenderView.
-    Vector<IntRect> subRectsToUpdate = regionToUpdate.rects();
-    size_t viewportConstrainedObjectsCount = subRectsToUpdate.size();
-    for (size_t i = 0; i < viewportConstrainedObjectsCount; ++i) {
-        IntRect updateRect = subRectsToUpdate[i];
-        IntRect scrolledRect = updateRect;
-        scrolledRect.move(-scrollDelta);
-        updateRect.unite(scrolledRect);
-        // FIXME: We should be able to issue these invalidations separately and before we actually scroll.
-        renderView()->layer()->paintInvalidator().setBackingNeedsPaintInvalidationInRect(rootViewToContents(updateRect));
+        paintInvalidationRectIncludingNonCompositingDescendants(layer);
     }
 
     return true;
@@ -1524,32 +1498,6 @@ void FrameView::updateLayersAndCompositingAfterScrollIfNeeded()
     }
 }
 
-void FrameView::updateFixedElementPaintInvalidationRectsAfterScroll()
-{
-    if (!hasViewportConstrainedObjects())
-        return;
-
-    // Update the paint invalidation rects for fixed elements after scrolling and invalidation to reflect
-    // the new scroll position.
-    ViewportConstrainedObjectSet::const_iterator end = m_viewportConstrainedObjects->end();
-    for (ViewportConstrainedObjectSet::const_iterator it = m_viewportConstrainedObjects->begin(); it != end; ++it) {
-        RenderObject* renderer = *it;
-        // m_viewportConstrainedObjects should not contain non-viewport constrained objects.
-        ASSERT(renderer->style()->hasViewportConstrainedPosition());
-
-        // Fixed items should always have layers.
-        ASSERT(renderer->hasLayer());
-
-        RenderLayer* layer = toRenderBoxModelObject(renderer)->layer();
-
-        // Don't need to do this for composited fixed items.
-        if (layer->compositingState() == PaintsIntoOwnBacking)
-            continue;
-
-        layer->paintInvalidator().computePaintInvalidationRectsIncludingNonCompositingDescendants();
-    }
-}
-
 void FrameView::updateCompositedSelectionBoundsIfNeeded()
 {
     if (!RuntimeEnabledFeatures::compositedSelectionUpdatesEnabled())
@@ -2543,7 +2491,8 @@ void FrameView::updateLayoutAndStyleForPainting()
 
     updateWidgetPositionsIfNeeded();
 
-    if (RenderView* view = renderView()) {
+    RenderView* view = renderView();
+    if (view) {
         TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("devtools.timeline"), "UpdateLayerTree", "frame", m_frame.get());
         // FIXME(361045): remove InspectorInstrumentation calls once DevTools Timeline migrates to tracing.
         InspectorInstrumentation::willUpdateLayerTree(m_frame.get());
@@ -2556,11 +2505,13 @@ void FrameView::updateLayoutAndStyleForPainting()
         updateCompositedSelectionBoundsIfNeeded();
 
         InspectorInstrumentation::didUpdateLayerTree(m_frame.get());
-
-        invalidateTreeIfNeededRecursive();
     }
 
     scrollContentsIfNeededRecursive();
+
+    if (view)
+        invalidateTreeIfNeededRecursive();
+
     ASSERT(lifecycle().state() == DocumentLifecycle::PaintInvalidationClean);
 }