Send keyboard-derived commands only if the key events are sent

content/browser/renderer_host/render_widget_host_impl.cc

Index: content/browser/renderer_host/render_widget_host_impl.cc
diff --git a/content/browser/renderer_host/render_widget_host_impl.cc b/content/browser/renderer_host/render_widget_host_impl.cc
index 5e57094a1ac9f7a891f91b77b7cf643fce0768ed..5524c051001addafc6e44d8ca50c56a803375382 100644
--- a/content/browser/renderer_host/render_widget_host_impl.cc
+++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -1181,6 +1181,12 @@ void RenderWidgetHostImpl::ForwardTouchEventWithLatencyInfo(
 
 void RenderWidgetHostImpl::ForwardKeyboardEvent(
     const NativeWebKeyboardEvent& key_event) {
+  ForwardKeyboardEventWithCommands(key_event, nullptr);
+}
+
+void RenderWidgetHostImpl::ForwardKeyboardEventWithCommands(
+    const NativeWebKeyboardEvent& key_event,
+    const std::vector<EditCommand>* commands) {
   TRACE_EVENT0("input", "RenderWidgetHostImpl::ForwardKeyboardEvent");
   if (owner_delegate_ &&
       !owner_delegate_->MayRenderWidgetForwardKeyboardEvent(key_event)) {
@@ -1247,6 +1253,10 @@ void RenderWidgetHostImpl::ForwardKeyboardEvent(
                                                                latency_info);
   key_event_with_latency.event.isBrowserShortcut = is_shortcut;
   DispatchInputEventWithLatencyInfo(key_event, &key_event_with_latency.latency);
+  if (commands && !commands->empty()) {
+    Send(
+        new InputMsg_SetEditCommandsForNextKeyEvent(GetRoutingID(), *commands));
+  }
   input_router_->SendKeyboardEvent(key_event_with_latency);

[sadrul, 2017/01/20 18:20:15]

InputRouter can do additional queueing/filtering. So this can still go wrong, I
think. What are the chances that we can include the list of commands in the
WebKeyboardEvent itself?

[foolip, 2017/01/20 20:01:40]

At which point in InputRouterImpl will it be undoubtedly correct to pass along
the commands? Perhaps they can be passed as arguments until that point, or
NativeWebKeyboardEventWithLatencyInfo could be extended to
NativeWebKeyboardEventWithLatencyInfoAndCommands?

dtapuska@, what do you think about adding the commands to WebKeyboardEvent? It
had struck me as well, but would be a larger change and I was rather narrowly
focused on fixing my regression.

[sadrul, 2017/01/20 21:57:22]

InputRouterImpl::OfferToRendere [1] I believe is where we actually decide to
send the event to the renderer process. Carrying the list of commands until then
is probably going to be ... difficult (e.g. InputRouterImpl switches to the
generic WebInputEvent pretty early on, and EditCommands probably doesn't mean
anything for the rest of the event-types, so it will not be very clean).

[foolip, 2017/01/20 22:33:12]

I can believe that, even passing it alongside the event one hop as in this CL
was a bit of work.

One likely problem if trying to extend WebKeyboardEvent is that it's a POD, see
WebKeyboardEvent::textLengthCap. Unless the number of commands is guaranteed by
something to be small and the length of the command names/values of a fixed
length, I don't know how one would do it. Perhaps this is why it's currently
out-of-band.

sadrul@, it looks fairly straightforward to add an optional and nullable
argument for the commands at every point between here and
InputRouterImpl::OfferToRenderer so that the
InputMsg_SetEditCommandsForNextKeyEvent message is sent only if the
InputMsg_HandleInputEvent message is sent right after. But is this too unsightly
to consider? Another option would be to have a member on InputRouterImpl to hold
the commands that is set/unset before/after the SendKeyboardEvent call, but that
is hardly elegant either. Other ideas?

Note that if there are issues with InputRouter filtering similar to this, it may
have existed before any changes I made here. I just wanted to fix a Fullscreen
problem. If this CL is not risky or may make matters worse, it would be nice to
handle InputRouter separately.

 }