Add tilt_x and tilt_y to ppapi touchpoint.

Add tilt_x and tilt_y to ppapi touchpoint.

This patch adds tilt_x and tilt_y fields to pp_touch_point.

BUG=715289
TEST=

Review-Url: https://codereview.chromium.org/2890323002
Cr-Commit-Position: refs/heads/master@{#474977}
Committed: https://chromium.googlesource.com/chromium/src/+/424fd27db211d380ddc81f09f0f35b71661e6cdf

[jkwang, 2017-05-19 18:00:59]

Description was changed from

==========
Add tilt_x and tilt_y to ppapi touchpoint.

This patch adds tilt_x and tilt_y fields to pp_touch_point.

BUG=715289
TEST=
==========

to

==========
Add tilt_x and tilt_y to ppapi touchpoint.

This patch adds tilt_x and tilt_y fields to pp_touch_point.

BUG=715289
TEST=
==========

[jkwang, 2017-05-19 18:00:59]

jkwang@google.com changed reviewers:
+ bbudge@chromium.org

[bbudge, 2017-05-19 22:52:48]

Thanks for reworking to avoid versioning PP_TouchPoint.

https://codereview.chromium.org/2890323002/diff/1/ppapi/cpp/input_event.cc
File ppapi/cpp/input_event.cc (right):

https://codereview.chromium.org/2890323002/diff/1/ppapi/cpp/input_event.cc#ne...
ppapi/cpp/input_event.cc:327: if (!has_interface<PPB_TouchInputEvent_1_1>())
use brackets for multiline body.

Also, the conditional is wrong.

https://codereview.chromium.org/2890323002/diff/1/ppapi/cpp/touch_point.h
File ppapi/cpp/touch_point.h (right):

https://codereview.chromium.org/2890323002/diff/1/ppapi/cpp/touch_point.h#new...
ppapi/cpp/touch_point.h:54: /// and 90, with 0 indiciating 0 degree and 90
indicate 90 degrees.
nit s/degree/degrees

and below

https://codereview.chromium.org/2890323002/diff/1/ppapi/shared_impl/ppb_input...
File ppapi/shared_impl/ppb_input_event_shared.cc (right):

https://codereview.chromium.org/2890323002/diff/1/ppapi/shared_impl/ppb_input...
ppapi/shared_impl/ppb_input_event_shared.cc:168: }
This switch is now replicated 4 times. Perhaps a helper function to get the
touch list?

[jkwang, 2017-05-23 04:07:45]

https://codereview.chromium.org/2890323002/diff/1/ppapi/cpp/input_event.cc
File ppapi/cpp/input_event.cc (right):

https://codereview.chromium.org/2890323002/diff/1/ppapi/cpp/input_event.cc#ne...
ppapi/cpp/input_event.cc:327: if (!has_interface<PPB_TouchInputEvent_1_1>())
On 2017/05/19 22:52:47, bbudge wrote:
> use brackets for multiline body.
> 
> Also, the conditional is wrong.

Done.

https://codereview.chromium.org/2890323002/diff/1/ppapi/cpp/touch_point.h
File ppapi/cpp/touch_point.h (right):

https://codereview.chromium.org/2890323002/diff/1/ppapi/cpp/touch_point.h#new...
ppapi/cpp/touch_point.h:54: /// and 90, with 0 indiciating 0 degree and 90
indicate 90 degrees.
On 2017/05/19 22:52:47, bbudge wrote:
> nit s/degree/degrees
> 
> and below

Done.

https://codereview.chromium.org/2890323002/diff/1/ppapi/shared_impl/ppb_input...
File ppapi/shared_impl/ppb_input_event_shared.cc (right):

https://codereview.chromium.org/2890323002/diff/1/ppapi/shared_impl/ppb_input...
ppapi/shared_impl/ppb_input_event_shared.cc:168: }
On 2017/05/19 22:52:47, bbudge wrote:
> This switch is now replicated 4 times. Perhaps a helper function to get the
> touch list?

Done.

[bbudge, 2017-05-23 18:28:47]

A few more comments, if you can address issues, I'll review again today.

https://codereview.chromium.org/2890323002/diff/20001/ppapi/api/ppb_input_eve...
File ppapi/api/ppb_input_event.idl (right):

https://codereview.chromium.org/2890323002/diff/20001/ppapi/api/ppb_input_eve...
ppapi/api/ppb_input_event.idl:974: * @param[in] touch_id The id of the
touch-point.
this should be 'index'

https://codereview.chromium.org/2890323002/diff/20001/ppapi/cpp/touch_point.h
File ppapi/cpp/touch_point.h (right):

