Adapt and reland old position sticky implementation

third_party/WebKit/Source/core/frame/FrameView.cpp

Index: third_party/WebKit/Source/core/frame/FrameView.cpp
diff --git a/third_party/WebKit/Source/core/frame/FrameView.cpp b/third_party/WebKit/Source/core/frame/FrameView.cpp
index a956cd4ee393e62f18998302b8d94965cdf65e8a..e7a6f37acb34b59b737cb6904d742321b83aba9c 100644
--- a/third_party/WebKit/Source/core/frame/FrameView.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameView.cpp
@@ -1553,7 +1553,7 @@ void FrameView::didScrollTimerFired(Timer<FrameView>*)
     }
 }
 
-void FrameView::updateLayersAndCompositingAfterScrollIfNeeded()
+void FrameView::updateLayersAndCompositingAfterScrollIfNeeded(const DoubleSize& scrollDelta)
 {
     // Nothing to do after scrolling if there are no fixed position elements.
     if (!hasViewportConstrainedObjects())
@@ -1561,6 +1561,14 @@ void FrameView::updateLayersAndCompositingAfterScrollIfNeeded()
 
     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.
@@ -2129,7 +2137,7 @@ void FrameView::scrollTo(const DoublePoint& newPosition)
         m_pendingScrollDelta += scrollDelta;
 
     clearScrollAnchor();
-    updateLayersAndCompositingAfterScrollIfNeeded();
+    updateLayersAndCompositingAfterScrollIfNeeded(scrollDelta);
     scrollPositionChanged();
     frame().loader().client()->didChangeScrollOffset();
 }
@@ -3158,13 +3166,19 @@ int FrameView::scrollSize(ScrollbarOrientation orientation) const
     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));
 }