Remove PlatformTouchEvent/Point and use WebTouchEvent/Point instead

Remove PlatformTouchEvent/Point and use WebTouchEvent/Point instead

Perform similar cleanup as GestureEvents used in change:
https://codereview.chromium.org/2539283002/
https://codereview.chromium.org/2586133003

Cleanup proposal: https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7FphKI/edit

BUG=625684
Review-Url: https://codereview.chromium.org/2646163002
Cr-Commit-Position: refs/heads/master@{#446782}
Committed: https://chromium.googlesource.com/chromium/src/+/5f7cc1167a9026405c671be0f81ff551f9ebd47b

[dtapuska, 2017-01-20 19:34:07]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 19:34:33]

Dry run: 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-01-20 19:38:00]

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

[commit-bot: I haz the power, 2017-01-20 19:38:01]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dtapuska, 2017-01-20 19:54:38]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 19:55:10]

Dry run: 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-01-20 20:47:11]

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

[commit-bot: I haz the power, 2017-01-20 20:47:12]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[dtapuska, 2017-01-20 21:08:00]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 21:08:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dtapuska, 2017-01-20 21:45:09]

dtapuska@chromium.org changed reviewers:
+ mustaq@chromium.org, nzolghadr@chromium.org, rbyers@chromium.org

[commit-bot: I haz the power, 2017-01-20 22:46:50]

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

[commit-bot: I haz the power, 2017-01-20 22:46:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mustaq, 2017-01-23 20:05:17]

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:387:
static_cast<PlatformEvent::Modifiers>(event.modifiers()),
We will ultimately need a UIEventWithKeyState::setFromWebModifiers(). Let's add
it now & switch the type in create().

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right):

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/TouchEventManager.cpp:412:
.scaledBy(scaleFactor);
Do we have an explanation why this radius-only scaling? This looks like undoing
WebTouchEvent::flattenTransform(), right? Please add  a todo if the answer is
non-trivial.

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:239: WebTouchEvent
TransformWebTouchEvent(float frameScale,
- Shouldn't we flatten result first? Otherwise |result| would depend on whether
|event| was flattened before or not.

- I think this method should be in WebInputEvent (or WebTouchEvent for now).
Better to put all transforms in one place.

- Please make it clear that the passed event remains unchanged (even with the
const). TransformedWebTouchEvent? Or even CreateTransformedWebTouchEvent?

[dtapuska, 2017-01-24 14:26:14]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 14:26:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dtapuska, 2017-01-24 14:26:28]

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:387:
static_cast<PlatformEvent::Modifiers>(event.modifiers()),
On 2017/01/23 20:05:17, mustaq wrote:
> We will ultimately need a UIEventWithKeyState::setFromWebModifiers(). Let's
add
> it now & switch the type in create().

Done.

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right):

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/TouchEventManager.cpp:412:
.scaledBy(scaleFactor);
On 2017/01/23 20:05:17, mustaq wrote:
> Do we have an explanation why this radius-only scaling? This looks like
undoing
> WebTouchEvent::flattenTransform(), right? Please add  a todo if the answer is
> non-trivial. 
> 

The pagePoint was scaled above...

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:239: WebTouchEvent
TransformWebTouchEvent(float frameScale,
On 2017/01/23 20:05:17, mustaq wrote:
> - Shouldn't we flatten result first? Otherwise |result| would depend on
whether
> |event| was flattened before or not.
> 
> - I think this method should be in WebInputEvent (or WebTouchEvent for now).
> Better to put all transforms in one place.
> 
> - Please make it clear that the passed event remains unchanged (even with the
> const). TransformedWebTouchEvent? Or even CreateTransformedWebTouchEvent?

The API returns a copy of the current event. I'll DCHECK that it already isn't
transformed.

[commit-bot: I haz the power, 2017-01-24 16:21:53]

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

[commit-bot: I haz the power, 2017-01-24 16:21:54]

Dry run: This issue passed the CQ dry run.

[mustaq, 2017-01-24 16:59:41]

LGTM % a few nits.

Thanks for the whole platform event cleanup.

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:239: WebTouchEvent
TransformWebTouchEvent(float frameScale,
On 2017/01/24 14:26:27, dtapuska wrote:
> On 2017/01/23 20:05:17, mustaq wrote:
> > - Shouldn't we flatten result first? Otherwise |result| would depend on
> whether
> > |event| was flattened before or not.
> > 
> > - I think this method should be in WebInputEvent (or WebTouchEvent for now).
> > Better to put all transforms in one place.
> > 
> > - Please make it clear that the passed event remains unchanged (even with
the
> > const). TransformedWebTouchEvent? Or even CreateTransformedWebTouchEvent?
> 
> The API returns a copy of the current event. I'll DCHECK that it already isn't
> transformed.

Please also DCHECK frameTranslate.

Also consider s/TransformWebTouchEvent/CreateTransformedWebTouchEvent/ as I
suggested above, to make it consistent with the "wrapper" below
(CreateTransformedWebTouchEventVector()).

https://codereview.chromium.org/2646163002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorInputAgent.cpp (right):

https://codereview.chromium.org/2646163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInputAgent.cpp:220: 
I guess we need to call event.setFrameTranslate() here too?

[dtapuska, 2017-01-24 21:09:11]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 21:09:36]

Dry run: 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-01-24 23:40:33]

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

