Bug 60228 - Isolate member functions and variables for selection in EventHandler
Summary: Isolate member functions and variables for selection in EventHandler
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Nobody
Depends on: 60234
Blocks: 56410
  Show dependency treegraph
Reported: 2011-05-04 16:28 PDT by Ryosuke Niwa
Modified: 2011-11-07 18:56 PST (History)
7 users (show)

See Also:

work in progress (30.94 KB, patch)
2011-05-04 16:31 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
initial patch (42.42 KB, patch)
2011-05-05 12:22 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
second patch (37.76 KB, patch)
2011-05-05 12:35 PDT, Ryosuke Niwa
eric: review-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (4.91 MB, application/zip)
2011-05-05 14:09 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-05-04 16:28:16 PDT
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).
Comment 1 Ryosuke Niwa 2011-05-04 16:31:13 PDT
Created attachment 92344 [details]
work in progress
Comment 2 Ryosuke Niwa 2011-05-04 17:57:06 PDT
After much discussion on IRC, we've concluded that we should call new class SelectionController after renaming the existing SelectionController to something else.
Comment 3 Ryosuke Niwa 2011-05-05 12:22:56 PDT
Created attachment 92445 [details]
initial patch
Comment 4 Eric Seidel (no email) 2011-05-05 12:29:23 PDT
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*
Comment 5 Ryosuke Niwa 2011-05-05 12:31:53 PDT
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.
Comment 6 Ryosuke Niwa 2011-05-05 12:35:13 PDT
Created attachment 92447 [details]
second patch
Comment 7 Ryosuke Niwa 2011-05-05 12:39:26 PDT
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.
Comment 8 Build Bot 2011-05-05 13:34:53 PDT
Attachment 92447 [details] did not build on win:
Build output: http://queues.webkit.org/results/8551973
Comment 9 WebKit Review Bot 2011-05-05 14:09:48 PDT
Attachment 92447 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8571528
New failing tests:
Comment 10 WebKit Review Bot 2011-05-05 14:09:57 PDT
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
Comment 11 Eric Seidel (no email) 2011-05-11 20:17:24 PDT
Comment on attachment 92447 [details]
second patch

Failures make sad panda.