Extend MotionEvent to have tool type and button state

Extend MotionEvent to have tool type and button state

BUG=384803
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278936

[Changwan Ryu, 2014-06-20 08:07:47]

This is needed by https://codereview.chromium.org/342633003.

[jdduke (slow), 2014-06-20 15:33:25]

https://codereview.chromium.org/348813002/diff/1/content/browser/android/cont...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/348813002/diff/1/content/browser/android/cont...
content/browser/android/content_view_core_impl.h:114: jint android_tool_type_0,
Sigh, this method has grown so large, but I don't really know of a better or
more efficient way of doing this.

https://codereview.chromium.org/348813002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/348813002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/motion_event_android.cc:89: 
I would say drop the "ToAndroidToolType" and "ToAndroidButtonState" helpers as
it doesn't look like we need them.

https://codereview.chromium.org/348813002/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/348813002/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/motion_event.h:19: ACTION_DOWN = 100,
Any reason for the 100/200?

https://codereview.chromium.org/348813002/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/motion_event.h:34: const static int BUTTON_NONE = 0;
I would go ahead and use an enum bit mask here, e.g., 

enum ButtonType {
  BUTTON_PRIMARY = 1 << 0,
  BUTTON_SECONDARY = 1 << 1,
  ...
}

https://codereview.chromium.org/348813002/diff/1/ui/events/gestures/motion_ev...
File ui/events/gestures/motion_event_aura.cc (right):

https://codereview.chromium.org/348813002/diff/1/ui/events/gestures/motion_ev...
ui/events/gestures/motion_event_aura.cc:161: return 0;
Maybe return TOOL_TYPE_UNKNOWN?

https://codereview.chromium.org/348813002/diff/1/ui/events/gestures/motion_ev...
File ui/events/gestures/motion_event_aura.h (right):

https://codereview.chromium.org/348813002/diff/1/ui/events/gestures/motion_ev...
ui/events/gestures/motion_event_aura.h:51: virtual ToolType GetToolType(size_t
pointer_index) const OVERRIDE;
Rick, would this data ever be useful for WebTouchEvents? If not, I wonder if we
should keep "GetToolType()" and "GetButtonState()" as methods on
MotionEventAndroid, and pass MotionEventAndroid to RWHVAndroid instead of the
ui::MotionEvent.  If they become necessary/desirable in the future, we could
hoist them to ui::MotionEvent.

[Changwan Ryu, 2014-06-20 21:00:34]

https://codereview.chromium.org/348813002/diff/1/content/browser/android/cont...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/348813002/diff/1/content/browser/android/cont...
content/browser/android/content_view_core_impl.h:114: jint android_tool_type_0,
On 2014/06/20 15:33:25, jdduke wrote:
> Sigh, this method has grown so large, but I don't really know of a better or
> more efficient way of doing this.

One idea to encapsulate these parameters would be to use a light weight proto
buffer. But there still may be some overhead.

https://codereview.chromium.org/348813002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/348813002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/motion_event_android.cc:89: 
On 2014/06/20 15:33:25, jdduke wrote:
> I would say drop the "ToAndroidToolType" and "ToAndroidButtonState" helpers as
> it doesn't look like we need them.

Done.

https://codereview.chromium.org/348813002/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/348813002/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/motion_event.h:19: ACTION_DOWN = 100,
On 2014/06/20 15:33:25, jdduke wrote:
> Any reason for the 100/200?

No specific reason. I just wanted to assign SOME distinct values so that when we
debug we can tell which enum value was used from debug message and log (such as
in ToAndroid... functions).

https://codereview.chromium.org/348813002/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/motion_event.h:34: const static int BUTTON_NONE = 0;
On 2014/06/20 15:33:25, jdduke wrote:
> I would go ahead and use an enum bit mask here, e.g., 
> 
> enum ButtonType {
>   BUTTON_PRIMARY = 1 << 0,
>   BUTTON_SECONDARY = 1 << 1,
>   ...
> }

Done.

https://codereview.chromium.org/348813002/diff/1/ui/events/gestures/motion_ev...
File ui/events/gestures/motion_event_aura.cc (right):

