Remove PlatformGestureEvent in favour of using WebGestureEvent

Remove PlatformGestureEvent in favour of using WebGestureEvent

This change is intended to have no behavioural changes. Some of the math
is slightly adjusted the root frame coordinates. The root frame
coordinates are calculated via:
A/scale - B/scale + VO + OO
as it was previously
(A-B)/scale + VO + OO

If this is a problem we can store translateX and translateY as the actual
coordinates instead instead a computation.

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

BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/7a17610ebf43599366634ea11c9cac7ff933973b
Cr-Commit-Position: refs/heads/master@{#439153}

[dtapuska, 2016-11-30 22:19:16]

Description was changed from

==========
Remove PlatformGestureEvent in favour of using WebGestureEvent

This change is intended to have no behavioural changes. Some of the math
is slightly adjusted the root frame coordinates. The root frame
coordinates are calculated via:
A/scale - B/scale + VO + OO
as it was previously
(A-B)/scale + VO + OO

If this is a problem we can store translateX and translateY as the actual
coordinates instead instead a computation.

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

to

==========
Remove PlatformGestureEvent in favour of using WebGestureEvent

This change is intended to have no behavioural changes. Some of the math
is slightly adjusted the root frame coordinates. The root frame
coordinates are calculated via:
A/scale - B/scale + VO + OO
as it was previously
(A-B)/scale + VO + OO

If this is a problem we can store translateX and translateY as the actual
coordinates instead instead a computation.

BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[dtapuska, 2016-11-30 22:24:07]

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

[dtapuska, 2016-11-30 22:24:08]

Soliciting feedback on this approach. PTAL.

[majidvp, 2016-12-01 15:46:31]

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandlerTest.cpp:46: data.tap.height =
5;
One of the advantage of PlatformGestureEvent was that it had
constructors which helped ensure we initialize the data members
correctly. 

At the moment, tt is possible to construct WebGestureEvent 
incorrectly e.g., very easy to forget to initialize timeStampSeconds. Is there a
way to avoid this?

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1509: void
PaintLayerScrollableArea::resize(const WebGestureEvent& evt,
Feels like the two resize method here don't really belong in 
|PaintLayerScrollableArea| and can be factored out to helper
functions elsewhere.

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebInputEvent.cpp (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebInputEvent.cpp:44: };
At the moment the additional fields are only used in WebGestureEvent should we
then have these field there
until you start removing other platform events?

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebGestureEvent.h:186: // back to 1.
+ and translate back to (0,0)

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebGestureEvent.h:187: BLINK_PLATFORM_EXPORT
void flattenScale();
It is both scale and transform so perhaps flattenScaleAndTransform?

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebInputEvent.h:232: float frameScale;      
// Scalar applied at frame time.
frame time? do you mean frame conversion time?

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebInputEvent.h:234: float frameTranslateY; 
// post scale translation y value.
should frameTranslateX and frameTranslateY just be a WebFloatPoint?

[dtapuska, 2016-12-01 16:26:43]

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandlerTest.cpp:46: data.tap.height =
5;
On 2016/12/01 15:46:30, majidvp wrote:
> One of the advantage of PlatformGestureEvent was that it had
> constructors which helped ensure we initialize the data members
> correctly. 
> 
> At the moment, tt is possible to construct WebGestureEvent 
> incorrectly e.g., very easy to forget to initialize timeStampSeconds. Is there
a
> way to avoid this?

So my eventual goal is to have WebInputEvent be a full C++ class and then we can
have constructors. That can be done independently. I don't care for the wild
west struct.

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1509: void
PaintLayerScrollableArea::resize(const WebGestureEvent& evt,
On 2016/12/01 15:46:30, majidvp wrote:
> Feels like the two resize method here don't really belong in 
> |PaintLayerScrollableArea| and can be factored out to helper
> functions elsewhere.

Yes I agree. There are only two calls into resize; and the point translation
could actually be done in the call sites instead of this class.

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebInputEvent.cpp (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebInputEvent.cpp:44: };
On 2016/12/01 15:46:30, majidvp wrote:
> At the moment the additional fields are only used in WebGestureEvent should we
> then have these field there
> until you start removing other platform events?

I'm working on WebMouseWheelEvent now as well. So I don't expect it to be long
for another.

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebGestureEvent.h:186: // back to 1.
On 2016/12/01 15:46:30, majidvp wrote:
> + and translate back to (0,0)

Acknowledged.

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebGestureEvent.h:187: BLINK_PLATFORM_EXPORT
void flattenScale();
On 2016/12/01 15:46:30, majidvp wrote:
> It is both scale and transform so perhaps flattenScaleAndTransform?

sgtm; I didn't care for the name.

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebInputEvent.h:232: float frameScale;      
// Scalar applied at frame time.
On 2016/12/01 15:46:31, majidvp wrote:
> frame time? do you mean frame conversion time?

Ya this comment needs some work. I mean that when we convert at the web layer.
Applying the root frame to the translations.

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebInputEvent.h:234: float frameTranslateY; 
// post scale translation y value.
On 2016/12/01 15:46:31, majidvp wrote:
> should frameTranslateX and frameTranslateY just be a WebFloatPoint?

WebFloatPoint isn't packed. So I might be able to pack it and see if it doesn't
increase the size of the struct. But good idea.

[bokan, 2016-12-01 17:29:41]

High level question, have you thought about going the other way? Using
GestureEvent instead? I like the idea of insulating Blink from the outside. Once
a WebGestureEvent makes in into Blink Source/web, it gets converted to
GestureEvent. Then the divide is WebGestureEvent is external and GestureEvent is
internal to Blink core. We would then have a nice boundary for application of
non-Blink concepts like device emulation scale and translation. Currently, I get
confused very easily about where things like that happen. Though I've got a much
higher level of understanding here than you do so I appreciate if this is overly
simplistic.

[dtapuska, 2016-12-01 17:51:17]

On 2016/12/01 17:29:41, bokan wrote:
> High level question, have you thought about going the other way? Using
> GestureEvent instead? I like the idea of insulating Blink from the outside.
Once
> a WebGestureEvent makes in into Blink Source/web, it gets converted to
> GestureEvent. Then the divide is WebGestureEvent is external and GestureEvent
is
> internal to Blink core. We would then have a nice boundary for application of
> non-Blink concepts like device emulation scale and translation. Currently, I
get
> confused very easily about where things like that happen. Though I've got a
much
> higher level of understanding here than you do so I appreciate if this is
overly
> simplistic.

Ya I did think about that but it creates a fundamental security issue that can
be subtle. Being that an Event has a DOM object it would be easy for a developer
to just pass the object into another v8 context than the one it was created in
and call it a day.

[Rick Byers, 2016-12-02 16:57:44]

I didn't review the whole patch, but the concept of collapsing the platform
event types into the web event types is OK with me, but I'd like to better
understand the larger context and how it relates to efforts around Onion Soup
and mojoification.  Eg. will mojofication let us move away from Web*Event
objects being dumb POD types (eliminating the main difference between the Web*
and Platform* event types)?

Perhaps we should have a short design doc, since presumably we'll want to apply
this pattern much more broadly than to just gesture events?

[Navid Zolghadr, 2016-12-02 17:34:54]

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebGestureEvent.h:171: BLINK_PLATFORM_EXPORT
WebFloatPoint positionInRootFrame() const;
Is it worth adding an int version of this as well here since we have casted the
return value of this function with flooredIntPoint in quite a few places?

[dtapuska, 2016-12-02 18:22:54]

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebGestureEvent.h:171: BLINK_PLATFORM_EXPORT
WebFloatPoint positionInRootFrame() const;
On 2016/12/02 17:34:54, Navid Zolghadr wrote:
> Is it worth adding an int version of this as well here since we have casted
the
> return value of this function with flooredIntPoint in quite a few places?

There is no WebIntPoint exposed on the API. We could add it but I chose not to.
As shouldn't we really eventually deal with all the flooredIntPoint spots
fundamentally?

[bokan, 2016-12-02 18:24:10]

On 2016/12/02 18:22:54, dtapuska wrote:
>
https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
> File third_party/WebKit/public/platform/WebGestureEvent.h (right):
> 
>
https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/p...
> third_party/WebKit/public/platform/WebGestureEvent.h:171:
BLINK_PLATFORM_EXPORT
> WebFloatPoint positionInRootFrame() const;
> On 2016/12/02 17:34:54, Navid Zolghadr wrote:
> > Is it worth adding an int version of this as well here since we have casted
> the
> > return value of this function with flooredIntPoint in quite a few places?
> 
> There is no WebIntPoint exposed on the API. We could add it but I chose not
to.
> As shouldn't we really eventually deal with all the flooredIntPoint spots
> fundamentally?

I agree we should remove the flooring rather than adding int versions.

[dtapuska, 2016-12-02 21:46:19]

On 2016/12/02 16:57:44, Rick Byers wrote:
> I didn't review the whole patch, but the concept of collapsing the platform
> event types into the web event types is OK with me, but I'd like to better
> understand the larger context and how it relates to efforts around Onion Soup
> and mojoification.  Eg. will mojofication let us move away from Web*Event
> objects being dumb POD types (eliminating the main difference between the Web*
> and Platform* event types)?
> 
> Perhaps we should have a short design doc, since presumably we'll want to
apply
> this pattern much more broadly than to just gesture events?

It was originally proposed here. I've updated it with the updated approach:

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

[dtapuska, 2016-12-14 18:45:04]

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

[commit-bot: I haz the power, 2016-12-14 18:45:45]

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

[dtapuska, 2016-12-14 18:46:47]

Description was changed from

==========
Remove PlatformGestureEvent in favour of using WebGestureEvent

This change is intended to have no behavioural changes. Some of the math
is slightly adjusted the root frame coordinates. The root frame
coordinates are calculated via:
A/scale - B/scale + VO + OO
as it was previously
(A-B)/scale + VO + OO

If this is a problem we can store translateX and translateY as the actual
coordinates instead instead a computation.

BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Remove PlatformGestureEvent in favour of using WebGestureEvent

This change is intended to have no behavioural changes. Some of the math
is slightly adjusted the root frame coordinates. The root frame
coordinates are calculated via:
A/scale - B/scale + VO + OO
as it was previously
(A-B)/scale + VO + OO

If this is a problem we can store translateX and translateY as the actual
coordinates instead instead a computation.

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

BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[dtapuska, 2016-12-14 18:47:57]

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

[dtapuska, 2016-12-14 18:47:57]

esprehn@chromium.org; do you mind looking at these changes. I don't care for the
INSIDE_BLINK stuff but jbroman didn't want functions being added to the C++ API
outside of blink.

[esprehn, 2016-12-14 19:09:37]

Trying to understand this better, if this type is never used outside blink can
we just have the type in platform/ and not public/platform? This patch doesn't
change any content code...

[dtapuska, 2016-12-14 19:15:55]

On 2016/12/14 19:09:37, esprehn wrote:
> Trying to understand this better, if this type is never used outside blink can
> we just have the type in platform/ and not public/platform? This patch doesn't
> change any content code...

WebGestureEvent is used outside blink. But PlatformGestureEvent is used inside
core/

This patch essentially removes PlatformGestureEvent and uses WebGestureEvent
inside core/

[esprehn, 2016-12-14 19:23:59]

lgtm, the INSIDE_BLINK thing is nice because we limit the API usage outside
blink and in general we're trying to get everyone to move their code inside.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebGestureEvent.h:189: bool
preventPropagation() const {
Weird, what's the purpose of this if it's not virtual?

[commit-bot: I haz the power, 2016-12-14 22:33:53]

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

[commit-bot: I haz the power, 2016-12-14 22:33:54]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2016-12-15 14:16:09]

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebGestureEvent.h:189: bool
preventPropagation() const {
On 2016/12/14 19:23:59, esprehn wrote:
> Weird, what's the purpose of this if it's not virtual?

It is left over from the PlatformGestureEvent; I gotta talk to tdresser@ but I
think this can be purged.

[majidvp, 2016-12-15 17:40:46]

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/GestureEvent.h (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/GestureEvent.h:45: const WebGestureEvent&
originalEvent() const { return m_nativeEvent; }
nit: better to have the accessor and field names match.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1765: adjustedPoint,
adjustedNode);
UGH! The adjustedPoint here is actually an output parameter but it is a
reference instead of a pointer. It took me some time to figure it out. This was
FYI and something we should probably fix in a different patch.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:47: #include
"public/platform/WebGestureEvent.h"
nit: why not forward declare?

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/ScrollManager.cpp:513:
PlatformEvent::RightButtonDown)) == 0)
This line is replacing 
"if (!document.frame()->eventHandler().mousePressed())"

but I don't think it does the same thing. Even if it does I suggest 
doing this as a follow-up patch.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/ScrollManager.h (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/ScrollManager.h:77: WebInputEventResult
handleGestureScrollEvent(const WebGestureEvent&);
nit: I expected to see a forward decl or an #include for this !?

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/WebGestureEvent.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebGestureEvent.cpp:98: frameTranslate.x =
rootFrameCoords.x - (x / frameScale);
nit: the original method had a comment which was insightful. Can we
keep that please?

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/InspectorOverlay.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/InspectorOverlay.cpp:239: // Only let GestureTap
in we only need.
nit: this comment needs some revision to read well.

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

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:54: float scale =
1.0f;
nit: I believe .0f is not needed according to Blink style. Is this no longer the
case?

https://www.chromium.org/blink/coding-style#TOC-Floating-point-literals

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:127:
visualViewport.y() + overscrollOffset.height());
I guess the reason that we cannot re-use |frameTranslation| here is to avoid the
scaling precision loss that may occur. Just checking.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputEventConversion.h (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.h:53: class WheelEvent;
nit: please either #include WebGestureEvent or forward decl it.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.h:123: WEB_EXPORT
WebGestureEvent ScaleWebGestureEvent(Widget*,
This is a scale & translate. I think a better name may be
TransformWebGestureEvent which is consistent with flattenTransform.

Also a comment to explain what is being transformed in relation to the widget is
useful.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:731:
ScaleWebGestureEvent(mainFrameImpl()->frameView(), event);
One thing that we are going to miss when we remove PlatformGestureEvent is that
previously any function which received a Platform event knew that the event was
properly scaled. Now it is possible to pass in an unscaled event by mistake.

Can we add that safety back in somehow?
I was thinking to perhaps have a private boolean field in WebGestureEvent that
we set in ScaleWebGestureEvent and 
ASSERT on places where we would have previously take in a
Platform type. This can all be behind a DCHECK to avoid cost.

[dtapuska, 2016-12-15 19:06:18]

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

[mustaq, 2016-12-15 19:25:46]

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/GestureEvent.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/GestureEvent.cpp:87: nullptr),
Curious: shouldn't we have sourceCapabilities=firesTouch?

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/GestureEvent.h (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/GestureEvent.h:45: const WebGestureEvent&
originalEvent() const { return m_nativeEvent; }
On 2016/12/15 17:40:45, majidvp wrote:
> nit: better to have the accessor and field names match.

+1, "original" could be confusing.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/WebGestureEvent.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebGestureEvent.cpp:140: x = (x / frameScale)
+ frameTranslate.x;
Any way we can do these transforms in a common place? Since frameScale &
frameTranslate belong to WebInputEvent, perhaps add transform methods scale(&x,
&y) and translate(&x, &y) there. Plus: can it possibly hide frameScale &
frameTranslate members in WebInputEvent?

[dtapuska, 2016-12-15 21:29:16]

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

[dtapuska, 2016-12-15 21:29:39]

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/GestureEvent.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/GestureEvent.cpp:87: nullptr),
On 2016/12/15 19:25:46, mustaq wrote:
> Curious: shouldn't we have sourceCapabilities=firesTouch?

This event isn't actually ever exposed to the web so I don't think it matters;
and it didn't provide them before either.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/GestureEvent.h (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/GestureEvent.h:45: const WebGestureEvent&
originalEvent() const { return m_nativeEvent; }
On 2016/12/15 17:40:45, majidvp wrote:
> nit: better to have the accessor and field names match.

Done.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1765: adjustedPoint,
adjustedNode);
On 2016/12/15 17:40:45, majidvp wrote:
> UGH! The adjustedPoint here is actually an output parameter but it is a
> reference instead of a pointer. It took me some time to figure it out. This
was
> FYI and something we should probably fix in a different patch.

Acknowledged.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:47: #include
"public/platform/WebGestureEvent.h"
On 2016/12/15 17:40:45, majidvp wrote:
> nit: why not forward declare?

Done.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/ScrollManager.cpp:513:
PlatformEvent::RightButtonDown)) == 0)
On 2016/12/15 17:40:45, majidvp wrote:
> This line is replacing 
> "if (!document.frame()->eventHandler().mousePressed())"
> 
> but I don't think it does the same thing. Even if it does I suggest 
> doing this as a follow-up patch.

