Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2800383003:
[Notifications] Drop tap gestures for ArcCustomNotificationView. (Closed)
|
Created:
3 years, 8 months ago by Eliot Courtney Modified:
3 years, 8 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org, victorhsieh+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Notifications] Drop tap gestures for ArcCustomNotificationView.
The tap gesture is already handled on the Android side.
BUG=709911
Review-Url: https://codereview.chromium.org/2800383003
Cr-Commit-Position: refs/heads/master@{#463944}
Committed: https://chromium.googlesource.com/chromium/src/+/39f8e8cde10ced24bf0dbcae7187995221f009fe
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comments. #Messages
Total messages: 16 (9 generated)
The CQ bit was checked by edcourtney@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...
Description was changed from ========== [Notifications] Drop tap gestures for ArcCustomNotificationView. The tap gesture is already handled on the Android side. BUG=709911 ========== to ========== [Notifications] Drop tap gestures for ArcCustomNotificationView. The tap gesture is already handled on the Android side. BUG=709911 ==========
edcourtney@chromium.org changed reviewers: + yhanada@chromium.org, yoshiki@chromium.org
PTAL~!
Don't we need to drop other types of gesture events? I guess that we need to forward SWIPE events but other events can be dropped. https://codereview.chromium.org/2800383003/diff/1/ui/arc/notification/arc_cus... File ui/arc/notification/arc_custom_notification_view.cc (right): https://codereview.chromium.org/2800383003/diff/1/ui/arc/notification/arc_cus... ui/arc/notification/arc_custom_notification_view.cc:57: } else if (!event->IsTouchEvent()) { How about adding |event->type() != ui::ET_GESTURE_TAP| here and updating the comments below?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I had a look, and it seems like at least the ET_GESTURE_TAP_DOWN ET_GESTURE_TAP_CANCEL ET_GESTURE_TAP_END ET_GESTURE_SCROLL_BEGIN ET_GESTURE_SCROLL_UPDATE ET_GESTURE_SCROLL_END gesture events are used. AFAICT not blocking the other gestures isn't a problem at the moment. Making a whitelist of gestures to pass through seems harder to maintain to me compared to blacklisting the ones that cause problems. What do you think? https://codereview.chromium.org/2800383003/diff/1/ui/arc/notification/arc_cus... File ui/arc/notification/arc_custom_notification_view.cc (right): https://codereview.chromium.org/2800383003/diff/1/ui/arc/notification/arc_cus... ui/arc/notification/arc_custom_notification_view.cc:57: } else if (!event->IsTouchEvent()) { On 2017/04/10 09:18:13, yhanada wrote: > How about adding |event->type() != ui::ET_GESTURE_TAP| here and updating the > comments below? Done.
lgtm On 2017/04/11 06:27:21, Eliot Courtney wrote: > I had a look, and it seems like at least the > ET_GESTURE_TAP_DOWN > ET_GESTURE_TAP_CANCEL > ET_GESTURE_TAP_END > ET_GESTURE_SCROLL_BEGIN > ET_GESTURE_SCROLL_UPDATE > ET_GESTURE_SCROLL_END > > gesture events are used. AFAICT not blocking the other gestures isn't a problem > at the moment. Thank you for investigating this! I don't know the other gesture events are used... > Making a whitelist of gestures to pass through seems harder to > maintain to me compared to blacklisting the ones that cause problems. What do > you think? I agree with you. > https://codereview.chromium.org/2800383003/diff/1/ui/arc/notification/arc_cus... > File ui/arc/notification/arc_custom_notification_view.cc (right): > > https://codereview.chromium.org/2800383003/diff/1/ui/arc/notification/arc_cus... > ui/arc/notification/arc_custom_notification_view.cc:57: } else if > (!event->IsTouchEvent()) { > On 2017/04/10 09:18:13, yhanada wrote: > > How about adding |event->type() != ui::ET_GESTURE_TAP| here and updating the > > comments below? > > Done. Thanks!
LGTM. Thanks!
The CQ bit was checked by edcourtney@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": 1491979617030510,
"parent_rev": "c556f5a3a662c0b79ae7961f8bdbfaf2cc4236fb", "commit_rev":
"39f8e8cde10ced24bf0dbcae7187995221f009fe"}
Message was sent while issue was closed.
Description was changed from ========== [Notifications] Drop tap gestures for ArcCustomNotificationView. The tap gesture is already handled on the Android side. BUG=709911 ========== to ========== [Notifications] Drop tap gestures for ArcCustomNotificationView. The tap gesture is already handled on the Android side. BUG=709911 Review-Url: https://codereview.chromium.org/2800383003 Cr-Commit-Position: refs/heads/master@{#463944} Committed: https://chromium.googlesource.com/chromium/src/+/39f8e8cde10ced24bf0dbcae7187... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/39f8e8cde10ced24bf0dbcae7187... |
