Touch emulator: allow multiple touch streams.

Touch emulator: allow multiple touch streams.
    
When both native and emulated touch streams are available,
we should block one stream while another is active.
To achieve this RenderWidgetHostImpl gives TouchEmulator a chance
to handle native touch event, so it can be effectively blocked.
    
BUG=384522
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284475

[dgozman, 2014-07-10 14:30:04]

Jared, what do you think about this approach fro multiple touch streams?
Note: no tests yet.

[jdduke (slow), 2014-07-10 14:52:58]

On 2014/07/10 14:30:04, dgozman wrote:
> Jared, what do you think about this approach fro multiple touch streams?
> Note: no tests yet.

This seams reasonable, although I think I'd prefer it if TouchEmulator contains
the TouchStreamTracker (or the logic directly), simplifying how things are wired
in RenderWidgetHostImpl.

So then, we'd add a |bool HandleTouchEvent()| method to TouchEmulator where it
would perform the touch filtering from the system (using the client's
ForwardEmulatedTouchEvent() to differentiate when the touch should be filtered).

https://codereview.chromium.org/375863005/diff/40001/content/browser/renderer...
File content/browser/renderer_host/input/touch_stream_tracker.cc (right):

https://codereview.chromium.org/375863005/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/touch_stream_tracker.cc:21: return
INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS;
We probably want to return INPUT_EVENT_ACK_STATE_CONSUMED, as otherwise the UI
will do things with the touch that we may not want (e.g., gesture detection).

[dgozman, 2014-07-14 11:43:46]

> This seams reasonable, although I think I'd prefer it if TouchEmulator
contains
> the TouchStreamTracker (or the logic directly), simplifying how things are
wired
> in RenderWidgetHostImpl.
> 
> So then, we'd add a |bool HandleTouchEvent()| method to TouchEmulator where it
> would perform the touch filtering from the system (using the client's
> ForwardEmulatedTouchEvent() to differentiate when the touch should be
filtered).

I've tried this and it looks ugly. TouchEmulatorClient's is not actually
ForwardEmulatedTouchEvent, since it should be able to forward native events too.
The typical native event route would be:
view -> host::ForwardTouchEvent -> emulator::HandleTouchEvent ->
host::AnotherForwardTouchEventThisTimeDontUseEmulator ->
inputRouter::SendTouchEvent -> ........ -> inputRouter::OnTouchEventAck ->
host::OnTouchEventAck -> emulator::OnTouchEventAck ->
host::AnotherTouchEventAckThisTimeDontUseEmulator ->
view::ProcessAckedTouchEvent.
At the same time, emulator should be able to ack early and call
host::AnotherTouchEventAckThisTimeDontUseEmulator from
emulator::HandleTouchEvent.

I think, stream tracker concept is closer to input router side rather than to
touch emulator side. What about adding |stream| member to
TouchEventWithLatencyInfo and moving TouchStramTracker to InputRouterImpl?

[jdduke (slow), 2014-07-14 16:29:34]

On 2014/07/14 11:43:46, dgozman wrote:
> > This seams reasonable, although I think I'd prefer it if TouchEmulator
> contains
> > the TouchStreamTracker (or the logic directly), simplifying how things are
> wired
> > in RenderWidgetHostImpl.
> > 
> > So then, we'd add a |bool HandleTouchEvent()| method to TouchEmulator where
it
> > would perform the touch filtering from the system (using the client's
> > ForwardEmulatedTouchEvent() to differentiate when the touch should be
> filtered).
> 
> I've tried this and it looks ugly. TouchEmulatorClient's is not actually
> ForwardEmulatedTouchEvent, since it should be able to forward native events
too.
> The typical native event route would be:
> view -> host::ForwardTouchEvent -> emulator::HandleTouchEvent ->
> host::AnotherForwardTouchEventThisTimeDontUseEmulator ->
> inputRouter::SendTouchEvent -> ........ -> inputRouter::OnTouchEventAck ->
> host::OnTouchEventAck -> emulator::OnTouchEventAck ->
> host::AnotherTouchEventAckThisTimeDontUseEmulator ->
> view::ProcessAckedTouchEvent.
> At the same time, emulator should be able to ack early and call
> host::AnotherTouchEventAckThisTimeDontUseEmulator from
> emulator::HandleTouchEvent.
> 
> I think, stream tracker concept is closer to input router side rather than to
> touch emulator side. What about adding |stream| member to
> TouchEventWithLatencyInfo and moving TouchStramTracker to InputRouterImpl?

