Android mouse events shouldn't appear as TouchEvents

content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java

Index: content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
index 209f0a4b18d350dd1b8429c8d3a79f71f6b5e848..b0cf7d08ba6faf35e381774cf0b075d6c5c87d01 100644
--- a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
+++ b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
@@ -1019,8 +1019,57 @@ public class ContentViewCore implements AccessibilityStateChangeListener, Displa
      * @see View#onTouchEvent(MotionEvent)
      */
     public boolean onTouchEvent(MotionEvent event) {
+        // TODO(mustaq): Should we include MotionEvent.TOOL_TYPE_STYLUS here?
+        // crbug.com/592082
+        if (event.getToolType(0) == MotionEvent.TOOL_TYPE_MOUSE) {
+            // Pre-Andorid-L mouse button info is incomplete

[aelias_OOO_until_Jul13, 2016/11/09 02:43:14]

"Mouse button info is incomplete on L and below"

[mustaq, 2016/11/09 17:04:33]

Done.

+            int apiVersion = Build.VERSION.SDK_INT;
+            if (apiVersion >= android.os.Build.VERSION_CODES.M) {
+                return sendMouseEvent(event);
+            }
+        }
+
         final boolean isTouchHandleEvent = false;
-        return onTouchEventImpl(event, isTouchHandleEvent);
+        return sendTouchEvent(event, isTouchHandleEvent);
+    }
+
+    @SuppressLint("NewApi")

[aelias_OOO_until_Jul13, 2016/11/09 02:43:14]

I think it would work to do @TargetApi(Build.VERSION_CODES.M) instead?

[mustaq, 2016/11/09 17:04:33]

Done, thanks.

