Bug 145598

Summary: [GTK] Get rid of GetEditorCommandsForKeyEvent sync message
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: 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 Flags
Patch
none
Updated patch zan: review+

Carlos Garcia Campos
Reported 2015-06-02 23:19:10 PDT
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.
Attachments
Patch (38.02 KB, patch)
2015-06-02 23:40 PDT, Carlos Garcia Campos 		no flags
DetailsFormatted DiffDiff
Updated patch (36.69 KB, patch)
2015-06-10 05:32 PDT, Carlos Garcia Campos 		zan: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 2015-06-02 23:28:19 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 :)
Carlos Garcia Campos
Comment 2 2015-06-02 23:40:35 PDT
Created attachment 254159 [details] Patch
WebKit Commit Bot
Comment 3 2015-06-02 23:42:51 PDT
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
Zan Dobersek
Comment 4 2015-06-10 02:32:13 PDT
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<...>;
Carlos Garcia Campos
Comment 5 2015-06-10 02:36:37 PDT
(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.
Zan Dobersek
Comment 6 2015-06-10 03:25:25 PDT
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.
Carlos Garcia Campos
Comment 7 2015-06-10 03:32:05 PDT
(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.
Carlos Garcia Campos
Comment 8 2015-06-10 05:32:54 PDT
Created attachment 254647 [details] Updated patch Addressed review comments
WebKit Commit Bot
Comment 9 2015-06-10 05:34:47 PDT
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.
Zan Dobersek
Comment 10 2015-06-10 06:15:48 PDT
Comment on attachment 254647 [details] Updated patch r=me
Carlos Garcia Campos
Comment 11 2015-06-10 06:33:06 PDT
Committed r185415: <http://trac.webkit.org/changeset/185415>
Note You need to log in before you can comment on or make changes to this bug.