Fix OSK flickering issue

content/renderer/render_widget.cc

 // Copyright (c) 2012 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 "content/renderer/render_widget.h"
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 480 matching lines @@
       next_paint_flags_(0),
       auto_resize_mode_(false),
       need_update_rect_for_auto_resize_(false),
       did_show_(false),
       is_hidden_(hidden),
       compositor_never_visible_(never_visible),
       is_fullscreen_granted_(false),
       display_mode_(blink::WebDisplayModeUndefined),
       handling_input_event_(false),
       handling_event_overscroll_(nullptr),
-      handling_ime_event_(false),
+      ime_guard_count_(0),
+      pending_show_ime_if_needed_(false),
       handling_event_type_(WebInputEvent::Undefined),
       ignore_ack_for_mouse_move_from_debugger_(false),
       closing_(false),
       host_closing_(false),
       is_swapped_out_(swapped_out),
       for_oopif_(false),
       text_input_type_(ui::TEXT_INPUT_TYPE_NONE),
       text_input_mode_(ui::TEXT_INPUT_MODE_DEFAULT),
       text_input_flags_(0),
       can_compose_inline_(true),
       popup_type_(popup_type),
       pending_window_rect_count_(0),
       suppress_next_char_events_(false),
       screen_info_(screen_info),
       device_scale_factor_(screen_info_.deviceScaleFactor),
       next_output_surface_id_(0),
 #if defined(OS_ANDROID)
       text_field_is_dirty_(false),
+      pending_update_from_non_ime_(false),
 #endif
       popup_origin_scale_for_emulation_(0.f),
       frame_swap_message_queue_(new FrameSwapMessageQueue()),
       resizing_mode_selector_(new ResizingModeSelector()),
       context_menu_source_type_(ui::MENU_SOURCE_MOUSE),
       has_host_context_menu_location_(false) {
   if (!swapped_out)
     RenderProcess::current()->AddRefProcess();
   DCHECK(RenderThread::Get());
   device_color_profile_.push_back('0');
@@ skipping 533 matching lines @@
       &handling_event_type_, input_event->type);
 
   // Calls into |didOverscroll()| while handling this event will populate
   // |event_overscroll|, which in turn will be bundled with the event ack.
   scoped_ptr<DidOverscrollParams> event_overscroll;
   base::AutoReset<scoped_ptr<DidOverscrollParams>*>
       handling_event_overscroll_resetter(&handling_event_overscroll_,
                                          &event_overscroll);
 
 #if defined(OS_ANDROID)
