ppapi: Add support for touch events.

ppapi: Add support for touch events.

The corresponding webkit side patch is at https://bugs.webkit.org/show_bug.cgi?id=89089

BUG=128236
TEST=manually, using the browser-plugin

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

[sadrul, 2012-06-14 10:17:44]

Hi! This is a first iteration of the CL to add touch-event support for pepper
plugins. It'd be great if you could take a look and suggest improvements etc.

[I had to 'git cl upload --bypass-hooks' because otherwise the presubmit script
was failing on the existing TODOs in ppb_input_event.idl/ppb_input_event.c
files]

[brettw, 2012-06-15 21:34:26]

I'd like to see what your PPB_InputEvent_Touch API would be to retrieve the
stuff from the message.

There was some previous abandoned work on touch for this. I think that had an
array of touch points for input events. Here I see you're only sending one. What
is your plan for more than one point?

[sadrul, 2012-06-18 18:51:40]

Hi!

On 2012/06/15 21:34:26, brettw wrote:
> I'd like to see what your PPB_InputEvent_Touch API would be to retrieve the
> stuff from the message.

I have added to the PPB_InputEvent_API interface for this in patchset 2. (more
changes will be necessary on top of this for remoting/nacl etc., but this is
perhaps an acceptable first step?)

> There was some previous abandoned work on touch for this. I think that had an
> array of touch points for input events. Here I see you're only sending one.
What
> is your plan for more than one point?

I went with the simpler approach of each pepper-touch-event representing an
update of a single touch-point. This is how the native systems currently do this
(i.e. a system event from X11 represents a change of a single touch-point). To
elaborate a bit more: if you have two fingers pressed, and move both fingers at
the same time, then there will be two events: one event represents the move of
one finger, and the other event represents the move of the second finger.

My understanding is there is discussion happening to have the web-touch-event
model also do the same. This also matches the touch-event model in flash:
http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/even...

The code in the patch (AppendTouchEvent) creates one InputEventData for each
WebTouchPoint that has changed since the last time the points were reported (but
looking at the WebTouchEvent::changedTouches[] list of WebTouchPoints). So a
single WebTouchEvent can in theory generate more than one InputEventData. Does
this sound reasonable?

[brettw, 2012-06-18 21:27:12]

Why are we modeling our API off of an X11 touch API that nobody programs to?
There are millions of developers writing touch APIs on Android and iOS that (I
believe) all send all fingers at once.

We get events from WebKit. Your API should basically mirror
WebKit::WebTouchEvent which is how we will receive the events (leaving out
unnecessary internals as appropriate).

[sadrul, 2012-06-18 21:42:25]

On 2012/06/18 21:27:12, brettw wrote:
> Why are we modeling our API off of an X11 touch API that nobody programs to?
> There are millions of developers writing touch APIs on Android and iOS that (I
> believe) all send all fingers at once.

The touch-event model in flash had a greater influence on this approach than the
X11 API.

[sadrul, 2012-06-18 23:18:02]

Hi!
 
> We get events from WebKit. Your API should basically mirror
> WebKit::WebTouchEvent which is how we will receive the events (leaving out
> unnecessary internals as appropriate).

