Bug 59580

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.

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.

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

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?

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

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

Comment on attachment 91229 [details] work in progress if it's wip, sounds like you don't want it marked r?