Refactor ScrollView::updateScrollbars()

Source/platform/scroll/ScrollView.cpp

Index: Source/platform/scroll/ScrollView.cpp
diff --git a/Source/platform/scroll/ScrollView.cpp b/Source/platform/scroll/ScrollView.cpp
index 12c7d974106dbb2fff98c5776cc851f42525dcfc..07d927bf20937509e7828e6ec45e3e9bc6346d50 100644
--- a/Source/platform/scroll/ScrollView.cpp
+++ b/Source/platform/scroll/ScrollView.cpp
@@ -31,6 +31,7 @@
 #include "platform/HostWindow.h"
 #include "platform/scroll/ScrollbarTheme.h"
 #include "wtf/StdLibExtras.h"
+#include "wtf/TemporaryChange.h"
 
 using namespace std;
 
@@ -44,7 +45,6 @@ ScrollView::ScrollView()
     , m_scrollbarsAvoidingResizer(0)
     , m_scrollbarsSuppressed(false)
     , m_inUpdateScrollbars(false)
-    , m_updateScrollbarsPass(0)
     , m_drawPanScrollIcon(false)
     , m_paintsEntireContents(false)
     , m_clipsRepaints(true)
@@ -324,29 +324,19 @@ void ScrollView::windowResizerRectChanged()
     updateScrollbars(scrollOffset());
 }
 
-static const unsigned cMaxUpdateScrollbarsPass = 2;
-
-void ScrollView::updateScrollbars(const IntSize& desiredOffset)
+static bool useOverlayScrollbars()
 {
-    if (m_inUpdateScrollbars)
-        return;
-
-    // If we came in here with the view already needing a layout, then go ahead and do that
-    // first.  (This will be the common case, e.g., when the page changes due to window resizing for example).
-    // This layout will not re-enter updateScrollbars and does not count towards our max layout pass total.
-    if (!m_scrollbarsSuppressed) {
-        m_inUpdateScrollbars = true;
-        scrollbarExistenceDidChange();
-        m_inUpdateScrollbars = false;
-    }
-
-    IntRect oldScrollCornerRect = scrollCornerRect();
+    // FIXME: Need to consider CSS custom scrollbars.

[Julien - ping for review, 2014/06/05 21:10:03]

Custom CSS scrollbars are not-overlay scrollbars (see
RenderScrollbar::isOverlayScrollbar()) so it's weird to me that we have this
FIXME. Also as it stands this ASSERT is not very actionable nor precise so we
would need to clarify it.

[trchen, 2014/06/05 23:55:27]

How about this:
// FIXME: Need to detect the presence of CSS custom scrollbars, which are
non-overlay regardless the ScrollbarTheme.

+    return ScrollbarTheme::theme()->usesOverlayScrollbars();
+}
 
