Bug 104764 – Handling autoscroll in EventHandler should be re-factor

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

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.

[Early Warning System Bot]

Comment on attachment 179003 [details]
Patch 1

Attachment 179003 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15280432

[WebKit Review Bot]

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...

[Build Bot]

Comment on attachment 179010 [details]
Patch 2

Attachment 179010 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15275617

[Hajime Morrita]

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().

[Hajime Morrita]

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.

[Hajime Morrita]

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.

[Roger Fong]

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.

[Hajime Morrita]

(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

[Hajime Morrita]

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>

All reviewed patches have been landed.  Closing bug.