Refactor ScrollView::updateScrollbars()

Refactor ScrollView::updateScrollbars()

This CL splits ScrollView::updateScrollbars() into 3 independent helper
functions, namely:

computeScrollbarExistence() computes whether scrollbars are needed given
the current ScrollView state.

adjustScrollbarExistence() creates/removes scrollbars and invoke appropriate
callbacks to update layout (if applicable).

updateScrollbarGeometry() positions the scrollbars and pushes the scroll
extents.

Also we no longer do update scrollbar passes in recursion. Instead we do
it in a loop and early exits if updateScrollbars() is re-entered.

No tests as there should be no behavior difference.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175852

[trchen, 2014-06-03 22:59:41]

Hello James,
Do you have a few minutes to look at this CL? This CL should make the scrollbar
update logic cleaner and easier to work on. Also the independent helper
functions will be useful to implement overflow recalc fast path without layout.
Thanks!

[jamesr, 2014-06-03 23:28:59]

I think somebody who has been working in this area lately should review - I have
not.

[trchen, 2014-06-03 23:56:24]

Hello Eric, Do you have a few minutes to look at this CL? Thanks!

[eseidel, 2014-06-03 23:59:26]

Julien is probably your best reviewer,but if he hasn't taken care of this by
tomorrow please ping again and I'm happy to look more seriously.

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

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollView.cpp (left):

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:395:
ScrollableArea::setScrollOrigin(IntPoint(scrollOrigin().x(), scrollOrigin().y()
- m_horizontalScrollbar->height()));
Don't we need to keep this call to make sure the scroll origin is up-to-date?

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:483:
invalidateScrollCornerRect(oldScrollCornerRect);
Don't we need to invalidate the scroll-corner if we lose it or is this covered
by invalidation you added below?

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:497:
m_horizontalScrollbar->offsetDidChange();
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.

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollView.cpp (right):

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:329: // FIXME: Need to consider CSS custom
scrollbars.
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.

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:466: if
(!adjustScrollbarExistence(updateScrollbarsPass ? Lazy :
AttemptToRemoveScrollbarsInCriticalCase))
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]

Thanks for reviewing! I'll upload a corrected CL soon.

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollView.cpp (left):

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:395:
ScrollableArea::setScrollOrigin(IntPoint(scrollOrigin().x(), scrollOrigin().y()
- m_horizontalScrollbar->height()));
On 2014/06/05 21:10:03, Julien Chaffraix - PST wrote:
> Don't we need to keep this call to make sure the scroll origin is up-to-date?

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.

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:483:
invalidateScrollCornerRect(oldScrollCornerRect);
On 2014/06/05 21:10:03, Julien Chaffraix - PST wrote:
> Don't we need to invalidate the scroll-corner if we lose it or is this covered
> by invalidation you added below?

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.

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:497:
m_horizontalScrollbar->offsetDidChange();
On 2014/06/05 21:10:03, Julien Chaffraix - PST wrote:
> 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.

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.

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollView.cpp (right):

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:329: // FIXME: Need to consider CSS custom
scrollbars.
On 2014/06/05 21:10:03, Julien Chaffraix - PST wrote:
> 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.

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

https://codereview.chromium.org/314583008/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:466: if
(!adjustScrollbarExistence(updateScrollbarsPass ? Lazy :
AttemptToRemoveScrollbarsInCriticalCase))
On 2014/06/05 21:10:03, Julien Chaffraix - PST wrote:
> 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.

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.

[trchen, 2014-06-06 00:10:16]

Patch Set 2 uploaded.

[trchen, 2014-06-09 22:28:51]

Ping?

[Julien - ping for review, 2014-06-10 02:45:38]

lgtm

[trchen, 2014-06-10 03:00:03]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2014-06-10 03:00:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/trchen@chromium.org/314583008/20001

[commit-bot: I haz the power, 2014-06-10 04:07:37]

Change committed as 175852