I have updated the patch from borenet@ (http://codereview.chromium.org/7669019/)
and uploaded it here in patchset-3. It is working (I tested this with
browser-plugin too), however I am not sure if some of the new code needs to be
adjusted farther to fit in with ToT (architecture/style-wise). Please review.

[brettw, 2012-06-19 21:20:52]

Thanks, I think I like this overall approach better.

http://codereview.chromium.org/10543159/diff/16001/ppapi/c/dev/ppb_touch_even...
File ppapi/c/dev/ppb_touch_event_dev.h (right):

http://codereview.chromium.org/10543159/diff/16001/ppapi/c/dev/ppb_touch_even...
ppapi/c/dev/ppb_touch_event_dev.h:5: #ifndef PPAPI_C_DEV_PPB_TOUCH_EVENT_DEV_H_
You should convert this file to IDL and then generate .h file from that using
ppapi/generator/generator.py

http://codereview.chromium.org/10543159/diff/16001/ppapi/c/dev/ppb_touch_even...
ppapi/c/dev/ppb_touch_event_dev.h:31: * The x-y coordinates of this TouchPoint,
in DOM coordinate space.
One thing that we need to clarify is the coordinate space here. I don't know
what the DOM coordinate space is. Is it relative to the upper left of the
plugin? I assume not.

We're working on high DPI and the coordinate spaces can get kind of confusing,
so we should be very clear about what we mean here.

Probably relative to the plugin so that touch events match mouse events would
seem to make the most sense to me. I'm not sure if that's what the current spec
does.

http://codereview.chromium.org/10543159/diff/16001/ppapi/c/dev/ppb_touch_even...
ppapi/c/dev/ppb_touch_event_dev.h:78: struct PPB_TouchInputEvent_Dev {
I think we will want a "create" function like we have for the other input
events. These ended up being important for testing, and it will be a pain to rev
the interface later to add.

http://codereview.chromium.org/10543159/diff/16001/ppapi/shared_impl/ppb_inpu...
File ppapi/shared_impl/ppb_input_event_shared.cc (right):

http://codereview.chromium.org/10543159/diff/16001/ppapi/shared_impl/ppb_inpu...
ppapi/shared_impl/ppb_input_event_shared.cc:185: for (uint32_t i = 0; i <
points->size(); i++) {
Can you use size_t here instead?

http://codereview.chromium.org/10543159/diff/16001/ppapi/thunk/ppb_input_even...
File ppapi/thunk/ppb_input_event_thunk.cc (right):

http://codereview.chromium.org/10543159/diff/16001/ppapi/thunk/ppb_input_even...
ppapi/thunk/ppb_input_event_thunk.cc:405: if (enter.failed()) {
No {}. Same on the next fn.

[sadrul, 2012-06-20 20:19:03]

I have addressed all the comments.

http://codereview.chromium.org/10543159/diff/16001/ppapi/c/dev/ppb_touch_even...
File ppapi/c/dev/ppb_touch_event_dev.h (right):

http://codereview.chromium.org/10543159/diff/16001/ppapi/c/dev/ppb_touch_even...
ppapi/c/dev/ppb_touch_event_dev.h:5: #ifndef PPAPI_C_DEV_PPB_TOUCH_EVENT_DEV_H_
On 2012/06/19 21:20:52, brettw wrote:
> You should convert this file to IDL and then generate .h file from that using
> ppapi/generator/generator.py

Done.

http://codereview.chromium.org/10543159/diff/16001/ppapi/c/dev/ppb_touch_even...
ppapi/c/dev/ppb_touch_event_dev.h:31: * The x-y coordinates of this TouchPoint,
in DOM coordinate space.
On 2012/06/19 21:20:52, brettw wrote:
> One thing that we need to clarify is the coordinate space here. I don't know
> what the DOM coordinate space is. Is it relative to the upper left of the
> plugin? I assume not.
> 
> We're working on high DPI and the coordinate spaces can get kind of confusing,
> so we should be very clear about what we mean here.
> 
> Probably relative to the plugin so that touch events match mouse events would
> seem to make the most sense to me. I'm not sure if that's what the current
spec
> does.

These values are indeed in the same coordinate space as the mouse-events. I have
updated the docstring here to be similar to the docstring for
PPB_MouseInputEvent::GetPosition.

http://codereview.chromium.org/10543159/diff/16001/ppapi/c/dev/ppb_touch_even...
ppapi/c/dev/ppb_touch_event_dev.h:78: struct PPB_TouchInputEvent_Dev {
On 2012/06/19 21:20:52, brettw wrote:
> I think we will want a "create" function like we have for the other input
> events. These ended up being important for testing, and it will be a pain to
rev
> the interface later to add.

Done. I have currently left CreateTouchInputEvent unimplemented, and it only
returns NULL. I plan on adding implementation for this in the next CL (since it
looks like it also requires adding to the ResourceCreationAPI interface). Does
that sound acceptable?

http://codereview.chromium.org/10543159/diff/16001/ppapi/shared_impl/ppb_inpu...
File ppapi/shared_impl/ppb_input_event_shared.cc (right):

http://codereview.chromium.org/10543159/diff/16001/ppapi/shared_impl/ppb_inpu...
ppapi/shared_impl/ppb_input_event_shared.cc:185: for (uint32_t i = 0; i <
points->size(); i++) {
On 2012/06/19 21:20:52, brettw wrote:
> Can you use size_t here instead?

Done.

http://codereview.chromium.org/10543159/diff/16001/ppapi/thunk/ppb_input_even...
File ppapi/thunk/ppb_input_event_thunk.cc (right):

http://codereview.chromium.org/10543159/diff/16001/ppapi/thunk/ppb_input_even...
ppapi/thunk/ppb_input_event_thunk.cc:405: if (enter.failed()) {
On 2012/06/19 21:20:52, brettw wrote:
> No {}. Same on the next fn.

Done.

[sadrul, 2012-06-21 20:49:27]

ping!

[brettw, 2012-06-21 21:09:32]

lgtm

http://codereview.chromium.org/10543159/diff/22002/ppapi/api/ppb_touch_point.idl
File ppapi/api/ppb_touch_point.idl (right):

http://codereview.chromium.org/10543159/diff/22002/ppapi/api/ppb_touch_point....
ppapi/api/ppb_touch_point.idl:8: * This file defines the API to create a
touch-point.
This file should be called pp_touch_point.idl instead (ppb is for interfaces,
and this is just a struct definition).

http://codereview.chromium.org/10543159/diff/22002/ppapi/c/ppb_input_event.h
File ppapi/c/ppb_input_event.h (right):

http://codereview.chromium.org/10543159/diff/22002/ppapi/c/ppb_input_event.h#...
ppapi/c/ppb_input_event.h:778: * for internal use. The type must be one of the
touch event types.
Can you note that you should call AddTouchPoint to the returned resource to
modify it?

http://codereview.chromium.org/10543159/diff/22002/ppapi/c/ppb_input_event.h#...
ppapi/c/ppb_input_event.h:798: * Adds a touch point to the touch event in the
specified toucyh-list.
toucyh -> touch

[sadrul, 2012-06-21 21:25:12]

Thanks!

http://codereview.chromium.org/10543159/diff/22002/ppapi/api/ppb_touch_point.idl
File ppapi/api/ppb_touch_point.idl (right):

http://codereview.chromium.org/10543159/diff/22002/ppapi/api/ppb_touch_point....
ppapi/api/ppb_touch_point.idl:8: * This file defines the API to create a
touch-point.
On 2012/06/21 21:09:32, brettw wrote:
> This file should be called pp_touch_point.idl instead (ppb is for interfaces,
> and this is just a struct definition).

Done.

http://codereview.chromium.org/10543159/diff/22002/ppapi/c/ppb_input_event.h
File ppapi/c/ppb_input_event.h (right):

http://codereview.chromium.org/10543159/diff/22002/ppapi/c/ppb_input_event.h#...
ppapi/c/ppb_input_event.h:778: * for internal use. The type must be one of the
touch event types.
On 2012/06/21 21:09:32, brettw wrote:
> Can you note that you should call AddTouchPoint to the returned resource to
> modify it?

Done.

http://codereview.chromium.org/10543159/diff/22002/ppapi/c/ppb_input_event.h#...
ppapi/c/ppb_input_event.h:798: * Adds a touch point to the touch event in the
specified toucyh-list.
On 2012/06/21 21:09:32, brettw wrote:
> toucyh -> touch

Done.