Mark touchcancel events as uncancelable

Mark touchcancel events as uncancelable

The touch events spec says that all touch events are cancelable except for
touchcancel, and this is exposed to JS via the cancelable property.
Previously we were marking all touch events as cancelable, but as of
https://src.chromium.org/viewvc/blink?revision=172329&view=revision the
decision is up to the browser (since only it can decide if it will wait for
an ACK).  This change ensures all touchcancel events are marked as
uncancelable, and all other touch events remain cancelable. 

There are a lot of different places that create WebTouchEvents, and we
don't want some central location (like the InputRouter) to be modifying
these events.  So I've added a WebTouchEventTraits::ResetType function to
reset properties of a WebTouchEvent, applying appropriate default policy
like cancelable state.

Then we can DCHECK at the central location that an a event is cancelable
if and only if we intend to wait for it's ACK.

I've updated a handful of unit tests to verify that the cancelable bit 
is set correctly in various scenarios.

This also extends EventSender to allow the cancelable bit to be set so that
we can add a blink LayoutTest, and removes an unused
SyntheticWebTouchEventBuilder.

BUG=365681
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265991

[jdduke (slow), 2014-04-22 18:38:55]

Not sure how I feel about forcing all touch event producers to be aware of this
constraint.  If blink already sets |cancelable = false| for web-facing
TouchCancel events, do we really need the browser to do that as well?

https://codereview.chromium.org/247433003/diff/1/content/common/input/synthet...
File content/common/input/synthetic_web_input_event_builders.cc (right):

https://codereview.chromium.org/247433003/diff/1/content/common/input/synthet...
content/common/input/synthetic_web_input_event_builders.cc:159: type =
WebInputEvent::Undefined;
cancelable = true;

[Rick Byers, 2014-04-22 18:56:37]

On 2014/04/22 18:38:55, jdduke wrote:
> Not sure how I feel about forcing all touch event producers to be aware of
this
> constraint.  If blink already sets |cancelable = false| for web-facing
> TouchCancel events, do we really need the browser to do that as well?

