Bug 145598 – [GTK] Get rid of GetEditorCommandsForKeyEvent sync message

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

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.

[Sergio Villar Senin]

(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]

Created attachment 254159 [details]
Patch

[WebKit Commit Bot]

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 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]

(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 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]

(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]

Created attachment 254647 [details]
Updated patch

Addressed review comments

[WebKit Commit Bot]

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 on attachment 254647 [details]
Updated patch

r=me

[Carlos Garcia Campos]

Committed r185415: <http://trac.webkit.org/changeset/185415>