Remove PlatformTouchEvent/Point and use WebTouchEvent/Point instead (issue 2646163002 by dtapuska@chromium.org)

3 views
Skip to first unread message

dtap...@chromium.org

unread,
Jan 20, 2017, 4:45:10 PM1/20/17
to nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org
Reviewers: Navid Zolghadr, mustaq, Rick Byers
CL: https://codereview.chromium.org/2646163002/

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

Affected files (+404, -977 lines):
M third_party/WebKit/Source/core/events/PointerEvent.h
M third_party/WebKit/Source/core/events/PointerEventFactory.h
M third_party/WebKit/Source/core/events/PointerEventFactory.cpp
M third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp
M third_party/WebKit/Source/core/events/TouchEvent.h
M third_party/WebKit/Source/core/events/TouchEvent.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/PointerEventManager.h
M third_party/WebKit/Source/core/input/PointerEventManager.cpp
M third_party/WebKit/Source/core/input/TouchEventManager.h
M third_party/WebKit/Source/core/input/TouchEventManager.cpp
M third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp
M third_party/WebKit/Source/core/inspector/InspectorInputAgent.cpp
M third_party/WebKit/Source/platform/BUILD.gn
M third_party/WebKit/Source/platform/PlatformEvent.h
D third_party/WebKit/Source/platform/PlatformTouchEvent.h
D third_party/WebKit/Source/platform/PlatformTouchPoint.h
A third_party/WebKit/Source/platform/WebTouchEvent.cpp
M third_party/WebKit/Source/web/InspectorOverlay.h
M third_party/WebKit/Source/web/InspectorOverlay.cpp
M third_party/WebKit/Source/web/PageWidgetDelegate.cpp
M third_party/WebKit/Source/web/WebInputEventConversion.h
M third_party/WebKit/Source/web/WebInputEventConversion.cpp
M third_party/WebKit/Source/web/WebPluginContainerImpl.cpp
M third_party/WebKit/Source/web/tests/WebInputEventConversionTest.cpp
M third_party/WebKit/public/platform/WebTouchEvent.h
M third_party/WebKit/public/platform/WebTouchPoint.h


mus...@chromium.org

unread,
Jan 23, 2017, 3:05:17 PM1/23/17
to dtap...@chromium.org, nzol...@chromium.org, rby...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org

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

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Source/core/input/PointerEventManager.cpp#newcode387
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/Source/core/input/TouchEventManager.cpp
File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right):

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Source/core/input/TouchEventManager.cpp#newcode412
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/Source/web/WebInputEventConversion.cpp
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Source/web/WebInputEventConversion.cpp#newcode239
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?

https://codereview.chromium.org/2646163002/

dtap...@chromium.org

unread,
Jan 24, 2017, 9:26:28 AM1/24/17
to nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org

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

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Source/core/input/PointerEventManager.cpp#newcode387
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/Source/core/input/TouchEventManager.cpp
File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right):

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Source/core/input/TouchEventManager.cpp#newcode412
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/Source/web/WebInputEventConversion.cpp
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Source/web/WebInputEventConversion.cpp#newcode239
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.

https://codereview.chromium.org/2646163002/

mus...@chromium.org

unread,
Jan 24, 2017, 11:59:41 AM1/24/17
to dtap...@chromium.org, nzol...@chromium.org, rby...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org
LGTM % a few nits.

Thanks for the whole platform event cleanup.



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

https://codereview.chromium.org/2646163002/diff/60001/third_party/WebKit/Source/web/WebInputEventConversion.cpp#newcode239
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/Source/core/inspector/InspectorInputAgent.cpp
File third_party/WebKit/Source/core/inspector/InspectorInputAgent.cpp
(right):

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

https://codereview.chromium.org/2646163002/

dtap...@chromium.org

unread,
Jan 24, 2017, 9:43:34 PM1/24/17
to nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org
On 2017/01/24 16:59:41, mustaq wrote:
> I guess we need to call event.setFrameTranslate() here too?