Yeah I'm unsure about this as well.  First my blink CL doesn't set
cancelable=false for touchcancel events - it uses the cancelable field provided
by the browser (the principle being that it's really the browser's decision
whether it's going to wait for an ACK - blink can't effect that).  What you're
probably seeing is the special case of a synthetic touch event being created
from JavaScript via the initTouchEvent API (see
http://www.w3.org/TR/touch-events/#methods-1).  This API doesn't currently take
a cancelable field, so for now rather than change a web-exposed API here I've
just initialized it with a suitable value based on the type of event.  In
practice no-one uses this API as far as I can tell, so it's mostly irrelevant.

I think the messiness here is really a symptom of using this POD type all over
the place.  Ideally we'd have a constructor and initialize cancelable to a
suitable default based on the event type in one place, and only override it
elsewhere.

Absent that, we could use a helper function to centralize this creation logic.

Alternately we could munge the event somewhere in the pipeline, but that feels a
little wrong to me (should we really need to switch to non-const event types for
this?).

WDYT?

> 
>
https://codereview.chromium.org/247433003/diff/1/content/common/input/synthet...
> File content/common/input/synthetic_web_input_event_builders.cc (right):
> 
>
https://codereview.chromium.org/247433003/diff/1/content/common/input/synthet...
> content/common/input/synthetic_web_input_event_builders.cc:159: type =
> WebInputEvent::Undefined;
> cancelable = true;

[Rick Byers, 2014-04-22 19:00:58]

https://codereview.chromium.org/247433003/diff/1/content/common/input/synthet...
File content/common/input/synthetic_web_input_event_builders.cc (right):

https://codereview.chromium.org/247433003/diff/1/content/common/input/synthet...
content/common/input/synthetic_web_input_event_builders.cc:159: type =
WebInputEvent::Undefined;
On 2014/04/22 18:38:55, jdduke wrote:
> cancelable = true;

Thanks, done.

[jdduke (slow), 2014-04-22 19:11:24]

On 2014/04/22 19:00:58, Rick Byers wrote:
>
https://codereview.chromium.org/247433003/diff/1/content/common/input/synthet...
> File content/common/input/synthetic_web_input_event_builders.cc (right):
> 
>
https://codereview.chromium.org/247433003/diff/1/content/common/input/synthet...
> content/common/input/synthetic_web_input_event_builders.cc:159: type =
> WebInputEvent::Undefined;
> On 2014/04/22 18:38:55, jdduke wrote:
> > cancelable = true;
> 
> Thanks, done.

Yeah, I don't think the router should do this.  Ideally it would be
enforced/implemented near the Blink boundary, but maybe that's not much better?

I guess we could have a WebTouchEvent(InputEventType) constructor that does the
assignment upon construction, based on the type.  This doesn't solve all cases,
though, as we still need a void constructor.  It could also be confusing as
other event types take an unsigned parameter indicating the size of the object.

[Rick Byers, 2014-04-23 21:19:59]

Jared, please take a look - this is ready for full review now.

[Rick Byers, 2014-04-23 21:21:26]

On 2014/04/22 19:11:24, jdduke wrote:
> On 2014/04/22 19:00:58, Rick Byers wrote:
> >
>
https://codereview.chromium.org/247433003/diff/1/content/common/input/synthet...
> > File content/common/input/synthetic_web_input_event_builders.cc (right):
> > 
> >
>
https://codereview.chromium.org/247433003/diff/1/content/common/input/synthet...
> > content/common/input/synthetic_web_input_event_builders.cc:159: type =
> > WebInputEvent::Undefined;
> > On 2014/04/22 18:38:55, jdduke wrote:
> > > cancelable = true;
> > 
> > Thanks, done.
> 
> Yeah, I don't think the router should do this.  Ideally it would be
> enforced/implemented near the Blink boundary, but maybe that's not much
better?
> 
> I guess we could have a WebTouchEvent(InputEventType) constructor that does
the
> assignment upon construction, based on the type.  This doesn't solve all
cases,
> though, as we still need a void constructor.  It could also be confusing as
> other event types take an unsigned parameter indicating the size of the
object.

By the way (for the record), Jared and I talked offline.  We agree this is
really chromium's responsibility (as only it knows whether it's going to wait
for an ACK or not), not blinks.  We agreed to rely on a helper to centralize
this logic.  Most of the time we don't create new WebTouchEvent objects, just
update existing ones so the helper is not a builder/factory.

[jdduke (slow), 2014-04-23 21:55:47]

https://codereview.chromium.org/247433003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/247433003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_guest.cc:541:
host_->ForwardTouchEventWithLatencyInfo(cancel_event, *event->latency());
Hmm, yeah, at the least this should be touchesLength = 1...

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