+void ScrollView::computeScrollbarExistence(bool& newHasHorizontalScrollbar, bool& newHasVerticalScrollbar, ComputeScrollbarExistenceOption option) const
+{
     bool hasHorizontalScrollbar = m_horizontalScrollbar;
     bool hasVerticalScrollbar = m_verticalScrollbar;
 
-    bool newHasHorizontalScrollbar = hasHorizontalScrollbar;
-    bool newHasVerticalScrollbar = hasVerticalScrollbar;
+    newHasHorizontalScrollbar = hasHorizontalScrollbar;
+    newHasVerticalScrollbar = hasVerticalScrollbar;
 
     ScrollbarMode hScroll = m_horizontalScrollbarMode;
     ScrollbarMode vScroll = m_verticalScrollbarMode;
@@ -356,86 +346,40 @@ void ScrollView::updateScrollbars(const IntSize& desiredOffset)
     if (vScroll != ScrollbarAuto)
         newHasVerticalScrollbar = (vScroll == ScrollbarAlwaysOn);
 
-    if (m_scrollbarsSuppressed || (hScroll != ScrollbarAuto && vScroll != ScrollbarAuto)) {
-        if (hasHorizontalScrollbar != newHasHorizontalScrollbar)
-            setHasHorizontalScrollbar(newHasHorizontalScrollbar);
-        if (hasVerticalScrollbar != newHasVerticalScrollbar)
-            setHasVerticalScrollbar(newHasVerticalScrollbar);
-    } else {
-        bool scrollbarExistenceChanged = false;
+    if (m_scrollbarsSuppressed || (hScroll != ScrollbarAuto && vScroll != ScrollbarAuto))
+        return;
 
-        IntSize docSize = contentsSize();
-        IntSize fullVisibleSize = visibleContentRect(IncludeScrollbars).size();
+    IntSize docSize = contentsSize();
 
-        bool scrollbarsAreOverlay = ScrollbarTheme::theme()->usesOverlayScrollbars();
+    if (hScroll == ScrollbarAuto)
+        newHasHorizontalScrollbar = docSize.width() > visibleWidth();
+    if (vScroll == ScrollbarAuto)
+        newHasVerticalScrollbar = docSize.height() > visibleHeight();
 
-        if (hScroll == ScrollbarAuto) {
-            newHasHorizontalScrollbar = docSize.width() > visibleWidth();
-            if (!scrollbarsAreOverlay && newHasHorizontalScrollbar && !m_updateScrollbarsPass && docSize.width() <= fullVisibleSize.width() && docSize.height() <= fullVisibleSize.height())
-                newHasHorizontalScrollbar = false;
-        }
-        if (vScroll == ScrollbarAuto) {
-            newHasVerticalScrollbar = docSize.height() > visibleHeight();
-            if (!scrollbarsAreOverlay && newHasVerticalScrollbar && !m_updateScrollbarsPass && docSize.width() <= fullVisibleSize.width() && docSize.height() <= fullVisibleSize.height())
-                newHasVerticalScrollbar = false;
-        }
-
-        if (!scrollbarsAreOverlay) {
-            // If we ever turn one scrollbar off, always turn the other one off too.  Never ever
-            // try to both gain/lose a scrollbar in the same pass.
-            if (!newHasHorizontalScrollbar && hasHorizontalScrollbar && vScroll != ScrollbarAlwaysOn)
-                newHasVerticalScrollbar = false;
-            if (!newHasVerticalScrollbar && hasVerticalScrollbar && hScroll != ScrollbarAlwaysOn)
-                newHasHorizontalScrollbar = false;
-        }
-
-        if (hasHorizontalScrollbar != newHasHorizontalScrollbar) {
-            scrollbarExistenceChanged = true;
-            if (scrollOrigin().y() && !newHasHorizontalScrollbar && !scrollbarsAreOverlay)
-                ScrollableArea::setScrollOrigin(IntPoint(scrollOrigin().x(), scrollOrigin().y() - m_horizontalScrollbar->height()));

[Julien - ping for review, 2014/06/05 21:10:03]

Don't we need to keep this call to make sure the scroll origin is up-to-date?

[trchen, 2014/06/05 23:55:27]

This line is actually bogus. There is no way for ScrollView to predict the new
scrollOrigin after layout, and it is fishy that we don't do the opposite when we
gain a scrollbar.

FrameView take its responsibility to setScrollOrigin() in
FrameView::adjustViewSize(), and RenderLayer does it in
RenderLayerScrollableArea::updateAfterLayout(). I'm not aware of other
implementation that uses non-zero scrollOrigin.

-            if (hasHorizontalScrollbar)
-                m_horizontalScrollbar->invalidate();
-            setHasHorizontalScrollbar(newHasHorizontalScrollbar);
-        }
+    if (useOverlayScrollbars())
+        return;
 
-        if (hasVerticalScrollbar != newHasVerticalScrollbar) {
-            scrollbarExistenceChanged = true;
-            if (scrollOrigin().x() && !newHasVerticalScrollbar && !scrollbarsAreOverlay)
-                ScrollableArea::setScrollOrigin(IntPoint(scrollOrigin().x() - m_verticalScrollbar->width(), scrollOrigin().y()));
-            if (hasVerticalScrollbar)
-                m_verticalScrollbar->invalidate();
-            setHasVerticalScrollbar(newHasVerticalScrollbar);
-        }
+    IntSize fullVisibleSize = visibleContentRect(IncludeScrollbars).size();
 
-        if (scrollbarExistenceChanged) {
-            if (scrollbarsAreOverlay) {
-                // Synchronize status of scrollbar layers if necessary.
-                m_inUpdateScrollbars = true;
-                scrollbarExistenceDidChange();
-                m_inUpdateScrollbars = false;
-            } else if (m_updateScrollbarsPass < cMaxUpdateScrollbarsPass) {
-                m_updateScrollbarsPass++;
-                contentsResized();
-                scrollbarExistenceDidChange();
-                IntSize newDocSize = contentsSize();
-                if (newDocSize == docSize) {
-                    // The layout with the new scroll state had no impact on
-                    // the document's overall size, so updateScrollbars didn't get called.
-                    // Recur manually.
-                    updateScrollbars(desiredOffset);
-                }
-                m_updateScrollbarsPass--;
-            }
-        }
+    bool attemptToRemoveScrollbars = (option == AttemptToRemoveScrollbarsInCriticalCase
+        && docSize.width() <= fullVisibleSize.width() && docSize.height() <= fullVisibleSize.height());
+    if (attemptToRemoveScrollbars) {
+        if (hScroll == ScrollbarAuto)
+            newHasHorizontalScrollbar = false;
+        if (vScroll == ScrollbarAuto)
+            newHasVerticalScrollbar = false;
     }
 
-    // Set up the range, but only do this if we're not in a nested call (to avoid
-    // doing it multiple times).
-    if (m_updateScrollbarsPass)
-        return;
-
-    m_inUpdateScrollbars = true;
+    // If we ever turn one scrollbar off, always turn the other one off too.
+    // Never ever try to both gain/lose a scrollbar in the same pass.
+    if (!newHasHorizontalScrollbar && hasHorizontalScrollbar && vScroll != ScrollbarAlwaysOn)
+        newHasVerticalScrollbar = false;
+    if (!newHasVerticalScrollbar && hasVerticalScrollbar && hScroll != ScrollbarAlwaysOn)
+        newHasHorizontalScrollbar = false;
+}
 
+void ScrollView::updateScrollbarGeometry()
+{
     if (m_horizontalScrollbar) {
         int clientWidth = visibleWidth();
         IntRect oldRect(m_horizontalScrollbar->frameRect());
@@ -473,32 +417,84 @@ void ScrollView::updateScrollbars(const IntSize& desiredOffset)
         if (m_scrollbarsSuppressed)
             m_verticalScrollbar->setSuppressInvalidation(false);
     }
+}
 
-    if (hasHorizontalScrollbar != newHasHorizontalScrollbar || hasVerticalScrollbar != newHasVerticalScrollbar) {
+bool ScrollView::adjustScrollbarExistence(ComputeScrollbarExistenceOption option)
+{
+    ASSERT(m_inUpdateScrollbars);
+
+    // If we came in here with the view already needing a layout, then go ahead and do that
+    // first.  (This will be the common case, e.g., when the page changes due to window resizing for example).
+    // This layout will not re-enter updateScrollbars and does not count towards our max layout pass total.
+    if (!m_scrollbarsSuppressed)
+        scrollbarExistenceDidChange();
+
+    bool hasHorizontalScrollbar = m_horizontalScrollbar;
+    bool hasVerticalScrollbar = m_verticalScrollbar;
+
+    bool newHasHorizontalScrollbar = false;
+    bool newHasVerticalScrollbar = false;
+    computeScrollbarExistence(newHasHorizontalScrollbar, newHasVerticalScrollbar, option);
+
+    bool scrollbarExistenceChanged = hasHorizontalScrollbar != newHasHorizontalScrollbar || hasVerticalScrollbar != newHasVerticalScrollbar;
+    if (!scrollbarExistenceChanged)
+        return false;
+
+    setHasHorizontalScrollbar(newHasHorizontalScrollbar);
+    setHasVerticalScrollbar(newHasVerticalScrollbar);
+
+    if (m_scrollbarsSuppressed)
+        return true;
+
+    if (!useOverlayScrollbars())
+        contentsResized();
+    scrollbarExistenceDidChange();
+    return true;
+}
+
+void ScrollView::updateScrollbars(const IntSize& desiredOffset)
+{
+    if (m_inUpdateScrollbars)
+        return;
+    TemporaryChange<bool> inUpdateScrollbarsChange(m_inUpdateScrollbars, true);
+
+    IntSize oldVisibleSize = visibleSize();
+
+    bool scrollbarExistenceChanged = false;
+    int maxUpdateScrollbarsPass = useOverlayScrollbars() || m_scrollbarsSuppressed ? 1 : 3;
+    for (int updateScrollbarsPass = 0; updateScrollbarsPass < maxUpdateScrollbarsPass; updateScrollbarsPass++) {
+        if (!adjustScrollbarExistence(updateScrollbarsPass ? Lazy : AttemptToRemoveScrollbarsInCriticalCase))

[Julien - ping for review, 2014/06/05 21:10:03]

It's interesting that you call the passes Lazy vs
AttemptToRemoveScrollbarsInCriticalCase, from the code it was really first pass
vs other (extra) passes, which is usually EstimatePhase vs
RealScrollbarComputation.

[trchen, 2014/06/05 23:55:27]

Interesting! Our point of view is very different on this matter. How I see it is
that we always try to change scrollbar only if there is an immediate need (an
axis have contentSize>visibleSize and there was no scrollbar or
contentSize<=visibleSize and there was a scrollbar), but for the first pass we
try a little bit harder, if the total available size is bigger than the
contents, we try to remove both scrollbars even if the contents is bigger than
the current visible size. How about this, Incremental and FirstPass.

+            break;
+        scrollbarExistenceChanged = true;
+    }
+
+    updateScrollbarGeometry();
+
+    if (scrollbarExistenceChanged) {
         // FIXME: Is frameRectsChanged really necessary here? Have any frame rects changed?
         frameRectsChanged();
         positionScrollbarLayers();
         updateScrollCorner();
-        if (!m_horizontalScrollbar && !m_verticalScrollbar)
-            invalidateScrollCornerRect(oldScrollCornerRect);

[Julien - ping for review, 2014/06/05 21:10:03]

Don't we need to invalidate the scroll-corner if we lose it or is this covered
by invalidation you added below?

[trchen, 2014/06/05 23:55:27]

Yes. I also removed the invalidation when we lose a scrollbar. All we want to do
is to paint the newly revealed contents, that is what my new code does.

     }
 
+    // FIXME: We don't need to do this if we are composited.
+    IntSize newVisibleSize = visibleSize();
+    if (newVisibleSize.width() > oldVisibleSize.width()) {
+        if (shouldPlaceVerticalScrollbarOnLeft())
+            invalidateRect(IntRect(0, 0, newVisibleSize.width() - oldVisibleSize.width(), newVisibleSize.height()));
+        else
+            invalidateRect(IntRect(oldVisibleSize.width(), 0, newVisibleSize.width() - oldVisibleSize.width(), newVisibleSize.height()));
+    }
+    if (newVisibleSize.height() > oldVisibleSize.height())
+        invalidateRect(IntRect(0, oldVisibleSize.height(), newVisibleSize.width(), newVisibleSize.height() - oldVisibleSize.height()));
+
     IntPoint adjustedScrollPosition = IntPoint(desiredOffset);
     if (!isRubberBandInProgress())
         adjustedScrollPosition = adjustScrollPositionWithinRange(adjustedScrollPosition);
-
     if (adjustedScrollPosition != scrollPosition() || scrollOriginChanged()) {
         ScrollableArea::scrollToOffsetWithoutAnimation(adjustedScrollPosition);
         resetScrollOriginChanged();
     }
-
-    // Make sure the scrollbar offsets are up to date.
-    if (m_horizontalScrollbar)
-        m_horizontalScrollbar->offsetDidChange();

[Julien - ping for review, 2014/06/05 21:10:03]

Won't that break custom scrollbars as you don't send the update anymore?

If that's not needed, I would still prefer that we remove it in a follow-up as
this change is a refactor (ie not change in behavior, which would be lying) but
mostly because this has some real potential for trouble.

[trchen, 2014/06/05 23:55:27]

Ah I may have made a mistake here. I was thinking that
scrollToOffsetWithoutAnimation will update the scrollbar offsets, but that won't
be called always. If we have a brand new scrollbar, they can be left
uninitialized. Thank you for pointing this out!

I think we should combine this to ScrollView::updateScrollbarGeometry() above.

-    if (m_verticalScrollbar)
-        m_verticalScrollbar->offsetDidChange();
-
-    m_inUpdateScrollbars = false;
 }
 
 const int panIconSizeLength = 16;