Remove PlatformGestureEvent in favour of using WebGestureEvent (issue 2539283002 by dtapuska@chromium.org)

4 views
Skip to first unread message

dtap...@chromium.org

unread,
Nov 30, 2016, 5:24:08 PM11/30/16
to maj...@chromium.org, bo...@chromium.org, mus...@chromium.org, nzol...@chromium.org, rby...@chromium.org, chromium...@chromium.org, sh...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, dche...@chromium.org, apavlo...@chromium.org, kinuko...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, ericwill...@chromium.org, rjwr...@chromium.org, nzol...@chromium.org, lushnik...@chromium.org, alexis...@intel.com, devtools...@chromium.org, dtapuska+...@chromium.org, mlamouri+w...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, mac-r...@chromium.org, kozyatins...@chromium.org
Reviewers: majidvp, bokan, mustaq, Navid Zolghadr, Rick Byers
CL: https://codereview.chromium.org/2539283002/

Message:
Soliciting feedback on this approach. PTAL.


Description:
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

Affected files (+738, -1156 lines):
M third_party/WebKit/Source/core/editing/SelectionController.h
M third_party/WebKit/Source/core/editing/SelectionController.cpp
M third_party/WebKit/Source/core/events/GestureEvent.h
M third_party/WebKit/Source/core/events/GestureEvent.cpp
M third_party/WebKit/Source/core/input/EventHandler.h
M third_party/WebKit/Source/core/input/EventHandler.cpp
M third_party/WebKit/Source/core/input/EventHandlerTest.cpp
M third_party/WebKit/Source/core/input/GestureManager.h
M third_party/WebKit/Source/core/input/GestureManager.cpp
M third_party/WebKit/Source/core/input/MouseEventManager.cpp
M third_party/WebKit/Source/core/input/ScrollManager.h
M third_party/WebKit/Source/core/input/ScrollManager.cpp
M third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp
M third_party/WebKit/Source/core/page/EventWithHitTestResults.h
M third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
M third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
M third_party/WebKit/Source/platform/BUILD.gn
M third_party/WebKit/Source/platform/PlatformEvent.h
D third_party/WebKit/Source/platform/PlatformGestureEvent.h
M third_party/WebKit/Source/platform/PlatformMouseEvent.h
A third_party/WebKit/Source/platform/WebGestureEvent.cpp
M third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm
M third_party/WebKit/Source/platform/scroll/ScrollTypes.h
M third_party/WebKit/Source/platform/scroll/Scrollbar.h
M third_party/WebKit/Source/platform/scroll/Scrollbar.cpp
M third_party/WebKit/Source/web/DevToolsEmulator.cpp
M third_party/WebKit/Source/web/InspectorOverlay.h
M third_party/WebKit/Source/web/InspectorOverlay.cpp
M third_party/WebKit/Source/web/LinkHighlightImplTest.cpp
M third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
M third_party/WebKit/Source/web/WebInputEvent.cpp
M third_party/WebKit/Source/web/WebInputEventConversion.h
M third_party/WebKit/Source/web/WebInputEventConversion.cpp
M third_party/WebKit/Source/web/WebPagePopupImpl.cpp
M third_party/WebKit/Source/web/WebPluginContainerImpl.cpp
M third_party/WebKit/Source/web/WebViewImpl.cpp
M third_party/WebKit/Source/web/tests/VisualViewportTest.cpp
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp
M third_party/WebKit/Source/web/tests/WebInputEventConversionTest.cpp
M third_party/WebKit/public/platform/WebGestureEvent.h
M third_party/WebKit/public/platform/WebInputEvent.h


maj...@chromium.org

