Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2778853005:
Add comments to WebInputEvent for touch enum types. (Closed)
|
Created:
3 years, 8 months ago by aelias_OOO_until_Jul13 Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink, mustaq, tdresser Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd comments to WebInputEvent for touch enum types.
I had to do a bit of research today to remember the difference between
subtle events like GestureTapDown/GestureShowPress, and
GestureLongPress/GestureLongTap. Add some comments to clarify the
distinctions for the future.
BUG=
Review-Url: https://codereview.chromium.org/2778853005
Cr-Commit-Position: refs/heads/master@{#460561}
Committed: https://chromium.googlesource.com/chromium/src/+/1e95549dd5cdba6a746c29cea7aac9dfea3fc0f6
Patch Set 1 #
Total comments: 10
Patch Set 2 : Code review comments #Messages
Total messages: 23 (13 generated)
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
aelias@chromium.org changed reviewers: + dtapuska@chromium.org
Hi dtapuska@, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tdresser@chromium.org changed reviewers: + tdresser@chromium.org
A few driveby nits. https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebInputEvent.h:116: // WebGestureEvent - touch pointers already interpreted semi-sematically. sematically -> semantically? It's a bit tricky, because we also use some of these gestures for wheel scrolling now. https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebInputEvent.h:138: // the "main" event which maps to a synthetic mouse click event. On long tap, we don't send a tap event, do we?
https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebInputEvent.h:109: // KeyDown is a single event combining RawKeyDown and Char. If KeyDown is Probably adding a piece of text here like assuming a platform generates only KeyDown events is wrong. As DevTools & WebDriver can generate RawKeyDown events.
mustaq@chromium.org changed reviewers: + mustaq@chromium.org
Thanks for adding comments, gesture event names keep confusing me. One quick question for now: can't we have platform independent comments? E.g.: https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebInputEvent.h:154: GestureTapUnconfirmed, Am I right to recall that CrOS also fires GestureTapUnconfirmed?
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebInputEvent.h:109: // KeyDown is a single event combining RawKeyDown and Char. If KeyDown is On 2017/03/29 at 13:01:55, dtapuska wrote: > Probably adding a piece of text here like assuming a platform generates only KeyDown events is wrong. As DevTools & WebDriver can generate RawKeyDown events. Done. https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebInputEvent.h:116: // WebGestureEvent - touch pointers already interpreted semi-sematically. On 2017/03/29 at 12:26:48, tdresser wrote: > sematically -> semantically? > > It's a bit tricky, because we also use some of these gestures for wheel scrolling now. Done. https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebInputEvent.h:138: // the "main" event which maps to a synthetic mouse click event. On 2017/03/29 at 12:26:48, tdresser wrote: > On long tap, we don't send a tap event, do we? Switched to "for a simple, static, quick finger tap" https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebInputEvent.h:154: GestureTapUnconfirmed, On 2017/03/29 at 18:27:28, mustaq wrote: > Am I right to recall that CrOS also fires GestureTapUnconfirmed? No, I had grepped this and only place that sets it is https://cs.chromium.org/chromium/src/ui/events/gesture_detection/gesture_prov... . This can never happen on Aura platforms because of https://cs.chromium.org/chromium/src/ui/events/gesture_detection/gesture_conf... makes IsDoubleTapEnabled() return false.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebInputEvent.h:154: GestureTapUnconfirmed, And in general, there's no appetite at all for shipping double-tap zoom on non-Android because of the tap delay problem, so I'm not worried that this comment will become obsolete. I think it's clarifying to mention that this event is effectively platform-specific, instead of being intentionally vague/generic.
On 2017/03/29 19:10:56, aelias wrote: > https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... > File third_party/WebKit/public/platform/WebInputEvent.h (right): > > https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... > third_party/WebKit/public/platform/WebInputEvent.h:154: GestureTapUnconfirmed, > And in general, there's no appetite at all for shipping double-tap zoom on > non-Android because of the tap delay problem, so I'm not worried that this > comment will become obsolete. I think it's clarifying to mention that this > event is effectively platform-specific, instead of being intentionally > vague/generic. lgtm
LGTM, thanks! https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2778853005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebInputEvent.h:154: GestureTapUnconfirmed, On 2017/03/29 19:10:55, aelias wrote: > And in general, there's no appetite at all for shipping double-tap zoom on > non-Android because of the tap delay problem, so I'm not worried that this > comment will become obsolete. I think it's clarifying to mention that this > event is effectively platform-specific, instead of being intentionally > vague/generic. +1
The CQ bit was unchecked by aelias@chromium.org
The CQ bit was checked by aelias@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490817246037420,
"parent_rev": "ee2be17a3a10b3c6c556cf43061e0a9d1f239b18", "commit_rev":
"1e95549dd5cdba6a746c29cea7aac9dfea3fc0f6"}
Message was sent while issue was closed.
Description was changed from ========== Add comments to WebInputEvent for touch enum types. I had to do a bit of research today to remember the difference between subtle events like GestureTapDown/GestureShowPress, and GestureLongPress/GestureLongTap. Add some comments to clarify the distinctions for the future. BUG= ========== to ========== Add comments to WebInputEvent for touch enum types. I had to do a bit of research today to remember the difference between subtle events like GestureTapDown/GestureShowPress, and GestureLongPress/GestureLongTap. Add some comments to clarify the distinctions for the future. BUG= Review-Url: https://codereview.chromium.org/2778853005 Cr-Commit-Position: refs/heads/master@{#460561} Committed: https://chromium.googlesource.com/chromium/src/+/1e95549dd5cdba6a746c29cea7aa... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/1e95549dd5cdba6a746c29cea7aa... |
