[webkit-reviews] review denied: [Bug 104764] Handling autoscroll in EventHandler should be re-factor : [Attachment 179010] Patch 2

[bugzilla-daemon at webkit.org]

Hajime Morrita <morrita at google.com> has denied yosin at chromium.org's request for
review:
Bug 104764: Handling autoscroll in EventHandler should be re-factor
https://bugs.webkit.org/show_bug.cgi?id=104764

Attachment 179010: Patch 2
https://bugs.webkit.org/attachment.cgi?id=179010&action=review

------- Additional Comments from Hajime Morrita <morrita at google.com>
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.