[Webkit-unassigned] [Bug 45005] New: Re-architect scrolling in WebCore

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45005

           Summary: Re-architect scrolling in WebCore
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: PC
        OS/Version: All
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: WebCore Misc.
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: pkasting at google.com
                CC: hyatt at apple.com, mitz at webkit.org, pkasting at google.com


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]

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.