Bug 65093 - [GTK][Webkit2] Add support for IME Composition
: [GTK][Webkit2] Add support for IME Composition
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: Unspecified Linux
: P2 Normal
Assigned To:
:
:
: 55946 84556 88698
:
  Show dependency treegraph
 
Reported: 2011-07-24 20:56 PST by
Modified: 2012-12-28 12:33 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-07-24 20:56:47 PST
The WebKit2 Gtk+ port should support IME Composition, which allow us to type Korean Hangul.
------- Comment #1 From 2011-07-25 02:03:19 PST -------
Created an attachment (id=101856) [details]
Proposed Patch
------- Comment #2 From 2011-07-25 12:05:11 PST -------
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 From 2012-01-14 13:39:20 PST -------
*** Bug 55946 has been marked as a duplicate of this bug. ***
------- Comment #4 From 2012-04-19 15:54:50 PST -------
Created an attachment (id=137996) [details]
Test page

This HTML file can be used to test key events and IME events.
------- Comment #5 From 2012-06-10 16:14:16 PST -------
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 From 2012-06-10 16:39:24 PST -------
Created an attachment (id=146755) [details]
Patch
------- Comment #7 From 2012-06-14 00:38:19 PST -------
(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.

> 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 From 2012-06-14 00:58:39 PST -------
(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. 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 From 2012-06-14 01:13:06 PST -------
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 146755 [details] [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 From 2012-06-14 06:05:23 PST -------
(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 From 2012-06-14 08:11:19 PST -------
(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 From 2012-06-14 08:13:59 PST -------
(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 From 2012-08-02 08:38:21 PST -------
Created an attachment (id=156097) [details]
Respond to review comments
------- Comment #14 From 2012-08-02 08:41:25 PST -------
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 From 2012-08-03 03:18:40 PST -------
(From update of attachment 156097 [details])
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 From 2012-12-28 12:33:13 PST -------
Committed r138544: <http://trac.webkit.org/changeset/138544>