+    private boolean sendMouseEvent(MotionEvent event) {
+        TraceEvent.begin("sendMouseEvent");
+
+        MotionEvent offsetEvent = createOffsetMotionEvent(event);
+        try {
+            mContainerView.removeCallbacks(mFakeMouseMoveRunnable);

[aelias_OOO_until_Jul13, 2016/11/09 02:43:14]

That reminds me that this thing exists... could you add a "TODO(mustaq): Delete
this, see http://crbug.com/492738" next to uses of it in this file?

[mustaq, 2016/11/09 17:04:32]

Commented the main usage instead. Does it look enough?

+            if (mNativeContentViewCore != 0) {

[aelias_OOO_until_Jul13, 2016/11/09 02:43:14]

Please do "if (mNativeContentViewCore == 0) return false;" in order to avoid the
indentation, and also because returning false seems more correct than returning
true.

[mustaq, 2016/11/09 17:04:32]

Done.

[mustaq, 2016/11/09 17:04:32]

Done.

+                int eventAction = event.getActionMasked();
+
+                // For mousedown and mouseup events, we use ACTION_BUTTON_PRESS
+                // and ACTION_BUTTON_RELEASE respectively because they provide
+                // info about the changed-button.
+                if (eventAction == MotionEvent.ACTION_DOWN
+                        || eventAction == MotionEvent.ACTION_UP) {
+                    return false;
+                }
+
+                // The method getActionButton() is defined only for

[aelias_OOO_until_Jul13, 2016/11/09 02:43:14]

What happens if you call getActionButton() on one of the other event types? 
Does it crash/assert/log?  If it doesn't have any undesirable side effects and
just returns 0 anyway, then please delete the unnecessary if statement below.

[mustaq, 2016/11/09 17:04:32]

Assuming it neither throws nor emits any logs, even if we found the return value
to be 0 in our test cases, there is no guarantee of it across all devices or in
future OSes. Since MouseEvent.button (=action button value) is crucial for web
compat, I would prefer avoiding the unspeced behavior here. Wdyt?

[aelias_OOO_until_Jul13, 2016/11/09 18:57:03]

That seems excessively paranoid.  Almost no Android API is specced or tested,
therefore you could make a similar argument about any Android API whatsoever and
justify an infinite amount of unnecessary filtering cruft that way.  Also, I
have no reason to believe OEMs or future Android releases will touch this and
would be fine to triage a bug arising from this if they did (it would likely
come along with other interesting mouse changes we'd want to pay attention to).

A common cause of hairy code complexity is fear -- weird branches accumulate
because people are afraid of breaking a hypothetical scenario.  I'm OK taking a
low risk of an unknown bug to keep the code healthy.

[mustaq, 2016/11/09 19:55:59]

Just tested on an N7/M, even with chorded button presses, and it just works. So
let's go for the simplest code.

+                // ACTION_BUTTON_PRESS and ACTION_BUTTON_RELEASE
+                int changedButton = 0;
+                if (eventAction == MotionEvent.ACTION_BUTTON_PRESS
+                        || eventAction == MotionEvent.ACTION_BUTTON_RELEASE) {
+                    changedButton = event.getActionButton();
+                }
+
+                nativeSendMouseEvent(mNativeContentViewCore, event.getEventTime(), eventAction,
+                        offsetEvent.getX(), offsetEvent.getY(), event.getPointerId(0),
+                        event.getPressure(0), event.getOrientation(0),
+                        event.getAxisValue(MotionEvent.AXIS_TILT, 0), changedButton,
+                        event.getButtonState(), event.getMetaState(), event.getToolType(0));
+            }
+            return true;
+        } finally {
+            offsetEvent.recycle();
+            TraceEvent.end("sendMouseEvent");
+        }
     }
 
     /**
@@ -1029,11 +1078,11 @@ public class ContentViewCore implements AccessibilityStateChangeListener, Displa
      */
     public boolean onTouchHandleEvent(MotionEvent event) {
         final boolean isTouchHandleEvent = true;
-        return onTouchEventImpl(event, isTouchHandleEvent);
+        return sendTouchEvent(event, isTouchHandleEvent);
     }
 
-    private boolean onTouchEventImpl(MotionEvent event, boolean isTouchHandleEvent) {
-        TraceEvent.begin("onTouchEvent");
+    private boolean sendTouchEvent(MotionEvent event, boolean isTouchHandleEvent) {
+        TraceEvent.begin("sendTouchEvent");
         try {
             int eventAction = event.getActionMasked();
 
@@ -1092,7 +1141,7 @@ public class ContentViewCore implements AccessibilityStateChangeListener, Displa
             if (offset != null) offset.recycle();
             return consumed;
         } finally {
-            TraceEvent.end("onTouchEvent");
+            TraceEvent.end("sendTouchEvent");
         }
     }
 
@@ -1573,18 +1622,28 @@ public class ContentViewCore implements AccessibilityStateChangeListener, Displa
     public boolean onHoverEvent(MotionEvent event) {
         TraceEvent.begin("onHoverEvent");
 
+        int eventAction = event.getActionMasked();
+
+        // Work around Android bug where the x, y coordinates of a hover exit

[aelias_OOO_until_Jul13, 2016/11/09 02:43:14]

This if statement is now no-op given your other early-return below.

[aelias_OOO_until_Jul13, 2016/11/09 20:08:51]

Looks like you missed my previous comment.  Please delete this if statement
before you land.

[mustaq, 2016/11/09 20:22:58]

Oops, done.

+        // event are incorrect when touch exploration is on.
+        if (mTouchExplorationEnabled && eventAction == MotionEvent.ACTION_HOVER_EXIT) {
+            return true;
+        }
+
+        // Ignore ACTION_HOVER_ENTER & ACTION_HOVER_EXIT: every mouse-down on
+        // Android follows a hover-exit and is followed by a hover-enter. The
+        // MotionEvent spec seems to support this behavior indirectly.
+        if (eventAction == MotionEvent.ACTION_HOVER_ENTER
+                || eventAction == MotionEvent.ACTION_HOVER_EXIT) {
+            return false;
+        }
+
         MotionEvent offset = createOffsetMotionEvent(event);
         try {
             if (mBrowserAccessibilityManager != null && !mIsObscuredByAnotherView) {
                 return mBrowserAccessibilityManager.onHoverEvent(offset);
             }
 
-            // Work around Android bug where the x, y coordinates of a hover exit
-            // event are incorrect when touch exploration is on.
-            if (mTouchExplorationEnabled && offset.getAction() == MotionEvent.ACTION_HOVER_EXIT) {
-                return true;
-            }
-
             // TODO(lanwei): Remove this switch once experimentation is complete -
             // crbug.com/418188
             if (event.getToolType(0) == MotionEvent.TOOL_TYPE_FINGER) {
@@ -1597,8 +1656,11 @@ public class ContentViewCore implements AccessibilityStateChangeListener, Displa
 
             mContainerView.removeCallbacks(mFakeMouseMoveRunnable);
             if (mNativeContentViewCore != 0) {
-                nativeSendMouseMoveEvent(mNativeContentViewCore, offset.getEventTime(),
-                        offset.getX(), offset.getY(), event.getToolType(0));
+                nativeSendMouseEvent(mNativeContentViewCore, event.getEventTime(), eventAction,
+                        offset.getX(), offset.getY(), event.getPointerId(0), event.getPressure(0),
+                        event.getOrientation(0), event.getAxisValue(MotionEvent.AXIS_TILT, 0),
+                        0 /* changedButton */, event.getButtonState(), event.getMetaState(),
+                        event.getToolType(0));
             }
             return true;
         } finally {
@@ -1615,7 +1677,7 @@ public class ContentViewCore implements AccessibilityStateChangeListener, Displa
         if ((event.getSource() & InputDevice.SOURCE_CLASS_POINTER) != 0) {
             mLastFocalEventX = event.getX();
             mLastFocalEventY = event.getY();
-            switch (event.getAction()) {
+            switch (event.getActionMasked()) {
                 case MotionEvent.ACTION_SCROLL:
                     if (mNativeContentViewCore == 0) return false;
 
@@ -1638,6 +1700,13 @@ public class ContentViewCore implements AccessibilityStateChangeListener, Displa
                     };
                     mContainerView.postDelayed(mFakeMouseMoveRunnable, 250);
                     return true;
+                case MotionEvent.ACTION_BUTTON_PRESS:
+                case MotionEvent.ACTION_BUTTON_RELEASE:
+                    // TODO(mustaq): Should we include MotionEvent.TOOL_TYPE_STYLUS here?
+                    // crbug.com/592082
+                    if (event.getToolType(0) == MotionEvent.TOOL_TYPE_MOUSE) {
+                        return sendMouseEvent(event);
+                    }
             }
         } else if ((event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) != 0) {
             if (mJoystickScrollProvider.onMotion(event)) return true;
@@ -3349,8 +3418,9 @@ public class ContentViewCore implements AccessibilityStateChangeListener, Displa
             int androidButtonState, int androidMetaState,
             boolean isTouchHandleEvent);
 
-    private native int nativeSendMouseMoveEvent(
-            long nativeContentViewCoreImpl, long timeMs, float x, float y, int toolType);
+    private native int nativeSendMouseEvent(long nativeContentViewCoreImpl, long timeMs, int action,
+            float x, float y, int pointerId, float pressure, float orientaton, float tilt,
+            int changedButton, int buttonState, int metaState, int toolType);
 
     private native int nativeSendMouseWheelEvent(long nativeContentViewCoreImpl, long timeMs,
             float x, float y, float ticksX, float ticksY, float pixelsPerTick);