Bug 60228 – Isolate member functions and variables for selection in EventHandler

• Component: UI Events
• Status: RESOLVED WONTFIX

[Ryosuke Niwa]

One of the reasons EventHandler is hard to understand is the interactions of states and the fact it handles selection, drag, etc... all at once.

We can address this issue by splitting EventHandler into smaller classes that each deals with selection, dragging, etc... and make EventHandler a facade (in the sense of http://en.wikipedia.org/wiki/Facade_pattern).

[Ryosuke Niwa]

Created attachment 92344 [details]
work in progress

[Ryosuke Niwa]

After much discussion on IRC, we've concluded that we should call new class SelectionController after renaming the existing SelectionController to something else.

[Ryosuke Niwa]

Created attachment 92445 [details]
initial patch

[Eric Seidel (no email)]

Comment on attachment 92445 [details]
initial patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92445&action=review

> Source/WebCore/page/EventHandler.cpp:-173
>  
> -EventHandler::EventHandler(Frame* frame)
> -    : m_frame(frame)
> -    , m_mousePressed(false)

Should probably not move these to keep the diff small.

> Source/WebCore/page/EventHandler.cpp:192
> +void EventHandler::SelectionController::selectClosestWordFromMouseEvent(Frame* frame, const MouseEventWithHitTestResults& result)

seems like you just want a m_frame pointer, since all these methods take a Frame*

[Ryosuke Niwa]

I'm not exactly sure this is an improvement. It does separate selection-related states into one class but makes a lot of things opaque in EventHandler. After all, EventHandler still accesses FrameSelection after this change to figure out how to set focus.

[Ryosuke Niwa]

Created attachment 92447 [details]
second patch

[Ryosuke Niwa]

Comment on attachment 92447 [details]
second patch

I'll keep the patch here but I'm not going to land it until someone else says this is a good change.

[Build Bot]

Attachment 92447 [details] did not build on win:
Build output: http://queues.webkit.org/results/8551973

[WebKit Review Bot]

Attachment 92447 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8571528
New failing tests:
fast/css/resize-corner-tracking-transformed.html
fast/overflow/hit-test-overflow-controls.html
fast/css/resize-corner-tracking.html

[WebKit Review Bot]

Created attachment 92463 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-22-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

Comment on attachment 92447 [details]
second patch

Failures make sad panda.