Adapt and reland old position sticky implementation

third_party/WebKit/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 1535 matching lines @@
     frame().loader().saveScrollState();
 }
 
 void FrameView::didScrollTimerFired(Timer<FrameView>*)
 {
     if (m_frame->document() && m_frame->document()->layoutView()) {
         ResourceLoadPriorityOptimizer::resourceLoadPriorityOptimizer()->updateAllImageResourcePriorities();
     }
 }
 
-void FrameView::updateLayersAndCompositingAfterScrollIfNeeded()
+void FrameView::updateLayersAndCompositingAfterScrollIfNeeded(const DoubleSize& scrollDelta)
 {
     // Nothing to do after scrolling if there are no fixed position elements.
     if (!hasViewportConstrainedObjects())
         return;
 
     RefPtrWillBeRawPtr<FrameView> protect(this);
 
+    // Update sticky positioned elements which scroll with the viewport.

[chrishtr, 2015/10/06 17:08:33]

Are position: sticky elements spec'ed to stick to the viewport only, or to the
containing scroller?

https://drafts.csswg.org/css-position-3/#sticky-pos

indicates it should be the containing scroller. Just the viewport would be very
problematic, and introduce another way in which containing block and paint
properties disagree, when in fact the paint team is pushing to go in the
opposite direction.

[flackr, 2015/10/07 20:38:12]

They're spec'ed to stick to the viewport if no ancestor has a scrolling box.
Otherwise, they just stick to that containing scroller. In the latest patch I've
updated this to only add sticky elements with no ancestor scrolling box to the
viewport constrained objects.

+    for (const auto& viewportConstrainedObject : *m_viewportConstrainedObjects) {
+        LayoutObject* layoutObject = viewportConstrainedObject;
+        DeprecatedPaintLayer* layer = toLayoutBoxModelObject(layoutObject)->layer();
+        if (layoutObject->style()->position() == StickyPosition && layer->scrollsWithViewport())
+            layer->updateLayerPositionsAfterOverflowScroll(scrollDelta);
+    }
+
     // 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 (LayoutView* layoutView = this->layoutView())
             layoutView->layer()->setNeedsCompositingInputsUpdate();
     }
 }
 
@@ skipping 548 matching lines @@
         // Don't scroll the FrameView!
         ASSERT_NOT_REACHED();
     }
 
     m_scrollPosition = newPosition;
 
     if (!scrollbarsSuppressed())
         m_pendingScrollDelta += scrollDelta;
 
     clearScrollAnchor();
-    updateLayersAndCompositingAfterScrollIfNeeded();
+    updateLayersAndCompositingAfterScrollIfNeeded(scrollDelta);
     scrollPositionChanged();
     frame().loader().client()->didChangeScrollOffset();
 }
 
 void FrameView::invalidatePaintForTickmarks() const
 {
     if (Scrollbar* scrollbar = verticalScrollbar())
         scrollbar->invalidate();
 }
 
@@ skipping 1008 matching lines @@
     // If no scrollbars are present, the content may still be scrollable.
     if (!scrollbar) {
         IntSize scrollSize = m_contentsSize - visibleContentRect().size();
         scrollSize.clampNegativeToZero();
         return orientation == HorizontalScrollbar ? scrollSize.width() : scrollSize.height();
     }
 
     return scrollbar->totalSize() - scrollbar->visibleSize();
 }
 
-void FrameView::setScrollOffset(const IntPoint& offset, ScrollType)
+void FrameView::setScrollOffset(const IntPoint& offset, ScrollType type)
 {
+    // Scrolling is triggered directly in response to events but should be
+    // processed after compositing update. http://crbug.com/420741
+    DisableCompositingQueryAsserts disabler;

[chrishtr, 2015/10/06 17:08:33]

What is the code path that is now active which leads to querying compositing
state at the wrong time?

[flackr, 2015/10/07 20:38:12]

The stack trace is here:
#0  0x00007fdaea9281b1 in blink::PaintLayer::compositingState
(this=0x12df96c10e80) at
../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2213
#1  0x00007fdaea9294a9 in blink::PaintLayer::isPaintInvalidationContainer
(this=0x12df96c10e80) at
../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:878
#2  0x00007fdaea9293a8 in
blink::PaintLayer::updateLayerPositionsAfterOverflowScroll (this=0x12df96c10e80,
scrollDelta=...) at
../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:330
#3  0x00007fdaea6f8515 in
blink::FrameView::updateLayersAndCompositingAfterScrollIfNeeded
(this=0x1fffcd344010, scrollDelta=...) at
../../third_party/WebKit/Source/core/frame/FrameView.cpp:1547
#4  0x00007fdaea6fa212 in blink::FrameView::scrollTo (this=0x1fffcd344010,
newPosition=...) at
../../third_party/WebKit/Source/core/frame/FrameView.cpp:2104

Basically we try to query if we're scrolling a paint invalidation container and
update paint invalidation for the scroll when we update layer positions in
response to the scroll.

What do you think about adding PaintLayer::updateLayerPositionAfterFrameScroll
which shouldn't need to recurse and (not sure how correct this is) doesn't
update paint invalidation? (See latest patch) This doesn't require disabling
compositing query asserts.

[chrishtr, 2015/11/04 01:40:44]

Why is the compositing data stale when setScrollOffset is called here? Accessing
that data is only illegal when the document lifecycle has not yet been fully
advanced.

[flackr, 2015/11/25 21:03:01]

We have the same disabler in PaintLayerScrollableArea::setScrollOffset
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
It's a known bug that we're processing scroll updates before compositing update:
http://crbug.com/420741

     scrollTo(clampScrollPosition(offset));
 }
 
-void FrameView::setScrollOffset(const DoublePoint& offset, ScrollType)
+void FrameView::setScrollOffset(const DoublePoint& offset, ScrollType type)
 {
+    // Scrolling is triggered directly in response to events but should be
+    // processed after compositing update. http://crbug.com/420741
+    DisableCompositingQueryAsserts disabler;

[chrishtr, 2015/10/06 17:08:33]

Same question here.

[flackr, 2015/10/07 20:38:12]

Same as above.

     scrollTo(clampScrollPosition(offset));
 }
 
 void FrameView::windowResizerRectChanged()
 {
     updateScrollbars(scrollOffsetDouble());
 }
 
 bool FrameView::hasOverlayScrollbars() const
 {
@@ skipping 757 matching lines @@
 
     if (!graphicsLayer)
         return;
 
     DeprecatedPaintLayer::mapRectToPaintInvalidationBacking(localFrame->contentLayoutObject(), paintInvalidationContainer, viewRect);
 
     graphicsLayerTimingRequests.add(graphicsLayer, Vector<std::pair<int64_t, WebRect>>()).storedValue->value.append(std::make_pair(m_frame->frameID(), enclosingIntRect(viewRect)));
 }
 
 } // namespace blink