Delay scrollContents until the next paint

Source/core/frame/FrameView.cpp

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Dirk Mueller <mueller@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  *           (C) 2006 Graham Dennis (graham.dennis@gmail.com)
  *           (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
@@ skipping 1241 matching lines @@
     return false;
 }
 
 bool FrameView::useSlowRepaintsIfNotOverlapped() const
 {
     return useSlowRepaints(false);
 }
 
 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();
 }
 
 bool FrameView::contentsInCompositedLayer() const
 {
     RenderView* renderView = this->renderView();
     if (renderView && renderView->compositingState() == PaintsIntoOwnBacking) {
         GraphicsLayer* layer = renderView->layer()->compositedLayerMapping()->mainGraphicsLayer();
         if (layer && layer->drawsContent())
             return true;
@@ skipping 119 matching lines @@
 {
     return m_frame->eventHandler().lastKnownMousePosition();
 }
 
 bool FrameView::shouldSetCursor() const
 {
     Page* page = frame().page();
     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()) {
         hostWindow()->scroll(scrollDelta, rectToScroll, clipRect);
         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
     Region regionToUpdate;
     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;
 
@@ skipping 42 matching lines @@
 
     // 1) scroll
     hostWindow()->scroll(scrollDelta, rectToScroll, clipRect);
 
     // 2) update the area of fixed objects that has been invalidated
     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);
+        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);
             ASSERT(renderView());
             renderView()->layer()->repainter().setBackingNeedsRepaintInRect(updateRect);
             continue;
         }
         if (clipsRepaints())
             updateRect.intersect(rectToScroll);
         hostWindow()->invalidateContentsAndRootView(updateRect);
     }
 
     return true;
 }
 
 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());
         renderView()->layer()->repainter().setBackingNeedsRepaintInRect(updateRect);
     }
     if (RenderPart* frameRenderer = m_frame->ownerRenderer()) {
         if (isEnclosedInCompositingLayer()) {
             LayoutRect rect(frameRenderer->borderLeft() + frameRenderer->paddingLeft(),
                             frameRenderer->borderTop() + frameRenderer->paddingTop(),
                             visibleWidth(), visibleHeight());
@@ skipping 168 matching lines @@
     frame().loader().saveScrollState();
 }
 
 void FrameView::didScrollTimerFired(Timer<FrameView>*)
 {
     if (m_frame->document() && m_frame->document()->renderer()) {
         ResourceLoadPriorityOptimizer::resourceLoadPriorityOptimizer()->updateAllImageResourcePriorities();
     }
 }
 
-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();
     }
+
+    // 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::updateFixedElementsAfterScrolling()
+void FrameView::updateFixedElementRepaintRectsAfterScroll()
 {
-    if (m_nestedLayoutCount <= 1 && hasViewportConstrainedObjects()) {
-        if (RenderView* renderView = this->renderView())
-            renderView->compositor()->setNeedsCompositingUpdate(CompositingUpdateOnScroll);
+    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();
     if (!page)
         return ScrollView::shouldRubberBandInDirection(direction);
     return page->chrome().client().shouldRubberBandInDirection(direction);
 }
@@ skipping 588 matching lines @@
 bool FrameView::isActive() const
 {
     Page* page = frame().page();
     return page && page->focusController().isActive();
 }
 
 void FrameView::scrollTo(const IntSize& newOffset)
 {
     LayoutSize offset = scrollOffset();
     ScrollView::scrollTo(newOffset);
-    if (offset != scrollOffset())
+    if (offset != scrollOffset()) {
+        updateLayersAndCompositingAfterScrollIfNeeded();
         scrollPositionChanged();
+    }
     frame().loader().client()->didChangeScrollOffset();
 }
 
 void FrameView::invalidateScrollbarRect(Scrollbar* scrollbar, const IntRect& rect)
 {
     // Add in our offset within the FrameView.
     IntRect dirtyRect = rect;
     dirtyRect.moveBy(scrollbar->location());
 
     if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled() && isInPerformLayout()) {
@@ skipping 457 matching lines @@
 
     ScrollView::paintOverhangAreas(context, horizontalOverhangArea, verticalOverhangArea, dirtyRect);
 }
 
 void FrameView::updateLayoutAndStyleForPainting()
 {
     // Updating layout can run script, which can tear down the FrameView.
     RefPtr<FrameView> protector(this);
 
     updateLayoutAndStyleIfNeededRecursive();
+
     if (RenderView* view = renderView())
         view->compositor()->updateCompositingLayers();
+
+    scrollContentsIfNeeded();
 }
 
 void FrameView::updateLayoutAndStyleIfNeededRecursive()
 {
     // We have to crawl our entire tree looking for any FrameViews that need
     // layout and make sure they are up to date.
     // Mac actually tests for intersection with the dirty region and tries not to
     // update layout for frames that are outside the dirty region.  Not only does this seem
     // pointless (since those frames will have set a zero timer to layout anyway), but
     // it is also incorrect, since if two frames overlap, the first could be excluded from the dirty
@@ skipping 413 matching lines @@
 void FrameView::willRemoveScrollbar(Scrollbar* scrollbar, ScrollbarOrientation orientation)
 {
     ScrollableArea::willRemoveScrollbar(scrollbar, orientation);
     if (AXObjectCache* cache = axObjectCache()) {
         cache->remove(scrollbar);
         cache->handleScrollbarUpdate(this);
     }
 }
 
 } // namespace WebCore