[commit-bot: I haz the power, 2017-01-24 23:40:34]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-01-25 02:43:34]

https://codereview.chromium.org/2646163002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorInputAgent.cpp (right):

https://codereview.chromium.org/2646163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInputAgent.cpp:220: 
On 2017/01/24 16:59:41, mustaq wrote:
> I guess we need to call event.setFrameTranslate() here too?

Done.

[dtapuska, 2017-01-25 02:45:12]

dtapuska@chromium.org changed reviewers:
+ bokan@chromium.org

[dtapuska, 2017-01-25 02:45:13]

bokan@chromium.org: Can you take a look at this and then perhaps Rick can stamp
it based on your review? As I think only the public changes are outside of your
ownership

[bokan, 2017-01-25 15:19:48]

Looks great, mostly a few nits and questions.

For my own understanding: previously, WebTouchPoint was always in "viewport
coordinates" and converting it to a PlatformTouchPoint applied the pinch-zoom
scale/translation so that PTP was always in "root frame coordinates", right?
With this change, WebTouchPoint when entering a WebViewImpl is still in
"viewport coordinates" but when it goes through WebInputEventConversion we add
the m_frameScale and m_frameTranslate values to it. Code that previously used
PTP now simply uses WebTouchEvent.touchPointInRootFrame to get the WebTouchPoint
but transformed into the root frame? (with Plugins "flattening" the transform
and having the plugin location "baked in"). Is my understanding correct?

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/TouchEvent.cpp:227:
m_nativeEvent.reset(new WebTouchEvent(event));
Why do we have to clone the WebTouchEvent rather than just keeping a pointer to
it?

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:40: for (unsigned
point = 0; point < touchEvent.touchesLength; ++point) {
nit: since this is just an index now, s/point/i

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebInputEventConversion.h (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebInputEventConversion.h:92:
WebTouchEventBuilder(const LayoutItem, const TouchEvent&);
You removed the implementation, presumably this class is unneeded now?

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:828:
event->nativeEvent()->touchPointInRootFrame(i).position;
I think this can just be transformedEvent->touches[i].position, right?

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebTouchPoint.h (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebTouchPoint.h:65: WebFloatPoint position;
Q: In the design doc you proposed making the members protected and giving them
accessors. Is that still the plan?

[bokan, 2017-01-25 15:22:35]

Also, if you haven't, please do a quick test of DevTools' device emulation to
make sure we're still handling the emulation transform correctly (device
emulation adds another layer of transform before the "pinch-zoom" transform,
that's how they shrink and center the screen when emulating mobile). The code
looks right to me but just a quick smoke test would make me more confident.

[dtapuska, 2017-01-25 17:59:17]

Yes bokan@ you are correct in your assumptions on how it works. I've tried on in
devtools and on a windows touch screen device an all seems well.

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/TouchEvent.cpp:227:
m_nativeEvent.reset(new WebTouchEvent(event));
On 2017/01/25 15:19:48, bokan wrote:
> Why do we have to clone the WebTouchEvent rather than just keeping a pointer
to
> it?

The lifecycle of the TouchEvent is related to the DOM event so it extends beyond
the lifecycle of the passed in parameter and not every TouchEvent will have a
native event (ie synthetic events)

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:40: for (unsigned
point = 0; point < touchEvent.touchesLength; ++point) {
On 2017/01/25 15:19:48, bokan wrote:
> nit: since this is just an index now, s/point/i

Done.

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebInputEventConversion.h (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebInputEventConversion.h:92:
WebTouchEventBuilder(const LayoutItem, const TouchEvent&);
On 2017/01/25 15:19:48, bokan wrote:
> You removed the implementation, presumably this class is unneeded now?

yup not needed anymore. The native event is stored on the DOM event so we don't
need to reconstitute an event from a DOM Event.

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:828:
event->nativeEvent()->touchPointInRootFrame(i).position;
On 2017/01/25 15:19:48, bokan wrote:
> I think this can just be transformedEvent->touches[i].position, right?

yes; done

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebTouchPoint.h (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebTouchPoint.h:65: WebFloatPoint position;
On 2017/01/25 15:19:48, bokan wrote:
> Q: In the design doc you proposed making the members protected and giving them
> accessors. Is that still the plan?

Yes I plan on getting rid of mouse event extra fields and then moving these into
pointer properties as accessors..

[bokan, 2017-01-25 18:05:45]

thanks, lgtm

[dtapuska, 2017-01-25 18:11:39]

On 2017/01/25 18:05:45, bokan wrote:
> thanks, lgtm

rbyers@ can you review/stamp */platform/*

[dtapuska, 2017-01-25 18:47:57]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 18:48:37]

Dry run: 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-01-25 21:47:11]

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

[commit-bot: I haz the power, 2017-01-25 21:47:13]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-01-26 13:46:59]

On 2017/01/25 18:11:39, dtapuska wrote:
> On 2017/01/25 18:05:45, bokan wrote:
> > thanks, lgtm
> 
> rbyers@ can you review/stamp */platform/*

rbyers@ ping

[dtapuska, 2017-01-26 16:11:38]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 16:12:10]

Dry run: 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-01-26 18:05:59]

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

[commit-bot: I haz the power, 2017-01-26 18:06:00]

Dry run: This issue passed the CQ dry run.

[Rick Byers, 2017-01-27 17:27:59]

public/platform and platform/ LGTM with nit

The rest RS LGTM - you guys are the experts(and bokan is a core/ OWNER).

https://codereview.chromium.org/2646163002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebTouchEvent.h (right):

https://codereview.chromium.org/2646163002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebTouchEvent.h:57: BLINK_PLATFORM_EXPORT
WebTouchPoint
nit: please add a comment describing the purpose of this method too (I see it's
related, but doesn't update anything itself).  We try to be prettty thorough
with API comments at the "public" boundary.

[dtapuska, 2017-01-27 19:22:50]

https://codereview.chromium.org/2646163002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebTouchEvent.h (right):

https://codereview.chromium.org/2646163002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebTouchEvent.h:57: BLINK_PLATFORM_EXPORT
WebTouchPoint
On 2017/01/27 17:27:59, Rick Byers wrote:
> nit: please add a comment describing the purpose of this method too (I see
it's
> related, but doesn't update anything itself).  We try to be prettty thorough
> with API comments at the "public" boundary.

Done.

[dtapuska, 2017-01-27 19:23:07]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2017-01-27 19:23:08]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org,
bokan@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2646163002/#ps180001
(title: "Fix nit")

[commit-bot: I haz the power, 2017-01-27 19:23:48]

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-01-27 21:37:22]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485544987844220,
"parent_rev": "168f89bd6a4f37942fe8d05f59b903fe822dfcc2", "commit_rev":
"5f7cc1167a9026405c671be0f81ff551f9ebd47b"}

[commit-bot: I haz the power, 2017-01-27 21:38:01]

Description was changed from

==========
Remove PlatformTouchEvent/Point and use WebTouchEvent/Point instead

Perform similar cleanup as GestureEvents used in change:
https://codereview.chromium.org/2539283002/
https://codereview.chromium.org/2586133003

Cleanup proposal:
https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7Fph...

BUG=625684
==========

to

==========
Remove PlatformTouchEvent/Point and use WebTouchEvent/Point instead

Perform similar cleanup as GestureEvents used in change:
https://codereview.chromium.org/2539283002/
https://codereview.chromium.org/2586133003

Cleanup proposal:
https://docs.google.com/document/d/1s4Lfy22CNU1OZ5Rec6Oano_5BvIhdK6uFVsVe7Fph...

BUG=625684

Review-Url: https://codereview.chromium.org/2646163002
Cr-Commit-Position: refs/heads/master@{#446782}
Committed:
https://chromium.googlesource.com/chromium/src/+/5f7cc1167a9026405c671be0f81f...
==========

[commit-bot: I haz the power, 2017-01-27 21:38:03]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/5f7cc1167a9026405c671be0f81f...