New issue
Advanced search Search tips

Issue 693605 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Change MotionEvent::Action to an "enum class" to avoid mix-up with enums from Java.

Project Member Reported by mustaq@chromium.org, Feb 17 2017

Issue description

Currently MotionEvent::Action's are enums, which is not type safe anyway. The worse thing in this particular case is that the ACTION_* constants "imported" from Java MotionEvent are integers with non-matching values.


For example: MotionEvent.ACTION_CANCEL is 3 but MotionEvent::ACTION_CANCEL is 4.

The first one is speced here:
https://developer.android.com/reference/android/view/MotionEvent.html#ACTION_CANCEL
and available in Chromium codebase (Java & C) through
https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/android-23/android/view/MotionEvent.java?rcl=b43a6a289a7588b1769814f04dd6c7d7176974cc&l=217

The second one is defined in our code here:
https://cs.chromium.org/chromium/src/ui/events/gesture_detection/motion_event.h?l=27


We recently spotted one "mismatched" comparison in motion_event_android.cc, which was a lucky case because the compared values just happen to match between the two definitions:
https://cs.chromium.org/chromium/src/ui/events/android/motion_event_android.cc?rcl=7a4e3a4e6fa17894c3a4cdc0f03ed3380fe468f9&l=232

I think the easiest way to fix this is to switch the definition of MotionEvent::Action from a plain "enum" to an "enum class" which would immediately catch assignment/comparison to integers.

 

Comment 1 by mustaq@chromium.org, Feb 17 2017

Labels: Hotlist-Input-Dev
Project Member

Comment 2 by sheriffbot@chromium.org, Feb 21 2018

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: eirage@chromium.org
Status: Available (was: Untriaged)
Mustaq, was there any blockers from moving it to enum class at the time? Does it need a lot of changes across different files or something?

Comment 4 by mustaq@chromium.org, Feb 22 2018

This is a simple cleanup (with only ~34 ref fixes) that would avoid future mixups.  I just didn't get time to fix them.
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174

commit 0ea11d1dc1b0b6524d3087805b55d8fcf33d6174
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Thu Feb 22 18:53:18 2018

Move ACTION_ and TOOL_TYPE_ into enum classes.

Because of a double definition of the attributes it is possible the
wrong defined enum can get used. Rename them so they are enum classes
so misuse doesn't happen.

BUG= 693605 

Change-Id: Iae77fd142bfe12851aea0f93807410e159479dc5
Reviewed-on: https://chromium-review.googlesource.com/927102
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538502}
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/content/browser/accessibility/web_contents_accessibility_android.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/content/browser/renderer_host/input/motion_event_web.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/content/browser/renderer_host/input/motion_event_web_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/content/browser/renderer_host/input/stylus_text_selector.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/content/browser/renderer_host/input/stylus_text_selector_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/content/browser/renderer_host/input/web_input_event_builders_android_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/content/browser/renderer_host/input/web_input_event_util_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/android/motion_event_android.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/android/motion_event_android.h
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/android/motion_event_android_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/blink/blink_event_util.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/blink/blink_event_util.h
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/filtered_gesture_provider.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/gesture_detector.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/gesture_event_data.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/gesture_event_data_packet.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/gesture_event_data_packet_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/gesture_provider.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/gesture_provider_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/gesture_touch_uma_histogram.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/motion_event.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/motion_event.h
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/motion_event_buffer.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/motion_event_buffer_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/motion_event_generic.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/motion_event_generic_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/scale_gesture_detector.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/scale_gesture_detector.h
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/snap_scroll_controller.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/touch_disposition_gesture_filter.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/velocity_tracker.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gesture_detection/velocity_tracker_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gestures/motion_event_aura.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/gestures/motion_event_aura_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/events/test/motion_event_test_utils.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/touch_selection/longpress_drag_selector.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/touch_selection/touch_handle.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/touch_selection/touch_handle_unittest.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/touch_selection/touch_selection_controller.cc
[modify] https://crrev.com/0ea11d1dc1b0b6524d3087805b55d8fcf33d6174/ui/touch_selection/touch_selection_controller_unittest.cc

Owner: dtapu...@chromium.org
Status: Fixed (was: Available)

Sign in to add a comment