Done.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/ScrollManager.h (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/ScrollManager.h:77: WebInputEventResult
handleGestureScrollEvent(const WebGestureEvent&);
On 2016/12/15 17:40:45, majidvp wrote:
> nit: I expected to see a forward decl or an #include for this !?

Done.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/WebGestureEvent.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebGestureEvent.cpp:98: frameTranslate.x =
rootFrameCoords.x - (x / frameScale);
On 2016/12/15 17:40:46, majidvp wrote:
> nit: the original method had a comment which was insightful. Can we
> keep that please?

Done.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebGestureEvent.cpp:140: x = (x / frameScale)
+ frameTranslate.x;
On 2016/12/15 19:25:46, mustaq wrote:
> Any way we can do these transforms in a common place? Since frameScale &
> frameTranslate belong to WebInputEvent, perhaps add transform methods
scale(&x,
> &y) and translate(&x, &y) there. Plus: can it possibly hide frameScale &
> frameTranslate members in WebInputEvent?

I think that adding inline for div operators will kind of hide the math. I think
it is easier to read this way.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/InspectorOverlay.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/InspectorOverlay.cpp:239: // Only let GestureTap
in we only need.
On 2016/12/15 17:40:46, majidvp wrote:
> nit: this comment needs some revision to read well.

Done.

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

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:54: float scale =
1.0f;
On 2016/12/15 17:40:46, majidvp wrote:
> nit: I believe .0f is not needed according to Blink style. Is this no longer
the
> case?
> 
> https://www.chromium.org/blink/coding-style#TOC-Floating-point-literals

Done.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputEventConversion.h (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.h:53: class WheelEvent;
On 2016/12/15 17:40:46, majidvp wrote:
> nit: please either #include WebGestureEvent or forward decl it.

Done.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.h:123: WEB_EXPORT
WebGestureEvent ScaleWebGestureEvent(Widget*,
On 2016/12/15 17:40:46, majidvp wrote:
> This is a scale & translate. I think a better name may be
> TransformWebGestureEvent which is consistent with flattenTransform.
> 
> Also a comment to explain what is being transformed in relation to the widget
is
> useful.

Done.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:731:
ScaleWebGestureEvent(mainFrameImpl()->frameView(), event);
On 2016/12/15 17:40:46, majidvp wrote:
> One thing that we are going to miss when we remove PlatformGestureEvent is
that
> previously any function which received a Platform event knew that the event
was
> properly scaled. Now it is possible to pass in an unscaled event by mistake.
> 
> Can we add that safety back in somehow?
> I was thinking to perhaps have a private boolean field in WebGestureEvent that
> we set in ScaleWebGestureEvent and 
> ASSERT on places where we would have previously take in a
> Platform type. This can all be behind a DCHECK to avoid cost.
> 

I've adjusted the framescale to be zero based which will cause a div0 exception
if someone uses it inappropriately. I've also added a dcheck in event handler.

[commit-bot: I haz the power, 2016-12-15 21:29:56]

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

[majidvp, 2016-12-15 21:45:37]

lgtm. Looking forward to seeing this happens to other 
Platfrom*Events too.

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:731:
ScaleWebGestureEvent(mainFrameImpl()->frameView(), event);
On 2016/12/15 21:29:39, dtapuska wrote:
> On 2016/12/15 17:40:46, majidvp wrote:
> > One thing that we are going to miss when we remove PlatformGestureEvent is
> that
> > previously any function which received a Platform event knew that the event
> was
> > properly scaled. Now it is possible to pass in an unscaled event by mistake.
> > 
> > Can we add that safety back in somehow?
> > I was thinking to perhaps have a private boolean field in WebGestureEvent
that
> > we set in ScaleWebGestureEvent and 
> > ASSERT on places where we would have previously take in a
> > Platform type. This can all be behind a DCHECK to avoid cost.
> > 
> 
> I've adjusted the framescale to be zero based which will cause a div0
exception
> if someone uses it inappropriately. I've also added a dcheck in event handler.

Neat trick.

[mustaq, 2016-12-15 22:16:14]

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/WebGestureEvent.cpp (right):

https://codereview.chromium.org/2539283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebGestureEvent.cpp:140: x = (x / frameScale)
+ frameTranslate.x;
On 2016/12/15 21:29:39, dtapuska wrote:
> On 2016/12/15 19:25:46, mustaq wrote:
> > Any way we can do these transforms in a common place? Since frameScale &
> > frameTranslate belong to WebInputEvent, perhaps add transform methods
> scale(&x,
> > &y) and translate(&x, &y) there. Plus: can it possibly hide frameScale &
> > frameTranslate members in WebInputEvent?
> 
> I think that adding inline for div operators will kind of hide the math. I
think
> it is easier to read this way.

I didn't mean operator overload, sorry for being too concise. Let me elaborate:

- Scaling is atomic for x- & y-dimensions. Same for translation: we won't expect
translation along one dimension only. So it's logical to pack x & y mods in a
function to avoid future mistakes.

- I find it more readable too: instead of Lines 128 & 129 above,
    scale(data.tapDown.width, data.tapDown.height);
seems better.

- Scale and/or translate operations have been done so many times here. They will
most likely be used in other event types too. Not repeating the logic everywhere
seems better.

- This last point is debatable: subclasses of WebInputEvent may not need to
access the scale & translate parameters after that.

Are you convinced now?

[mustaq, 2016-12-15 22:18:07]

LGTM otherwise. We should start nuking other Platform* events in this way.

[dtapuska, 2016-12-16 14:31:22]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-12-16 14:31:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, majidvp@chromium.org
Link to the patchset: https://codereview.chromium.org/2539283002/#ps60001
(title: "Add missing copyright on new file")

[commit-bot: I haz the power, 2016-12-16 14:31:51]

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, 2016-12-16 16:59:57]

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

[commit-bot: I haz the power, 2016-12-16 16:59:59]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dtapuska, 2016-12-16 17:00:40]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2016-12-16 17:01:04]

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, 2016-12-16 18:25:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481907640419400,
"parent_rev": "5b76167b892816f065f1a43dba92d80688d97b7f", "commit_rev":
"5b5d0eaf76f1b70edd03d075d32bb7d3083162ac"}

