Initial browser-side implementation for touch-action

Initial browser-side implementation for touch-action

Receive SetTouchAction messages and filter GestureEvents in the browser
based on them.

The logic here so far is pretty trivial, but will get more complex, eg:
 - addition of pinch and double-tap gesture handling
 - support for pan-x, pan-y, pan-x/y and potentially other touch actions
 - more sophisticated handling of multiple fingers (pending WG discussion)

See touch-action design doc at http://goo.gl/KcKbxQ for more details.

Depends on blink-side change in https://codereview.chromium.org/16507017/

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

[Rick Byers, 2013-11-16 03:37:27]

Sadrul, please review.

Jared - take a look if you like, it's the basic design we discussed offline, but
happy to get any feedback you have.

I've tested this on ChromeOS and Android devices using the test cases I wrote
for the blink side of the patch, and verified the behavior is consistent with
IE11 (modulo one blink/spec bug, and unresolved multi-finger weirdness in IE -
both pending w3c working group discussions).

Thanks,
   Rick

[jdduke (slow), 2013-11-18 15:56:00]

https://codereview.chromium.org/67383002/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/67383002/diff/1/content/common/view_messages....
content/common/view_messages.h:1865:
IPC_MESSAGE_ROUTED2(ViewHostMsg_SetTouchAction,

Could we make this an InputHostMsg and put in
content/common/input/input_messages.h?

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:234: if
(touch_action_filter_.FilterGestureEvent(gesture_event.event))

Hmm, do you know why |HandleGestureScroll()| is in both |SendGestureEvent()| and
|SendGestureEventImmediately()|?  It apears  we're doing some double bookkeeping
here that could be somewhat racy, and I wonder if the same might be happening
with |touch_action_filter_|... ideally the filter would be called but once per
gesture event?

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:487:
content::TouchAction touchAction)

Nit: Brace on same line as closing paren.

[sadrul, 2013-11-19 10:36:44]

Mostly nits. Otherwise looks reasonable. (I haven't commented on all the style
issues, but a lot of the indentation/line-breaks/variable-names are currently in
blink-style).

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:487:
content::TouchAction touchAction)
touch_action

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/input/touch_event_queue.cc:205: {
{ in the previous line

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/input/touch_event_queue.h:67: bool
IsPendingAckTouchStart(int touch_id);
This could be const?

https://codereview.chromium.org/67383002/diff/90001/content/public/common/tou...
File content/public/common/touch_action.h (right):

https://codereview.chromium.org/67383002/diff/90001/content/public/common/tou...
content/public/common/touch_action.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
This file isn't necessary (instead just use the version in
content/common/input/)?

https://codereview.chromium.org/67383002/diff/90001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/67383002/diff/90001/content/renderer/render_w...
content/renderer/render_widget.cc:2788: content::TouchAction contentTouchAction;
touch_id, touch_action, etc.

https://codereview.chromium.org/67383002/diff/90001/content/renderer/render_w...
content/renderer/render_widget.cc:2790: case WebWidgetClient::TouchActionNone:
indent

[Rick Byers, 2013-11-19 22:14:59]

Thanks guys.

I think I found all the blink style issues (somehow my brain has a hard time
switching back and forth all the time <grin>).

I'm expecting to have another change in the blink API as a result of the review
of that CL, so you may want to wait for my next update to review again.

https://codereview.chromium.org/67383002/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/67383002/diff/1/content/common/view_messages....
content/common/view_messages.h:1865:
IPC_MESSAGE_ROUTED2(ViewHostMsg_SetTouchAction,
On 2013/11/18 15:56:00, jdduke wrote:
> 
> Could we make this an InputHostMsg and put in
> content/common/input/input_messages.h?

Done.  I saw this as similar to ViewHostMsg_HasTouchEventHandlers - wasn't sure
why that was a ViewHostMsg.  I could move it here too if you like.

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:234: if
(touch_action_filter_.FilterGestureEvent(gesture_event.event))
On 2013/11/18 15:56:00, jdduke wrote:
> 
> Hmm, do you know why |HandleGestureScroll()| is in both |SendGestureEvent()|
and
> |SendGestureEventImmediately()|?  It apears  we're doing some double
bookkeeping
> here that could be somewhat racy, and I wonder if the same might be happening
> with |touch_action_filter_|... ideally the filter would be called but once per
> gesture event?

Ah, I was thinking a gesture event could come in by either path - I forgot that
ForwardGestureEventImmediately should only have events that have already gone
through ForwardGestureEvent.

Perhaps the HandleGestureScroll was making the same mistake.  miletus@ you added
this, right?  Can you look into this?

Anyway fixed the touch action case here.

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:487:
content::TouchAction touchAction)
On 2013/11/18 15:56:00, jdduke wrote:
> 
> Nit: Brace on same line as closing paren.

Done.

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:487:
content::TouchAction touchAction)
On 2013/11/19 10:36:44, sadrul wrote:
> touch_action