On 2014/07/14 11:43:46, dgozman wrote:
> > This seams reasonable, although I think I'd prefer it if TouchEmulator
> contains
> > the TouchStreamTracker (or the logic directly), simplifying how things are
> wired
> > in RenderWidgetHostImpl.
> > 
> > So then, we'd add a |bool HandleTouchEvent()| method to TouchEmulator where
it
> > would perform the touch filtering from the system (using the client's
> > ForwardEmulatedTouchEvent() to differentiate when the touch should be
> filtered).
> 
> I've tried this and it looks ugly. TouchEmulatorClient's is not actually
> ForwardEmulatedTouchEvent, since it should be able to forward native events
too.
> The typical native event route would be:
> view -> host::ForwardTouchEvent -> emulator::HandleTouchEvent ->
> host::AnotherForwardTouchEventThisTimeDontUseEmulator ->
> inputRouter::SendTouchEvent -> ........ -> inputRouter::OnTouchEventAck ->
> host::OnTouchEventAck -> emulator::OnTouchEventAck ->
> host::AnotherTouchEventAckThisTimeDontUseEmulator ->
> view::ProcessAckedTouchEvent.
> At the same time, emulator should be able to ack early and call
> host::AnotherTouchEventAckThisTimeDontUseEmulator from
> emulator::HandleTouchEvent.
> 
> I think, stream tracker concept is closer to input router side rather than to
> touch emulator side. What about adding |stream| member to
> TouchEventWithLatencyInfo and moving TouchStramTracker to InputRouterImpl?

Hmm, why do we need to TouchEmulator to forward native events? I would think the
code for native events would look like:

RWHI::ForwardTouchEventWithLatencyInfo() {
  ...
  if (touch_emulator &&
touch_emulator_->HandleTouchEvent(touch_with_latency.event)) {
    if (view_)
      view_->ProcessAckedTouchEvent(touch_with_latency,
INPUT_EVENT_ACK_STATE_CONSUMED);
    return;
  }
  ...
}

then you just need a 

RWHI::ForwardEmulatedTouchEvent(touch_event) {
  input_router_->SendTouchEvent(
      TouchEventWithLatencyInfo(
          touch_event, CreateRWHLatencyInfoIfNotExist(NULL, touch_event.type)));
}

Maybe I'm missing something obvious?

[dgozman, 2014-07-15 16:14:29]

> Maybe I'm missing something obvious?
You are absolutely right. I thought there will be a problem with ack in this
case, but everything is ok.

Please take another look.

[jdduke (slow), 2014-07-15 19:43:03]

This looks really good, thanks! I only have two concerns:

1) If the touch stream is backed up, we might see a forwarded sequence like:

touchstart -> touchend -> touchstart, then suppose we get the ack for the first
touchstart and touchend pair.  The touchend ack will overwrite the
|native_stream_active_| variable that was set by the *second* touchstart. This
could happen if a user quickly taps then scrolls and the tap touch sequence ack
is delayed.

2) There's some touch short-circuiting logic in RWHVAura:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

Basically, if the renderer doesn't have a touch handler, or if the initial
touchstart event doesn't have a touch handler, the view will bypass forwarding
events.  So if the initial (native) touchstart gets forwarded, but acks with
INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS, the view won't forward any more events
for that stream and we'll be stuck in the |native_stream_active_| state until we
get a new sequence with a handler.

Issue (1) also applies to the emulated stream (a click then click drag with the
click touch acks delayed), but (2) is only for the native touch stream.

