[webkit-reviews] review granted: [Bug 84556] [GTK] Rework IME handling to fix bugs and prepare for WebKit2 : [Attachment 139043] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 84556: [GTK] Rework IME handling to fix bugs and prepare for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=84556

Attachment 139043: Patch
https://bugs.webkit.org/attachment.cgi?id=139043&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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?