Remove abstract Clone and Cancel methods from MotionEvent

Remove abstract Clone and Cancel methods from MotionEvent

Create a single implementation of MotionEvent's |Clone()| and
|Cancel()| methods using MotionEventGeneric. Expand
MotionEventGeneric to support historical events, also removing
CompoundMotionEvent from the MotionEventBuffer implementation.

By avoiding JNI and garbage creation, this reduces the cost of both
methods by ~10x on Android, from ~25us call to ~2us.

BUG=501503003
Committed: https://crrev.com/b5a33976eb45777e6ca7c8785677d4df08eb04fd
Cr-Commit-Position: refs/heads/master@{#301136}

[jdduke (slow), 2014-08-25 22:26:02]

Patchset #2 (id:20001) has been deleted

[jdduke (slow), 2014-08-25 22:26:11]

Patchset #3 (id:60001) has been deleted

[jdduke (slow), 2014-08-27 15:11:22]

jdduke@chromium.org changed reviewers:
+ tdresser@chromium.org

[jdduke (slow), 2014-08-27 15:11:22]

tdresser@: Do you feel this patch is worth pursuing? In practice, the
performance win isn't likely to show up anywhere, but reducing clone/cancel code
duplication might have some merit. Note that the empty history vector in
MotionEventGeneric will (for all current usages) be empty and so not add any
meaningful overhead.

[jdduke (slow), 2014-10-16 17:01:59]

tdresser@: Alright, I think this is good for review. There's one remaining
integration test on Android I need to work out, but I think that's about it.
Thanks.

[tdresser, 2014-10-20 15:19:15]

LGTM, with nits.

https://codereview.chromium.org/502993004/diff/610001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android_unittest.cc
(right):

https://codereview.chromium.org/502993004/diff/610001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android_unittest.cc:156: NULL,
These (and other occurrences of NULL) should be nullptr now.

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.cc (right):

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.cc:104: }
This indentation looks wrong.

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_buffer.cc (right):

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_buffer.cc:156: // Note that it is
assumed that the events are in non-decreasing (time) order.
I'd prefer:
"Note that it is assumed that the events are in non-decreasing (time) order." ->
"Events must be in non-decreasing (time) order."

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_buffer.cc:179: const ui::MotionEvent*
event0 = NULL;
Might as well nullptr'ify these while you're here.

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic.h (right):

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic.h:74: // pointer count, and
must be of the same type (ACTION_MOVE), as this event.
This is a little confusing.

Based on the code, perhaps: 

// Add an event to the history. Note that |this| and |event| must have the 
// same pointer count and must both have an action of ACTION_MOVE.

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic_unittest.cc (right):

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic_unittest.cc:118:
EXPECT_EQ(event.ToString(), cancel->ToString());
Is this ToString method just to make debugging failed expectations easier? If
so, we should be doing this instead:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Teaching_Google_Test_...

and using the previously defined == operator.

[jdduke (slow), 2014-10-21 22:19:58]

https://codereview.chromium.org/502993004/diff/610001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android_unittest.cc
(right):

https://codereview.chromium.org/502993004/diff/610001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android_unittest.cc:156: NULL,
On 2014/10/20 15:19:15, tdresser wrote:
> These (and other occurrences of NULL) should be nullptr now.

Done.

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.cc (right):

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.cc:104: }
On 2014/10/20 15:19:15, tdresser wrote:
> This indentation looks wrong.

Done.

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_buffer.cc (right):

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_buffer.cc:156: // Note that it is
assumed that the events are in non-decreasing (time) order.
On 2014/10/20 15:19:15, tdresser wrote:
> I'd prefer:
> "Note that it is assumed that the events are in non-decreasing (time) order."
->
> "Events must be in non-decreasing (time) order."

Done.

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_buffer.cc:179: const ui::MotionEvent*
event0 = NULL;
On 2014/10/20 15:19:15, tdresser wrote:
> Might as well nullptr'ify these while you're here.