It's ugly (not really though), but we could fix issue (1) by changing the
boolean state variables for |native_stream_active_| and
|emulated_stream_active_| to counters, allowing for multiple sequences (of the
same type, emulated or native) to be in-flight at the same time.  (2) is a bit
trickier and it's actually been a source of problems in the past (see
https://code.google.com/p/chromium/issues/detail?id=375940). I'll try to give
(2) some thought.

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:151: mouse_pressed_ =
true;
Is the mouse_pressed_ variable redundant with |emulated_stream_active_|?

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:200: if
(native_stream_active_ ||
Hmm, maybe split out the conditional? 

if (!emulated_stream_active_ &&
    WebTouchEventTraits::IsTouchSequenceStart(event))) {
  native_stream_active_ = true;
}

return !native_stream_active_;


Actually, maybe that's not any better...

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:219:
gesture_provider_.OnTouchEventAck(true);
Nit: For readability maybe add something like

const bool event_consumed = true;
gesture_provider_.OnTouchEventAck(event_consumed);

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:969:
TRACE_EVENT0("input", "RenderWidgetHostImpl::ForwardTouchEvent");
TRACE_EVENT0("input", "RenderWidgetHostImpl::ForwardEmulatedTouchEvent");

https://codereview.chromium.org/375863005/diff/60001/content/common/input/web...
File content/common/input/web_touch_event_traits.cc (right):

