Refactor scroll updates during flexbox layout.

third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp

Index: third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
index 06d8fea21ad6e59ffb86fbca2ff2044ae4c4a6e4..a6375862b13cece03bc84dace62a6306b28bd223 100644
--- a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
@@ -623,54 +623,41 @@ IntSize PaintLayerScrollableArea::originAdjustmentForScrollbars() const
     return size;
 }
 
-void PaintLayerScrollableArea::computeScrollDimensions()
+void PaintLayerScrollableArea::updateScrollOrigin()
+{
+    if (!overflowRect().size().isZero()) {

[cbiesinger, 2016/04/29 20:02:50]

  if (!overflowRect().isEmpty()) ?

But can you explain / add a comment why this check is necessary?

[szager1, 2016/05/12 20:44:50]

This is a hack to prevent this from running before first layout (which will
crash for some reason I can't remember).  I searched for a more idiomatic way to
figure out that this is first layout, but couldn't find anything.  Added a
comment.

+        LayoutPoint scrollableOverflow = m_overflowRect.location() - LayoutSize(box().borderLeft(), box().borderTop());
+        setScrollOrigin(flooredIntPoint(-scrollableOverflow) + originAdjustmentForScrollbars());
+    }
+}
+
+void PaintLayerScrollableArea::updateScrollDimensions()
 {
     m_overflowRect = box().layoutOverflowRect();
     box().flipForWritingMode(m_overflowRect);
-
-    LayoutPoint scrollableOverflow = m_overflowRect.location() - LayoutSize(box().borderLeft(), box().borderTop());
-    setScrollOrigin(flooredIntPoint(-scrollableOverflow) + originAdjustmentForScrollbars());
+    updateScrollOrigin();
 }
 
 void PaintLayerScrollableArea::scrollToPosition(const DoublePoint& scrollPosition, ScrollOffsetClamping clamp, ScrollBehavior scrollBehavior, ScrollType scrollType)
 {
-    cancelProgrammaticScrollAnimation();

[cbiesinger, 2016/04/29 20:02:50]

BTW, why did you remove this? Just because it's not necessary because
setScrollPosition does this anyway?

[szager1, 2016/05/12 20:44:51]

It already gets called further down the call stack, I'm just eliminating a
redundant call.

-
     DoublePoint newScrollPosition = clamp == ScrollOffsetClamped ? clampScrollPosition(scrollPosition) : scrollPosition;
     if (newScrollPosition != scrollPositionDouble())
         ScrollableArea::setScrollPosition(newScrollPosition, scrollType, scrollBehavior);
 }
 
-bool PaintLayerScrollableArea::updateAfterLayout(SubtreeLayoutScope* delayedLayoutScope)
+void PaintLayerScrollableArea::updateAfterLayout()
 {
     ASSERT(box().hasOverflowClip());
 
-    bool didMarkForDelayedLayout = false;
+    bool relayoutIsPrevented = PreventRelayoutScope::relayoutIsPrevented();
+    bool scrollbarsAreFrozen = FreezeScrollbarsScope::scrollbarsAreFrozen();
 
     if (needsScrollbarReconstruction()) {
-        m_scrollbarManager.setCanDetachScrollbars(false);
         setHasHorizontalScrollbar(false);
         setHasVerticalScrollbar(false);
     }
 
-    m_scrollbarManager.setCanDetachScrollbars(true);
-
-    IntPoint originalOrigin = scrollOrigin();
-    computeScrollDimensions();
-
-    // Layout may cause us to be at an invalid scroll position. In this case we need
-    // to pull our scroll offsets back to the max (or push them up to the min).
-    DoublePoint clampedScrollPosition = clampScrollPosition(scrollPositionDouble());
-    if (clampedScrollPosition != scrollPositionDouble()) {
-        scrollToPosition(clampedScrollPosition);
-    } else if (originalOrigin != scrollOrigin()) {
-        // TODO: We should be able to use scrollOriginChanged() here, but we can't because
-        // PaintLayerScrollableArea does not maintain that flag: it gets set, but it never
-        // gets unset.  We should unset the flag after layout.
-        scrollPositionChanged(scrollPositionDouble(), ProgrammaticScroll);
-    }
-
-    m_scrollbarManager.setCanDetachScrollbars(false);
+    updateScrollDimensions();
 
     bool hasHorizontalOverflow = this->hasHorizontalOverflow();
     bool hasVerticalOverflow = this->hasVerticalOverflow();
@@ -692,7 +679,7 @@ bool PaintLayerScrollableArea::updateAfterLayout(SubtreeLayoutScope* delayedLayo
         || (box().style()->overflowX() == OverflowScroll && !horizontalScrollbar());
     bool verticalScrollBarChanged = (box().hasAutoVerticalScrollbar() && (hasVerticalScrollbar() != hasVerticalOverflow))
         || (box().style()->overflowY() == OverflowScroll && !verticalScrollbar());
-    if (!visualViewportSuppliesScrollbars() && (horizontalScrollBarChanged || verticalScrollBarChanged)) {
+    if (!scrollbarsAreFrozen && !visualViewportSuppliesScrollbars() && (horizontalScrollBarChanged || verticalScrollBarChanged)) {
         if (box().hasAutoHorizontalScrollbar())
             setHasHorizontalScrollbar(hasHorizontalOverflow);
         else if (box().style()->overflowX() == OverflowScroll)
@@ -714,13 +701,12 @@ bool PaintLayerScrollableArea::updateAfterLayout(SubtreeLayoutScope* delayedLayo
         // Our proprietary overflow: overlay value doesn't trigger a layout.
         if ((horizontalScrollBarChanged && box().style()->overflowX() != OverflowOverlay) || (verticalScrollBarChanged && box().style()->overflowY() != OverflowOverlay)) {
             if (!m_inOverflowRelayout) {
-                m_inOverflowRelayout = true;
-                if (delayedLayoutScope) {
+                if (relayoutIsPrevented) {
                     if (box().isLayoutBlock())
                         toLayoutBlock(box()).scrollbarsChanged(horizontalScrollBarChanged, verticalScrollBarChanged);
-                    delayedLayoutScope->setNeedsLayout(&box(), LayoutInvalidationReason::ScrollbarChanged);
-                    didMarkForDelayedLayout = true;
+                    PreventRelayoutScope::setNeedsLayout(box());
                 } else {
+                    m_inOverflowRelayout = true;
                     SubtreeLayoutScope layoutScope(box());
                     layoutScope.setNeedsLayout(&box(), LayoutInvalidationReason::ScrollbarChanged);
                     if (box().isLayoutBlock()) {
@@ -730,11 +716,12 @@ bool PaintLayerScrollableArea::updateAfterLayout(SubtreeLayoutScope* delayedLayo
                     } else {
                         box().layout();
                     }
+                    m_inOverflowRelayout = false;
+                    m_scrollbarManager.destroyDetachedScrollbars();
                 }
                 LayoutObject* parent = box().parent();
                 if (parent && parent->isFlexibleBox())
                     toLayoutFlexibleBox(parent)->clearCachedMainSizeForChild(box());
-                m_inOverflowRelayout = false;
             }
         }
     }
@@ -754,20 +741,45 @@ bool PaintLayerScrollableArea::updateAfterLayout(SubtreeLayoutScope* delayedLayo
         }
     }
 
-    if (hasOverlayScrollbars()) {
+    if (!scrollbarsAreFrozen && hasOverlayScrollbars()) {
         if (!scrollSize(HorizontalScrollbar))
             setHasHorizontalScrollbar(false);
         if (!scrollSize(VerticalScrollbar))
             setHasVerticalScrollbar(false);
     }
 
-    bool hasOverflow = hasScrollableHorizontalOverflow() || hasScrollableVerticalOverflow();
-    updateScrollableAreaSet(hasOverflow);
+    clampScrollPositionsAfterLayout();
+
+    if (!scrollbarsAreFrozen) {
+        bool hasOverflow = hasScrollableHorizontalOverflow() || hasScrollableVerticalOverflow();
+        updateScrollableAreaSet(hasOverflow);
+    }
 
     DisableCompositingQueryAsserts disabler;
     positionOverflowControls();
+}
+
+
+void PaintLayerScrollableArea::clampScrollPositionsAfterLayout()
+{
+    // If a vertical scrollbar was removed, the min/max scroll positions may have changed,
+    // so the scroll positions needs to be clamped.  If the scroll position did not change,
+    // but the scroll origin *did* change, we still need to notify the scrollbars to
+    // update their dimensions.
+
+    if (DelayScrollPositionClampScope::clampingIsDelayed()) {
+        DelayScrollPositionClampScope::setNeedsClamp(this);
+        return;
+    }
+
+    DoublePoint newScrollPosition = clampScrollPosition(scrollPositionDouble());
+    if (newScrollPosition != scrollPositionDouble())
+        ScrollableArea::setScrollPosition(newScrollPosition, ProgrammaticScroll);
+    else if (scrollOriginChanged())
+        scrollPositionChanged(newScrollPosition, ProgrammaticScroll);
 
-    return didMarkForDelayedLayout;
+    setNeedsScrollPositionClamp(false);
+    resetScrollOriginChanged();
 }
 
 ScrollBehavior PaintLayerScrollableArea::scrollBehaviorStyle() const
@@ -835,7 +847,6 @@ void PaintLayerScrollableArea::updateAfterStyleChange(const ComputedStyle* oldSt
     EOverflow overflowX = box().style()->overflowX();
     EOverflow overflowY = box().style()->overflowY();
 
-    // To avoid doing a relayout in updateScrollbarsAfterLayout, we try to keep any automatic scrollbar that was already present.
     bool needsHorizontalScrollbar = (hasHorizontalScrollbar() && overflowDefinesAutomaticScrollbar(overflowX)) || overflowRequiresScrollbar(overflowX);
     bool needsVerticalScrollbar = (hasVerticalScrollbar() && overflowDefinesAutomaticScrollbar(overflowY)) || overflowRequiresScrollbar(overflowY);
 
@@ -906,7 +917,7 @@ bool PaintLayerScrollableArea::updateAfterCompositingChange()
 
 void PaintLayerScrollableArea::updateAfterOverflowRecalc()
 {
-    computeScrollDimensions();
+    updateScrollDimensions();
     if (Scrollbar* horizontalScrollbar = this->horizontalScrollbar()) {
         int clientWidth = box().pixelSnappedClientWidth();
         horizontalScrollbar->setProportion(clientWidth, overflowRect().width());
@@ -1038,12 +1049,17 @@ bool PaintLayerScrollableArea::needsScrollbarReconstruction() const
 
 void PaintLayerScrollableArea::setHasHorizontalScrollbar(bool hasScrollbar)
 {
+    if (FreezeScrollbarsScope::scrollbarsAreFrozen())
+        return;
+
     if (hasScrollbar == hasHorizontalScrollbar())
         return;
 
     setScrollbarNeedsPaintInvalidation(HorizontalScrollbar);
 
-    m_scrollbarManager.setHasHorizontalScrollbar(hasScrollbar);
+    m_scrollbarManager.setHasHorizontalScrollbar(hasScrollbar, !m_inOverflowRelayout);
+
+    updateScrollOrigin();
 
     // Destroying or creating one bar can cause our scrollbar corner to come and go. We need to update the opposite scrollbar's style.
     if (hasHorizontalScrollbar())
@@ -1060,12 +1076,17 @@ void PaintLayerScrollableArea::setHasHorizontalScrollbar(bool hasScrollbar)
 
 void PaintLayerScrollableArea::setHasVerticalScrollbar(bool hasScrollbar)
 {
+    if (FreezeScrollbarsScope::scrollbarsAreFrozen())
+        return;
+
     if (hasScrollbar == hasVerticalScrollbar())
         return;
 
     setScrollbarNeedsPaintInvalidation(VerticalScrollbar);
 
-    m_scrollbarManager.setHasVerticalScrollbar(hasScrollbar);
+    m_scrollbarManager.setHasVerticalScrollbar(hasScrollbar, !m_inOverflowRelayout);
+
+    updateScrollOrigin();
 
     // Destroying or creating one bar can cause our scrollbar corner to come and go. We need to update the opposite scrollbar's style.
     if (hasHorizontalScrollbar())
@@ -1536,7 +1557,6 @@ CompositorAnimationTimeline* PaintLayerScrollableArea::compositorAnimationTimeli
 
 PaintLayerScrollableArea::ScrollbarManager::ScrollbarManager(PaintLayerScrollableArea& scrollableArea)
     : m_scrollableArea(&scrollableArea)
-    , m_canDetachScrollbars(0)
     , m_hBarIsAttached(0)
     , m_vBarIsAttached(0)
 {
@@ -1544,25 +1564,22 @@ PaintLayerScrollableArea::ScrollbarManager::ScrollbarManager(PaintLayerScrollabl
 
 void PaintLayerScrollableArea::ScrollbarManager::dispose()
 {
-    m_canDetachScrollbars = m_hBarIsAttached = m_vBarIsAttached = 0;
+    m_hBarIsAttached = m_vBarIsAttached = 0;
     destroyScrollbar(HorizontalScrollbar);
     destroyScrollbar(VerticalScrollbar);
 }
 
-void PaintLayerScrollableArea::ScrollbarManager::setCanDetachScrollbars(bool detach)
+void PaintLayerScrollableArea::ScrollbarManager::destroyDetachedScrollbars()
 {
     ASSERT(!m_hBarIsAttached || m_hBar);
     ASSERT(!m_vBarIsAttached || m_vBar);
-    m_canDetachScrollbars = detach ? 1 : 0;
-    if (!detach) {
-        if (m_hBar && !m_hBarIsAttached)
-            destroyScrollbar(HorizontalScrollbar);
-        if (m_vBar && !m_vBarIsAttached)
-            destroyScrollbar(VerticalScrollbar);
-    }
+    if (m_hBar && !m_hBarIsAttached)
+        destroyScrollbar(HorizontalScrollbar);
+    if (m_vBar && !m_vBarIsAttached)
+        destroyScrollbar(VerticalScrollbar);
 }
 
-void PaintLayerScrollableArea::ScrollbarManager::setHasHorizontalScrollbar(bool hasScrollbar)
+void PaintLayerScrollableArea::ScrollbarManager::setHasHorizontalScrollbar(bool hasScrollbar, bool canDetach)
 {
     if (hasScrollbar) {
         // This doesn't hit in any tests, but since the equivalent code in setHasVerticalScrollbar
@@ -1576,15 +1593,14 @@ void PaintLayerScrollableArea::ScrollbarManager::setHasHorizontalScrollbar(bool
         } else {
             m_hBarIsAttached = 1;
         }
-
     } else {
         m_hBarIsAttached = 0;
-        if (!m_canDetachScrollbars)
+        if (!canDetach)
             destroyScrollbar(HorizontalScrollbar);
     }
 }
 
-void PaintLayerScrollableArea::ScrollbarManager::setHasVerticalScrollbar(bool hasScrollbar)
+void PaintLayerScrollableArea::ScrollbarManager::setHasVerticalScrollbar(bool hasScrollbar, bool canDetach)
 {
     if (hasScrollbar) {
         DisableCompositingQueryAsserts disabler;
@@ -1596,10 +1612,9 @@ void PaintLayerScrollableArea::ScrollbarManager::setHasVerticalScrollbar(bool ha
         } else {
             m_vBarIsAttached = 1;
         }
-
     } else {
         m_vBarIsAttached = 0;
-        if (!m_canDetachScrollbars)
+        if (!canDetach)
             destroyScrollbar(VerticalScrollbar);
     }
 }
@@ -1650,4 +1665,105 @@ DEFINE_TRACE(PaintLayerScrollableArea::ScrollbarManager)
     visitor->trace(m_vBar);
 }
 
+int PaintLayerScrollableArea::PreventRelayoutScope::m_count = 0;
+SubtreeLayoutScope* PaintLayerScrollableArea::PreventRelayoutScope::m_layoutScope = nullptr;
+bool PaintLayerScrollableArea::PreventRelayoutScope::m_relayoutNeeded = false;
+WTF::Vector<LayoutObject*>* PaintLayerScrollableArea::PreventRelayoutScope::m_needsRelayout = nullptr;
+
+PaintLayerScrollableArea::PreventRelayoutScope::PreventRelayoutScope(SubtreeLayoutScope& layoutScope)
+{
+    if (!m_count) {
+        ASSERT(!m_layoutScope);
+        ASSERT(!m_needsRelayout || m_needsRelayout->isEmpty());
+        m_layoutScope = &layoutScope;
+    }
+    PreventRelayoutScope::increment();

[cbiesinger, 2016/04/29 20:02:50]

Why not just increment()?

Also, why do increment() and decrement() even exist? Why not just move that code
to the constructor/destructor? I don't see any other callers, and
FreezeScrollbarsScope doesn't have them...

[szager1, 2016/05/12 20:44:51]

Yeah, I had other callers before, but they're gone.  Got rid of
increment/decrement.

+}
+
+PaintLayerScrollableArea::PreventRelayoutScope::~PreventRelayoutScope()
+{
+    PreventRelayoutScope::decrement();

[cbiesinger, 2016/04/29 20:02:50]

same here

[szager1, 2016/05/12 20:44:51]

Done.

+}
+
+void PaintLayerScrollableArea::PreventRelayoutScope::increment()
+{
+    ASSERT(m_count == 0 || m_layoutScope);
+    m_count++;
+}
+
+void PaintLayerScrollableArea::PreventRelayoutScope::decrement()
+{
+    if (--m_count == 0) {
+        if (m_relayoutNeeded) {
+            for (auto layoutObject : *m_needsRelayout)
+                m_layoutScope->setNeedsLayout(layoutObject, LayoutInvalidationReason::ScrollbarChanged);
+            m_needsRelayout->clear();
+        }
+        m_layoutScope = nullptr;
+    }
+}
+
+void PaintLayerScrollableArea::PreventRelayoutScope::setNeedsLayout(LayoutObject& layoutObject)
+{
+    ASSERT(m_count);
+    ASSERT(m_layoutScope);
+    m_relayoutNeeded = true;
+    if (!m_needsRelayout)
+        m_needsRelayout = new WTF::Vector<LayoutObject*>();
+    m_needsRelayout->append(&layoutObject);
+}
+
+void PaintLayerScrollableArea::PreventRelayoutScope::resetRelayoutNeeded()
+{
+    ASSERT(m_count == 0);
+    if (m_needsRelayout)
+        m_needsRelayout->clear();

[cbiesinger, 2016/04/29 20:02:50]

This is for the case that the second pass may have added more objects to this
vector? Seems worth adding a comment for that -- at first sight, it looks like
::decrement() should have cleared the vector already in this case.

[szager1, 2016/05/12 20:44:51]

Actually, no, this should ASSERT(!m_needsRelayout ||
m_needsRelayout->isEmpty()).  Added that.

+    m_relayoutNeeded = false;
+}
+
+int PaintLayerScrollableArea::FreezeScrollbarsScope::m_count = 0;
+
+int PaintLayerScrollableArea::DelayScrollPositionClampScope::m_count = 0;
+PersistentHeapVector<Member<PaintLayerScrollableArea>>* PaintLayerScrollableArea::DelayScrollPositionClampScope::m_needsClamp = nullptr;
+
+PaintLayerScrollableArea::DelayScrollPositionClampScope::DelayScrollPositionClampScope()
+{
+    if (!m_needsClamp)
+        m_needsClamp = new PersistentHeapVector<Member<PaintLayerScrollableArea>>();

[cbiesinger, 2016/04/29 20:02:50]

Oilpan confuses me, I trust that this is the right type to use...

In PreventRelayoutScope, you lazily create the vector when first calling
setNeedsLayout. Why create this one immediately?

[szager1, 2016/05/12 20:44:51]

These are basically static vectors that never get deleted, but oilpan complains
if I actually make them non-pointer static.  The vector is going to get
allocated eventually, it's not really important where it happens.  This was just
convenient. 

+    ASSERT(m_count > 0 || m_needsClamp->isEmpty());

[cbiesinger, 2016/04/29 20:02:50]

By the way, please name all static members as s_foo per the style guide:
https://www.chromium.org/blink/coding-style#TOC-Right:18

[szager1, 2016/05/12 20:44:51]

Done.

+    DelayScrollPositionClampScope::increment();

[cbiesinger, 2016/04/29 20:02:50]

Again, why have these increment/decrement methods instead of doing the work
directly in the ctor/dtor?

[szager1, 2016/05/12 20:44:51]

Got rid of these.

+}
+
+PaintLayerScrollableArea::DelayScrollPositionClampScope::~DelayScrollPositionClampScope()
+{
+    DelayScrollPositionClampScope::decrement();
+}
+
+void PaintLayerScrollableArea::DelayScrollPositionClampScope::increment()
+{
+    m_count++;
+}
+
+void PaintLayerScrollableArea::DelayScrollPositionClampScope::decrement()
+{
+    if (--m_count == 0)
+        DelayScrollPositionClampScope::clampScrollableAreas();
+}
+
+void PaintLayerScrollableArea::DelayScrollPositionClampScope::setNeedsClamp(PaintLayerScrollableArea* scrollableArea)
+{
+    if (!scrollableArea->needsScrollPositionClamp()) {
+        scrollableArea->setNeedsScrollPositionClamp(true);
+        m_needsClamp->append(scrollableArea);
+    }
+}
+
+void PaintLayerScrollableArea::DelayScrollPositionClampScope::clampScrollableAreas()
+{
+    for (auto& scrollableArea : *m_needsClamp)
+        scrollableArea->clampScrollPositionsAfterLayout();
+    delete m_needsClamp;
+    m_needsClamp = nullptr;
+}
+
 } // namespace blink