Bug 54230

Created attachment 82020 [details] Proposed patch

Comment on attachment 82020 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=82020&action=review > Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:83 > +static const KeyDownEntry keyDownEntries[] = { > + { VK_LEFT, 0, "MoveLeft" }, > + { VK_LEFT, ShiftKey, "MoveLeftAndModifySelection" }, > + { VK_LEFT, CtrlKey, "MoveWordLeft" }, > + { VK_LEFT, CtrlKey | ShiftKey, "MoveWordLeftAndModifySelection" }, > + { VK_RIGHT, 0, "MoveRight" }, > + { VK_RIGHT, ShiftKey, "MoveRightAndModifySelection" }, > + { VK_RIGHT, CtrlKey, "MoveWordRight" }, > + { VK_RIGHT, CtrlKey | ShiftKey, "MoveWordRightAndModifySelection" }, > + { VK_UP, 0, "MoveUp" }, > + { VK_UP, ShiftKey, "MoveUpAndModifySelection" }, > + { VK_PRIOR, ShiftKey, "MovePageUpAndModifySelection" }, > + { VK_DOWN, 0, "MoveDown" }, I think I'd rather see this done correctly up front than doing it quickly now and replacing it. We probably need to plumb through to the UI process to do the mapping between key events and editor actions.

Attachment 82020 [details] did not build on win: Build output: http://queues.webkit.org/results/7870424

Attachment 82020 [details] did not build on mac: Build output: http://queues.webkit.org/results/7874518

(In reply to comment #2) > (From update of attachment 82020 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82020&action=review > > > Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:83 > > +static const KeyDownEntry keyDownEntries[] = { > > + { VK_LEFT, 0, "MoveLeft" }, > > + { VK_LEFT, ShiftKey, "MoveLeftAndModifySelection" }, > > + { VK_LEFT, CtrlKey, "MoveWordLeft" }, > > + { VK_LEFT, CtrlKey | ShiftKey, "MoveWordLeftAndModifySelection" }, > > + { VK_RIGHT, 0, "MoveRight" }, > > + { VK_RIGHT, ShiftKey, "MoveRightAndModifySelection" }, > > + { VK_RIGHT, CtrlKey, "MoveWordRight" }, > > + { VK_RIGHT, CtrlKey | ShiftKey, "MoveWordRightAndModifySelection" }, > > + { VK_UP, 0, "MoveUp" }, > > + { VK_UP, ShiftKey, "MoveUpAndModifySelection" }, > > + { VK_PRIOR, ShiftKey, "MovePageUpAndModifySelection" }, > > + { VK_DOWN, 0, "MoveDown" }, > > I think I'd rather see this done correctly up front than doing it quickly now and replacing it. We probably need to plumb through to the UI process to do the mapping between key events and editor actions. Good point, I think just mac is currently doing it, probably is why they decided to do leave it. I have to review also compilation and check why we needed includes that other ports do not need.

Comment on attachment 82020 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=82020&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:32 > +#include "Cursor.h" Is this include necessary here? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:37 > +#include "Font.h" Why is Font.h necessary here? > Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:47 > + notImplemented(); I think you can remove this for now, since we don't actually have an platform initialization to do at the moment.

(In reply to comment #6) > (From update of attachment 82020 [details]) > > [...] > > > Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:47 > > + notImplemented(); > > I think you can remove this for now, since we don't actually have an platform initialization to do at the moment. Actually I'm testing now the patch using a sendSync to get the commands list from the UI :), I was planning about adding the input method support in the same patch, hopefully tomorrow I can upload it here. So I think we can wait for that patch.

Comment on attachment 82020 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=82020&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:40 > +#include "CachedResourceClient.h" > +#include "CachedResourceHandle.h" > +#include "Cursor.h" > #include "DataReference.h" > #include "DecoderAdapter.h" > #include "DrawingArea.h" > +#include "FocusController.h" > +#include "Font.h" > #include "InjectedBundle.h" > #include "InjectedBundleBackForwardList.h" > #include "MessageID.h" In fact, all of these changes seem unecessary. Please remove them.

(In reply to comment #8) > (From update of attachment 82020 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82020&action=review > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:40 > > +#include "CachedResourceClient.h" > > +#include "CachedResourceHandle.h" > > +#include "Cursor.h" > > #include "DataReference.h" > > #include "DecoderAdapter.h" > > #include "DrawingArea.h" > > +#include "FocusController.h" > > +#include "Font.h" > > #include "InjectedBundle.h" > > #include "InjectedBundleBackForwardList.h" > > #include "MessageID.h" > > In fact, all of these changes seem unecessary. Please remove them. Yeah, completely removed in the current version of the patch, it was a problem with X includes I fixed in other patch.

Created attachment 85059 [details] Proposed patch Adding message to handle commands generation in the UI process.

Comment on attachment 85059 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=85059&action=review r- because of some really minor things but this looks good to me otherwise! I'd really like to have an Apple person look over the IPC changes, since I'm not particularly familiar with that code. > Source/WebKit2/UIProcess/WebPageProxy.messages.in:139 > + # Keyboard support messages Might want to make this slightly more descriptive. "Support for GTK+ platform keybindings" or something like that. > Source/WebKit2/UIProcess/WebPageProxy.messages.in:140 > + GenerateEditorCommands() -> (Vector<WTF::String> commandsList) Likewise, this might be better named in a way to describe that it converts a keystroke into a editor commands. > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:39 > + // First try to interpret the command in the UI and get the commands.. Extra period here. > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:43 > + if (!WebProcess::shared().connection()->sendSync(Messages::WebPageProxy::GenerateEditorCommands(), > + Messages::WebPageProxy::GenerateEditorCommands::Reply(pendingEditorCommands), > + m_page->pageID(), CoreIPC::Connection::NoTimeout)) > + return; I don't think this return or if statement are necessary. > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:61 > + success = false; > + break; If you're going to break, you can probably just return false here and dispense with the success variable altogether.

Yep, I would like Apple review of the modifications of the IPC, adding them to the CC.

Created attachment 86284 [details] Proposed patch Fixed all the issues, thanks for the review, I hope some Apple dev can check the IPC part of the patch.

Attachment 86284 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:59: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 86284 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=86284&action=review > Source/WebKit2/UIProcess/gtk/WebView.cpp:300 > + m_pendingEditorCommands = &commandsList; Taking a pointer of a stack-based object like this is dangerous. What ensures that we don't end up with a dangling pointer here? Could we fix it in some other way? > Source/WebKit2/UIProcess/gtk/WebView.cpp:309 > + if (m_pendingEditorCommands->size() > 0) > + return; This should be if (!m_pendingEditorCommands.isEmpty()) > Source/WebKit2/UIProcess/gtk/WebView.cpp:312 > + static HashMap<int, const char*> keyDownCommandsMap; > + static HashMap<int, const char*> keyPressCommandsMap; These should use the DEFINE_STATIC_LOCAL to avoid exit-time destructors. > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:78 > + if (pendingEditorCommands.size() > 0) { It's better to do if (!pendingEditorCommands.isEmpty()) { > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:122 > + } else { > + // Don't insert null or control characters as they can result in unexpected behaviour > + if (event->charCode() < ' ') > + return; > + > + // Don't insert anything if a modifier is pressed > + if (platformEvent->ctrlKey() || platformEvent->altKey()) > + return; > + > + if (frame->editor()->insertText(platformEvent->text(), event)) > + event->setDefaultHandled(); > + } This entire block has wrong indentation.

Thanks for the review :), I'll fix the issues.

(In reply to comment #15) > (From update of attachment 86284 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=86284&action=review > > > Source/WebKit2/UIProcess/gtk/WebView.cpp:300 > > + m_pendingEditorCommands = &commandsList; > > Taking a pointer of a stack-based object like this is dangerous. What ensures that we don't end up with a dangling pointer here? Could we fix it in some other way? > Yep, you are right, in this case it is used just to pass the pointer to some callbacks that are only called through this method and do not get this pointer, this way we avoid a copy just for getting the result from the callbacks. To make sure no one uses this for other cases I'm adding a = 0, when the method finishes and asserting initially it does no have any value, and I'm adding transient to the name of the attribute. > > Source/WebKit2/UIProcess/gtk/WebView.cpp:312 > > + static HashMap<int, const char*> keyDownCommandsMap; > > + static HashMap<int, const char*> keyPressCommandsMap; > > These should use the DEFINE_STATIC_LOCAL to avoid exit-time destructors. > I added this to the StdLibExtras.h: // WEBKIT_COMMA: Required when using macros like DEFINE_STATIC_LOCAL and you // need to define a template with multiple arguments i.e. // DEFINE_STATIC_LOCAL(HasMap<type1 COMMA type2>, ...) #define WEBKIT_COMMA ,

Created attachment 86926 [details] Proposed patch

Comment on attachment 86926 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=86926&action=review Looks good, but I think you should switch to making m_pendingEditorCommands an object and just use append(). See below. > Source/JavaScriptCore/wtf/StdLibExtras.h:48 > +// WEBKIT_COMMA: Required when using macros like DEFINE_STATIC_LOCAL and you > +// need to define a template with multiple arguments i.e. > +// DEFINE_STATIC_LOCAL(HasMap<type1 COMMA type2>, ...) > +#define WEBKIT_COMMA , Hrm. I'm unsure about this change. Let's hold off on this or address it in another bug. > Source/WebKit2/UIProcess/gtk/WebView.cpp:305 > +void WebView::getEditorCommandsForKeyEvent(const NativeWebKeyboardEvent& event, Vector<WTF::String>& commandsList) commandsList -> commandList > Source/WebKit2/UIProcess/gtk/WebView.cpp:309 > + m_transientPendingEditorCommands = &commandsList; Instead of making this change, maybe it would be better to turn the m_pendingEditorCommands member into an object from a pointer and just do: m_pendingEditorCommands.clear() here. > Source/WebKit2/UIProcess/gtk/WebView.cpp:314 > + gtk_bindings_activate_event(GTK_OBJECT(m_nativeWidget.get()), reinterpret_cast<GdkEventKey*>(const_cast<GdkEvent*>(event.nativeEvent()))); > +#else > + gtk_bindings_activate_event(G_OBJECT(m_nativeWidget.get()), reinterpret_cast<GdkEventKey*>(const_cast<GdkEvent*>(event.nativeEvent()))); Here you should use event.nativeEvent()->key to get the key event. > Source/WebKit2/UIProcess/gtk/WebView.cpp:321 > + if (m_transientPendingEditorCommands->isEmpty()) { > + m_transientPendingEditorCommands = 0; > + return; > + } > + Here you can just do commandsList.append(m_pendingEditorCommands); > Source/WebKit2/UIProcess/gtk/WebView.cpp:323 > + DEFINE_STATIC_LOCAL(HashMap<int WEBKIT_COMMA const char*>, keyDownCommandsMap, ()); > + DEFINE_STATIC_LOCAL(HashMap<int WEBKIT_COMMA const char*>, keyPressCommandsMap, ()); For now instead of using WEBKIT_COMMA, just make a typedef like this at the top of the file: typedef HashMap<int, const char*> IntConstCharHashMap; and use that here. I believe it should fix the issues with DEFINE_STATIC_LOCAL.

(In reply to comment #19) > (From update of attachment 86926 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=86926&action=review > > Looks good, but I think you should switch to making m_pendingEditorCommands an object and just use append(). See below. Yeah, you are right, I always fall in the nonsense preoptimization. I'll review the patch, thanks for the reviews :).

Created attachment 86969 [details] Proposed patch

Comment on attachment 86969 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=86969&action=review Looks good. Please ensure that you have all the necessary WebPage stubs before landing. > Source/WebKit2/ChangeLog:1 > +2011-03-25 Amruth Raj <amruthraj@motorola.com> and Ravi Phaneendra Kasibhatla <ravi.kasibhatla@motorola.com> and Alejandro G. Castro <alex@igalia.com> Probably best to use First, Second, and here. > Source/WebKit2/UIProcess/WebPageProxy.h:-586 > - I would restore this newline. > Source/WebKit2/UIProcess/gtk/WebView.h:68 > + // Keyboard events handling. You can omit this coment. > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:42 > + m_page->pageID(), CoreIPC::Connection::NoTimeout); Does it make sense to require a timeout for this?

(In reply to comment #22) > > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:42 > > + m_page->pageID(), CoreIPC::Connection::NoTimeout); > > Does it make sense to require a timeout for this? Did not give much thought about it, I would say this kind of calls should never fail because it is something that just depends on the program processes and not something from the network and the user is expecting a response ASAP.

Landed http://trac.webkit.org/changeset/82929

http://trac.webkit.org/changeset/82929 might have broken Windows Release (Build) and Windows Debug (Build)