The current implementation of scrolling in WebCore is something of a mess. Scrollbar is not simply an event source and dumb view of the current scroll position, but actually contains logic to do scrolling in response to events (and then call back to an associated client to tell it what happened). Various classes currently subclass ScrollbarClient and contain Scrollbar members, and have to write glue code to tie those together (this will be worse after my patch on bug 32356 lands). The Scrollbar NULL-checks client(), so it doesn't even assume there always is a ScrollbarClient, although I think there is (?). Classes like ScrollView have myriad scrolling functions and members, some of which delegate scrolling to Scrollbars, others of which perform the scroll directly on the view and then tell the Scrollbars to update. As an example of how bad things currently are, here's a sample call chain (once bug 32356 lands) for how a class Foo which implements ScrollbarClient accomplishes a scroll: Foo::doSomethingThatScrolls() -> Scrollbar::scroll() [on its member Scrollbar] -> ScrollbarClient::scroll() [on client()] -> ScrollAnimator::scroll() [on m_scrollAnimator] -> Foo::setScrollOffsetFromAnimation() [on m_client] -> Scrollbar::setValue() [on its member Scrollbar] -> Scrollbar::setCurrentPos() [on itself] -> Foo::valueChanged() [on client()] After a chat with hyatt, here's a proposed redesign: * ScrollbarClient is changed to "Scrollable". Scrollable objects inherit from Scrollable. * Scrollable is a class that is meant to handle everything about scrolling. Therefore, it contains the horizontal and/or vertical scrollbars as members (these may be NULL if the associated view does not have scrollbars). It also contains the ScrollAnimator that knows how to convert scroll requests into scroll position updates (see bug 32356). It also contains the current scroll offset as well as the visible and total size along each axis. (These are moved out of Scrollbar.) * Scrollbar becomes a dumb view of the current scroll offsets. It is also an event source, and it plumbs the events directly to the Scrollable that owns it -- it doesn't try to figure out how to convert those events into scroll offsets. * ScrollAnimator no longer needs every Scrollable (formerly ScrollbarClient) implementor to plumb things like the scrollSize(), because it's available directly on the owning Scrollable. * Scrollable contains functions meant for subclasses to call to request scrolling, e.g. scrollBy(), scrollTo(). * It also contains a function meant for the ScrollAnimator to call in order to accomplish scrolling -- setScrollOffset(). Either subclasses would override this to do additional notification they needed to do, or we'd provide a callback function that this would call to let a subclass do post-scrolling work. Hopefully, this allows us to remove parts of Scrollbar, much of ScrollView, and various other bits of code; it also makes the scrolling path simpler and more consistent: Foo::doSomethingThatScrolls() -> Scrollable::scrollBy() [on itself] -> ScrollAnimator::scroll() [on m_scrollAnimator] -> Foo::setScrollOffset() [on m_parent]
Good idea!
I did pretty much this in 52779. It could still use some polish and more moved out of Scrollbar, but the basics are now done. *** This bug has been marked as a duplicate of bug 52779 ***