unread,
Dec 1, 2016, 10:46:31 AM12/1/16
to dtap...@chromium.org, bo...@chromium.org, mus...@chromium.org, nzol...@chromium.org, rby...@chromium.org, chromium...@chromium.org, sh...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, dche...@chromium.org, apavlo...@chromium.org, kinuko...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, ericwill...@chromium.org, rjwr...@chromium.org, nzol...@chromium.org, lushnik...@chromium.org, alexis...@intel.com, devtools...@chromium.org, dtapuska+...@chromium.org, mlamouri+w...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, mac-r...@chromium.org, kozyatins...@chromium.org

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

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/core/input/EventHandlerTest.cpp#newcode46
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/core/paint/PaintLayerScrollableArea.cpp
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
(right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp#newcode1509
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/web/WebInputEvent.cpp
File third_party/WebKit/Source/web/WebInputEvent.cpp (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/web/WebInputEvent.cpp#newcode44
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/platform/WebGestureEvent.h
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/platform/WebGestureEvent.h#newcode186
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/platform/WebGestureEvent.h#newcode187
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/platform/WebInputEvent.h
File third_party/WebKit/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/platform/WebInputEvent.h#newcode232
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/platform/WebInputEvent.h#newcode234
third_party/WebKit/public/platform/WebInputEvent.h:234: float
frameTranslateY; // post scale translation y value.
should frameTranslateX and frameTranslateY just be a WebFloatPoint?

https://codereview.chromium.org/2539283002/

dtap...@chromium.org

unread,
Dec 1, 2016, 11:26:43 AM12/1/16
to maj...@chromium.org, bo...@chromium.org, mus...@chromium.org, nzol...@chromium.org, rby...@chromium.org, chromium...@chromium.org, sh...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, dche...@chromium.org, apavlo...@chromium.org, kinuko...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, ericwill...@chromium.org, rjwr...@chromium.org, nzol...@chromium.org, lushnik...@chromium.org, alexis...@intel.com, devtools...@chromium.org, dtapuska+...@chromium.org, mlamouri+w...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, mac-r...@chromium.org, kozyatins...@chromium.org

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

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/core/input/EventHandlerTest.cpp#newcode46
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/core/paint/PaintLayerScrollableArea.cpp
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
(right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp#newcode1509
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.
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.
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/platform/WebGestureEvent.h#newcode187
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/platform/WebInputEvent.h
File third_party/WebKit/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/platform/WebInputEvent.h#newcode232
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/platform/WebInputEvent.h#newcode234
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.

https://codereview.chromium.org/2539283002/

bo...@chromium.org

unread,
Dec 1, 2016, 12:29:42 PM12/1/16
to dtap...@chromium.org, maj...@chromium.org, mus...@chromium.org, nzol...@chromium.org, rby...@chromium.org, chromium...@chromium.org, sh...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, dche...@chromium.org, apavlo...@chromium.org, kinuko...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, ericwill...@chromium.org, rjwr...@chromium.org, nzol...@chromium.org, lushnik...@chromium.org, alexis...@intel.com, devtools...@chromium.org, dtapuska+...@chromium.org, mlamouri+w...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, mac-r...@chromium.org, kozyatins...@chromium.org
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.

https://codereview.chromium.org/2539283002/

dtap...@chromium.org

unread,
Dec 1, 2016, 12:51:19 PM12/1/16
to maj...@chromium.org, bo...@chromium.org, mus...@chromium.org, nzol...@chromium.org, rby...@chromium.org, chromium...@chromium.org, sh...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, dche...@chromium.org, apavlo...@chromium.org, kinuko...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, ericwill...@chromium.org, rjwr...@chromium.org, nzol...@chromium.org, lushnik...@chromium.org, alexis...@intel.com, devtools...@chromium.org, dtapuska+...@chromium.org, mlamouri+w...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, mac-r...@chromium.org, kozyatins...@chromium.org
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.


https://codereview.chromium.org/2539283002/

rby...@chromium.org

unread,
Dec 2, 2016, 11:57:44 AM12/2/16
to dtap...@chromium.org, maj...@chromium.org, bo...@chromium.org, mus...@chromium.org, nzol...@chromium.org, chromium...@chromium.org, sh...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, dche...@chromium.org, apavlo...@chromium.org, kinuko...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, ericwill...@chromium.org, rjwr...@chromium.org, nzol...@chromium.org, lushnik...@chromium.org, alexis...@intel.com, devtools...@chromium.org, dtapuska+...@chromium.org, mlamouri+w...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, mac-r...@chromium.org, kozyatins...@chromium.org
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?

https://codereview.chromium.org/2539283002/

nzol...@chromium.org

unread,
Dec 2, 2016, 12:34:55 PM12/2/16
to dtap...@chromium.org, maj...@chromium.org, bo...@chromium.org, mus...@chromium.org, rby...@chromium.org, chromium...@chromium.org, sh...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, dche...@chromium.org, apavlo...@chromium.org, kinuko...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, ericwill...@chromium.org, rjwr...@chromium.org, lushnik...@chromium.org, alexis...@intel.com, devtools...@chromium.org, dtapuska+...@chromium.org, mlamouri+w...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, mac-r...@chromium.org, kozyatins...@chromium.org
https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/platform/WebGestureEvent.h#newcode171
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?

https://codereview.chromium.org/2539283002/

dtap...@chromium.org

unread,
Dec 2, 2016, 1:22:55 PM12/2/16
to maj...@chromium.org, bo...@chromium.org, mus...@chromium.org, nzol...@chromium.org, rby...@chromium.org, chromium...@chromium.org, sh...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, dche...@chromium.org, apavlo...@chromium.org, kinuko...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, ericwill...@chromium.org, rjwr...@chromium.org, nzol...@chromium.org, lushnik...@chromium.org, alexis...@intel.com, devtools...@chromium.org, dtapuska+...@chromium.org, mlamouri+w...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, mac-r...@chromium.org, kozyatins...@chromium.org

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

https://codereview.chromium.org/2539283002/diff/1/third_party/WebKit/public/platform/WebGestureEvent.h#newcode171
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?

https://codereview.chromium.org/2539283002/

bo...@chromium.org

unread,
Dec 2, 2016, 1:24:11 PM12/2/16
to dtap...@chromium.org, maj...@chromium.org, mus...@chromium.org, nzol...@chromium.org, rby...@chromium.org, chromium...@chromium.org, sh...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, dche...@chromium.org, apavlo...@chromium.org, kinuko...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, ericwill...@chromium.org, rjwr...@chromium.org, nzol...@chromium.org, lushnik...@chromium.org, alexis...@intel.com, devtools...@chromium.org, dtapuska+...@chromium.org, mlamouri+w...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, mac-r...@chromium.org, kozyatins...@chromium.org
I agree we should remove the flooring rather than adding int versions.

https://codereview.chromium.org/2539283002/

dtap...@chromium.org

unread,
Dec 2, 2016, 4:46:20 PM12/2/16
to maj...@chromium.org, bo...@chromium.org, mus...@chromium.org, nzol...@chromium.org, rby...@chromium.org, chromium...@chromium.org, sh...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, dche...@chromium.org, apavlo...@chromium.org, kinuko...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, ericwill...@chromium.org, rjwr...@chromium.org, nzol...@chromium.org, lushnik...@chromium.org, alexis...@intel.com, devtools...@chromium.org, dtapuska+...@chromium.org, mlamouri+w...@chromium.org, blink-revie...@chromium.org, blink-revi...@chromium.org, mac-r...@chromium.org, kozyatins...@chromium.org
Reply all
Reply to author
Forward
0 new messages