Remove references to WebTouchEvent.targetTouches

Remove references to WebTouchEvent.targetTouches

This field will soon be removed entirely, instead replaced by the
|WebTouchEvent.touches| field as the global container for all touches
offered to embedders. Use that field to populate Pepper's
changed and target touch lists, eliminating Pepper's dependency on
|WebTouchEvent.changedTouches| and |WebTouchEvent.targetTouches|.

This is part 1 of a 4-sided patch simplifying WebTouchEvent:
1) This change
2) https://codereview.chromium.org/558773002/
3) https://codereview.chromium.org/559693003/
4) https://codereview.chromium.org/560493003/

BUG=358132
Committed: https://crrev.com/dd03ea640f54cd5b6e8b718cb4ab754918d9701a
Cr-Commit-Position: refs/heads/master@{#294250}

[jdduke (slow), 2014-09-09 18:31:58]

jdduke@chromium.org changed reviewers:
+ bokan@chromium.org, dmichael@chromium.org

[jdduke (slow), 2014-09-09 18:31:59]

dmichael@: Could you take a look, and perhaps offer some  guidance on
deprecation of enum values? Is there a canonical way of warning when such a
value is used?

bokan@: Can you double check my logic here? You appear to fully reconsitute
|touches| in BrowserPlugin, so I believe we're safe using that as the sole
populator for Pepper's active and changed touches. I was tempted to deprecate
CHANGED_TOUCHES, but the Pepper touch points don't have a state member, so it
may be more trouble than it's worth in the short term. My plan is to move your
fix in BrowserPlugin into Blink, then remove that last references to
|WebTouchEvent.changesTouches|, then finally remove the field entirely in Blink.

[dmichael (off chromium), 2014-09-09 19:38:46]

Our basic promise is that we don't break ABI compatibility. That means any app
that's already using this API should be continue working even if we add a new
version of the API that doesn't have this feature. We haven't really had to
deprecate any features yet to the extent that it would break an existing app. To
do so, we'd want to get the word out to developers, keep support for some period
of time, and watch UMA statistics for usage of the API to help guide us in
finally removing the API.

Can you give more insight on this class of touch events and why you're removing
it? Is there any reasonable way to make it work the same way it does today for
existing NaCl/PNaCl apps? I don't know touch events well at all, so I confess I
don't know how it works today. I notice it's listed in the spec for Touch
Events:
http://www.w3.org/TR/touch-events/#touches-and-targettouches-of-a-touchevent
...so what's the rationale of removing it?

[jdduke (slow), 2014-09-09 20:05:29]

On 2014/09/09 19:38:46, dmichael wrote:
> Our basic promise is that we don't break ABI compatibility. That means any app
> that's already using this API should be continue working even if we add a new
> version of the API that doesn't have this feature. We haven't really had to
> deprecate any features yet to the extent that it would break an existing app.
To
> do so, we'd want to get the word out to developers, keep support for some
period
> of time, and watch UMA statistics for usage of the API to help guide us in
> finally removing the API.
> 
> Can you give more insight on this class of touch events and why you're
removing
> it? Is there any reasonable way to make it work the same way it does today for
> existing NaCl/PNaCl apps? I don't know touch events well at all, so I confess
I
> don't know how it works today. I notice it's listed in the spec for Touch
> Events:
> http://www.w3.org/TR/touch-events/#touches-and-targettouches-of-a-touchevent
> ...so what's the rationale of removing it?

Right now Pepper is the sole consumer of the |changedTouches| and
|targetTouches| properties on WebTouchEvent.  It's a relatively minor
optimization to remove them, but we think it worthwhile. Moreover, these
properties can be reconstituted on demand.

For maintaining ABI compatibility, we could continue populating Pepper's CHANGED
and TARGET touches, we would just do it dynamically (from
WebTouchEvent.touches), instead of using the changedTouches and targetTouches
values provided by Blink. CHANGED is the easy case, TARGET, it's a little less
clear, so I think we'd just fill it with all active touch events.

[dmichael (off chromium), 2014-09-09 20:13:56]

On 2014/09/09 20:05:29, jdduke wrote:
> On 2014/09/09 19:38:46, dmichael wrote:
> > Our basic promise is that we don't break ABI compatibility. That means any
app
> > that's already using this API should be continue working even if we add a
new
> > version of the API that doesn't have this feature. We haven't really had to
> > deprecate any features yet to the extent that it would break an existing
app.
> To
> > do so, we'd want to get the word out to developers, keep support for some
> period
> > of time, and watch UMA statistics for usage of the API to help guide us in
> > finally removing the API.
> > 
> > Can you give more insight on this class of touch events and why you're
> removing
> > it? Is there any reasonable way to make it work the same way it does today
for
> > existing NaCl/PNaCl apps? I don't know touch events well at all, so I
confess
> I
> > don't know how it works today. I notice it's listed in the spec for Touch
> > Events:
> > http://www.w3.org/TR/touch-events/#touches-and-targettouches-of-a-touchevent
> > ...so what's the rationale of removing it?
> 
> Right now Pepper is the sole consumer of the |changedTouches| and
> |targetTouches| properties on WebTouchEvent.  It's a relatively minor
> optimization to remove them, but we think it worthwhile. Moreover, these
> properties can be reconstituted on demand.
> 
> For maintaining ABI compatibility, we could continue populating Pepper's
CHANGED
> and TARGET touches, we would just do it dynamically (from
> WebTouchEvent.touches), instead of using the changedTouches and targetTouches
> values provided by Blink. CHANGED is the easy case, TARGET, it's a little less
> clear, so I think we'd just fill it with all active touch events.
That seems fine.

Would it make sense to just leave the API alone and not mark it deprecated?
Deprecating it for us would mean that we'd generate a whole new C API struct,
modify the C++ wrapper, etc so that only already existing binaries would use the
1.2 interface, and new code would be using 1.3, etc etc.

I'd rather just leave the .idl file and everything that gets generated from it
alone if practical.

[bokan, 2014-09-09 20:17:38]

Logic looks good to me assuming this was correct before. However, with the lack
of usage this gets I'm nervous it may not be (the browser_plugin fix I made
rendered multitouch completely broken and no one noticed).

