There are number of works regarding auto scrolling, e.g. drag-and-drop with autoscroll, drag-and-drop with gesture, and so on. It seems code for handling auto scroll will be increased, e.g. https://bugs.webkit.org/attachment.cgi?id=177950&action=prettypatch To make code of EventHandler clean and extensible, we should factor out autoscroll handling from EventHandler class.
Created attachment 179003 [details] Patch 1
Comment on attachment 179003 [details] Patch 1 Could you review this patch? Thanks in advance.
Comment on attachment 179003 [details] Patch 1 Attachment 179003 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15280432
Comment on attachment 179003 [details] Patch 1 Attachment 179003 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15280431
Created attachment 179010 [details] Patch 2
Comment on attachment 179010 [details] Patch 2 Could you review this patch? Thanks in advance. = Changes from the last patch = * Fix compilation error on Linux ** Windows compilation was succeeded on Patch 1...
Comment on attachment 179010 [details] Patch 2 Attachment 179010 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15275617
Comment on attachment 179010 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=179010&action=review > Source/WebCore/page/EventHandler.cpp:1057 > void EventHandler::stopAutoscrollTimer(bool rendererIsBeingDestroyed) This is bad. We should make this just a delegation to AutoScrollController then AutoScrollController can notify back to event handler to do cleanup. Hopefully more code could be move to the controller. > Source/WebCore/page/EventHandler.cpp:3940 > + , m_eventHandler(eventHandler) Can we pass this as a function parameter instead of holding this as a member? Cyclic reference is bad design and should be avoided as much as possible. > Source/WebCore/page/EventHandler.cpp:4078 > + bool east = m_panScrollStartPos.x() < (m_eventHandler->m_currentMousePosition.x() - ScrollView::noPanScrollRadius); If we can change to pass EventHandler as a parameter. We even no longer need to pass it: We only need to pass m_currentMousePosition and FrameView. > Source/WebCore/page/EventHandler.h:247 > + enum AutoscrollType { I think this is safe to be WebCore global. name looks unique enough. > Source/WebCore/page/EventHandler.h:255 > + class AutoscrollController { Let's move this out from EventHandler definition. nested class often results maintenance burden.
Created attachment 179210 [details] Patch 3
Created attachment 179211 [details] Patch 4
Created attachment 179212 [details] Patch 5
Comment on attachment 179212 [details] Patch 5 Could you review this patch? Thanks in advance. = Changes since the last review = * Move AutoscrollController class to its own files. * Remove m_eventHandler from AutoscrollController ** Using m_autoscrollRenderer->frame()->eventHandler() if we needed. * EventHandler::stopAutoscrollTimer() is just wrapper of AutoscrollController::stopAutoscrollTimer().
Comment on attachment 179212 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=179212&action=review This looks much better! > Source/WebCore/page/EventHandler.h:118 > + AutoscrollController& autoscrollController() { return m_autoscrollController; } Let's kill this. We can just provide didPanScrollStop() and didPanScrollStart() or something to limit the usage. > Source/WebCore/page/EventHandler.h:404 > + AutoscrollController m_autoscrollController; And make this OwnPtr. Then we can hide AutoscrollController.h in EventHandler.cpp.
Created attachment 179224 [details] Patch 6
Comment on attachment 179224 [details] Patch 6 Could you review this patch? Thanks in advance. = Changes since the last review = * In EventHandler, use OwnPtr for AutoscrollController instead of containment. * Introduce didPanScrollStart/didPanScrollTop not to exposure AutoscrollControll.
Comment on attachment 179224 [details] Patch 6 Please wait until bots become green.
Comment on attachment 179224 [details] Patch 6 Clearing flags on attachment: 179224 Committed r137691: <http://trac.webkit.org/changeset/137691>
All reviewed patches have been landed. Closing bug.
Regression caused: http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r137691%20(30897)/results.html platform/win/fast/events/panScroll-click-hyperlink.html platform/win/fast/events/panScroll-event-fired.html platform/win/fast/events/panScroll-nested-divs.html It says the tests fail because pan scrolling may not be supported on the platform. However, seeing as this is just a refactor, that does not seem to be the case. Reopening bug.
(In reply to comment #19) > Regression caused: > > http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r137691%20(30897)/results.html > > platform/win/fast/events/panScroll-click-hyperlink.html > platform/win/fast/events/panScroll-event-fired.html > platform/win/fast/events/panScroll-nested-divs.html > > It says the tests fail because pan scrolling may not be supported on the platform. > However, seeing as this is just a refactor, that does not seem to be the case. > > Reopening bug. Thanks for the taking care of this. Yes, this patch shouldn't change the behavior.
Sorry, rolling out now.
Reverted r137691 for reason: panscroll test on AppleWin failed Committed r137703: <http://trac.webkit.org/changeset/137703>
Created attachment 179418 [details] Patch 7
Comment on attachment 179418 [details] Patch 7 Could you review this patch? Thanks in advance. = Changes since the last patch = * In EventHandler::handleMouseReleaseEvent, calling stopAutoscrollTimer() if autoscroll in progress. Previous patch does't check it. ** So, during pan-scrolling mouse up stops pan-scrolling, then some tests failed, e.g. panscroll-clicking-hyperlink.html
On bug 104991, Yoshin is adding a test for Chromium port to cover this.
Comment on attachment 179418 [details] Patch 7 Clearing flags on attachment: 179418 Committed r137726: <http://trac.webkit.org/changeset/137726>