Fixes the timing to dispatch VKEY_PROCESSKEY.

ui/base/ime/input_method_auralinux.cc

Index: ui/base/ime/input_method_auralinux.cc
diff --git a/ui/base/ime/input_method_auralinux.cc b/ui/base/ime/input_method_auralinux.cc
index 1bf20174b4ea951c0851d468f7b2fb69b4c279cf..50a18220debb1f13f2394a5841804ebe9c15925f 100644
--- a/ui/base/ime/input_method_auralinux.cc
+++ b/ui/base/ime/input_method_auralinux.cc
@@ -12,7 +12,8 @@
 namespace ui {
 
 InputMethodAuraLinux::InputMethodAuraLinux(
-    internal::InputMethodDelegate* delegate) {
+    internal::InputMethodDelegate* delegate)
+    : allowed_to_fire_vkey_process_key_(false), vkey_processkey_flags_(0) {
   SetDelegate(delegate);
 }
 
@@ -50,22 +51,21 @@ bool InputMethodAuraLinux::DispatchKeyEvent(const ui::KeyEvent& event) {
   DCHECK(event.type() == ET_KEY_PRESSED || event.type() == ET_KEY_RELEASED);
   DCHECK(system_toplevel_window_focused());
 
+  // In order to not dispatch key events out of order, stop firing a
+  // VKEY_PROCESSKEY key event once a new key event arrives.
+  StopFiringProcessKey();

[Seigo Nonaka, 2014/05/14 04:15:03]

I'm bit confused about this. What's happened in the following case?

1. Arrives normal 'A' Down key event.
2. The key is consumed by IBus im context. And send composition update event.
3. The 'A' Up key event is arrived.
4. The composition update event fired in 2. is arrived.

Here, the VKEY_PROCESS_KEY is generated?

And I'm happy if you add the unit test for some corner cases.

[Yuki, 2014/05/14 06:30:23]

Exactly speaking, since we don't allow to fire VKEY_PROCESSKEY for key up
events, that specific case doesn't occur.  In addition, *normal* 'A' down key
event doesn't change the preedit, so we don't fire VKEY_PROCESSKEY.

But yes, if an IME is enabled and step 3 is "'B' down key event arrives", you're
right.  There is no way to prevent such case.

This code helps some cases but not all the cases.  I didn't find a better way
for this issue.

Hopefully, people don't type such fast in most cases, or
  1. keydown for 'A'
  2. keydown for 'B'
  3. VKEY_PROCESSKEY for 'a'
  4. VKEY_PROCESSKEY for 'b'
would be acceptable for most web sites.
It's a tough job.  We don't want to rely on a specific IME or GTK
implementation, so we don't want to use X11InputMethodContextImplGtk2.  Then, we
need a complex-scenario-customizable mock, but use of gmock is discouraged now a
days.  We can implement it for ourselves, but it seems too much this time to me.

[Seigo Nonaka, 2014/05/14 14:01:37]

Hmm, okay...

Could you double check if the other major IMEs or components works well (in
terms of comparing with old implementation or  with W3C specs). And nice to
leave comment if there are compatibility issues you gave up.



On 2014/05/14 06:30:23, Yuki wrote:

[Yuki, 2014/05/16 14:22:27]

Done.  I've tested this with XIM, SCIM and IBus.

As we talked offline, I removed this line, and added a note in the header.

+
   // If no text input client, do nothing.
   if (!GetTextInputClient())
     return DispatchKeyEventPostIME(event);
 
   // Let an IME handle the key event first.
-  if (input_method_context_->DispatchKeyEvent(event)) {
-    if (event.type() == ET_KEY_PRESSED &&
-        (event.flags() & ui::EF_IME_FABRICATED_KEY) == 0) {
-      const ui::KeyEvent fabricated_event(ET_KEY_PRESSED,
-                                          VKEY_PROCESSKEY,
-                                          event.flags(),
-                                          false);  // is_char
-      DispatchKeyEventPostIME(fabricated_event);
-    }
+  if (event.type() == ET_KEY_PRESSED &&
+      (event.flags() & ui::EF_IME_FABRICATED_KEY) == 0)
+    AllowToFireProcessKey(event);
+  if (input_method_context_->DispatchKeyEvent(event))
     return true;
-  }
+  StopFiringProcessKey();
 
   // Otherwise, insert the character.
   const bool handled = DispatchKeyEventPostIME(event);
@@ -124,6 +124,7 @@ bool InputMethodAuraLinux::IsCandidatePopupOpen() const {
 void InputMethodAuraLinux::OnCommit(const base::string16& text) {
   if (!IsTextInputTypeNone())
     GetTextInputClient()->InsertText(text);
+  MaybeFireProcessKey();
 }
 
 void InputMethodAuraLinux::OnPreeditChanged(
@@ -131,15 +132,19 @@ void InputMethodAuraLinux::OnPreeditChanged(
   TextInputClient* text_input_client = GetTextInputClient();
   if (text_input_client)
     text_input_client->SetCompositionText(composition_text);
+  MaybeFireProcessKey();
 }
 
 void InputMethodAuraLinux::OnPreeditEnd() {
   TextInputClient* text_input_client = GetTextInputClient();
   if (text_input_client && text_input_client->HasCompositionText())
     text_input_client->ClearCompositionText();
+  MaybeFireProcessKey();
 }
 
-void InputMethodAuraLinux::OnPreeditStart() {}
+void InputMethodAuraLinux::OnPreeditStart() {
+  MaybeFireProcessKey();
+}
 
 // Overridden from InputMethodBase.
 
@@ -153,4 +158,27 @@ void InputMethodAuraLinux::OnDidChangeFocusedClient(
   InputMethodBase::OnDidChangeFocusedClient(focused_before, focused);
 }
 
+// Helper functions to support VKEY_PROCESSKEY.
+
+void InputMethodAuraLinux::AllowToFireProcessKey(const ui::KeyEvent& event) {
+  allowed_to_fire_vkey_process_key_ = true;
+  vkey_processkey_flags_ = event.flags();
+}
+
+void InputMethodAuraLinux::MaybeFireProcessKey() {
+  if (allowed_to_fire_vkey_process_key_) {

[Seigo Nonaka, 2014/05/14 04:15:03]

nit: I like early exit style but up to you.

[Yuki, 2014/05/14 06:30:23]

I prefer early exit, too.

+    const ui::KeyEvent fabricated_event(ET_KEY_PRESSED,
+                                        VKEY_PROCESSKEY,
+                                        vkey_processkey_flags_,
+                                        false);  // is_char
+    DispatchKeyEventPostIME(fabricated_event);
+    StopFiringProcessKey();
+  }
+}
+
+void InputMethodAuraLinux::StopFiringProcessKey() {
+  allowed_to_fire_vkey_process_key_ = false;
+  vkey_processkey_flags_ = 0;
+}
+
 }  // namespace ui