dtap...@chromium.org

unread,
Jan 24, 2017, 9:45:13 PM1/24/17
to nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, bo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org
bo...@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

https://codereview.chromium.org/2646163002/

bo...@chromium.org

unread,
Jan 25, 2017, 10:19:48 AM1/25/17
to dtap...@chromium.org, nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org
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/Source/core/events/TouchEvent.cpp
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Source/core/events/TouchEvent.cpp#newcode227
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/Source/core/input/PointerEventManager.cpp
File third_party/WebKit/Source/core/input/PointerEventManager.cpp
(right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Source/core/input/PointerEventManager.cpp#newcode40
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/Source/web/WebInputEventConversion.h
File third_party/WebKit/Source/web/WebInputEventConversion.h (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Source/web/WebInputEventConversion.h#newcode92
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/Source/web/WebPluginContainerImpl.cpp
File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Source/web/WebPluginContainerImpl.cpp#newcode828
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/public/platform/WebTouchPoint.h
File third_party/WebKit/public/platform/WebTouchPoint.h (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/public/platform/WebTouchPoint.h#newcode65
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?

https://codereview.chromium.org/2646163002/

bo...@chromium.org

unread,
Jan 25, 2017, 10:22:36 AM1/25/17
to dtap...@chromium.org, nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org
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.

https://codereview.chromium.org/2646163002/

dtap...@chromium.org

unread,
Jan 25, 2017, 12:59:17 PM1/25/17
to nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, bo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org
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/Source/core/events/TouchEvent.cpp
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Source/core/events/TouchEvent.cpp#newcode227
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/Source/core/input/PointerEventManager.cpp
File third_party/WebKit/Source/core/input/PointerEventManager.cpp
(right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Source/core/input/PointerEventManager.cpp#newcode40
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/Source/web/WebInputEventConversion.h
File third_party/WebKit/Source/web/WebInputEventConversion.h (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Source/web/WebInputEventConversion.h#newcode92
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/Source/web/WebPluginContainerImpl.cpp
File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/2646163002/diff/100001/third_party/WebKit/Source/web/WebPluginContainerImpl.cpp#newcode828
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
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..

https://codereview.chromium.org/2646163002/

bo...@chromium.org

unread,
Jan 25, 2017, 1:05:46 PM1/25/17
to dtap...@chromium.org, nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org

dtap...@chromium.org

unread,
Jan 25, 2017, 1:11:39 PM1/25/17
to nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, bo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org
On 2017/01/25 18:05:45, bokan wrote:
> thanks, lgtm

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

https://codereview.chromium.org/2646163002/

dtap...@chromium.org

unread,
Jan 26, 2017, 8:47:00 AM1/26/17
to nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, bo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org
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/*

rby...@chromium.org

unread,
Jan 27, 2017, 12:27:59 PM1/27/17
to dtap...@chromium.org, nzol...@chromium.org, mus...@chromium.org, bo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org
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/public/platform/WebTouchEvent.h
File third_party/WebKit/public/platform/WebTouchEvent.h (right):

https://codereview.chromium.org/2646163002/diff/160001/third_party/WebKit/public/platform/WebTouchEvent.h#newcode57
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.

https://codereview.chromium.org/2646163002/

dtap...@chromium.org

unread,
Jan 27, 2017, 2:22:51 PM1/27/17
to nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, bo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org

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

https://codereview.chromium.org/2646163002/diff/160001/third_party/WebKit/public/platform/WebTouchEvent.h#newcode57
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.

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 27, 2017, 2:23:49 PM1/27/17
to dtap...@chromium.org, nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, bo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 27, 2017, 4:38:04 PM1/27/17
to dtap...@chromium.org, nzol...@chromium.org, mus...@chromium.org, rby...@chromium.org, bo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, dtapuska+...@chromium.org, blink-revi...@chromium.org, nzol...@chromium.org, eae+bli...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-re...@chromium.org, kozyatins...@chromium.org
Reply all
Reply to author
Forward
0 new messages