[webkit-reviews] review denied: [Bug 38129] Wrong target for contextmenu events : [Attachment 59941] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 38129: Wrong target for contextmenu events
https://bugs.webkit.org/show_bug.cgi?id=38129

Attachment 59941: Patch
https://bugs.webkit.org/attachment.cgi?id=59941&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
WebCore/manual-tests/chromium/contextmenu-key.html:76
 +	function cs(el)
Nit: this function should not be indented.

WebCore/ChangeLog:14
 +	    Use manual test since DRT does not handle context menu keys.
Can you file a bug for adding this?

WebCore/page/EventHandler.cpp:2027
 +	FrameView* v = m_frame->view();
Doesn't look like you use v anywhere. Also, do you need to null-check document?


WebKit/chromium/ChangeLog:10
 +	    clipped rect to determin eht position of the menu.
Typos: determin eht

WebCore/page/EventHandler.cpp:2024
 +  bool EventHandler::sendContextMenuEventForKey(const PlatformMouseEvent&
event)
It's weird to me that this takes a mouse event. I think this should take a
PlatformKeyEvent and then dispatch a mouse event below.

WebCore/page/EventHandler.cpp:2042
 +	return dispatchMouseEvent(eventNames().contextmenuEvent, targetNode,
true, 0, event, false);
Can you add a comment here that we need to dispatch a mouse event for web
compat?

WebKit/chromium/src/WebViewImpl.cpp: 
 +	coords = location + IntSize(0, -1);
This causes us to diverge from the WebKit/win implementation. Is that correct?