https://codereview.chromium.org/2890323002/diff/20001/ppapi/cpp/touch_point.h...
ppapi/cpp/touch_point.h:59: float tilt_y() const { return tilt_.y; }
Having a single 'tilt' getter returning a FloatPoint seems more consistent with
the existing code.

https://codereview.chromium.org/2890323002/diff/20001/ppapi/shared_impl/ppb_i...
File ppapi/shared_impl/ppb_input_event_shared.cc (right):

https://codereview.chromium.org/2890323002/diff/20001/ppapi/shared_impl/ppb_i...
ppapi/shared_impl/ppb_input_event_shared.cc:158: break;
breaks are unreachable, remove.

https://codereview.chromium.org/2890323002/diff/20001/ppapi/shared_impl/ppb_i...
ppapi/shared_impl/ppb_input_event_shared.cc:190: }
You could further reduce duplication by adding another utility method,
GetTouchByTypeAndId which calls GetTouchListByType, then searches for id.

https://codereview.chromium.org/2890323002/diff/20001/ppapi/shared_impl/ppb_i...
ppapi/shared_impl/ppb_input_event_shared.cc:191: return PP_MakeTouchPoint();
Then this function body would be just:

return GetTouchByTypeAndId(list, id).touch;

and below:

return GetTouchByTypeAndId(list, id).tilt;

[jkwang, 2017-05-23 22:07:57]

It turns out I forgot to add interface_name<PPB_TouchInputEvent_1_1>().

1.1 works just fine and I believe PPB_TouchInputEvent is versioned separately.
I did following test:

new_sdk = sdk build locally with this patch
old_sdk = released sdk without this patch
new_chrome = chrome build with this patch
old_chrome = chrome without this patch

I tested (new_sdk, old_chrome), (new_sdk, new_chrome), (old_sdk, new_chrome).
They all behave as expected. Currently, it is marked as stable. Please let me
know if I have to mark it dev.

https://codereview.chromium.org/2890323002/diff/20001/ppapi/api/ppb_input_eve...
File ppapi/api/ppb_input_event.idl (right):

https://codereview.chromium.org/2890323002/diff/20001/ppapi/api/ppb_input_eve...
ppapi/api/ppb_input_event.idl:974: * @param[in] touch_id The id of the
touch-point.
On 2017/05/23 18:28:46, bbudge wrote:
> this should be 'index'

Done.

https://codereview.chromium.org/2890323002/diff/20001/ppapi/cpp/touch_point.h
File ppapi/cpp/touch_point.h (right):

https://codereview.chromium.org/2890323002/diff/20001/ppapi/cpp/touch_point.h...
ppapi/cpp/touch_point.h:59: float tilt_y() const { return tilt_.y; }
On 2017/05/23 18:28:46, bbudge wrote:
> Having a single 'tilt' getter returning a FloatPoint seems more consistent
with
> the existing code.

Done.

https://codereview.chromium.org/2890323002/diff/20001/ppapi/shared_impl/ppb_i...
File ppapi/shared_impl/ppb_input_event_shared.cc (right):

https://codereview.chromium.org/2890323002/diff/20001/ppapi/shared_impl/ppb_i...
ppapi/shared_impl/ppb_input_event_shared.cc:158: break;
On 2017/05/23 18:28:47, bbudge wrote:
> breaks are unreachable, remove.

Done.

https://codereview.chromium.org/2890323002/diff/20001/ppapi/shared_impl/ppb_i...
ppapi/shared_impl/ppb_input_event_shared.cc:190: }
On 2017/05/23 18:28:47, bbudge wrote:
> You could further reduce duplication by adding another utility method,
> GetTouchByTypeAndId which calls GetTouchListByType, then searches for id.

Done.

https://codereview.chromium.org/2890323002/diff/20001/ppapi/shared_impl/ppb_i...
ppapi/shared_impl/ppb_input_event_shared.cc:191: return PP_MakeTouchPoint();
On 2017/05/23 18:28:46, bbudge wrote:
> Then this function body would be just:
> 
> return GetTouchByTypeAndId(list, id).touch;
> 
> and below:
> 
> return GetTouchByTypeAndId(list, id).tilt;

Done.

[bbudge, 2017-05-24 00:28:31]

https://codereview.chromium.org/2890323002/diff/40001/ppapi/api/ppb_input_eve...
File ppapi/api/ppb_input_event.idl (right):

https://codereview.chromium.org/2890323002/diff/40001/ppapi/api/ppb_input_eve...
ppapi/api/ppb_input_event.idl:979: [version=1.1]
You have to create a new version - otherwise the milestone will be wrong. So
above, add:

