Delay scrollContents until the next paint

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 c111f2c4bd18fec240b84fd55b0b9862c15f6f04..b9777dc976f955e8d7d68d5fd9f71551a05e6e58 100644
--- a/Source/core/frame/FrameView.cpp
+++ b/Source/core/frame/FrameView.cpp
@@ -1259,10 +1259,6 @@ bool FrameView::useSlowRepaintsIfNotOverlapped() const
 
 bool FrameView::shouldAttemptToScrollUsingFastPath() const
 {
-    // FIXME: useSlowRepaints reads compositing state in parent frames. Compositing state on the parent
-    // frames is not necessarily up to date.
-    // https://code.google.com/p/chromium/issues/detail?id=343766
-    DisableCompositingQueryAsserts disabler;
     return !useSlowRepaints();
 }
 
@@ -1402,6 +1398,14 @@ bool FrameView::shouldSetCursor() const
     return page && page->visibilityState() != PageVisibilityStateHidden && page->focusController().isActive();
 }
 
+void FrameView::scrollContentsIfNeeded()
+{
+    bool didScroll = !pendingScrollDelta().isZero();
+    ScrollView::scrollContentsIfNeeded();
+    if (didScroll)
+        updateFixedElementRepaintRectsAfterScroll();
+}
+
 bool FrameView::scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect)
 {
     if (!m_viewportConstrainedObjects || m_viewportConstrainedObjects->isEmpty()) {
@@ -1409,8 +1413,6 @@ bool FrameView::scrollContentsFastPath(const IntSize& scrollDelta, const IntRect
         return true;
     }
 
-    // https://code.google.com/p/chromium/issues/detail?id=343767
-    DisableCompositingQueryAsserts disabler;
     const bool isCompositedContentLayer = contentsInCompositedLayer();
 
     // Get the rects of the fixed objects visible in the rectToScroll
@@ -1473,7 +1475,7 @@ bool FrameView::scrollContentsFastPath(const IntSize& scrollDelta, const IntRect
     for (size_t i = 0; i < viewportConstrainedObjectsCount; ++i) {
         IntRect updateRect = subRectsToUpdate[i];
         IntRect scrolledRect = updateRect;
-        scrolledRect.move(scrollDelta);
+        scrolledRect.move(-scrollDelta);

[Ian Vollick, 2014/03/21 00:14:14]

Whoa. Was this a bug in the previous code?

[esprehn, 2014/03/24 21:50:43]

Yeah can you explain why do you changed this?

[ykyyip, 2014/03/24 21:56:10]

This changed because RenderView::computeRectForRepaint now returns the rect
without the pending scroll, so when an object gets promoted to a layer it
repaints the previous position in the former layer it painted into. It was OK
before because it returned the scrolled rect in the past.

         updateRect.unite(scrolledRect);
         if (isCompositedContentLayer) {
             updateRect = rootViewToContents(updateRect);
@@ -1491,11 +1493,6 @@ bool FrameView::scrollContentsFastPath(const IntSize& scrollDelta, const IntRect
 
 void FrameView::scrollContentsSlowPath(const IntRect& updateRect)
 {
-    // FIXME: This is called when JS calls scrollTo, at which point there's no guarantee that
-    // compositing state is up to date.
-    // https://code.google.com/p/chromium/issues/detail?id=343767
-    DisableCompositingQueryAsserts disabler;
-
     if (contentsInCompositedLayer()) {
         IntRect updateRect = visibleContentRect();
         ASSERT(renderView());
@@ -1684,26 +1681,50 @@ void FrameView::didScrollTimerFired(Timer<FrameView>*)
     }
 }
 
-void FrameView::repaintFixedElementsAfterScrolling()
+void FrameView::updateLayersAndCompositingAfterScrollIfNeeded()
 {
+    // Nothing to do after scrolling if there are no fixed position elements.
+    if (!hasViewportConstrainedObjects())
+        return;
+
     RefPtr<FrameView> protect(this);
-    // For fixed position elements, update widget positions and compositing layers after scrolling,
-    // but only if we're not inside of layout.
-    if (!m_nestedLayoutCount && hasViewportConstrainedObjects()) {
+
+    // If there fixed position elements, scrolling may cause compositing layers to change.
+    // Update widget and layer positions after scrolling, but only if we're not inside of
+    // layout.
+    if (!m_nestedLayoutCount) {
         updateWidgetPositions();
         if (RenderView* renderView = this->renderView())
             renderView->layer()->updateLayerPositionsAfterDocumentScroll();
     }
-}
 
-void FrameView::updateFixedElementsAfterScrolling()
-{
-    if (m_nestedLayoutCount <= 1 && hasViewportConstrainedObjects()) {
+    // Compositing layers may change after scrolling.

[Ian Vollick, 2014/03/21 00:14:14]

If we kill overlap testing and land squashing, I think this will no longer the
case. If that's true, perhaps add a FIXME so we remember to remove this?

[ykyyip, 2014/03/24 21:56:10]

Done.

[ykyyip, 2014/03/24 21:56:10]

I also wonder if we can revisit this code and make it simpler after we land
those things...

+    if (m_nestedLayoutCount <= 1) {

[esprehn, 2014/03/24 21:50:43]

How do we get into here in a nested layout?

[ykyyip, 2014/03/24 22:15:03]

I'm not sure. This code was moved from a different function that was called from
the same code and already had the check in it. I'll try removing the check for
nested layouts and see if anything breaks.

         if (RenderView* renderView = this->renderView())
             renderView->compositor()->setNeedsCompositingUpdate(CompositingUpdateOnScroll);
     }
 }
 
+void FrameView::updateFixedElementRepaintRectsAfterScroll()
+{
+    if (!hasViewportConstrainedObjects())
+        return;
+
+    // Update the repaint 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;
+        if (!renderer->style()->hasViewportConstrainedPosition())
+            continue;
+
+        // Fixed items should always have layers.
+        ASSERT(renderer->hasLayer());
+        RenderLayer* layer = toRenderBoxModelObject(renderer)->layer();

[Ian Vollick, 2014/03/21 00:14:14]

We don't need to do this if the layer is composited, right? (If it's guaranteed
that the layers won't be composited, perhaps add an assert?)

[ykyyip, 2014/03/24 21:56:10]

Yes, I think we don't need this if the layer has its own backing.

[ykyyip, 2014/03/24 21:56:10]

Done.

+        layer->repainter().computeRepaintRects(renderer->containerForRepaint());

[esprehn, 2014/03/24 21:50:43]

Does this make all fixed position things repaint constantly? If they have their
own layers we don't need to do that.

[ykyyip, 2014/03/24 22:15:03]

As far as I understand it, RenderLayerRepainter::computeRepaintRects() doesn't
cause a repaint. It just caches a rect for next call to repaintAfterLayout(), or
whoever asks for the rect next time.

+    }
+}
+
 bool FrameView::shouldRubberBandInDirection(ScrollDirection direction) const
 {
     Page* page = frame().page();
@@ -2309,8 +2330,10 @@ void FrameView::scrollTo(const IntSize& newOffset)
 {
     LayoutSize offset = scrollOffset();
     ScrollView::scrollTo(newOffset);
-    if (offset != scrollOffset())
+    if (offset != scrollOffset()) {
+        updateLayersAndCompositingAfterScrollIfNeeded();
         scrollPositionChanged();
+    }
     frame().loader().client()->didChangeScrollOffset();
 }
 
@@ -2788,8 +2811,11 @@ void FrameView::updateLayoutAndStyleForPainting()
     RefPtr<FrameView> protector(this);
 
     updateLayoutAndStyleIfNeededRecursive();
+
     if (RenderView* view = renderView())
         view->compositor()->updateCompositingLayers();
+
+    scrollContentsIfNeeded();
 }
 
 void FrameView::updateLayoutAndStyleIfNeededRecursive()