https://codereview.chromium.org/375863005/diff/60001/content/common/input/web...
content/common/input/web_touch_event_traits.cc:35: if (event.type !=
WebInputEvent::TouchEnd &&
Hmm, I should add some unit tests for this class (no need to do that here).

[jdduke (slow), 2014-07-16 01:31:19]

sadrul@: I noticed that you added the RWHVAura |ShouldForwardTouchEvent()| code
in https://chromiumcodereview.appspot.com/9233058 to suppress forwarding of
touch events to the renderer when there is no touch handler.  

However, now that the TouchEventQueue properly suppresses forwarding anyway, I'm
wondering if this optimization is worth it.  The reason I ask is that there are
several cases where having the complete touch event stream pass through the
InputRouter (and in this patch, the RWHImpl) is useful, particularly if the
first touchstart of that stream was seen by the InputRouter. My only hesitation
here is that there is some amount of overhead involved in forwarding the touch,
then ack'ing it immediately to RWHVAura to re-synthesize ui::TouchEvents.
Perhaps that code should be optimized anyhow? Thoughts?

[dgozman, 2014-07-16 09:05:04]

I've addressed the late ack problem by switching to sequence_count, as Jared
suggested.

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:151: mouse_pressed_ =
true;
On 2014/07/15 19:43:03, jdduke wrote:
> Is the mouse_pressed_ variable redundant with |emulated_stream_active_|?

mouse_pressed_ is a generation flag, we always send events to gesture provider
while it is |true|.

emulated_stream_active_ is a forwarding flag. We may not send event further, but
still have to ack them to gesture provider.

They should be separate.

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:200: if
(native_stream_active_ ||
On 2014/07/15 19:43:03, jdduke wrote:
> Hmm, maybe split out the conditional? 
> 
> if (!emulated_stream_active_ &&
>     WebTouchEventTraits::IsTouchSequenceStart(event))) {
>   native_stream_active_ = true;
> }
> 
> return !native_stream_active_;
> 
> 
> Actually, maybe that's not any better...

This code have changed now.

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_emulator.cc:219:
gesture_provider_.OnTouchEventAck(true);
On 2014/07/15 19:43:03, jdduke wrote:
> Nit: For readability maybe add something like
> 
> const bool event_consumed = true;
> gesture_provider_.OnTouchEventAck(event_consumed);

Done.

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/375863005/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:969:
TRACE_EVENT0("input", "RenderWidgetHostImpl::ForwardTouchEvent");
On 2014/07/15 19:43:03, jdduke wrote:
> TRACE_EVENT0("input", "RenderWidgetHostImpl::ForwardEmulatedTouchEvent");

Done.

[jdduke (slow), 2014-07-16 15:17:04]

What do you think of the workaround below?

https://codereview.chromium.org/375863005/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/375863005/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:1945: return
input_router_->ShouldForwardTouchEvent();
So, one intermediate solution (kind of hacky) would be to change this to:

// It's important that the emulator sees a complete native touch stream,
allowing it to perform touch filtering as appropriate.
// TODO(jdduke): Remove when touch stream forwarding issues resolved, see
crbug.com/375940.
if (touch_emulator_ && touch_emulator_->enabled())
  return true;

return input_router_->ShouldForwardTouchEvent();

[sadrul, 2014-07-16 18:48:46]

On 2014/07/16 01:31:19, jdduke wrote:
> sadrul@: I noticed that you added the RWHVAura |ShouldForwardTouchEvent()|
code
> in https://chromiumcodereview.appspot.com/9233058 to suppress forwarding of
> touch events to the renderer when there is no touch handler.  
> 
> However, now that the TouchEventQueue properly suppresses forwarding anyway,
I'm
> wondering if this optimization is worth it.  The reason I ask is that there
are
> several cases where having the complete touch event stream pass through the
> InputRouter (and in this patch, the RWHImpl) is useful, particularly if the
> first touchstart of that stream was seen by the InputRouter.

It sounds like a good idea to keep forwarding all touch-events if the first
touch-press event was forwarded.

If the first touch-press was not forwarded, then would it still be useful for
the subsequent touch events to go through InputRouter/RWHImpl?

> My only hesitation
> here is that there is some amount of overhead involved in forwarding the
touch,
> then ack'ing it immediately to RWHVAura to re-synthesize ui::TouchEvents.

+1 I also worry that it becomes less readable, but I suppose that's not a huge
concern, since we still do have the async acks.

> Perhaps that code should be optimized anyhow? Thoughts?

[jdduke (slow), 2014-07-16 18:53:59]

On 2014/07/16 18:48:46, sadrul wrote:
> It sounds like a good idea to keep forwarding all touch-events if the first
> touch-press event was forwarded.
> 
> If the first touch-press was not forwarded, then would it still be useful for
> the subsequent touch events to go through InputRouter/RWHImpl?

That sounds reasonable.  For other purposes, it's probably OK to bypass
forwarding if there is no touch handler.  For the emulator, that could lead to
conflicting gesture streams.  I'm thinking we might do a combination of both,
where we forward all touches to the router if the emulator is enabled, and
otherwise we forward all touches to the router if there is *any* touch handler
on the page (i.e., if the first touch press was forwarded to the renderer).

dgozman@: It sounds like adding the change I suggested in #9 would be useful,
and I'll take care of the other case as part of the touch-action reset patch.

[dgozman, 2014-07-16 20:16:21]

> That sounds reasonable.  For other purposes, it's probably OK to bypass
> forwarding if there is no touch handler.  For the emulator, that could lead to
> conflicting gesture streams.  I'm thinking we might do a combination of both,
> where we forward all touches to the router if the emulator is enabled, and
> otherwise we forward all touches to the router if there is *any* touch handler
> on the page (i.e., if the first touch press was forwarded to the renderer).

I think, if we don't forward native sequence to RWHI at all, it's fine to have
emulated one at the same time.
Native touches may affect browser UI, while emulated touches will be only
visible to the renderer, right? There should be no conflict.

[dgozman, 2014-07-18 06:04:28]

Ok, so this seems like addressing all the concerns. Please take another look.
Am I right, that landing this is blocked on touch-action reset work? Is there a
bug/patch for me to follow?

[jdduke (slow), 2014-07-18 15:01:05]

content/browser/renderer_host/input and content/common/input lgtm. No need to
wait on the touch-action cleanup, I think we're OK here.

https://codereview.chromium.org/375863005/diff/140001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/375863005/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:51:
emulated_stream_active_sequence_count_(0),
Good catch.

[dgozman, 2014-07-18 18:19:21]

+sky for content/public/test

[sky, 2014-07-18 19:22:12]

LGTM

[dgozman, 2014-07-21 13:24:24]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-07-21 13:25:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dgozman@chromium.org/375863005/140001

[commit-bot: I haz the power, 2014-07-21 16:22:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-21 19:24:52]

Change committed as 284475