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

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted 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 60410: Patch
https://bugs.webkit.org/attachment.cgi?id=60410&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
Nice cleanup. A few minor things. Feel free to commit after that.
---------------------------------
http://wkrietveld.appspot.com/38129/diff/2001/3001
File WebCore/ChangeLog (right):

http://wkrietveld.appspot.com/38129/diff/2001/3001#newcode12
WebCore/ChangeLog:12: +        For the keboard case We now use the focused node
as the target for the
s/We/we

http://wkrietveld.appspot.com/38129/diff/2001/3003
File WebCore/page/EventHandler.cpp (right):

http://wkrietveld.appspot.com/38129/diff/2001/3003#newcode2019
WebCore/page/EventHandler.cpp:2019: swallowEvent =
dispatchMouseEvent(eventNames().contextmenuEvent, mev.targetNode(), true, 0,
event, false);
Does this match the behavior of other Windows browsers? If so, that's fine.

http://wkrietveld.appspot.com/38129/diff/2001/3006
File WebKit/chromium/src/WebViewImpl.cpp (right):

http://wkrietveld.appspot.com/38129/diff/2001/3006#newcode668
WebKit/chromium/src/WebViewImpl.cpp:668: FrameView* view =
m_page->mainFrame()->view();
Do you need to null-check view here?

http://wkrietveld.appspot.com/38129/diff/2001/3006#newcode672
WebKit/chromium/src/WebViewImpl.cpp:672: Frame* focusedFrame =
page()->focusController()->focusedOrMainFrame();
Nit: This should move down to be before line 683. Since it's not used till
then.