https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:460:
ProcessInputEventAck(input_event.type,
Let's put the DCHECK inside this block (for the touch), then it can just be

if (WebInputEvent::isTouchEventType(input_event.type)) {
  DCHECK(!static_cast<const blink::WebTouchEvent&>(input_event).cancelable);
}

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

https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/motion_event_web.cc:20: case
WebInputEvent::TouchStart:
Is this "git cl format" ed?

https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/motion_event_web.cc:144: // FIXME: Shouldn't
we use a fresh timestamp?
Hmm... yeah, probably.

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

https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:37:
event.event.timeStampSeconds,
Nah, I don't think it matters at this point.

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

https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/web_input_event_util.cc:221:
ToWebInputEventType(event.GetAction()),
Yeah, could you run "git cl format" on the patch?

https://codereview.chromium.org/247433003/diff/40001/content/common/input/web...
File content/common/input/web_touch_event_traits.h (right):

https://codereview.chromium.org/247433003/diff/40001/content/common/input/web...
content/common/input/web_touch_event_traits.h:37: static void
ResetType(blink::WebTouchEvent* event,
I'd much prefer two methods here, |ResetType| and |ResetTypeAndTouchStates|, one
augmenting the other internally.

Normally "out" type parameters are last in the ordering.  Any reason you put it
first?

[Rick Byers, 2014-04-23 23:15:34]

https://codereview.chromium.org/247433003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/247433003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_guest.cc:541:
host_->ForwardTouchEventWithLatencyInfo(cancel_event, *event->latency());
On 2014/04/23 21:55:47, jdduke wrote:
> Hmm, yeah, at the least this should be touchesLength = 1...

But then there's the question of what the co-ordinates should be for the touch,
etc.  I think I'd prefer to avoid changing this now in this CL, wdyt?  I assume
this just affects webview / out-of-process-iframes?

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

https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:460:
ProcessInputEventAck(input_event.type,
On 2014/04/23 21:55:47, jdduke wrote:
> Let's put the DCHECK inside this block (for the touch), then it can just be
> 
> if (WebInputEvent::isTouchEventType(input_event.type)) {
>   DCHECK(!static_cast<const blink::WebTouchEvent&>(input_event).cancelable);
> }

But I also want to DCHECK the other case as well - that events that don't ignore
ACK are marked cancelable=true.  I could do that with two DCHECKs if you think
that would be clearer.

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

https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/motion_event_web.cc:144: // FIXME: Shouldn't
we use a fresh timestamp?
On 2014/04/23 21:55:47, jdduke wrote:
> Hmm... yeah, probably.

This is simple enough that I can just fix it here if you like (but then I should
add a test for it - perhaps better in a follow-up CL).

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

https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:37:
event.event.timeStampSeconds,
On 2014/04/23 21:55:47, jdduke wrote:
> Nah, I don't think it matters at this point.

Why?  Shouldn't an app be able to see that a timeout touchcancel is always
generated ~150ms after the previous touch event?  Also I think apps may expect
touch events to arrive in monotonically increasing timeout value (eg. don't
expect a negative delta from one to the next).

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

https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/web_input_event_util.cc:221:
ToWebInputEventType(event.GetAction()),
On 2014/04/23 21:55:47, jdduke wrote:
> Yeah, could you run "git cl format" on the patch?

Done.

https://codereview.chromium.org/247433003/diff/40001/content/common/input/web...
File content/common/input/web_touch_event_traits.h (right):

https://codereview.chromium.org/247433003/diff/40001/content/common/input/web...
content/common/input/web_touch_event_traits.h:37: static void
ResetType(blink::WebTouchEvent* event,
On 2014/04/23 21:55:47, jdduke wrote:
> I'd much prefer two methods here, |ResetType| and |ResetTypeAndTouchStates|,
one
> augmenting the other internally.

Done.

> Normally "out" type parameters are last in the ordering.  Any reason you put
it
> first?

I was thinking of it as in/out, but it looks like the style guide prefers those
to be last as well (although it's a little vague).  I've moved it last.

[jdduke (slow), 2014-04-23 23:29:48]

Nice, thanks! content/browser/renderer_host/input and content/common/input lgtm.

https://codereview.chromium.org/247433003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/247433003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_guest.cc:541:
host_->ForwardTouchEventWithLatencyInfo(cancel_event, *event->latency());
On 2014/04/23 23:15:35, Rick Byers wrote:
> On 2014/04/23 21:55:47, jdduke wrote:
> > Hmm, yeah, at the least this should be touchesLength = 1...
> 
> But then there's the question of what the co-ordinates should be for the
touch,
> etc.  I think I'd prefer to avoid changing this now in this CL, wdyt?  I
assume
> this just affects webview / out-of-process-iframes?

Sounds good,  yeah, I'm not super familiar with this class.

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

https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:460:
ProcessInputEventAck(input_event.type,
On 2014/04/23 23:15:35, Rick Byers wrote:
> On 2014/04/23 21:55:47, jdduke wrote:
> > Let's put the DCHECK inside this block (for the touch), then it can just be
> > 
> > if (WebInputEvent::isTouchEventType(input_event.type)) {
> >   DCHECK(!static_cast<const blink::WebTouchEvent&>(input_event).cancelable);
> > }
> 
> But I also want to DCHECK the other case as well - that events that don't
ignore
> ACK are marked cancelable=true.  I could do that with two DCHECKs if you think
> that would be clearer.
> 

Well, I was thinking of the impending case where other types will in fact be
cancelable.  I guess I should just update this class appropriately then?

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

https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:37:
event.event.timeStampSeconds,
On 2014/04/23 23:15:35, Rick Byers wrote:
> On 2014/04/23 21:55:47, jdduke wrote:
> > Nah, I don't think it matters at this point.
> 
> Why?  Shouldn't an app be able to see that a timeout touchcancel is always
> generated ~150ms after the previous touch event?  Also I think apps may expect
> touch events to arrive in monotonically increasing timeout value (eg. don't
> expect a negative delta from one to the next). 

I think we're monotonically non-decreasing, and I see your point about the
timestamps.  Doesn't blink end up over-writing all of the provided timestamps
anyway? I guess that's still not a good reason to lie here... The FIXME is fine
:)

[jdduke (slow), 2014-04-23 23:30:34]

Also, one last nit, could you format the issue description to 80 character
columns?

[Rick Byers, 2014-04-23 23:42:13]

On 2014/04/23 23:29:48, jdduke wrote:
> Nice, thanks! content/browser/renderer_host/input and content/common/input
lgtm.
> 
>
https://codereview.chromium.org/247433003/diff/40001/content/browser/frame_ho...
> File content/browser/frame_host/render_widget_host_view_guest.cc (right):
> 
>
https://codereview.chromium.org/247433003/diff/40001/content/browser/frame_ho...
> content/browser/frame_host/render_widget_host_view_guest.cc:541:
> host_->ForwardTouchEventWithLatencyInfo(cancel_event, *event->latency());
> On 2014/04/23 23:15:35, Rick Byers wrote:
> > On 2014/04/23 21:55:47, jdduke wrote:
> > > Hmm, yeah, at the least this should be touchesLength = 1...
> > 
> > But then there's the question of what the co-ordinates should be for the
> touch,
> > etc.  I think I'd prefer to avoid changing this now in this CL, wdyt?  I
> assume
> > this just affects webview / out-of-process-iframes?
> 
> Sounds good,  yeah, I'm not super familiar with this class.
> 
>
https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/input/input_router_impl.cc (right):
> 
>
https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
> content/browser/renderer_host/input/input_router_impl.cc:460:
> ProcessInputEventAck(input_event.type,
> On 2014/04/23 23:15:35, Rick Byers wrote:
> > On 2014/04/23 21:55:47, jdduke wrote:
> > > Let's put the DCHECK inside this block (for the touch), then it can just
be
> > > 
> > > if (WebInputEvent::isTouchEventType(input_event.type)) {
> > >   DCHECK(!static_cast<const
blink::WebTouchEvent&>(input_event).cancelable);
> > > }
> > 
> > But I also want to DCHECK the other case as well - that events that don't
> ignore
> > ACK are marked cancelable=true.  I could do that with two DCHECKs if you
think
> > that would be clearer.
> > 
> 
> Well, I was thinking of the impending case where other types will in fact be
> cancelable.  I guess I should just update this class appropriately then?

Yeah I haven't thought through all the details of how this merges with your CL. 
I think it's worth verifying both here to show that I got this CL right, but
yeah with your the state collapses into a single bit, so then all we need to
verify is that TouchCancel is always uncancelable.

>
https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/input/touch_event_queue.cc (right):
> 
>
https://codereview.chromium.org/247433003/diff/40001/content/browser/renderer...
> content/browser/renderer_host/input/touch_event_queue.cc:37:
> event.event.timeStampSeconds,
> On 2014/04/23 23:15:35, Rick Byers wrote:
> > On 2014/04/23 21:55:47, jdduke wrote:
> > > Nah, I don't think it matters at this point.
> > 
> > Why?  Shouldn't an app be able to see that a timeout touchcancel is always
> > generated ~150ms after the previous touch event?  Also I think apps may
expect
> > touch events to arrive in monotonically increasing timeout value (eg. don't
> > expect a negative delta from one to the next). 
> 
> I think we're monotonically non-decreasing, and I see your point about the
> timestamps.  Doesn't blink end up over-writing all of the provided timestamps
> anyway? I guess that's still not a good reason to lie here... The FIXME is
fine
> :)

Yeah we don't expose these timestamps yet, but we want to.  Yeah let's leave
with a FIXME for now.

On 2014/04/23 23:30:34, jdduke wrote:
> Also, one last nit, could you format the issue description to 80 character
> columns?

Done.  But this has always confused me - are there tools that behave badly
without this?  Is there some rule that we're supposed to manually wrap?  If its
important to enforce this then we probably need tooling (I know I've certainly
landed without doing manual wrapping).

[Rick Byers, 2014-04-23 23:49:34]

+avi for OWNERS of:
content/browser/frame_host/
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
content/browser/renderer_host/ui_events_helper.cc
content/renderer/
content/shell/

[jdduke (slow), 2014-04-23 23:53:00]

On 2014/04/23 23:42:13, Rick Byers wrote:
> Done.  But this has always confused me - are there tools that behave badly
> without this?  Is there some rule that we're supposed to manually wrap?  If
its
> important to enforce this then we probably need tooling (I know I've certainly
> landed without doing manual wrapping).

I imagine it's for readability?  I've always been reminded to do it in the past,
though I honestly haven't a clue why it's not automatically enforced.

[Rick Byers, 2014-04-24 00:03:59]

On 2014/04/23 23:53:00, jdduke wrote:
> On 2014/04/23 23:42:13, Rick Byers wrote:
> > Done.  But this has always confused me - are there tools that behave badly
> > without this?  Is there some rule that we're supposed to manually wrap?  If
> its
> > important to enforce this then we probably need tooling (I know I've
certainly
> > landed without doing manual wrapping).
> 
> I imagine it's for readability?  I've always been reminded to do it in the
past,
> though I honestly haven't a clue why it's not automatically enforced.

Yeah it looks like 'git show' doesn't wrap comments (although all our web tools
seem to do a good job).  One recent thread on this:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/rqGhY7PfkYY. 
Sounds like there's general agreement that asking people to do this manually
won't really work reliably, but no-one has improved the tooling yet...

[Avi (use Gerrit), 2014-04-24 02:26:56]

LGTM with whiny nit.

https://codereview.chromium.org/247433003/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/247433003/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_guest.cc:535: // FIXME: This
event has no touches in it.  Don't we need to know what
Chromium doesn't have "FIXME". It's spelled "TODO(yourldap):"

[Rick Byers, 2014-04-24 03:20:33]

On 2014/04/24 02:26:56, Avi wrote:
> LGTM with whiny nit.
> 
>
https://codereview.chromium.org/247433003/diff/60001/content/browser/frame_ho...
> File content/browser/frame_host/render_widget_host_view_guest.cc (right):
> 
>
https://codereview.chromium.org/247433003/diff/60001/content/browser/frame_ho...
> content/browser/frame_host/render_widget_host_view_guest.cc:535: // FIXME:
This
> event has no touches in it.  Don't we need to know what
> Chromium doesn't have "FIXME". It's spelled "TODO(yourldap):"

Thanks, replaced (x3).

[Rick Byers, 2014-04-24 03:20:40]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2014-04-24 03:23:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/247433003/80001

[commit-bot: I haz the power, 2014-04-24 05:18:35]

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

[commit-bot: I haz the power, 2014-04-24 05:18:35]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Rick Byers, 2014-04-24 14:17:21]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2014-04-24 14:18:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/247433003/80001

[commit-bot: I haz the power, 2014-04-24 21:05:45]

Change committed as 265991