For starters, I'm highly ignorant of how Pepper works; do Pepper plugin events
go through browser_plugin? That fix I made was because <webview> apps on
ChromeOS were busted so I'd just like to confirm they use the same path.

I'd feel much better if we had (ideally) a test, or at least some app that had a
pepper plugin like Rick's painting app so that we could verify basic
functionality.

https://codereview.chromium.org/557863002/diff/1/content/renderer/browser_plu...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/557863002/diff/1/content/renderer/browser_plu...
content/renderer/browser_plugin/browser_plugin.cc:591: if
(touch_event->changedTouchesLength)
Nit: I should have done this when i was here...can you add curly braces around
this 'if'

https://codereview.chromium.org/557863002/diff/1/content/renderer/pepper/even...
File content/renderer/pepper/event_conversion.cc (right):

https://codereview.chromium.org/557863002/diff/1/content/renderer/pepper/even...
content/renderer/pepper/event_conversion.cc:336: event.touches, states_map,
web_event->touches, &web_event->touchesLength);
Where do the InputEventData events come from? Will they always be created in
AppendTouchEvent above? If not, are we sure that event.touches will include both
stationary and changed touches?

https://codereview.chromium.org/557863002/diff/1/content/renderer/pepper/even...
content/renderer/pepper/event_conversion.cc:340:
SetWebTouchPoints(event.changed_touches,
Same question again, are we sure ToucheEnd/Cancel wont already be in the touches
list?

[jdduke (slow), 2014-09-09 20:41:06]

Patchset #3 (id:40001) has been deleted

[jdduke (slow), 2014-09-09 20:43:42]

jdduke@chromium.org changed reviewers:
+ fsamuel@chromium.org

[jdduke (slow), 2014-09-09 20:43:43]

dmichael@: OK, I think we can get away without deprecation while still avoiding
the dependency on said WebInputEvent fields.

PTAL for owner review, thanks.

fsamuel@: content/renderer/browser_plugin/browser_plugin.cc
pfeldman@: content/shell/renderer/test_runner/test_plugin.cc

[jdduke (slow), 2014-09-09 20:52:24]

https://codereview.chromium.org/557863002/diff/1/content/renderer/browser_plu...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/557863002/diff/1/content/renderer/browser_plu...
content/renderer/browser_plugin/browser_plugin.cc:591: if
(touch_event->changedTouchesLength)
On 2014/09/09 20:17:38, bokan wrote:
> Nit: I should have done this when i was here...can you add curly braces around
> this 'if'

Done.

https://codereview.chromium.org/557863002/diff/1/content/renderer/pepper/even...
File content/renderer/pepper/event_conversion.cc (right):

https://codereview.chromium.org/557863002/diff/1/content/renderer/pepper/even...
content/renderer/pepper/event_conversion.cc:336: event.touches, states_map,
web_event->touches, &web_event->touchesLength);
On 2014/09/09 20:17:38, bokan wrote:
> Where do the InputEventData events come from? Will they always be created in
> AppendTouchEvent above? If not, are we sure that event.touches will include
both
> stationary and changed touches?

