Port specific changes for bug https://bugs.webkit.org/show_bug.cgi?id=69137
Sorry I was probably not clear. What I meant is that you need to file blockers and fix them first, not the other way around.
Created attachment 111964 [details] patch for WebCore changes. Updated the WebCore changes ..
Comment on attachment 111964 [details] patch for WebCore changes. View in context: https://bugs.webkit.org/attachment.cgi?id=111964&action=review > Source/WebCore/page/EventHandler.cpp:2264 > + m_frame->selection()->setCaretBlinkingSuspended(true); This patch shouldn't be adding a bug fix. Instead, you should add an empty stub functions that are called by each port.
Created attachment 112334 [details] updated patch with chromium + gtk port changes. Patch includes changes for chromium and gtk port changes to notify EventHandler about context menu show & hide actions. I don't have the infrastructure at my side to make the changes for other ports, so it would be great if someone can add the patches for those ports in the bug.
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 112334 [details] updated patch with chromium + gtk port changes. View in context: https://bugs.webkit.org/attachment.cgi?id=112334&action=review I'd say r- due to various nits. How about win, mac, qt, etc...? > Source/WebCore/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). This line should appear before the description and blank lines before and after. > Source/WebCore/ChangeLog:9 > + No new tests. (OOPS!) Please remove this line or explain why this change can't be tested. > Source/WebKit/chromium/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). Again this line should appear above the description and a blank line. > Source/WebKit/chromium/public/WebView.h:328 > + virtual void didCloseContextMenu() { } Shouldn't this be a pure virtual function? > Source/WebKit/gtk/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). Ditto. > Source/WebKit/gtk/webkit/webkitwebview.cpp:320 > + HitTestResult result = controller->hitTestResult(); > + > + Node* node = result.innerNonSharedNode(); > + if (!node) > + return; Why do we need to hit-test? > Source/WebKit/gtk/webkit/webkitwebview.cpp:321 > + sleep(1000); Where did this sleep(1000); come from?
Comment on attachment 112334 [details] updated patch with chromium + gtk port changes. View in context: https://bugs.webkit.org/attachment.cgi?id=112334&action=review Also, I'm wondering >> Source/WebKit/gtk/webkit/webkitwebview.cpp:320 >> + return; > > Why do we need to hit-test? Ah, now I see why you need a hit testing here to access frame. I would move this code into ContextMenuController if I were you.
Comment on attachment 112334 [details] updated patch with chromium + gtk port changes. View in context: https://bugs.webkit.org/attachment.cgi?id=112334&action=review > Source/WebCore/page/EventHandler.cpp:2269 > +void EventHandler::willShowContextMenu() > +{ > +} > + > +void EventHandler::respondToContextMenuDismissal() > +{ > +} > + What do these blank functions do?
(In reply to comment #8) > (From update of attachment 112334 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112334&action=review > > > Source/WebCore/page/EventHandler.cpp:2269 > > +void EventHandler::willShowContextMenu() > > +{ > > +} > > + > > +void EventHandler::respondToContextMenuDismissal() > > +{ > > +} > > + > > What do these blank functions do? Ryosuke told me they will be implemented in a follow-up patch. So I think it's good to put notImplemented() there.