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

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

There was a regression in https://codereview.chromium.org/2482853002

Previously, when a keydown was handled on the browser side, the keypress
was suppressed but the keyup was not. Now, all three events are
suppressed. However, the fix did not take into consideration the
InputMsg_SetEditCommandsForNextKeyEvent messages sent right before the
call to RenderWidgetHostImpl::ForwardKeyboardEvent at two call sites.
Because the keyup event was also suppressed, those commands would
instead be associated with a following keydown.

See https://bugs.chromium.org/p/chromium/issues/detail?id=677827#c11 for
how that could end up dropping the keypress event of what followed.

The fix is to introduce a ForwardKeyboardEventWithCommands which sends
the commands only if they key itself is sent.

BUG=677827
Review-Url: https://codereview.chromium.org/2644843004
Cr-Commit-Position: refs/heads/master@{#445687}
Committed: https://chromium.googlesource.com/chromium/src/+/b7b0799fa40dd39cc93e826e6bacfea41231d989

[foolip, 2017-01-19 13:31:33]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 13:31:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[foolip, 2017-01-19 14:08:32]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 14:08:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 14:55:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 14:56:00]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-01-19 15:50:53]

foolip@chromium.org changed reviewers:
+ dtapuska@chromium.org, mgiuca@chromium.org, sadrul@chromium.org

[foolip, 2017-01-19 15:50:53]

PTAL at this follow-up to https://codereview.chromium.org/2482853002

Since this is an M56 blocker speedy review is appreciated, and please shout if
anything here seems risky to merge this close to stable.

If the final reviewer could send to CQ, that would be much appreciated.

[dtapuska, 2017-01-19 16:00:17]

On 2017/01/19 15:50:53, foolip_slow_very_sorry wrote:
> PTAL at this follow-up to https://codereview.chromium.org/2482853002
> 
> Since this is an M56 blocker speedy review is appreciated, and please shout if
> anything here seems risky to merge this close to stable.
> 
> If the final reviewer could send to CQ, that would be much appreciated.

lgtm but we should probably remove SetEditCommandsForNextKeyEvent since it looks
like it is now dead code.

[Matt Giuca, 2017-01-20 00:59:41]

I'm really struggling to understand how this works (I'm not familiar with any of
this code), but I don't see anything obviously wrong, so my lgtm.

I would like to wait for sadrul though (I think you have to for OWNERS).

[foolip, 2017-01-20 10:27:32]

On 2017/01/19 16:00:17, dtapuska wrote:
> On 2017/01/19 15:50:53, foolip_slow_very_sorry wrote:
> > PTAL at this follow-up to https://codereview.chromium.org/2482853002
> > 
> > Since this is an M56 blocker speedy review is appreciated, and please shout
if
> > anything here seems risky to merge this close to stable.
> > 
> > If the final reviewer could send to CQ, that would be much appreciated.
> 
> lgtm but we should probably remove SetEditCommandsForNextKeyEvent since it
looks
> like it is now dead code.

This was unused before and after this change, but I've sent
https://codereview.chromium.org/2643643010/ to get rid of it.

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

https://codereview.chromium.org/2644843004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2644843004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1260:
input_router_->SendKeyboardEvent(key_event_with_latency);
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]

https://codereview.chromium.org/2644843004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2644843004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1260:
input_router_->SendKeyboardEvent(key_event_with_latency);
On 2017/01/20 18:20:15, sadrul wrote:
> 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?

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]

https://codereview.chromium.org/2644843004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2644843004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1260:
input_router_->SendKeyboardEvent(key_event_with_latency);
On 2017/01/20 20:01:40, foolip_slow_very_sorry wrote:
> On 2017/01/20 18:20:15, sadrul wrote:
> > 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?
> 
> 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.

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]

https://codereview.chromium.org/2644843004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2644843004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1260:
input_router_->SendKeyboardEvent(key_event_with_latency);
On 2017/01/20 21:57:22, sadrul wrote:
> On 2017/01/20 20:01:40, foolip_slow_very_sorry wrote:
> > On 2017/01/20 18:20:15, sadrul wrote:
> > > 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?
> > 
> > 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.
> 
> 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).

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.