We're not sure, so we have the branch below. I think we can assume that touches
should include *active* touches, but not necessarily released touches.

https://codereview.chromium.org/557863002/diff/1/content/renderer/pepper/even...
content/renderer/pepper/event_conversion.cc:340:
SetWebTouchPoints(event.changed_touches,
On 2014/09/09 20:17:38, bokan wrote:
> Same question again, are we sure ToucheEnd/Cancel wont already be in the
touches
> list?

Ah, you're right, I thought SetWebTouchPoints checked for redundancy, I'll have
to adjust it.

[Fady Samuel, 2014-09-09 20:59:18]

browser_plugin lgtm

[jdduke (slow), 2014-09-09 21:49:33]

jdduke@chromium.org changed reviewers:
+ pfeldman@chromium.org

[jdduke (slow), 2014-09-09 21:49:34]

Oops, actually adding pfeldman@ as reviewer now...

pfeldman@: content/shell/renderer/test_runner/test_plugin.cc

bokan@: I added a unit test for conversion, would you mind taking another look?
Thanks.

[bokan, 2014-09-09 21:55:21]

On 2014/09/09 21:49:34, jdduke wrote:
> Oops, actually adding pfeldman@ as reviewer now...
> 
> pfeldman@: content/shell/renderer/test_runner/test_plugin.cc
> 
> bokan@: I added a unit test for conversion, would you mind taking another
look?
> Thanks.

Awesome, lgtm!

[dmichael (off chromium), 2014-09-09 23:03:30]

pepper lgtm.

Thanks for the new unit test!

We also have an integration test here:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/tests/test_i...
...but it doesn't test the detail that the new unit test does, obviously.

[jdduke (slow), 2014-09-10 02:43:48]

Patchset #5 (id:100001) has been deleted

[pfeldman, 2014-09-10 18:39:34]

content/shell lgtm

[jdduke (slow), 2014-09-10 18:45:11]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-09-10 18:52:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/557863002/120001

[commit-bot: I haz the power, 2014-09-10 20:35:47]

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

[commit-bot: I haz the power, 2014-09-10 20:35:48]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jdduke (slow), 2014-09-10 20:37:47]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-09-10 21:01:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/557863002/120001

[commit-bot: I haz the power, 2014-09-10 22:59:14]

Committed patchset #5 (id:120001) as 3e032755ee50674ad5011451f25b08662c5bdc10

[commit-bot: I haz the power, 2014-09-10 23:10:42]

Patchset 5 (id:??) landed as
https://crrev.com/dd03ea640f54cd5b6e8b718cb4ab754918d9701a
Cr-Commit-Position: refs/heads/master@{#294250}