Bug 65093 - [GTK][Webkit2] Add support for IME Composition
: [GTK][Webkit2] Add support for IME Composition
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: Unspecified Linux
: P2 Normal
Assigned To: Joone Hur
:
Depends on: 55946 84556 88698
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-24 20:56 PDT by Joone Hur
Modified: 2012-12-28 12:33 PST (History)
10 users (show)

See Also:


Attachments
Proposed Patch (12.65 KB, patch)
2011-07-25 02:03 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Test page (1.39 KB, text/html)
2012-04-19 15:54 PDT, Joone Hur
no flags Details
Patch (60.34 KB, patch)
2012-06-10 16:39 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Respond to review comments (63.91 KB, patch)
2012-08-02 08:38 PDT, Martin Robinson
cgarcia: review+
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 2011-07-24 20:56:47 PDT
The WebKit2 Gtk+ port should support IME Composition, which allow us to type Korean Hangul.
Comment 1 Joone Hur 2011-07-25 02:03:19 PDT
Created attachment 101856 [details]
Proposed Patch
Comment 2 Martin Robinson 2011-07-25 12:05:11 PDT
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.
Comment 3 Martin Robinson 2012-01-14 13:39:20 PST
*** Bug 55946 has been marked as a duplicate of this bug. ***
Comment 4 Joone Hur 2012-04-19 15:54:50 PDT
Created attachment 137996 [details]
Test page

This HTML file can be used to test key events and IME events.
Comment 5 Martin Robinson 2012-06-10 16:14:16 PDT
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.
Comment 6 Martin Robinson 2012-06-10 16:39:24 PDT
Created attachment 146755 [details]
Patch
Comment 7 Carlos Garcia Campos 2012-06-14 00:38:19 PDT
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.
Comment 8 Martin Robinson 2012-06-14 00:58:39 PDT
(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.
Comment 9 Carlos Garcia Campos 2012-06-14 01:13:06 PDT
(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.
Comment 10 Martin Robinson 2012-06-14 06:05:23 PDT
(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?
Comment 11 Carlos Garcia Campos 2012-06-14 08:11:19 PDT
(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.
Comment 12 Martin Robinson 2012-06-14 08:13:59 PDT
(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.
Comment 13 Martin Robinson 2012-08-02 08:38:21 PDT
Created attachment 156097 [details]
Respond to review comments
Comment 14 WebKit Review Bot 2012-08-02 08:41:25 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 15 WebKit Review Bot 2012-08-03 03:18:40 PDT
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
Comment 16 Martin Robinson 2012-12-28 12:33:13 PST
Committed r138544: <http://trac.webkit.org/changeset/138544>