Introduce ThreadedInputConnection behind a switch

content/renderer/render_widget.cc

Index: content/renderer/render_widget.cc
diff --git a/content/renderer/render_widget.cc b/content/renderer/render_widget.cc
index 0ddaf21eee1fdc7dddc323c79dda769bc3dbb592..e2787610e137184b568f2d25bab39de06b4c5947 100644
--- a/content/renderer/render_widget.cc
+++ b/content/renderer/render_widget.cc
@@ -713,6 +713,8 @@ bool RenderWidget::OnMessageReceived(const IPC::Message& message) {
     IPC_MESSAGE_HANDLER(ViewMsg_SetSurfaceIdNamespace, OnSetSurfaceIdNamespace)
 #if defined(OS_ANDROID)
     IPC_MESSAGE_HANDLER(InputMsg_ImeEventAck, OnImeEventAck)
+    IPC_MESSAGE_HANDLER(InputMsg_RequestTextInputStateUpdate,
+                        OnRequestTextInputStateUpdate)
     IPC_MESSAGE_HANDLER(ViewMsg_ShowImeIfNeeded, OnShowImeIfNeeded)
 #endif
     IPC_MESSAGE_HANDLER(ViewMsg_HandleCompositorProto, OnHandleCompositorProto)
@@ -1164,9 +1166,16 @@ void RenderWidget::UpdateTextInputState(ShowIme show_ime,
 
   bool new_can_compose_inline = CanComposeInline();
 
+  bool use_ime_thread = false;
+#if defined(OS_ANDROID)
+  use_ime_thread = base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kUseImeThread);
+#endif
+
   // Only sends text input params if they are changed or if the ime should be
   // shown.
   if (show_ime == ShowIme::IF_NEEDED ||
+      (use_ime_thread && change_source == ChangeSource::FROM_IME) ||
       (text_input_type_ != new_type || text_input_mode_ != new_mode ||
        text_input_info_ != new_info ||
        can_compose_inline_ != new_can_compose_inline)
@@ -1715,12 +1724,19 @@ void RenderWidget::OnImeEventAck() {
   DCHECK_GE(text_input_info_history_.size(), 1u);
   text_input_info_history_.pop_front();
 }
+
+void RenderWidget::OnRequestTextInputStateUpdate() {
+  ImeEventGuard guard(this);

[aelias_OOO_until_Jul13, 2016/01/21 07:43:05]

I don't like that there's nothing to distinguish this ImeEventGuard from all the
others.  You have an implicit contract that a bunch of
not-specifically-enumerated renderer methods issue FROM_IME state updates, and
if ChromiumInputConnection doesn't consume the exact right number, the IME
thread either hangs or is being permanently one update behind.  And it's
difficult to prove/enforce that the implicit contract is met.

I suggest you make the change that the OnRequestTextInputStateUpdate's reply has
a boolean that *only it* has.  It can either be the existing from_ime (if you
eliminate all other uses of it), or it can be a new third boolean if that's
easier.  And the IME thread should *only* block on this boolean.  In
ChromiumInputConnection, everywhere you block the IME thread waiting for a state
update, you should first send an OnRequestTextInputStateUpdate IPC.  (That will
cause two IPCs for every IME update, but I don't think we should worry about
optimizing IPC count right now.)

[Changwan Ryu, 2016/01/22 10:22:17]

That's a great idea. Done.

+}
 #endif
 
 bool RenderWidget::ShouldHandleImeEvent() {
 #if defined(OS_ANDROID)
   if (!webwidget_)
     return false;
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kUseImeThread))
+    return true;
 
   // We cannot handle IME events if there is any chance that the event we are
   // receiving here from the browser is based on the state that is different
@@ -2113,7 +2129,9 @@ void RenderWidget::setTouchAction(
 
 void RenderWidget::didUpdateTextOfFocusedElementByNonUserInput() {
 #if defined(OS_ANDROID)
-  text_field_is_dirty_ = true;
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kUseImeThread))
+    text_field_is_dirty_ = true;
 #endif
 }