Fixes the timing to dispatch VKEY_PROCESSKEY.

ui/base/ime/input_method_auralinux.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/base/ime/input_method_auralinux.h"
 
 #include "base/environment.h"
 #include "ui/base/ime/linux/linux_input_method_context_factory.h"
 #include "ui/base/ime/text_input_client.h"
 #include "ui/events/event.h"
 
 namespace ui {
 
 InputMethodAuraLinux::InputMethodAuraLinux(
-    internal::InputMethodDelegate* delegate) {
+    internal::InputMethodDelegate* delegate)
+    : allowed_to_fire_vkey_process_key_(false), vkey_processkey_flags_(0) {
   SetDelegate(delegate);
 }
 
 InputMethodAuraLinux::~InputMethodAuraLinux() {}
 
 // Overriden from InputMethod.
 
 void InputMethodAuraLinux::Init(bool focused) {
   CHECK(LinuxInputMethodContextFactory::instance())
       << "This failure was likely caused because "
@@ skipping 17 matching lines @@
 bool InputMethodAuraLinux::OnUntranslatedIMEMessage(
     const base::NativeEvent& event,
     NativeEventResult* result) {
   return false;
 }
 
 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);
   if (event.type() == ET_KEY_PRESSED && GetTextInputClient()) {
     const uint16 ch = event.GetCharacter();
     if (ch) {
       GetTextInputClient()->InsertChar(ch, event.flags());
       return true;
     }
   }
@@ skipping 38 matching lines @@
 bool InputMethodAuraLinux::IsCandidatePopupOpen() const {
   // There seems no way to detect candidate windows or any popups.
   return false;
 }
 
 // Overriden from ui::LinuxInputMethodContextDelegate
 
 void InputMethodAuraLinux::OnCommit(const base::string16& text) {
   if (!IsTextInputTypeNone())
     GetTextInputClient()->InsertText(text);
+  MaybeFireProcessKey();
 }
 
 void InputMethodAuraLinux::OnPreeditChanged(
     const CompositionText& composition_text) {
   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.
 
 void InputMethodAuraLinux::OnDidChangeFocusedClient(
     TextInputClient* focused_before,
     TextInputClient* focused) {
   input_method_context_->Reset();
   input_method_context_->OnTextInputTypeChanged(
       focused ? focused->GetTextInputType() : TEXT_INPUT_TYPE_NONE);
 
   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