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..84bccf963c61a7d4c270e876e632968b203d7a33 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,24 @@ bool FrameView::shouldSetCursor() const
     return page && page->visibilityState() != PageVisibilityStateHidden && page->focusController().isActive();
 }
 
+void FrameView::scrollContentsIfNeededRecursive()
+{
+    scrollContentsIfNeeded();
+
+    for (LocalFrame* child = m_frame->tree().firstChild(); child; child = child->tree().nextSibling()) {
+        if (FrameView* view = child->view())
+            view->scrollContentsIfNeededRecursive();
+    }
+}
+
+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 +1423,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 +1485,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);
         updateRect.unite(scrolledRect);
         if (isCompositedContentLayer) {
             updateRect = rootViewToContents(updateRect);
@@ -1491,11 +1503,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 +1691,62 @@ 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.
+    // FIXME: Maybe no longer needed after we land squashing and kill overlap testing?
+    if (m_nestedLayoutCount <= 1) {
         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())

[esprehn, 2014/03/25 02:37:44]

It seems really weird that m_viewportConstrainedObjects can contain things that
report false to hasViewportConstrainedPosition()

[ykyyip, 2014/03/25 23:02:00]

I don't know why this check is here either. m_viewportConstrainedObjects is
modified in RenderLayerModelObject::styleDidChange(). I'll remove the check and
add an ASSERT instead.

+            continue;
+
+        // 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)

[esprehn, 2014/03/25 02:37:44]

Fixed position elements are always composited now, is this code even needed?

[ykyyip, 2014/03/25 23:02:00]

Really, that code still gets run when there are fixed position objects, e.g.
android.com/kitkat

+            continue;
+
+        // Also don't need to do this for invisible items.
+        if (layer->viewportConstrainedNotCompositedReason() == RenderLayer::NotCompositedForBoundsOutOfView
+            || layer->viewportConstrainedNotCompositedReason() == RenderLayer::NotCompositedForNoVisibleContent)
+            continue;
+
+        layer->repainter().computeRepaintRects(renderer->containerForRepaint());
+    }
+}
+
 bool FrameView::shouldRubberBandInDirection(ScrollDirection direction) const
 {
     Page* page = frame().page();
@@ -2309,8 +2352,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 +2833,11 @@ void FrameView::updateLayoutAndStyleForPainting()
     RefPtr<FrameView> protector(this);
 
     updateLayoutAndStyleIfNeededRecursive();
+
     if (RenderView* view = renderView())
         view->compositor()->updateCompositingLayers();
+
+    scrollContentsIfNeededRecursive();
 }
 
 void FrameView::updateLayoutAndStyleIfNeededRecursive()