Revert r112160 ("Send one WebKeyboardEvent to the RenderWidget at a time.")

This change has resulted in severe regressions in keyboard event dispatching -
the keydown and keypress events are no longer dispatched synchronously. Reverting as suggested by Darin Fisher.

BUG=106224
TEST=manual
TBR=darin


Review URL: http://codereview.chromium.org/8885009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@113603 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/renderer_host/render_widget_host.cc

@@ -102,8 +102,7 @@ RenderWidgetHost::RenderWidgetHost(content::RenderProcessHost* process,
       text_direction_updated_(false),
       text_direction_(WebKit::WebTextDirectionLeftToRight),
       text_direction_canceled_(false),
-      suppress_incoming_char_events_(false),
-      suppress_outgoing_char_events_(false),
+      suppress_next_char_events_(false),
       pending_mouse_lock_request_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
   if (routing_id_ == MSG_ROUTING_NONE)
@@ -576,7 +575,7 @@ void RenderWidgetHost::ForwardMouseEvent(const WebMouseEvent& mouse_event) {
     OnUserGesture();
   }
 
-  ForwardInputEvent(mouse_event, sizeof(WebMouseEvent));
+  ForwardInputEvent(mouse_event, sizeof(WebMouseEvent), false);
 }
 
 void RenderWidgetHost::OnMouseActivate() {
@@ -614,7 +613,7 @@ void RenderWidgetHost::ForwardWheelEvent(
   HISTOGRAM_COUNTS_100("MPArch.RWH_WheelQueueSize",
                        coalesced_mouse_wheel_events_.size());
 
-  ForwardInputEvent(wheel_event, sizeof(WebMouseWheelEvent));
+  ForwardInputEvent(wheel_event, sizeof(WebMouseWheelEvent), false);
 }
 
 void RenderWidgetHost::ForwardGestureEvent(
@@ -623,7 +622,7 @@ void RenderWidgetHost::ForwardGestureEvent(
   if (ignore_input_events_ || process_->IgnoreInputEvents())
     return;
 
-  ForwardInputEvent(gesture_event, sizeof(WebGestureEvent));
+  ForwardInputEvent(gesture_event, sizeof(WebGestureEvent), false);
 }
 
 void RenderWidgetHost::ForwardKeyboardEvent(
@@ -632,37 +631,35 @@ void RenderWidgetHost::ForwardKeyboardEvent(
   if (ignore_input_events_ || process_->IgnoreInputEvents())
     return;
 
-  // Double check the type to make sure caller hasn't sent us nonsense that
-  // will mess up our key queue.
-  if (!WebInputEvent::isKeyboardEventType(key_event.type))
-    return;
-
   if (key_event.type == WebKeyboardEvent::Char &&
       (key_event.windowsKeyCode == ui::VKEY_RETURN ||
        key_event.windowsKeyCode == ui::VKEY_SPACE)) {
     OnUserGesture();
   }
 
-  if (suppress_incoming_char_events_) {
-    // If the preceding RawKeyDown event was handled by the browser, then we
-    // need to suppress all Char events generated by it. Please note that, one
+  // Double check the type to make sure caller hasn't sent us nonsense that
+  // will mess up our key queue.
+  if (WebInputEvent::isKeyboardEventType(key_event.type)) {
+    if (suppress_next_char_events_) {
+      // If preceding RawKeyDown event was handled by the browser, then we need
+      // suppress all Char events generated by it. Please note that, one
       // RawKeyDown event may generate multiple Char events, so we can't reset
-    // |suppress_incoming_char_events_| until we get a KeyUp or a RawKeyDown.
+      // |suppress_next_char_events_| until we get a KeyUp or a RawKeyDown.
       if (key_event.type == WebKeyboardEvent::Char)
         return;
       // We get a KeyUp or a RawKeyDown event.
-    suppress_incoming_char_events_ = false;
+      suppress_next_char_events_ = false;
     }
 
     bool is_keyboard_shortcut = false;
     // Only pre-handle the key event if it's not handled by the input method.
     if (!key_event.skip_in_browser) {
-    // We need to set |suppress_incoming_char_events_| to true if
+      // We need to set |suppress_next_char_events_| to true if
       // PreHandleKeyboardEvent() returns true, but |this| may already be
-    // destroyed at that time. So set |suppress_incoming_char_events_| true
-    // here, then revert it afterwards when necessary.
+      // destroyed at that time. So set |suppress_next_char_events_| true here,
+      // then revert it afterwards when necessary.
       if (key_event.type == WebKeyboardEvent::RawKeyDown)
-      suppress_incoming_char_events_ = true;
+        suppress_next_char_events_ = true;
 
       // Tab switching/closing accelerators aren't sent to the renderer to avoid
       // a hung/malicious renderer from interfering.
@@ -670,46 +667,28 @@ void RenderWidgetHost::ForwardKeyboardEvent(
         return;
 
       if (key_event.type == WebKeyboardEvent::RawKeyDown)
-      suppress_incoming_char_events_ = false;
+        suppress_next_char_events_ = false;
     }
 
     // Don't add this key to the queue if we have no way to send the message...
     if (!process_->HasConnection())
       return;
 
-  bool has_pending_key = !key_queue_.empty();
-
-  // We keep track of all pending keyboard events so that if any are not
-  // processed by the renderer, we have the ability to process them in the
-  // browser (via UnhandledKeyboardEvent).  We also delay sending the next
-  // keyboard event until the previous has been ACK'd by the renderer.
-  key_queue_.push_back(Key(key_event, is_keyboard_shortcut));
+    // Put all WebKeyboardEvent objects in a queue since we can't trust the
+    // renderer and we need to give something to the UnhandledInputEvent
+    // handler.
+    key_queue_.push_back(key_event);
     HISTOGRAM_COUNTS_100("Renderer.KeyboardQueueSize", key_queue_.size());
 
-  if (!has_pending_key)
-    ForwardNextKeyboardEvent();
-}
-
-void RenderWidgetHost::ForwardNextKeyboardEvent() {
-  while (!key_queue_.empty()) {
-    const Key& front_item = key_queue_.front();
-
-    if (suppress_outgoing_char_events_) {
-      if (front_item.event.type == WebInputEvent::Char) {
-        key_queue_.pop_front();
-        continue;
-      }
-      suppress_outgoing_char_events_ = false;
-    }
-
-    // The renderer only cares about the platform-independent event data.
-    ForwardInputEvent(front_item.event, sizeof(WebKeyboardEvent));
-    break;
+    // Only forward the non-native portions of our event.
+    ForwardInputEvent(key_event, sizeof(WebKeyboardEvent),
+                      is_keyboard_shortcut);
   }
 }
 
 void RenderWidgetHost::ForwardInputEvent(const WebInputEvent& input_event,
-                                         int event_size) {
+                                         int event_size,
+                                         bool is_keyboard_shortcut) {
   TRACE_EVENT0("renderer_host", "RenderWidgetHost::ForwardInputEvent");
 
   if (!process_->HasConnection())
@@ -720,6 +699,9 @@ void RenderWidgetHost::ForwardInputEvent(const WebInputEvent& input_event,
   IPC::Message* message = new ViewMsg_HandleInputEvent(routing_id_);
   message->WriteData(
       reinterpret_cast<const char*>(&input_event), event_size);
+  // |is_keyboard_shortcut| only makes sense for RawKeyDown events.
+  if (input_event.type == WebInputEvent::RawKeyDown)
+    message->WriteBool(is_keyboard_shortcut);
   input_event_start_time_ = TimeTicks::Now();
   Send(message);
 
@@ -752,7 +734,7 @@ void RenderWidgetHost::ForwardTouchEvent(
     touch_move_pending_ = true;
   else
     touch_move_pending_ = false;
-  ForwardInputEvent(touch_event, sizeof(WebKit::WebTouchEvent));
+  ForwardInputEvent(touch_event, sizeof(WebKit::WebTouchEvent), false);
 }
 
 void RenderWidgetHost::RendererExited(base::TerminationStatus status,
@@ -772,8 +754,7 @@ void RenderWidgetHost::RendererExited(base::TerminationStatus status,
 
   // Must reset these to ensure that keyboard events work with a new renderer.
   key_queue_.clear();
-  suppress_incoming_char_events_ = false;
-  suppress_outgoing_char_events_ = false;
+  suppress_next_char_events_ = false;
 
   // Reset some fields in preparation for recovering from a crash.
   resize_ack_pending_ = false;
@@ -1363,40 +1344,29 @@ void RenderWidgetHost::ProcessKeyboardEventAck(int type, bool processed) {
   if (key_queue_.empty()) {
     LOG(ERROR) << "Got a KeyEvent back from the renderer but we "
                << "don't seem to have sent it to the renderer!";
-  } else if (key_queue_.front().event.type != type) {
+  } else if (key_queue_.front().type != type) {
     LOG(ERROR) << "We seem to have a different key type sent from "
-               << "the renderer. (" << key_queue_.front().event.type << " vs. "
+               << "the renderer. (" << key_queue_.front().type << " vs. "
                << type << "). Ignoring event.";
 
     // Something must be wrong. Clear the |key_queue_| and
-    // |suppress_incoming_char_events_| so that we can resume from the error.
+    // |suppress_next_char_events_| so that we can resume from the error.
     key_queue_.clear();
-    suppress_incoming_char_events_ = false;
-    suppress_outgoing_char_events_ = false;
+    suppress_next_char_events_ = false;
   } else {
-    Key front_item = key_queue_.front();
+    NativeWebKeyboardEvent front_item = key_queue_.front();
     key_queue_.pop_front();
 
 #if defined(OS_MACOSX)
-    if (!is_hidden_ && view_->PostProcessEventForPluginIme(front_item.event))
+    if (!is_hidden_ && view_->PostProcessEventForPluginIme(front_item))
       return;
 #endif
 
-    // If this RawKeyDown event corresponds to a browser keyboard shortcut and
-    // it's not processed by the renderer, then we need to suppress the
-    // upcoming Char event.
-    if (!processed && front_item.is_shortcut) {
-      DCHECK(front_item.event.type == WebInputEvent::RawKeyDown);
-      suppress_outgoing_char_events_ = true;
-    }
-
-    ForwardNextKeyboardEvent();
-
     // We only send unprocessed key event upwards if we are not hidden,
     // because the user has moved away from us and no longer expect any effect
     // of this key event.
-    if (!processed && !is_hidden_ && !front_item.event.skip_in_browser) {
-      UnhandledKeyboardEvent(front_item.event);
+    if (!processed && !is_hidden_ && !front_item.skip_in_browser) {
+      UnhandledKeyboardEvent(front_item);
 
       // WARNING: This RenderWidgetHost can be deallocated at this point
       // (i.e.  in the case of Ctrl+W, where the call to

content/browser/renderer_host/render_widget_host.h

@@ -283,7 +283,6 @@ class CONTENT_EXPORT RenderWidgetHost : public IPC::Channel::Listener,
   void ForwardWheelEvent(const WebKit::WebMouseWheelEvent& wheel_event);
   void ForwardGestureEvent(const WebKit::WebGestureEvent& gesture_event);
   virtual void ForwardKeyboardEvent(const NativeWebKeyboardEvent& key_event);
-  virtual void ForwardNextKeyboardEvent();
   virtual void ForwardTouchEvent(const WebKit::WebTouchEvent& touch_event);
 
 
@@ -459,7 +458,7 @@ class CONTENT_EXPORT RenderWidgetHost : public IPC::Channel::Listener,
  protected:
   // Internal implementation of the public Forward*Event() methods.
   void ForwardInputEvent(const WebKit::WebInputEvent& input_event,
-                         int event_size);
+                         int event_size, bool is_keyboard_shortcut);
 
   // Called when we receive a notification indicating that the renderer
   // process has gone. This will reset our state so that our state will be
@@ -760,14 +759,7 @@ class CONTENT_EXPORT RenderWidgetHost : public IPC::Channel::Listener,
   base::TimeTicks repaint_start_time_;
 
   // Queue of keyboard events that we need to track.
-  struct Key {
-    Key(const NativeWebKeyboardEvent& event, bool is_shortcut)
-        : event(event), is_shortcut(is_shortcut) {
-    }
-    NativeWebKeyboardEvent event;
-    bool is_shortcut;
-  };
-  typedef std::deque<Key> KeyQueue;
+  typedef std::deque<NativeWebKeyboardEvent> KeyQueue;
 
   // A queue of keyboard events. We can't trust data from the renderer so we
   // stuff key events into a queue and pop them out on ACK, feeding our copy
@@ -787,27 +779,19 @@ class CONTENT_EXPORT RenderWidgetHost : public IPC::Channel::Listener,
   bool text_direction_canceled_;
 
   // Indicates if the next sequence of Char events should be suppressed or not.
-  //
-  // The system may translate a RawKeyDown event into zero or more Char events,
+  // System may translate a RawKeyDown event into zero or more Char events,
   // usually we send them to the renderer directly in sequence. However, If a
-  // RawKeyDown event was not handled by the renderer but was handled by our
-  // UnhandledKeyboardEvent() method, e.g. as an accelerator key, then we shall
-  // not send the following sequence of Char events, which was generated by
-  // this RawKeyDown event, to the renderer. Otherwise the renderer may handle
-  // the Char events and cause unexpected behavior.  For example, pressing
-  // alt-2 may let the browser switch to the second tab, but the Char event
-  // generated by alt-2 may also activate a HTML element if its accesskey
-  // happens to be "2", then the user may get confused when switching back to
-  // the original tab, because the content may already be changed.
-  //
-  // If true, suppress_incoming_char_events_ prevents Char events from being
-  // added to key_queue_.
-  //
-  // If true, suppress_outgoing_char_events_ prevents Char events from being
-  // removed from key_queue_ and forwarded to the renderer.
-  //
-  bool suppress_incoming_char_events_;
-  bool suppress_outgoing_char_events_;
+  // RawKeyDown event was not handled by the renderer but was handled by
+  // our UnhandledKeyboardEvent() method, e.g. as an accelerator key, then we
+  // shall not send the following sequence of Char events, which was generated
+  // by this RawKeyDown event, to the renderer. Otherwise the renderer may
+  // handle the Char events and cause unexpected behavior.
+  // For example, pressing alt-2 may let the browser switch to the second tab,
+  // but the Char event generated by alt-2 may also activate a HTML element
+  // if its accesskey happens to be "2", then the user may get confused when
+  // switching back to the original tab, because the content may already be
+  // changed.
+  bool suppress_next_char_events_;
 
   std::vector<gfx::PluginWindowHandle> deferred_plugin_handles_;
 

content/common/view_messages.h

@@ -747,7 +747,10 @@ IPC_MESSAGE_ROUTED4(ViewMsg_PaintAtSize,
 // This signals the render view that it can send another UpdateRect message.
 IPC_MESSAGE_ROUTED0(ViewMsg_UpdateRect_ACK)
 
-// Message payload includes a blob that should be cast to WebInputEvent
+// Message payload includes:
+// 1. A blob that should be cast to WebInputEvent
+// 2. An optional boolean value indicating if a RawKeyDown event is associated
+//    to a keyboard shortcut of the browser.
 IPC_MESSAGE_ROUTED0(ViewMsg_HandleInputEvent)
 
 // This message notifies the renderer that the next key event is bound to one

content/renderer/render_widget.cc

@@ -91,6 +91,7 @@ RenderWidget::RenderWidget(WebKit::WebPopupType popup_type)
       can_compose_inline_(true),
       popup_type_(popup_type),
       pending_window_rect_count_(0),
+      suppress_next_char_events_(false),
       is_accelerated_compositing_active_(false),
       animation_update_pending_(false),
       animation_task_posted_(false),
@@ -445,6 +446,11 @@ void RenderWidget::OnHandleInputEvent(const IPC::Message& message) {
   const WebInputEvent* input_event =
       reinterpret_cast<const WebInputEvent*>(data);
 
+  bool is_keyboard_shortcut = false;
+  // is_keyboard_shortcut flag is only available for RawKeyDown events.
+  if (input_event->type == WebInputEvent::RawKeyDown)
+    message.ReadBool(&iter, &is_keyboard_shortcut);
+
   bool prevent_default = false;
   if (WebInputEvent::isMouseEventType(input_event->type)) {
     prevent_default = WillHandleMouseEvent(
@@ -452,8 +458,17 @@ void RenderWidget::OnHandleInputEvent(const IPC::Message& message) {
   }
 
   bool processed = prevent_default;
+  if (input_event->type != WebInputEvent::Char || !suppress_next_char_events_) {
+    suppress_next_char_events_ = false;
     if (!processed && webwidget_)
       processed = webwidget_->handleInputEvent(*input_event);
+  }
+
+  // If this RawKeyDown event corresponds to a browser keyboard shortcut and
+  // it's not processed by webkit, then we need to suppress the upcoming Char
+  // events.
+  if (!processed && is_keyboard_shortcut)
+    suppress_next_char_events_ = true;
 
   IPC::Message* response =
       new ViewHostMsg_HandleInputEvent_ACK(routing_id_, input_event->type,

content/renderer/render_widget.h

@@ -459,6 +459,9 @@ class CONTENT_EXPORT RenderWidget
 
   scoped_ptr<IPC::Message> pending_input_event_ack_;
 
+  // Indicates if the next sequence of Char events should be suppressed or not.
+  bool suppress_next_char_events_;
+
   // Set to true if painting to the window is handled by the accelerated
   // compositor.
   bool is_accelerated_compositing_active_;