[webkit-reviews] review granted: [Bug 24193] [GTK] Checkbuttons not activated with space : [Attachment 28010] imevent.patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Xan Lopez
<xan.lopez at gmail.com>'s request for review:
Bug 24193: [GTK] Checkbuttons not activated with space
https://bugs.webkit.org/show_bug.cgi?id=24193

Attachment 28010: imevent.patch
https://bugs.webkit.org/attachment.cgi?id=28010&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
>	  Do not swallow key events with GtkIMContext for non-editable
>	  frames (FIXME: I don't think frame is the right name here).

That's non-editable content.

> +    if (!targetFrame || !targetFrame->editor()->canEdit())
> +	   return;

Can targetFrame really be null here?

It's not obvious to me that no input method has useful functions for
non-editable content - some Mac IMs have such. So, it may be not quite correct
to do an early return here. But it's clear that this code is in need of large
refactoring anyway to support input events, and I think that this patch
improves the behavior more than it (potentially) regresses it.

>      // TODO: Dispatch IE-compatible text input events for IM events.

We use FIXME, not TODO. Even thought this is old code, it seems appropriate to
correct it now.