[sadrul, 2017-01-23 18:11:11]

On 2017/01/20 22:33:12, foolip_slow_very_sorry wrote:
>
https://codereview.chromium.org/2644843004/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2644843004/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_impl.cc:1260:
> input_router_->SendKeyboardEvent(key_event_with_latency);
> On 2017/01/20 21:57:22, sadrul wrote:
> > On 2017/01/20 20:01:40, foolip_slow_very_sorry wrote:
> > > On 2017/01/20 18:20:15, sadrul wrote:
> > > > 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?
> > > 
> > > 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.
> > 
> > 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).
> 
> 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?

I think so, but I can live with it if dtapuska@ et. al. are OK with this
approach.

> 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.

Agree on this not being elegant.

> 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.

Sure. That sound reasonable to me. If you could file a bug and link that from a
comment in the code, that would be excellent. With that, lgtm

[foolip, 2017-01-24 05:48:57]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 05:49:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 07:24:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 07:24:23]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-01-24 09:07:12]

The follow up issue is https://crbug.com/684298, landing this now.

[foolip, 2017-01-24 09:07:18]

The CQ bit was checked by foolip@chromium.org

[foolip, 2017-01-24 09:07:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, sadrul@chromium.org, mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/2644843004/#ps40001
(title: "add TODO")

[commit-bot: I haz the power, 2017-01-24 09:07:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[foolip, 2017-01-24 09:08:11]

On 2017/01/24 09:07:12, foolip_UTC7 wrote:
> The follow up issue is https://crbug.com/684298, landing this now.

I should also be clear that although the TODO is in my name, the bug is not
assigned to me and I do not intend to look into this myself.

[commit-bot: I haz the power, 2017-01-24 09:11:20]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485248838357720,
"parent_rev": "ad21067abff252ddb8ab07afa83ca5674efbcb6e", "commit_rev":
"b7b0799fa40dd39cc93e826e6bacfea41231d989"}

[commit-bot: I haz the power, 2017-01-24 09:11:52]

Description was changed from

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

There was a regression in https://codereview.chromium.org/2482853002

Previously, when a keydown was handled on the browser side, the keypress
was suppressed but the keyup was not. Now, all three events are
suppressed. However, the fix did not take into consideration the
InputMsg_SetEditCommandsForNextKeyEvent messages sent right before the
call to RenderWidgetHostImpl::ForwardKeyboardEvent at two call sites.
Because the keyup event was also suppressed, those commands would
instead be associated with a following keydown.

See https://bugs.chromium.org/p/chromium/issues/detail?id=677827#c11 for
how that could end up dropping the keypress event of what followed.

The fix is to introduce a ForwardKeyboardEventWithCommands which sends
the commands only if they key itself is sent.

BUG=677827
==========

to

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

There was a regression in https://codereview.chromium.org/2482853002

Previously, when a keydown was handled on the browser side, the keypress
was suppressed but the keyup was not. Now, all three events are
suppressed. However, the fix did not take into consideration the
InputMsg_SetEditCommandsForNextKeyEvent messages sent right before the
call to RenderWidgetHostImpl::ForwardKeyboardEvent at two call sites.
Because the keyup event was also suppressed, those commands would
instead be associated with a following keydown.

See https://bugs.chromium.org/p/chromium/issues/detail?id=677827#c11 for
how that could end up dropping the keypress event of what followed.

The fix is to introduce a ForwardKeyboardEventWithCommands which sends
the commands only if they key itself is sent.

BUG=677827

Review-Url: https://codereview.chromium.org/2644843004
Cr-Commit-Position: refs/heads/master@{#445687}
Committed:
https://chromium.googlesource.com/chromium/src/+/b7b0799fa40dd39cc93e826e6bac...
==========

[commit-bot: I haz the power, 2017-01-24 09:11:53]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b7b0799fa40dd39cc93e826e6bac...