Summary: | [GTK] Get rid of GetEditorCommandsForKeyEvent sync message | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | berto, commit-queue, esprehn+autocc, gustavo, kangil.han, mcatanzaro, mrobinson, svillar, zan | ||||||
Priority: | P2 | Keywords: | Gtk | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 145923 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2015-06-02 23:19:10 PDT
(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> |