https://codereview.chromium.org/348813002/diff/1/ui/events/gestures/motion_ev...
ui/events/gestures/motion_event_aura.cc:161: return 0;
On 2014/06/20 15:33:25, jdduke wrote:
> Maybe return TOOL_TYPE_UNKNOWN?

Done.

[jdduke (slow), 2014-06-20 21:19:41]

lgtm with a couple nits (also please run "git cl format", I think the formatting
should be OK but just to be sure), and please wait for Rick to sign off as well.
 We're hoping this pushes forward more general discussion on exposing these
kinds of properties to the web via touch events. 

After this lands we can take up discussion in the follow-up patch, thanks!

https://codereview.chromium.org/348813002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/348813002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/motion_event_web.cc:149: return
ui::MotionEvent::TOOL_TYPE_UNKNOWN;
Nit: No need for ui::MotionEvent:: prefix here.

https://codereview.chromium.org/348813002/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/348813002/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/motion_event.h:19: ACTION_DOWN = 100,
On 2014/06/20 21:00:34, Changwan Ryu wrote:
> On 2014/06/20 15:33:25, jdduke wrote:
> > Any reason for the 100/200?
> 
> No specific reason. I just wanted to assign SOME distinct values so that when
we
> debug we can tell which enum value was used from debug message and log (such
as
> in ToAndroid... functions).

I see, let's stick with not assigning them specific non-zero values for now, as
the user would have to look at the header anyway to figure out the corresponding
value, and by then they'd know the context (whether it's action or tool type).

[jdduke (slow), 2014-06-20 21:21:26]

Adding tdresser@ as ui/events/gestures OWNER.  Tim, I think NOTIMPLEMENTED() is
fine here as we want to make sure we populate these values for Aura before
they're used in a meaningful way.  Should we file a bug for this?

[tdresser, 2014-06-20 21:26:35]

On 2014/06/20 21:21:26, jdduke wrote:
> Adding tdresser@ as ui/events/gestures OWNER.  Tim, I think NOTIMPLEMENTED()
is
> fine here as we want to make sure we populate these values for Aura before
> they're used in a meaningful way.  Should we file a bug for this?

ui/events/gestures LGTM.

[Rick Byers, 2014-06-20 21:31:23]

On 2014/06/20 21:26:35, tdresser wrote:
> On 2014/06/20 21:21:26, jdduke wrote:
> > Adding tdresser@ as ui/events/gestures OWNER.  Tim, I think NOTIMPLEMENTED()
> is
> > fine here as we want to make sure we populate these values for Aura before
> > they're used in a meaningful way.  Should we file a bug for this?
> 
> ui/events/gestures LGTM.

This is great, thanks for doing this!

[Changwan Ryu, 2014-06-20 21:32:02]

Ran git cl format and didn't get any complaint.

https://codereview.chromium.org/348813002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/348813002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/motion_event_web.cc:149: return
ui::MotionEvent::TOOL_TYPE_UNKNOWN;
On 2014/06/20 21:19:41, jdduke wrote:
> Nit: No need for ui::MotionEvent:: prefix here.

Done.

https://codereview.chromium.org/348813002/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/348813002/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/motion_event.h:19: ACTION_DOWN = 100,
On 2014/06/20 21:19:41, jdduke wrote:
> On 2014/06/20 21:00:34, Changwan Ryu wrote:
> > On 2014/06/20 15:33:25, jdduke wrote:
> > > Any reason for the 100/200?
> > 
> > No specific reason. I just wanted to assign SOME distinct values so that
when
> we
> > debug we can tell which enum value was used from debug message and log (such
> as
> > in ToAndroid... functions).
> 
> I see, let's stick with not assigning them specific non-zero values for now,
as
> the user would have to look at the header anyway to figure out the
corresponding
> value, and by then they'd know the context (whether it's action or tool type).

Done.

[Changwan Ryu, 2014-06-20 21:32:52]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-06-20 21:34:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/changwan@chromium.org/348813002/40001

[Changwan Ryu, 2014-06-20 21:50:52]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-06-20 21:53:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/changwan@chromium.org/348813002/60001

[Changwan Ryu, 2014-06-21 00:16:53]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-06-21 00:18:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/changwan@chromium.org/348813002/80001

[commit-bot: I haz the power, 2014-06-21 05:46:31]

Change committed as 278936