Fix conflicts between accelerator keys and HTML DOM accesskeys....

chrome/browser/renderer_host/render_widget_host.cc

Index: chrome/browser/renderer_host/render_widget_host.cc
===================================================================
--- chrome/browser/renderer_host/render_widget_host.cc	(revision 29767)
+++ chrome/browser/renderer_host/render_widget_host.cc	(working copy)
@@ -71,7 +71,10 @@
       view_being_painted_(false),
       text_direction_updated_(false),
       text_direction_(WebKit::WebTextDirectionLeftToRight),
-      text_direction_canceled_(false) {
+      text_direction_canceled_(false),
+      pending_key_events_(false),
+      suppress_next_char_events_(false),
+      death_flag_(NULL) {
   if (routing_id_ == MSG_ROUTING_NONE)
     routing_id_ = process_->GetNextRoutingID();
 
@@ -82,6 +85,10 @@
 }
 
 RenderWidgetHost::~RenderWidgetHost() {
+  // Force the method that destroys this object to exit immediately.
+  if (death_flag_)

[darin (slow to review), 2009/11/24 07:47:36]

this seems like a very ugly hack for the fact that we don't have the lifetime
management worked out properly.

[James Su, 2009/11/24 09:10:56]

I don't like it either, but I don't know how to fix the lifetime management.
IMHO, this approach is simple enough to live with for now.

+    *death_flag_ = true;
+
   // Clear our current or cached backing store if either remains.
   BackingStoreManager::RemoveBackingStore(this);
 
@@ -395,22 +402,95 @@
     if (!process_->HasConnection())
       return;
 
+    // To help understand following logic, please refer to the comments
+    // explaining |pending_key_events_| and |suppress_next_char_events_|
+    // members. And the comments in OnMsgInputEventAck() method, which contains
+    // the bottom half of the logic.
+    //
+    // There are to different situations for us to handle key events:

[darin (slow to review), 2009/11/24 07:47:36]

nit: "There are two..."

[James Su, 2009/11/24 09:10:56]

This is fixed in CL 400012.

+    // 1. After sending a key event to the renderer, we receive its ACK message
+    // from the renderer before sending the next key event.
+    // In this case, there is always no more than 1 key event in |key_queue_|,
+    // and we send the key event one by one in this method, except that a
+    // sequence of Char events may be suppressed directly if the preceding
+    // RawKeyDown event was handled as an accelerator key in the browser.
+    //
+    // 2. We get the next key event before receving the previous one's ACK
+    // message from the renderer.
+    // In this case, when we get a key event, the previous key event is still in
+    // |keq_queue_| waiting for being handled in OnMsgInputEventAck() method.
+    // Then we need handle following cases differently:
+    //   1) If we get a Char event, then the previous key event in |key_queue_|
+    //   waiting for ACK must be a RawKeyDown event. Then we need keep this Char
+    //   event in |key_queue_| rather than sending it immediately, because we
+    //   can't determine if we should send it to the renderer or just suppress
+    //   it, until we receive the preceding RawKeyDown event's ACK message.
+    //   We increase the count of |pending_key_events_| to help remember this
+    //   Char event is not sent to the renderer yet.
+    //   2) If there is already one or more key event pending in |key_queue_|
+    //   (|pending_key_events_| > 0), we can not send a new key event
+    //   immediately no matter what type it is. Otherwise the key events will be
+    //   in wrong order. In this case we just keep them in |key_queue_| and
+    //   increase |pending_key_events_| to remember them.
+    //
+    // Then all pending key events in |key_queue_| will be handled properly in
+    // OnMsgInputEventAck() method. Please refer to that method for details.
+
     // Tab switching/closing accelerators aren't sent to the renderer to avoid a
     // hung/malicious renderer from interfering.
     if (!ShouldSendToRenderer(key_event)) {
       UnhandledKeyboardEvent(key_event);
+      if (key_event.type == WebKeyboardEvent::RawKeyDown)
+        suppress_next_char_events_ = true;
       return;
     }
 
+    bool is_char = (key_event.type == WebKeyboardEvent::Char);
+
+    // Handle the first situation mentioned above.
+    if (suppress_next_char_events_) {

[darin (slow to review), 2009/11/24 07:51:45]

couldn't we implement this discarding of events in the renderer?  if the
renderer decides not to handle a RawKeyDown, then it could queue incoming input
events until it receives acknowledgement from the browser that the RawKeyDown
was consumed as an accelerator.

that way we only queue events if the renderer decides not to handle the event.

in other words, the case of typing into an edit field remains unchanged.  the
renderer would continue consuming input events as fast as it can.

[James Su, 2009/11/24 09:10:56]

Good suggestion. I'll look into this approach to see if it's feasible. The two
issues I can see for now are:
1. We need send at least one extra message for each RawKeyDown event.
2. We still can't solve http://crbug.com/27932 without adding things similar
than CL 435002. Because no matter where we queue the events, the renderer can't
process the events in batch without waiting for the ack messages.

So I think no matter we implement this logic in the host or the renderer, the
complexity should have no big difference.

+      // 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_next_char_events_| until we get a KeyUp or a RawKeyDown.
+      if (is_char)
+        return;
+      // We get a KeyUp or a RawKeyDown event.
+      suppress_next_char_events_ = false;
+    }
+
     // 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(key_event);
+    key_queue_.push_back(key_event);
     HISTOGRAM_COUNTS_100("Renderer.KeyboardQueueSize", key_queue_.size());
+
+    // Handle the second situation mentioned above.
+    size_t key_queue_size = key_queue_.size();
+    if ((is_char && key_queue_size > 1) || pending_key_events_) {
+      // The event is not send to the renderer, increase |pending_key_events_|
+      // to remember it.
+      ++pending_key_events_;
+
+      // Some sanity checks.
+      // At least one key event in the front of |key_queue_| has been sent to
+      // the renderer, otherwise we won't be able to get any ACK back from the
+      // renderer.
+      DCHECK(key_queue_size > pending_key_events_);
+      // Char events must be preceded by RawKeyDown events.
+      // TODO(suzhe): verify it on all platforms.
+      DCHECK(key_queue_[key_queue_size - pending_key_events_ - 1].type ==
+             WebKeyboardEvent::RawKeyDown);
+      // The first pending key event must be a Char event. Other key events must
+      // already be sent to the renderer at this point.
+      DCHECK(key_queue_[key_queue_size - pending_key_events_].type ==
+             WebKeyboardEvent::Char);
+    } else {
+      // Fall into the first situation, so we can send the event immediately.
+      // Only forward the non-native portions of our event.
+      ForwardInputEvent(key_event, sizeof(WebKeyboardEvent));
+    }
   }
-
-  // Only forward the non-native portions of our event.
-  ForwardInputEvent(key_event, sizeof(WebKeyboardEvent));
 }
 
 void RenderWidgetHost::ForwardInputEvent(const WebInputEvent& input_event,
@@ -721,6 +801,10 @@
 }
 
 void RenderWidgetHost::OnMsgInputEventAck(const IPC::Message& message) {
+  // Track if |this| is destroyed or not.
+  bool is_dead = false;
+  death_flag_ = &is_dead;
+
   // Log the time delta for processing an input event.
   TimeDelta delta = TimeTicks::Now() - input_event_start_time_;
   UMA_HISTOGRAM_TIMES("MPArch.RWH_InputEventDelta", delta);
@@ -751,23 +835,86 @@
       LOG(ERROR) << "We seem to have a different key type sent from "
                  << "the renderer. (" << key_queue_.front().type << " vs. "
                  << type << "). Ignoring event.";
+
+      // Something must be wrong. |key_queue_| must be cleared here to make sure
+      // the feedback loop for sending upcoming keyboard events can be resumed
+      // correctly.
+      key_queue_.clear();
+      pending_key_events_ = 0;
+      suppress_next_char_events_ = false;
     } else {
       bool processed = false;
       if (!message.ReadBool(&iter, &processed))
         process()->ReceivedBadMessage(message.type());
 
       NativeWebKeyboardEvent front_item = key_queue_.front();
-      key_queue_.pop();
+      key_queue_.pop_front();
 
+      bool processed_by_browser = false;
       if (!processed) {
-        UnhandledKeyboardEvent(front_item);
+        processed_by_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
         // UnhandledKeyboardEvent destroys this RenderWidgetHost).
       }
+
+      // This RenderWidgetHost was already deallocated, we can't do anything
+      // from now on, including resetting |death_flag_|. So just return.
+      if (is_dead)
+        return;
+
+      // Suppress the following Char events if the RawKeyDown event was handled
+      // by the browser rather than the renderer.
+      if (front_item.type == WebKeyboardEvent::RawKeyDown)
+        suppress_next_char_events_ = processed_by_browser;
+
+      // If more than one key events in |key_queue_| were already sent to the
+      // renderer but haven't got ACK messages yet, we must wait for ACK
+      // messages of these key events before sending more key events to the
+      // renderer.
+      if (pending_key_events_ && key_queue_.size() == pending_key_events_) {
+        size_t i = 0;
+        if (suppress_next_char_events_) {
+          // Suppress the sequence of pending Char events if preceding
+          // RawKeyDown event was handled by the browser.
+          while (pending_key_events_ &&
+                 key_queue_[0].type == WebKeyboardEvent::Char) {
+            --pending_key_events_;
+            key_queue_.pop_front();
+          }
+        } else {
+          // Otherwise, send these pending Char events to the renderer.
+          // Note: we can't remove them from |key_queue_|, as we still need to
+          // wait for their ACK messages from the renderer.
+          while (pending_key_events_ &&
+                 key_queue_[i].type == WebKeyboardEvent::Char) {
+            --pending_key_events_;
+            ForwardInputEvent(key_queue_[i++], sizeof(WebKeyboardEvent));
+          }
+        }
+
+        // Reset |suppress_next_char_events_| if there is still one or more
+        // pending KeyUp or RawKeyDown events in the queue.
+        // We can't reset it if there is no pending event anymore, because we
+        // don't know if the following event is a Char event or not.
+        if (pending_key_events_)
+          suppress_next_char_events_ = false;
+
+        // We can safely send following pending KeyUp and RawKeyDown events to
+        // the renderer, until we meet another Char event.
+        while (pending_key_events_ &&
+               key_queue_[i].type != WebKeyboardEvent::Char) {
+          --pending_key_events_;
+          ForwardInputEvent(key_queue_[i++], sizeof(WebKeyboardEvent));
+        }
+      }
     }
   }
+
+  // Reset |death_flag_| to NULL, otherwise it'll point to an invalid memory
+  // address after returning from this method.
+  death_flag_ = NULL;
 }
 
 void RenderWidgetHost::OnMsgFocus() {