Bug 84556 – [GTK] Rework IME handling to fix bugs and prepare for WebKit2

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

The way IME handling works now it's quite complicated to deal with misbehaving input methods. It's also tightly integrated into the WebKit1 WebCoreSupport infrastructure. This bug tracks decoupling the logic from WebKit1 and fixing a few remaining bugs.

[Martin Robinson]

Created attachment 138547 [details]
Patch

[Martin Robinson]

Created attachment 139043 [details]
Patch

[Martin Robinson]

Carlos pointed out a memory leak and some small style mistakes. I've fixed those and updated the patch.

[Gustavo Noronha (kov)]

Comment on attachment 139043 [details]
Patch

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

I'm going to dream of compositions and preedits, oh yeah. I only have a couple concerns over code flow I'd like you to double check for sanity. I love how it's taken lots of complexity into a couple objects, though, I can see how this will help WebKit2 big time.

> Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:66
> +    ASSERT(!m_widget);

Should we also ASSERT that the widget is not realized? As with this other ASSERT it seems pretty much impossible given we are only doing this on the instance init, but I guess that'd make the sanity checking complete.

> Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:135
> +        if (!m_composingTextCurrently && !m_preeditChanged && m_confirmedComposition.length() == 1) {
> +            bool result = sendSimpleKeyEvent(event, m_confirmedComposition);
> +            if (!m_confirmedComposition.isEmpty()) {
> +                m_composingTextCurrently = false;
> +                m_confirmedComposition = String();
> +            }

I could not figure out in which case this condition will be false, is it a side effect?

> Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:162
> +    // If this keystroke was not filtered, but resulted in an updated composition
> +    // or preedit, we send those before the key event. The following event sequence
> +    // copied from the Chromium source code illustrates the reasoning:

This all sounds logical to me, but the way the code is structured it looks like we can fall in here even if filtered is true (since inside that if there are no unconditional returns). Does this still hold in that case or did I miss something?

[Martin Robinson]

Comment on attachment 139043 [details]
Patch

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

>> Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:66
>> +    ASSERT(!m_widget);
> 
> Should we also ASSERT that the widget is not realized? As with this other ASSERT it seems pretty much impossible given we are only doing this on the instance init, but I guess that'd make the sanity checking complete.

This is a good idea. I'll add the ASSERT.

>> Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:135
>> +            }
> 
> I could not figure out in which case this condition will be false, is it a side effect?

I think you're right, so I'll remove this check.

>> Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:162
>> +    // copied from the Chromium source code illustrates the reasoning:
> 
> This all sounds logical to me, but the way the code is structured it looks like we can fall in here even if filtered is true (since inside that if there are no unconditional returns). Does this still hold in that case or did I miss something?

It's a bit unclear, but we want keyup events that were filtered to be processed here as well.  I can try to make this clearer by changing the comment  below from:

    // We also treat key release events the same way, as the IME code in
    // EditorClient doesn't get to look at them, as for key press events.

to

    // We also treat filtered key release events  the same way, as the IME code in
    // EditorClient doesn't get to look at them, as for key press events.

I can also try to simplify the cases above to be like:

if (filtered && !m_composingTextCurrently && !m_preeditChanged && m_confirmedComposition.length() == 1) {
...
}

if (filtered && event->type == GDK_KEY_PRESS)
{
...
}

if (event->type == GDK_KEY_RELEASE && lastFilteredKeyPressCodeWithNoResults == event->keyval)
    return true;

// Now we should only have unfiltered key events that produced results and filtered key up events.

[Martin Robinson]

Committed r116114: <http://trac.webkit.org/changeset/116114>

[Csaba Osztrogonác]

Comment on attachment 139043 [details]
Patch

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

> Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:445
> +    ASSERT(platformEvent->string().isNull());

It broke the GTK debug build:

../../Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp: In member function ‘bool WebKit::EditorClient::handleInputMethodKeyboardEvent(WebCore::KeyboardEvent*)’:
../../Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:445: error: ‘const class WebCore::PlatformKeyboardEvent’ has no member named ‘string’

[Martin Robinson]

(In reply to comment #7)
> (From update of attachment 139043 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139043&action=review
> 
> > Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:445
> > +    ASSERT(platformEvent->string().isNull());
> 
> It broke the GTK debug build:
> 
> ../../Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp: In member function ‘bool WebKit::EditorClient::handleInputMethodKeyboardEvent(WebCore::KeyboardEvent*)’:
> ../../Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:445: error: ‘const class WebCore::PlatformKeyboardEvent’ has no member named ‘string’

Thanks for letting me know. I fixed the GTK+ build here: http://trac.webkit.org/changeset/116135