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+

Description Carlos Garcia Campos 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.
Comment 1 Sergio Villar Senin 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 :)
Comment 2 Carlos Garcia Campos 2015-06-02 23:40:35 PDT
Created attachment 254159 [details]
Patch
Comment 3 WebKit Commit Bot 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
Comment 4 Zan Dobersek 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<...>;
Comment 5 Carlos Garcia Campos 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.
Comment 6 Zan Dobersek 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.
Comment 7 Carlos Garcia Campos 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.
Comment 8 Carlos Garcia Campos 2015-06-10 05:32:54 PDT
Created attachment 254647 [details]
Updated patch

Addressed review comments
Comment 9 WebKit Commit Bot 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.
Comment 10 Zan Dobersek 2015-06-10 06:15:48 PDT
Comment on attachment 254647 [details]
Updated patch

r=me
Comment 11 Carlos Garcia Campos 2015-06-10 06:33:06 PDT
Committed r185415: <http://trac.webkit.org/changeset/185415>