Bug 59580 – Extract KeyboardEventHandler from EventHandler

• Component: UI Events
• Status: RESOLVED WONTFIX

[Ryosuke Niwa]

Right now EventHandler deals with both keyboard events and mouse events.  Since keyboard and mouse events don't interact each other, we should split EventHandler into two classes KeyboardEventHandler and MouseEventHandler.

And as the first step, we should extract KeyboardEventHandler from EventHandler.

[Ryosuke Niwa]

Created attachment 91229 [details]
work in progress

Here's a work in progress patch (r? only because I want EWS to find build errors)

Sam & Alexey, could you tell me what CurrentEventScope is trying to do in EventHandlerMac.mm?  I removed EventHandler::keyEvent(NSEvent*) and replaced call sites in WebHTMLView.mm by keyEvent(PlatformKeyboardEvent(event)) but I didn't add CurrentEventScope in those places.  Should I be adding them?  As far as I looked at the definition of CurrentEventScope, they only seem to have a retain pointer for NSEvent which makes me think that the code is there only to resolve some binding issues between C++ and Objective-C but I'm entirely sure.

[Alexey Proskuryakov]

CurrentEventScope makes sure that currentNSEvent() returns the right thing. It's used in many places in the code, but I can't say anything about reasons for its existence without reading svn history.

[Ryosuke Niwa]

(In reply to comment #2)
> CurrentEventScope makes sure that currentNSEvent() returns the right thing. It's used in many places in the code, but I can't say anything about reasons for its existence without reading svn history.

Ah... now I see what it's trying to do. But it seems like currentNSEvent() is only called in mouse-related methods.

[Ryosuke Niwa]

It seems like CurrentEventScope was introduced in http://trac.webkit.org/changeset/17770.

We only call currentNSEvent in EventHandlerMac.mm and
WebCore/platform/mac/PopupMenuMac.mm:    NSEvent* event = [frame->eventHandler()->currentNSEvent() retain];
WebKit/mac/WebCoreSupport/WebDragClient.mm:    NSEvent *event = linkDrag ? frame->eventHandler()->currentNSEvent() : [htmlView.get() _mouseDownEvent];
WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:        NSEvent* currentNSEvent = frame->eventHandler()->currentNSEvent();
WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:            [(WebBaseNetscapePluginView *)platformWidget() handleMouseMoved:currentNSEvent];
WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:            [(WebBaseNetscapePluginView *)platformWidget() handleMouseEntered:currentNSEvent];
WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:            [(WebBaseNetscapePluginView *)platformWidget() handleMouseExited:currentNSEvent];

So maybe we need to add macEvent to PlatformMouseEvent as well and pump Event through PopupMenuMac::show and DragClient::startDrag.  Regardless, I don't think we should have a global object just so that we can access to NSEvent in all those places.

Darin, do you have any suggestion as to how to clean this up?

[Darin Adler]

(In reply to comment #4)
> Darin, do you have any suggestion as to how to clean this up?

To me the global variable seems fine. If you’d prefer we could instead attach it to the DOM event object somehow, or to the event handler, but it could be tricky to get the logic right.

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #4)
> > Darin, do you have any suggestion as to how to clean this up?
> 
> To me the global variable seems fine. If you’d prefer we could instead attach it to the DOM event object somehow, or to the event handler, but it could be tricky to get the logic right.

The problem is that it's preventing us from splitting KeyboardEventHandler and MouseEventHandler.  PlatformMouseEvent already has a pointer to NSEvent so I think we can follow the same pattern but you're right that getting the logic right might be tricky given how under tested this code is.

It'll be ideal if someone more familiar with Mac port's event handling could write a patch to add NSEvent* to PlatformMouseEvent instead and get rid of CurrentEventScope.

[Eric Seidel (no email)]

Comment on attachment 91229 [details]
work in progress

if it's wip, sounds like you don't want it marked r?