-  // On Android, when a key is pressed or sent from the Keyboard using IME,
-  // |AdapterInputConnection| generates input key events to make sure all JS
-  // listeners that monitor KeyUp and KeyDown events receive the proper key
-  // code. Since this input key event comes from IME, we need to set the
-  // IME event guard here to make sure it does not interfere with other IME
-  // events.
-  scoped_ptr<ImeEventGuard> ime_event_guard_maybe;
-  if (WebInputEvent::isKeyboardEventType(input_event->type)) {
-    const WebKeyboardEvent& key_event =
-        *static_cast<const WebKeyboardEvent*>(input_event);
-    // Some keys are special and it's essential that no events get blocked.
-    if (key_event.nativeKeyCode != AKEYCODE_TAB &&
-        key_event.nativeKeyCode != AKEYCODE_DPAD_CENTER &&
-        key_event.nativeKeyCode != AKEYCODE_DPAD_LEFT &&
-        key_event.nativeKeyCode != AKEYCODE_DPAD_RIGHT &&
-        key_event.nativeKeyCode != AKEYCODE_DPAD_UP &&
-        key_event.nativeKeyCode != AKEYCODE_DPAD_DOWN)
-      ime_event_guard_maybe.reset(new ImeEventGuard(this));
-  }
+  const bool is_keyboard_event =
+      WebInputEvent::isKeyboardEventType(input_event->type);
+
+  // For non-keyboard events, we want the change source to be FROM_NON_IME.
+  ImeEventGuard guard(this, !is_keyboard_event);
 #endif
 
   base::TimeTicks start_time;
   if (base::TimeTicks::IsHighResolution())
     start_time = base::TimeTicks::Now();
 
   TRACE_EVENT1("renderer,benchmark", "RenderWidget::OnHandleInputEvent",
                "event", WebInputEventTraits::GetName(input_event->type));
   TRACE_EVENT_SYNTHETIC_DELAY_BEGIN("blink.HandleInputEvent");
   TRACE_EVENT_WITH_FLOW1("input,benchmark",
@@ skipping 768 matching lines @@
 
 static bool IsDateTimeInput(ui::TextInputType type) {
   return type == ui::TEXT_INPUT_TYPE_DATE ||
       type == ui::TEXT_INPUT_TYPE_DATE_TIME ||
       type == ui::TEXT_INPUT_TYPE_DATE_TIME_LOCAL ||
       type == ui::TEXT_INPUT_TYPE_MONTH ||
       type == ui::TEXT_INPUT_TYPE_TIME ||
       type == ui::TEXT_INPUT_TYPE_WEEK;
 }
 
-
-void RenderWidget::StartHandlingImeEvent() {
-  DCHECK(!handling_ime_event_);
-  handling_ime_event_ = true;
+void RenderWidget::OnImeEventGuardStart() {
+  ++ime_guard_count_;
 }
 
-void RenderWidget::FinishHandlingImeEvent() {
-  DCHECK(handling_ime_event_);
-  handling_ime_event_ = false;
+void RenderWidget::OnImeEventGuardFinish(bool from_non_ime) {
+  --ime_guard_count_;
+  CHECK_GE(ime_guard_count_, 0);
+#if defined(OS_ANDROID)
+  pending_update_from_non_ime_ |= from_non_ime;

[no sievers, 2015/10/26 20:13:12]

Since we already have the guard class, it'd be nice to encapsulate these
variables there (also this patch ends up duplicating the 'from_non_ime'
tracking).

One way to do it would be to track the guard instance at the bottom of the frame
instead of the counter in this class:

void RenderWidget::OnImeEventGuardStart(ImeGuard* guard) {
  if (!ime_guard_)
    ime_guard_ = guard;
}

void RenderWidget::OnImeEventGuardFinish(ImeGuard* guard) {
  if (ime_guard_ != guard)
    return;
  ime_guard_ = nullptr;
  UpdateSelectionBounds();
  UpdateTextInputState(guard->show_ime, guard->change_source);
}

void RenderWidget::UpdateTextInputState() {
  if (image_guard_) {
    if (show_ime == SHOW_IME_IF_NEEDED)
      ime_guard_->show_ime_ = show_ime;
    return;
  }

  ui::TextInputType new_type = GetTextInputType();
  ...
}

wdyt?

[Changwan Ryu, 2015/10/28 05:17:07]

Hmm... The lifetime of one ImeEventGuard may be nesting or overlapping that of
another. In case of nesting, the first call to OnImeEventGuardFinish() will
trigger UpdateTextInputState() which isn't intended. Without keeping the count,
we cannot tell whether the current ImeEventGuard is the last one or not.

So if we are to move some of the  variables inside ImeEventGuard, we will still
need to keep ime_guard_count_ and pending_update_from_non_ime_ outside of
ImeEventGuard, and only pending_show_ime_if_needed_ can be moved inside it. I'm
not sure if that can increase the readability here. Please advise.

[Changwan Ryu, 2015/10/28 16:25:51]

My bad. Overlapping is not a possible scenario here.
I've updated the code as you suggested. Looks much cleaner now! PTAL.

+#endif
+  if (ime_guard_count_ > 0)
+    return;
+
   // While handling an ime event, text input state and selection bounds updates
   // are ignored. These must explicitly be updated once finished handling the
   // ime event.
   UpdateSelectionBounds();
 #if defined(OS_ANDROID)
-  UpdateTextInputState(NO_SHOW_IME, FROM_IME);
+  UpdateTextInputState(NO_SHOW_IME,
+                       pending_update_from_non_ime_ ? FROM_NON_IME : FROM_IME);
+  pending_update_from_non_ime_ = false;
 #endif
 }
 
 void RenderWidget::UpdateTextInputState(ShowIme show_ime,
                                         ChangeSource change_source) {
   TRACE_EVENT0("renderer", "RenderWidget::UpdateTextInputState");
-  if (handling_ime_event_)
+  if (ime_guard_count_ > 0) {
+    if (show_ime == SHOW_IME_IF_NEEDED)
+      pending_show_ime_if_needed_ = true;
+
     return;
+  }
+
+  if (pending_show_ime_if_needed_) {
+    show_ime = SHOW_IME_IF_NEEDED;
+    pending_show_ime_if_needed_ = false;
+  }
+
   ui::TextInputType new_type = GetTextInputType();
   if (IsDateTimeInput(new_type))
     return;  // Not considered as a text input field in WebKit/Chromium.
 
   blink::WebTextInputInfo new_info;
   if (webwidget_)
     new_info = webwidget_->textInputInfo();
   const ui::TextInputMode new_mode = ConvertInputMode(new_info.inputMode);
 
   bool new_can_compose_inline = CanComposeInline();
@@ skipping 45 matching lines @@
   WebRect anchor_webrect;
   webwidget_->selectionBounds(focus_webrect, anchor_webrect);
   *focus = focus_webrect;
   *anchor = anchor_webrect;
 }
 
 void RenderWidget::UpdateSelectionBounds() {
   TRACE_EVENT0("renderer", "RenderWidget::UpdateSelectionBounds");
   if (!webwidget_)
     return;
-  if (handling_ime_event_)
+  if (ime_guard_count_ > 0)
     return;
 
 #if defined(USE_AURA)
   // TODO(mohsen): For now, always send explicit selection IPC notifications for
   // Aura beucause composited selection updates are not working for webview tags
   // which regresses IME inside webview. Remove this when composited selection
   // updates are fixed for webviews. See, http://crbug.com/510568.
   bool send_ipc = true;
 #else
   // With composited selection updates, the selection bounds will be reported
@@ skipping 418 matching lines @@
 void RenderWidget::RegisterVideoHoleFrame(RenderFrameImpl* frame) {
   video_hole_frames_.AddObserver(frame);
 }
 
 void RenderWidget::UnregisterVideoHoleFrame(RenderFrameImpl* frame) {
   video_hole_frames_.RemoveObserver(frame);
 }
 #endif  // defined(VIDEO_HOLE)
 
 }  // namespace content