Bug 60228

Summary:

Isolate member functions and variables for selection in EventHandler

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

UI Events

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED WONTFIX

Severity:

Enhancement

CC:

ap, buildbot, cmarcelo, dglazkov, eric, sam, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

60234

Bug Blocks:

56410

Attachments:

Description	Flags
work in progress	none
initial patch	none
second patch	eric: review-
Archive of layout-test-results from ec2-cr-linux-02	none

Ryosuke Niwa

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

Attachments
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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2011-05-04 16:31:13 PDT

Created attachment 92344 [details] work in progress

Ryosuke Niwa

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

Ryosuke Niwa

Comment 3 2011-05-05 12:22:56 PDT

Created attachment 92445 [details] initial patch

Eric Seidel (no email)

Comment 4 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*

Ryosuke Niwa

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

Ryosuke Niwa

Comment 6 2011-05-05 12:35:13 PDT

Created attachment 92447 [details] second patch

Ryosuke Niwa

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

Build Bot

Comment 8 2011-05-05 13:34:53 PDT

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

WebKit Review Bot

Comment 9 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: fast/css/resize-corner-tracking-transformed.html fast/overflow/hit-test-overflow-controls.html fast/css/resize-corner-tracking.html

WebKit Review Bot

Comment 10 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

Eric Seidel (no email)

Comment 11 2011-05-11 20:17:24 PDT

Comment on attachment 92447 [details] second patch Failures make sad panda.

Note You need to log in before you can comment on or make changes to this bug.