M60 = 1.4

[jkwang, 2017-05-24 17:56:32]

jkwang@google.com changed reviewers:
+ dcheng@chromium.org, jochen@chromium.org

[jkwang, 2017-05-24 17:57:42]

jochen,
It's another part for this design doc sent by tbuckley@.
https://docs.google.com/a/google.com/document/d/1YpOR3sSqlzBFv0HFqNqlM9GpStW1...

dcheng,
Could you do an owner review of ppapi/proxy/*

https://codereview.chromium.org/2890323002/diff/40001/ppapi/api/ppb_input_eve...
File ppapi/api/ppb_input_event.idl (right):

https://codereview.chromium.org/2890323002/diff/40001/ppapi/api/ppb_input_eve...
ppapi/api/ppb_input_event.idl:979: [version=1.1]
On 2017/05/24 00:28:31, bbudge wrote:
> You have to create a new version - otherwise the milestone will be wrong. So
> above, add:
> 
> M60 = 1.4

Done.

[jochen (gone - plz use gerrit), 2017-05-24 19:32:39]

I agree that this is covered by the design doc, so lgtm on that

please wait for bbudge and dcheng to approve as well

[bbudge, 2017-05-25 21:38:52]

lgtm

[dcheng, 2017-05-25 21:55:02]

LGTM with some nits.

(The naming/comment nit can be addressed separately--assuming other people feel
it makes sense).

https://codereview.chromium.org/2890323002/diff/60001/ppapi/cpp/touch_point.h
File ppapi/cpp/touch_point.h (right):

https://codereview.chromium.org/2890323002/diff/60001/ppapi/cpp/touch_point.h...
ppapi/cpp/touch_point.h:16: /// Wrapper class for PP_TouchPoint.
Nit: this comment and/or class name might need updating. It's a bit confusing
that we have TouchPointWithTilt for IPC and TouchPoint holding a TouchPoint and
a FloatPoint (presumably for tilt) here.

https://codereview.chromium.org/2890323002/diff/60001/ppapi/shared_impl/ppb_i...
File ppapi/shared_impl/ppb_input_event_shared.cc (right):

https://codereview.chromium.org/2890323002/diff/60001/ppapi/shared_impl/ppb_i...
ppapi/shared_impl/ppb_input_event_shared.cc:162: default:
Nit: remove default, move return nullptr to outside the switch. That way the
compiler will warn for future unhandled values.

https://codereview.chromium.org/2890323002/diff/60001/ppapi/shared_impl/ppb_i...
ppapi/shared_impl/ppb_input_event_shared.cc:205: if (!points || index >=
points->size()) {
Super minor nit: be consistent with { }'s around one-line ifs (line 196 and line
215 both aren't wrapped in { }'s)

[jkwang, 2017-05-25 22:14:36]

The CQ bit was checked by jkwang@google.com

[jkwang, 2017-05-25 22:14:37]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
jochen@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2890323002/#ps80001
(title: "Add tilt_x and tilt_y to ppapi touchpoint.")

[commit-bot: I haz the power, 2017-05-25 22:15:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 22:56:00]

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

[commit-bot: I haz the power, 2017-05-25 22:56:02]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jkwang, 2017-05-26 09:18:59]

The CQ bit was checked by jkwang@google.com

[jkwang, 2017-05-26 09:18:59]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
jochen@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2890323002/#ps100001
(title: "Add tilt_x and tilt_y to ppapi touchpoint.")

[commit-bot: I haz the power, 2017-05-26 09:19:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 10:22:28]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495790339277690,
"parent_rev": "0ed1a20ddb4f1e7020211d3328150d56fdda21a6", "commit_rev":
"424fd27db211d380ddc81f09f0f35b71661e6cdf"}

[commit-bot: I haz the power, 2017-05-26 10:22:39]

Description was changed from

==========
Add tilt_x and tilt_y to ppapi touchpoint.

This patch adds tilt_x and tilt_y fields to pp_touch_point.

BUG=715289
TEST=
==========

to

==========
Add tilt_x and tilt_y to ppapi touchpoint.

This patch adds tilt_x and tilt_y fields to pp_touch_point.

BUG=715289
TEST=

Review-Url: https://codereview.chromium.org/2890323002
Cr-Commit-Position: refs/heads/master@{#474977}
Committed:
https://chromium.googlesource.com/chromium/src/+/424fd27db211d380ddc81f09f0f3...
==========

[commit-bot: I haz the power, 2017-05-26 10:22:45]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/424fd27db211d380ddc81f09f0f3...