Bug 65093 – [GTK][Webkit2] Add support for IME Composition

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

[Joone Hur]

The WebKit2 Gtk+ port should support IME Composition, which allow us to type Korean Hangul.

[Joone Hur]

Created attachment 101856 [details]
Proposed Patch

[Martin Robinson]

This is quite a bit simpler than the corresponding code in WebKit1. I worry that it will break key events considerably. How thoroughly have you tested it? Does it properly handle normal key input via the Simple IME engine? One problem I found there was that each keystroke looked like an IME composition, which isn't correct.

[Martin Robinson]

*** Bug 55946 has been marked as a duplicate of this bug. ***

[Joone Hur]

Created attachment 137996 [details]
Test page

This HTML file can be used to test key events and IME events.

[Martin Robinson]

I've recently moved the IME code to WebCore so that it can be shared with WebKit2. I'll post a patch shortly just that creates the same glue in WebKit2.

[Martin Robinson]

Created attachment 146755 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 146755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146755&action=review

> Source/WebCore/platform/gtk/GtkInputMethodFilter.h:64
> +    enum EventFaked {
> +        FakeEvent,
> +        RealEvent
> +    };

This sounds a bit weird to me, the enum is called EventFaked but contains RealEvent value. Will this enum be extended in the future? because in this paticular case, I see a boolean easier to understand than this enum, something like bool isFakeEvent.

> Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.cpp:38
> +void WebViewBaseInputMethodFilter::setWebView(WebKitWebViewBase* webView)
> +{
> +    m_webView = webView;
> +    GtkInputMethodFilter::setWidget(GTK_WIDGET(webView));
> +}

You are keeping the webView, but it's only used to get the page. The web view is then used without checking whether it has been set already or not. I think this class could be created with a page, and this method would simply call GtkInputMethodFilter::setWidget() without keeping the web view. You couldn't allocate the class in the stack, but I don't think it's a problem.

> Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.cpp:48
> +    webkitWebViewBaseGetPage(m_webView)->handleKeyboardEvent(NativeWebKeyboardEvent(reinterpret_cast<GdkEvent*>(event),
> +                                                             CompositionResults(simpleString), faked == FakeEvent));

Here you would use the page directly, and you wouldn't need to check whether it's NULL or not

> Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.h:29
> +class WebViewBaseInputMethodFilter : public WebCore::GtkInputMethodFilter {

I think this class could be called just WebInputMethodFilter

> Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.h:31
> +    void setWebView(WebKitWebViewBase*);

This could take a GtkWidget* instead and you don't need the forward declaration.

[Martin Robinson]

(In reply to comment #7)
> (From update of attachment 146755 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146755&action=review
> 
> > Source/WebCore/platform/gtk/GtkInputMethodFilter.h:64
> > +    enum EventFaked {
> > +        FakeEvent,
> > +        RealEvent
> > +    };
> 
> This sounds a bit weird to me, the enum is called EventFaked but contains RealEvent value. Will this enum be extended in the future? because in this paticular case, I see a boolean easier to understand than this enum, something like bool isFakeEvent.

Originally this was a boolean, and changing it to an enum made the code easier to read. The general direction of WebKit is to use less booleans as arguments. If it's only the name of the enum that you object to I'm fine with renaming it. How about:

enum EventType {
    FakeEventForComposition,
    RealEventFromWidget
};

> 
> > Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.cpp:38
> > +void WebViewBaseInputMethodFilter::setWebView(WebKitWebViewBase* webView)
> > +{
> > +    m_webView = webView;
> > +    GtkInputMethodFilter::setWidget(GTK_WIDGET(webView));
> > +}
> 
> You are keeping the webView, but it's only used to get the page. The web view is then used without checking whether it has been set already or not. I think this class could be created with a page, and this method would simply call GtkInputMethodFilter::setWidget() without keeping the web view. You couldn't allocate the class in the stack, but I don't think it's a problem.

Doing dynamic allocation exchanges the possibility of not setting the WebView with the possibility of not actually allocating the InputMethodFilter. Would you be okay with simply adding ASSERTs verifying that the WebView is set? I believe I did this for the WebKit1 version.

> > Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.cpp:48
> > +    webkitWebViewBaseGetPage(m_webView)->handleKeyboardEvent(NativeWebKeyboardEvent(reinterpret_cast<GdkEvent*>(event),
> > +                                                             CompositionResults(simpleString), faked == FakeEvent));
> 
> Here you would use the page directly, and you wouldn't need to check whether it's NULL or not
> 
> > Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.h:29
> > +class WebViewBaseInputMethodFilter : public WebCore::GtkInputMethodFilter {
> 
> I think this class could be called just WebInputMethodFilter

I chose WebKitWebViewBaseInputMethodFilter to differentiate it from the WebKit1 version. I can change the name to WebKit2InputMethodFilter if you like that name better -- or if you have another suggestion that helps keep the difference between the two clear.

> > Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.h:31
> > +    void setWebView(WebKitWebViewBase*);
> 
> This could take a GtkWidget* instead and you don't need the forward declaration.

Using WebKitWebViewBase makes a lot of sense because we need to treat it like a WebKitWebViewBase to get the page. I think using a superclass, GtkWidget, makes the interface weaker -- somewhat like taking a void pointer. I want to only accept WebKitWebViewBases here.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 146755 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146755&action=review
> > 
> > > Source/WebCore/platform/gtk/GtkInputMethodFilter.h:64
> > > +    enum EventFaked {
> > > +        FakeEvent,
> > > +        RealEvent
> > > +    };
> > 
> > This sounds a bit weird to me, the enum is called EventFaked but contains RealEvent value. Will this enum be extended in the future? because in this paticular case, I see a boolean easier to understand than this enum, something like bool isFakeEvent.
> 
> Originally this was a boolean, and changing it to an enum made the code easier to read. 

Not for me :-P

> The general direction of WebKit is to use less booleans as arguments.

and I agree, that's why I said "in this particular case"

> If it's only the name of the enum that you object to I'm fine with renaming it. How about:
> 
> enum EventType {
>     FakeEventForComposition,
>     RealEventFromWidget
> };

Yes, that sounds better, although EventType is probably too generic, but at least it makes the code easier to read for me.

> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.cpp:38
> > > +void WebViewBaseInputMethodFilter::setWebView(WebKitWebViewBase* webView)
> > > +{
> > > +    m_webView = webView;
> > > +    GtkInputMethodFilter::setWidget(GTK_WIDGET(webView));
> > > +}
> > 
> > You are keeping the webView, but it's only used to get the page. The web view is then used without checking whether it has been set already or not. I think this class could be created with a page, and this method would simply call GtkInputMethodFilter::setWidget() without keeping the web view. You couldn't allocate the class in the stack, but I don't think it's a problem.
> 
> Doing dynamic allocation exchanges the possibility of not setting the WebView with the possibility of not actually allocating the InputMethodFilter.

hmm, but that's not a problem of this class, but the callers. You should allocate it in the web view constructor rigth after the page proxy is created.

> Would you be okay with simply adding ASSERTs verifying that the WebView is set? I believe I did this for the WebKit1 version.

The thing is that this class doesn't need a web view, but a WebPageProxy, so instead of using a web view just to get the page proxy all the time, it could be created with a page, like most of the classes that use a page proxy in UIProcess.

> > > Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.cpp:48
> > > +    webkitWebViewBaseGetPage(m_webView)->handleKeyboardEvent(NativeWebKeyboardEvent(reinterpret_cast<GdkEvent*>(event),
> > > +                                                             CompositionResults(simpleString), faked == FakeEvent));
> > 
> > Here you would use the page directly, and you wouldn't need to check whether it's NULL or not
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.h:29
> > > +class WebViewBaseInputMethodFilter : public WebCore::GtkInputMethodFilter {
> > 
> > I think this class could be called just WebInputMethodFilter
> 
> I chose WebKitWebViewBaseInputMethodFilter to differentiate it from the WebKit1 version. I can change the name to WebKit2InputMethodFilter if you like that name better -- or if you have another suggestion that helps keep the difference between the two clear.

In general, webcore or webkit1 classes are called without the Web prefix and webkit2 classes have the Web prefix (Page/WebPage, Frame/WebFRame, etc.)

> > > Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.h:31
> > > +    void setWebView(WebKitWebViewBase*);
> > 
> > This could take a GtkWidget* instead and you don't need the forward declaration.
> 
> Using WebKitWebViewBase makes a lot of sense because we need to treat it like a WebKitWebViewBase to get the page. I think using a superclass, GtkWidget, makes the interface weaker -- somewhat like taking a void pointer. I want to only accept WebKitWebViewBases here.

You are right, I was assuming this class would use a WebPageProxy instead of a web view.

[Martin Robinson]

(In reply to comment #9)

> > > You are keeping the webView, but it's only used to get the page. The web view is then used without checking whether it has been set already or not. I think this class could be created with a page, and this method would simply call GtkInputMethodFilter::setWidget() without keeping the web view. You couldn't allocate the class in the stack, but I don't think it's a problem.

> You are right, I was assuming this class would use a WebPageProxy instead of a web view.

Hrm, just so that I understand your suggestion, you are suggesting I take a WebPageProxy on construction and then take a GtkWidget later for the purposes of positioning the results window?

Wouldn't it be preferable just to take a WebKWebViewBase on construction?

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > > > You are keeping the webView, but it's only used to get the page. The web view is then used without checking whether it has been set already or not. I think this class could be created with a page, and this method would simply call GtkInputMethodFilter::setWidget() without keeping the web view. You couldn't allocate the class in the stack, but I don't think it's a problem.
> 
> > You are right, I was assuming this class would use a WebPageProxy instead of a web view.
> 
> Hrm, just so that I understand your suggestion, you are suggesting I take a WebPageProxy on construction and then take a GtkWidget later for the purposes of positioning the results window?
> 
> Wouldn't it be preferable just to take a WebKWebViewBase on construction?

Yes, that would work too, but I still think that class wants a page proxy, not a web view, and most of the UIProcess objects take a page proxy on construction. Some of them take both though.

[Martin Robinson]

(In reply to comment #11)

> > Wouldn't it be preferable just to take a WebKWebViewBase on construction?
> 
> Yes, that would work too, but I still think that class wants a page proxy, not a web view, and most of the UIProcess objects take a page proxy on construction. Some of them take both though.

Hrm. We definitely pass the widget, so perhaps it makes sense to only pass one of those.

[Martin Robinson]

Created attachment 156097 [details]
Respond to review comments

[WebKit Review 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

[WebKit Review Bot]

Comment on attachment 156097 [details]
Respond to review comments

Rejecting attachment 156097 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
48>At revision 134927.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13417848

[Martin Robinson]

Committed r138544: <http://trac.webkit.org/changeset/138544>