Done.

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic.h (right):

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic.h:74: // pointer count, and
must be of the same type (ACTION_MOVE), as this event.
On 2014/10/20 15:19:15, tdresser wrote:
> This is a little confusing.
> 
> Based on the code, perhaps: 
> 
> // Add an event to the history. Note that |this| and |event| must have the 
> // same pointer count and must both have an action of ACTION_MOVE.

Done.

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic_unittest.cc (right):

https://codereview.chromium.org/502993004/diff/610001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic_unittest.cc:118:
EXPECT_EQ(event.ToString(), cancel->ToString());
On 2014/10/20 15:19:15, tdresser wrote:
> Is this ToString method just to make debugging failed expectations easier? If
> so, we should be doing this instead:
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Teaching_Google_Test_...
> 
> and using the previously defined == operator.

Yeah, good call. I factored out operator<< and operator== into a
motion_event_test_utils helper.

[jdduke (slow), 2014-10-21 22:53:44]

jdduke@chromium.org changed reviewers:
+ sky@chromium.org

[jdduke (slow), 2014-10-21 22:53:44]

sky@: PTAL for owner approval of ui/events/test and ui/events/OWNERS? I'm not
sure when sadrul@ will be back (and I think the missing BUILD.gn exception in
OWNERS was an oversight). Thanks.

[sky, 2014-10-22 00:01:04]

https://codereview.chromium.org/502993004/diff/670001/ui/events/test/motion_e...
File ui/events/test/motion_event_test_utils.h (right):

https://codereview.chromium.org/502993004/diff/670001/ui/events/test/motion_e...
ui/events/test/motion_event_test_utils.h:18: bool operator==(const MotionEvent&
lhs, const MotionEvent& rhs);
I would rather you have add an explicit ToString function and if tests want to
compare that for equality, go for it.

[jdduke (slow), 2014-10-22 00:15:56]

https://codereview.chromium.org/502993004/diff/670001/ui/events/test/motion_e...
File ui/events/test/motion_event_test_utils.h (right):

https://codereview.chromium.org/502993004/diff/670001/ui/events/test/motion_e...
ui/events/test/motion_event_test_utils.h:18: bool operator==(const MotionEvent&
lhs, const MotionEvent& rhs);
On 2014/10/22 00:01:04, sky wrote:
> I would rather you have add an explicit ToString function and if tests want to
> compare that for equality, go for it.

Sure, that's the behavior I had in PS7 but changed per tdresser@'s
recommendation. I'll change it back.

[tdresser, 2014-10-22 12:18:02]

On 2014/10/22 00:15:56, jdduke wrote:
>
https://codereview.chromium.org/502993004/diff/670001/ui/events/test/motion_e...
> File ui/events/test/motion_event_test_utils.h (right):
> 
>
https://codereview.chromium.org/502993004/diff/670001/ui/events/test/motion_e...
> ui/events/test/motion_event_test_utils.h:18: bool operator==(const
MotionEvent&
> lhs, const MotionEvent& rhs);
> On 2014/10/22 00:01:04, sky wrote:
> > I would rather you have add an explicit ToString function and if tests want
to
> > compare that for equality, go for it.
> 
> Sure, that's the behavior I had in PS7 but changed per tdresser@'s
> recommendation. I'll change it back.

Sorry for causing that run-around.
Still LGTM.

[jdduke (slow), 2014-10-22 18:30:46]

OK, stringification is now explicit with ToString (also moved MockMotionEvent
into motion_event_test_utils.h).

[sky, 2014-10-22 20:22:15]

LGTM

[jdduke (slow), 2014-10-23 16:25:41]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-10-23 16:26:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/502993004/690001

[commit-bot: I haz the power, 2014-10-23 18:16:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-23 18:16:47]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[jdduke (slow), 2014-10-24 16:42:10]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-10-24 16:42:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/502993004/710001

[commit-bot: I haz the power, 2014-10-24 16:42:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/502993004/710001

[commit-bot: I haz the power, 2014-10-24 17:35:35]

Committed patchset #12 (id:710001)

[commit-bot: I haz the power, 2014-10-24 17:36:31]

Patchset 12 (id:??) landed as
https://crrev.com/b5a33976eb45777e6ca7c8785677d4df08eb04fd
Cr-Commit-Position: refs/heads/master@{#301136}