[commit-bot: I haz the power, 2016-12-16 18:25:45]

Description was changed from

==========
Remove PlatformGestureEvent in favour of using WebGestureEvent

This change is intended to have no behavioural changes. Some of the math
is slightly adjusted the root frame coordinates. The root frame
coordinates are calculated via:
A/scale - B/scale + VO + OO
as it was previously
(A-B)/scale + VO + OO

If this is a problem we can store translateX and translateY as the actual
coordinates instead instead a computation.

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

BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Remove PlatformGestureEvent in favour of using WebGestureEvent

This change is intended to have no behavioural changes. Some of the math
is slightly adjusted the root frame coordinates. The root frame
coordinates are calculated via:
A/scale - B/scale + VO + OO
as it was previously
(A-B)/scale + VO + OO

If this is a problem we can store translateX and translateY as the actual
coordinates instead instead a computation.

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

BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2539283002
==========

[commit-bot: I haz the power, 2016-12-16 18:25:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-16 18:29:09]

Description was changed from

==========
Remove PlatformGestureEvent in favour of using WebGestureEvent

This change is intended to have no behavioural changes. Some of the math
is slightly adjusted the root frame coordinates. The root frame
coordinates are calculated via:
A/scale - B/scale + VO + OO
as it was previously
(A-B)/scale + VO + OO

If this is a problem we can store translateX and translateY as the actual
coordinates instead instead a computation.

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

BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2539283002
==========

to

==========
Remove PlatformGestureEvent in favour of using WebGestureEvent

This change is intended to have no behavioural changes. Some of the math
is slightly adjusted the root frame coordinates. The root frame
coordinates are calculated via:
A/scale - B/scale + VO + OO
as it was previously
(A-B)/scale + VO + OO

If this is a problem we can store translateX and translateY as the actual
coordinates instead instead a computation.

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

BUG=625684
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/7a17610ebf43599366634ea11c9cac7ff933973b
Cr-Commit-Position: refs/heads/master@{#439153}
==========

[commit-bot: I haz the power, 2016-12-16 18:29:10]

Patchset 4 (id:??) landed as
https://crrev.com/7a17610ebf43599366634ea11c9cac7ff933973b
Cr-Commit-Position: refs/heads/master@{#439153}