Refactor ScrollView::updateScrollbars()

Source/platform/scroll/ScrollView.cpp

 /*
  * Copyright (C) 2006, 2007, 2008 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 13 matching lines @@
  */
 
 #include "config.h"
 #include "platform/scroll/ScrollView.h"
 
 #include "platform/graphics/GraphicsContextStateSaver.h"
 #include "platform/graphics/GraphicsLayer.h"
 #include "platform/HostWindow.h"
 #include "platform/scroll/ScrollbarTheme.h"
 #include "wtf/StdLibExtras.h"
+#include "wtf/TemporaryChange.h"
 
 using namespace std;
 
 namespace WebCore {
 
 ScrollView::ScrollView()
     : m_horizontalScrollbarMode(ScrollbarAuto)
     , m_verticalScrollbarMode(ScrollbarAuto)
     , m_horizontalScrollbarLock(false)
     , m_verticalScrollbarLock(false)
     , m_scrollbarsAvoidingResizer(0)
     , m_scrollbarsSuppressed(false)
     , m_inUpdateScrollbars(false)
-    , m_updateScrollbarsPass(0)
     , m_drawPanScrollIcon(false)
     , m_paintsEntireContents(false)
     , m_clipsRepaints(true)
 {
 }
 
 ScrollView::~ScrollView()
 {
 }
 
@@ skipping 259 matching lines @@
         stretch.setHeight(currentScrollPosition.y() - maxScrollPosition.y());
 
     return stretch;
 }
 
 void ScrollView::windowResizerRectChanged()
 {
     updateScrollbars(scrollOffset());
 }
 
-static const unsigned cMaxUpdateScrollbarsPass = 2;
+static bool useOverlayScrollbars()
+{
+    // 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::updateScrollbars(const IntSize& desiredOffset)
+void ScrollView::computeScrollbarExistence(bool& newHasHorizontalScrollbar, bool& newHasVerticalScrollbar, ComputeScrollbarExistenceOption option) const
 {
-    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();
-
     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;
 
     if (hScroll != ScrollbarAuto)
         newHasHorizontalScrollbar = (hScroll == ScrollbarAlwaysOn);
     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 (useOverlayScrollbars())
+        return;
 
-        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;
-        }
+    IntSize fullVisibleSize = visibleContentRect(IncludeScrollbars).size();
 
-        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 (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);
-        }
-
-        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;
+    // 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;
+}
 
-    m_inUpdateScrollbars = true;
-
+void ScrollView::updateScrollbarGeometry()
+{
     if (m_horizontalScrollbar) {
         int clientWidth = visibleWidth();
         IntRect oldRect(m_horizontalScrollbar->frameRect());
         IntRect hBarRect((shouldPlaceVerticalScrollbarOnLeft() && m_verticalScrollbar) ? m_verticalScrollbar->width() : 0,
                         height() - m_horizontalScrollbar->height(),
                         width() - (m_verticalScrollbar ? m_verticalScrollbar->width() : 0),
                         m_horizontalScrollbar->height());
         m_horizontalScrollbar->setFrameRect(hBarRect);
         if (!m_scrollbarsSuppressed && oldRect != m_horizontalScrollbar->frameRect())
             m_horizontalScrollbar->invalidate();
@@ skipping 17 matching lines @@
         if (!m_scrollbarsSuppressed && oldRect != m_verticalScrollbar->frameRect())
             m_verticalScrollbar->invalidate();
 
         if (m_scrollbarsSuppressed)
             m_verticalScrollbar->setSuppressInvalidation(true);
         m_verticalScrollbar->setEnabled(contentsHeight() > clientHeight);
         m_verticalScrollbar->setProportion(clientHeight, contentsHeight());
         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;
 
 IntRect ScrollView::rectToCopyOnScroll() const
 {
     IntRect scrollViewRect = convertToRootView(IntRect((shouldPlaceVerticalScrollbarOnLeft() && verticalScrollbar()) ? verticalScrollbar()->width() : 0, 0, visibleWidth(), visibleHeight()));
     if (hasOverlayScrollbars()) {
         int verticalScrollbarWidth = (verticalScrollbar() && !hasLayerForVerticalScrollbar()) ? verticalScrollbar()->width() : 0;
         int horizontalScrollbarHeight = (horizontalScrollbar() && !hasLayerForHorizontalScrollbar()) ? horizontalScrollbar()->height() : 0;
@@ skipping 602 matching lines @@
         return;
 
     ScrollableArea::setScrollOrigin(origin);
 
     // Update if the scroll origin changes, since our position will be different if the content size did not change.
     if (updatePositionAtAll && updatePositionSynchronously)
         updateScrollbars(scrollOffset());
 }
 
 } // namespace WebCore