Done.

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/input/touch_event_queue.cc:205: {
On 2013/11/19 10:36:44, sadrul wrote:
> { in the previous line

Done.

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/67383002/diff/90001/content/browser/renderer_...
content/browser/renderer_host/input/touch_event_queue.h:67: bool
IsPendingAckTouchStart(int touch_id);
On 2013/11/19 10:36:44, sadrul wrote:
> This could be const?

Done.

https://codereview.chromium.org/67383002/diff/90001/content/public/common/tou...
File content/public/common/touch_action.h (right):

https://codereview.chromium.org/67383002/diff/90001/content/public/common/tou...
content/public/common/touch_action.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/11/19 10:36:44, sadrul wrote:
> This file isn't necessary (instead just use the version in
> content/common/input/)?

Whoops, thanks.  I moved it and forgot to delete the original.

https://codereview.chromium.org/67383002/diff/90001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/67383002/diff/90001/content/renderer/render_w...
content/renderer/render_widget.cc:2788: content::TouchAction contentTouchAction;
On 2013/11/19 10:36:44, sadrul wrote:
> touch_id, touch_action, etc.

Dammit, sorry - I'll never get used to switching back and forth between blink
and chromium style :-(.

Done.

https://codereview.chromium.org/67383002/diff/90001/content/renderer/render_w...
content/renderer/render_widget.cc:2790: case WebWidgetClient::TouchActionNone:
On 2013/11/19 10:36:44, sadrul wrote:
> indent

Done.

[jdduke (slow), 2013-11-20 00:35:22]

https://codereview.chromium.org/67383002/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/67383002/diff/1/content/common/view_messages....
content/common/view_messages.h:1865:
IPC_MESSAGE_ROUTED2(ViewHostMsg_SetTouchAction,
On 2013/11/19 22:14:59, Rick Byers wrote:
> On 2013/11/18 15:56:00, jdduke wrote:
> > 
> > Could we make this an InputHostMsg and put in
> > content/common/input/input_messages.h?
> 
> Done.  I saw this as similar to ViewHostMsg_HasTouchEventHandlers - wasn't
sure
> why that was a ViewHostMsg.  I could move it here too if you like.

Ah, you're right of course.  I've been meaning to change that, but as long as we
already handle ViewHostMsg types in the router I'm fine with either ViewHost or
InputHost prefixes.

[Rick Byers, 2013-11-20 22:24:28]

On 2013/11/20 00:35:22, jdduke wrote:
> https://codereview.chromium.org/67383002/diff/1/content/common/view_messages.h
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/67383002/diff/1/content/common/view_messages....
> content/common/view_messages.h:1865:
> IPC_MESSAGE_ROUTED2(ViewHostMsg_SetTouchAction,
> On 2013/11/19 22:14:59, Rick Byers wrote:
> > On 2013/11/18 15:56:00, jdduke wrote:
> > > 
> > > Could we make this an InputHostMsg and put in
> > > content/common/input/input_messages.h?
> > 
> > Done.  I saw this as similar to ViewHostMsg_HasTouchEventHandlers - wasn't
> sure
> > why that was a ViewHostMsg.  I could move it here too if you like.
> 
> Ah, you're right of course.  I've been meaning to change that, but as long as
we
> already handle ViewHostMsg types in the router I'm fine with either ViewHost
or
> InputHost prefixes.

I think InputHost is the right place, so I'll leave it there.  But I'll avoid
polluting this CL with the unrelated change to move HasTouchEventHandlers.  I
can do that in another CL though.

I've made all the changes I anticipate as a result of the blink side review.  Do
you guys have any other feedback?

[Rick Byers, 2013-11-20 22:24:30]

On 2013/11/20 00:35:22, jdduke wrote:
> https://codereview.chromium.org/67383002/diff/1/content/common/view_messages.h
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/67383002/diff/1/content/common/view_messages....
> content/common/view_messages.h:1865:
> IPC_MESSAGE_ROUTED2(ViewHostMsg_SetTouchAction,
> On 2013/11/19 22:14:59, Rick Byers wrote:
> > On 2013/11/18 15:56:00, jdduke wrote:
> > > 
> > > Could we make this an InputHostMsg and put in
> > > content/common/input/input_messages.h?
> > 
> > Done.  I saw this as similar to ViewHostMsg_HasTouchEventHandlers - wasn't
> sure
> > why that was a ViewHostMsg.  I could move it here too if you like.
> 
> Ah, you're right of course.  I've been meaning to change that, but as long as
we
> already handle ViewHostMsg types in the router I'm fine with either ViewHost
or
> InputHost prefixes.

I think InputHost is the right place, so I'll leave it there.  But I'll avoid
polluting this CL with the unrelated change to move HasTouchEventHandlers.  I
can do that in another CL though.

I've made all the changes I anticipate as a result of the blink side review.  Do
you guys have any other feedback?

[jdduke (slow), 2013-11-20 23:03:02]

lgtm with a few nits, sadrul@ will probably also want to weigh in here.

The one question I have is w.r.t. security (or maybe just user experience).  If
the renderer is able to arbitrarily disable scrolling, that could leave the top
controls in a permanently hidden state on Android... Now, we have that problem
already with certain sites, but can you see this posing a risk? I haven't really
though through it.

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
File content/browser/renderer_host/input/touch_action_filter.cc (right):

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.cc:15: const
WebGestureEvent& gesture_event)

Nit: Brace on same line.

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.cc:48:
content::TouchAction touch_action)

Same as above.

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
File content/browser/renderer_host/input/touch_action_filter.h (right):

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.h:24:
TouchActionFilter() :

Nit: Could we move the inline constructor to the .cc?

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

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:205: if
(touch_queue_.empty())

Nit: Could you also DCHECK(!dispatching_touch_ack) here? In that case the head
of the queue is not actually pending an ack (with the current code that won't
ever be true but just to guard against future use of the public method).

https://codereview.chromium.org/67383002/diff/350001/content/common/input/tou...
File content/common/input/touch_action.h (right):

https://codereview.chromium.org/67383002/diff/350001/content/common/input/tou...
content/common/input/touch_action.h:10: enum TouchAction {

Nit: Could you perhaps put a short comment here describing how this type affects
touch behavior?  

Also, I know there's been a security push recently for serialized enum types to
have a |TOUCH_ACTION_TYPE_MAX| that you use with IPC_ENUM_TRAITS_MAX_VALUE.

[jdduke (slow), 2013-11-20 23:04:16]

On 2013/11/20 23:03:02, jdduke wrote:
> lgtm with a few nits, sadrul@ will probably also want to weigh in here.
> 
> The one question I have is w.r.t. security (or maybe just user experience). 
If
> the renderer is able to arbitrarily disable scrolling, that could leave the
top
> controls in a permanently hidden state on Android... Now, we have that problem
> already with certain sites, but can you see this posing a risk? I haven't
really
> though through it.
> 

Nevermind, the renderer can already do this by consuming TouchEvents after a
scroll, so this isn't really any added risk.

[sadrul, 2013-11-20 23:28:30]

LGTM

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/immediate_input_router.cc:485: // Synthetic
touchsstart events should get filtered out in RenderWidget.
*touchstart

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
File content/browser/renderer_host/input/touch_action_filter.cc (right):

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.cc:26: case
WebInputEvent::GestureScrollUpdateWithoutPropagation:
I don't believe we actually ever send this event

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.cc:40: default:
NOTREACHED()?

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.h:65: // Return whether
the currently pending touch event ACK is for a touch start.
*Returns

[Rick Byers, 2013-11-21 02:37:36]

Thanks guys, done.

+kenrb for security OWNERS in content/common/input_messages.h

+piman for OWNERS of content/renderer and the two .gypi files in content/.

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/immediate_input_router.cc:485: // Synthetic
touchsstart events should get filtered out in RenderWidget.
On 2013/11/20 23:28:30, sadrul wrote:
> *touchstart

Done.

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
File content/browser/renderer_host/input/touch_action_filter.cc (right):

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.cc:15: const
WebGestureEvent& gesture_event)
On 2013/11/20 23:03:02, jdduke wrote:
> 
> Nit: Brace on same line.

Done.

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.cc:26: case
WebInputEvent::GestureScrollUpdateWithoutPropagation:
On 2013/11/20 23:28:30, sadrul wrote:
> I don't believe we actually ever send this event

Yeah, looks like it's only ever generated in blink.  Removed.

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.cc:40: default:
On 2013/11/20 23:28:30, sadrul wrote:
> NOTREACHED()?

Nope - want to return false for all other types of gesture events (Tap, etc.). 
Added comment.

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.cc:48:
content::TouchAction touch_action)
On 2013/11/20 23:03:02, jdduke wrote:
> 
> Same as above.

Done.

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
File content/browser/renderer_host/input/touch_action_filter.h (right):

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.h:24:
TouchActionFilter() :
On 2013/11/20 23:03:02, jdduke wrote:
> 
> Nit: Could we move the inline constructor to the .cc?

Done.

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

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:205: if
(touch_queue_.empty())
On 2013/11/20 23:03:02, jdduke wrote:
> 
> Nit: Could you also DCHECK(!dispatching_touch_ack) here? In that case the head
> of the queue is not actually pending an ack (with the current code that won't
> ever be true but just to guard against future use of the public method).

Done.

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/67383002/diff/350001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.h:65: // Return whether
the currently pending touch event ACK is for a touch start.
On 2013/11/20 23:28:30, sadrul wrote:
> *Returns

Done.

https://codereview.chromium.org/67383002/diff/350001/content/common/input/tou...
File content/common/input/touch_action.h (right):

https://codereview.chromium.org/67383002/diff/350001/content/common/input/tou...
content/common/input/touch_action.h:10: enum TouchAction {
On 2013/11/20 23:03:02, jdduke wrote:
> 
> Nit: Could you perhaps put a short comment here describing how this type
affects
> touch behavior?  

Done.

> Also, I know there's been a security push recently for serialized enum types
to
> have a |TOUCH_ACTION_TYPE_MAX| that you use with IPC_ENUM_TRAITS_MAX_VALUE.

Thanks!  Done.

[piman, 2013-11-21 05:16:27]

LGTM+nit

https://codereview.chromium.org/67383002/diff/490001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/67383002/diff/490001/content/renderer/render_...
content/renderer/render_widget.cc:2809: NOTREACHED();
nit: remove default case to get a compile-time warning instead of a run-time
one, if the enum changes.

[Rick Byers, 2013-11-21 22:34:04]

Thanks Antoine.

https://codereview.chromium.org/67383002/diff/490001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/67383002/diff/490001/content/renderer/render_...
content/renderer/render_widget.cc:2809: NOTREACHED();
On 2013/11/21 05:16:27, piman wrote:
> nit: remove default case to get a compile-time warning instead of a run-time
> one, if the enum changes.

That would make it extremely painful to extend the blink API.  My next step is
to add some more values to the enum.  If we compile-time-asserted in chromium
then we'd have no way to add values to the definition in blink (other than
create a new enum and APIs that use it and migrate over).  I can avoid actually
sending the new values at runtime though until the chromium side has been
updated to use them.

Sigh - can't wait until blink and chromium are in the same tree ;-)

[piman, 2013-11-21 22:56:33]

On Thu, Nov 21, 2013 at 2:34 PM, <rbyers@chromium.org> wrote:

> Thanks Antoine.
>
>
>
> https://codereview.chromium.org/67383002/diff/490001/
> content/renderer/render_widget.cc
> File content/renderer/render_widget.cc (right):
>
> https://codereview.chromium.org/67383002/diff/490001/
> content/renderer/render_widget.cc#newcode2809
> content/renderer/render_widget.cc:2809: NOTREACHED();
> On 2013/11/21 05:16:27, piman wrote:
>
>> nit: remove default case to get a compile-time warning instead of a
>>
> run-time
>
>> one, if the enum changes.
>>
>
> That would make it extremely painful to extend the blink API.  My next
> step is to add some more values to the enum.  If we
> compile-time-asserted in chromium then we'd have no way to add values to
> the definition in blink (other than create a new enum and APIs that use
> it and migrate over).  I can avoid actually sending the new values at
> runtime though until the chromium side has been updated to use them.
>

I see - sort of like default implementations for webkit embedder APIs. LGTM
as is, with the promise to clean up once we merged the repos.


>
> Sigh - can't wait until blink and chromium are in the same tree ;-)
>
> https://codereview.chromium.org/67383002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kenrb, 2013-11-22 19:00:13]

lgtm

[commit-bot: I haz the power, 2013-12-09 19:17:16]

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

[commit-bot: I haz the power, 2013-12-10 00:35:56]

Change committed as 239611