We are sending GetEditorCommandsForKeyEvent sync message from web process to the UI process for every key pressed. And if the keydown event doesn't handle the key, the message is sent again for the keypress event, so in many cases it happens twice per keypress. We can get the list of commands when the key press event happens in the web view, and send it to the web process as part of the keyboard event like mac port does. In the web process, commands not inserting text will be handled by keydown and the rest in keypress without having to use any other IPC message for that.
(In reply to comment #0) > We are sending GetEditorCommandsForKeyEvent sync message from web process to > the UI process for every key pressed. And if the keydown event doesn't > handle the key, the message is sent again for the keypress event, so in many > cases it happens twice per keypress. We can get the list of commands when > the key press event happens in the web view, and send it to the web process > as part of the keyboard event like mac port does. In the web process, > commands not inserting text will be handled by keydown and the rest in > keypress without having to use any other IPC message for that. Wow that's really bad. Looking forward to seeing your patch for this :)
Created attachment 254159 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 254159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254159&action=review > Source/WebCore/dom/KeyboardEvent.cpp:233 > +#if PLATFORM(GTK) > +bool KeyboardEvent::handledByInputMethod() const > +{ > + return m_keyEvent ? m_keyEvent->handledByInputMethod() : false; > +} > +#endif Why not use the m_handledByInputMethod boolean, like it's used for the Cocoa platform? > Source/WebKit2/UIProcess/gtk/InputMethodFilter.h:58 > + typedef std::function<void (const WebCore::CompositionResults&, InputMethodFilter::EventFakedForComposition)> FilterKeyEventCompletionHandler; using FilterKeyEventCompletionHandler = std::function<...>;
(In reply to comment #4) > Comment on attachment 254159 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254159&action=review > > > Source/WebCore/dom/KeyboardEvent.cpp:233 > > +#if PLATFORM(GTK) > > +bool KeyboardEvent::handledByInputMethod() const > > +{ > > + return m_keyEvent ? m_keyEvent->handledByInputMethod() : false; > > +} > > +#endif > > Why not use the m_handledByInputMethod boolean, like it's used for the Cocoa > platform? Because we don't need to duplicate that, this is just a convenient method, to avoid having to get the platform event, check if it's null, and then get the boolean. > > Source/WebKit2/UIProcess/gtk/InputMethodFilter.h:58 > > + typedef std::function<void (const WebCore::CompositionResults&, InputMethodFilter::EventFakedForComposition)> FilterKeyEventCompletionHandler; > > using FilterKeyEventCompletionHandler = std::function<...>; Ok.
Comment on attachment 254159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254159&action=review >>> Source/WebCore/dom/KeyboardEvent.cpp:233 >>> +#endif >> >> Why not use the m_handledByInputMethod boolean, like it's used for the Cocoa platform? > > Because we don't need to duplicate that, this is just a convenient method, to avoid having to get the platform event, check if it's null, and then get the boolean. I don't think using m_handledByInputMethod the same way it's used for PLATFORM(COCOA) && USE(APPKIT) is duplicating. I do think adding a new GTK-specific method that queries some state that a KeyboardEvent constructor could already determine is duplicating. But given that this is a Web-exposed class, any platform-specific code is regrettable.
(In reply to comment #6) > Comment on attachment 254159 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254159&action=review > > >>> Source/WebCore/dom/KeyboardEvent.cpp:233 > >>> +#endif > >> > >> Why not use the m_handledByInputMethod boolean, like it's used for the Cocoa platform? > > > > Because we don't need to duplicate that, this is just a convenient method, to avoid having to get the platform event, check if it's null, and then get the boolean. > > I don't think using m_handledByInputMethod the same way it's used for > PLATFORM(COCOA) && USE(APPKIT) is duplicating. I didn't mean duplicating code, but duplicating the value in both objects. > I do think adding a new > GTK-specific method that queries some state that a KeyboardEvent constructor > could already determine is duplicating. > > But given that this is a Web-exposed class, any platform-specific code is > regrettable. I can avoid that entirely and use the platform keyboard event in the callers, as I said this was just a convenient method.
Created attachment 254647 [details] Updated patch Addressed review comments
Attachment 254647 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/gtk/InputMethodFilter.h:58: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 254647 [details] Updated patch r=me
Committed r185415: <http://trac.webkit.org/changeset/185415>