The helper functions for slop region check and subtraction are modified.

The helper functions for slop region check and subtraction are modified.

The unit test crashes in the old version and passes in the new patch.

BUG=629278
TEST=GestureProviderTest.ScrollStartWithSecondaryPointer

Committed: https://crrev.com/4479b653205f98f226e7f5d4cf1c6a78897ca97f
Cr-Commit-Position: refs/heads/master@{#407808}

[sahel, 2016-07-22 15:33:19]

sahel@chromium.org changed reviewers:
+ brettw@chromium.org, tdresser@chromium.org

[sahel, 2016-07-22 15:33:19]

brettw@chromium.org: Please review changes in 

tdresser@chromium.org: Please review changes in

[tdresser, 2016-07-22 15:57:45]

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
File ui/events/gesture_detection/gesture_detector.cc (right):

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.cc:548: const int pid =
ev.GetPointerId(i);
Throughout, let's use pointer_id instead of pid, for the sake of clarity.

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.cc:554:
DCHECK(source_pointer_down_event);
Return if source_pointer_down_event is nullptr.

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.cc:557: float dx =
source_pointer_down_event->GetX(source_index) - ev.GetX(i);
What happens if source_index is 0? Should we return before doing this?

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.cc:578: return NULL;
Should this ever happen? If not, maybe we should have a DCHECK.
Return nullptr.

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.cc:586: return -1;
Should this case ever happen? Should we have a DCHECK here (as well as the
caller)?

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

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.h:107: const int pid);
Make it clear that this only works for primary and secondary pointers.

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
File ui/events/gesture_detection/gesture_provider_unittest.cc (right):

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_provider_unittest.cc:1067:
TEST_F(GestureProviderTest, ScrollStartWithSecondaryPointer) {
Out of curiosity, what ended up being the solution to your previous problem
here?

https://codereview.chromium.org/2175803002/diff/1/ui/events/test/motion_event...
File ui/events/test/motion_event_test_utils.cc (right):

https://codereview.chromium.org/2175803002/diff/1/ui/events/test/motion_event...
ui/events/test/motion_event_test_utils.cc:121: MockMotionEvent&
MockMotionEvent::ReleasePoint() {
It should be pretty easy to make this a thin wrapper of ReleasePointAt, right?
It would mean adding a bit of code to ReleasePointAt, but it would probably be
cleaner overall.

https://codereview.chromium.org/2175803002/diff/1/ui/events/test/motion_event...
ui/events/test/motion_event_test_utils.cc:133: MockMotionEvent&
MockMotionEvent::ReleasePointAt(size_t index) {
ReleasePointAtIndex?

It's a bit confusing that we have both ids and indices floating around, so it
would be nice to be explicit about which we're referring to.

[sahel, 2016-07-22 19:45:20]

Patchset #2 (id:20001) has been deleted

[sahel, 2016-07-22 19:46:42]

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
File ui/events/gesture_detection/gesture_detector.cc (right):

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.cc:548: const int pid =
ev.GetPointerId(i);
On 2016/07/22 15:57:45, tdresser wrote:
> Throughout, let's use pointer_id instead of pid, for the sake of clarity.

Done.

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.cc:554:
DCHECK(source_pointer_down_event);
On 2016/07/22 15:57:45, tdresser wrote:
> Return if source_pointer_down_event is nullptr.

Done.

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.cc:557: float dx =
source_pointer_down_event->GetX(source_index) - ev.GetX(i);
On 2016/07/22 15:57:45, tdresser wrote:
> What happens if source_index is 0? Should we return before doing this?

I changed the code, an early return will happen if the source_index < 0,
otherwise it should be fine.

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.cc:578: return NULL;
On 2016/07/22 15:57:45, tdresser wrote:
> Should this ever happen? If not, maybe we should have a DCHECK.
> Return nullptr.
> 

Done.

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.cc:586: return -1;
On 2016/07/22 15:57:45, tdresser wrote:
> Should this case ever happen? Should we have a DCHECK here (as well as the
> caller)?

Done.

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

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_detector.h:107: const int pid);
On 2016/07/22 15:57:45, tdresser wrote:
> Make it clear that this only works for primary and secondary pointers.

Done.

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
File ui/events/gesture_detection/gesture_provider_unittest.cc (right):

https://codereview.chromium.org/2175803002/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/gesture_provider_unittest.cc:1067:
TEST_F(GestureProviderTest, ScrollStartWithSecondaryPointer) {
On 2016/07/22 15:57:45, tdresser wrote:
> Out of curiosity, what ended up being the solution to your previous problem
> here?

Creating events with the sequence of action_down, action_pointer_down,
action_pointer_up (even with setting the action index to be the first pointer),
didn't help.

Because each time new events were created and the ids would be set
appropriately. 

I used utility functions on the same event instead.
I also had to add a new utility function that can release a pointer with a given
index rather than always the last pointer.

https://codereview.chromium.org/2175803002/diff/1/ui/events/test/motion_event...
File ui/events/test/motion_event_test_utils.cc (right):

https://codereview.chromium.org/2175803002/diff/1/ui/events/test/motion_event...
ui/events/test/motion_event_test_utils.cc:121: MockMotionEvent&
MockMotionEvent::ReleasePoint() {
On 2016/07/22 15:57:45, tdresser wrote:
> It should be pretty easy to make this a thin wrapper of ReleasePointAt, right?
> It would mean adding a bit of code to ReleasePointAt, but it would probably be
> cleaner overall.

Done.

https://codereview.chromium.org/2175803002/diff/1/ui/events/test/motion_event...
ui/events/test/motion_event_test_utils.cc:133: MockMotionEvent&
MockMotionEvent::ReleasePointAt(size_t index) {
On 2016/07/22 15:57:45, tdresser wrote:
> ReleasePointAtIndex?
> 
> It's a bit confusing that we have both ids and indices floating around, so it
> would be nice to be explicit about which we're referring to.

Done.

[tdresser, 2016-07-22 19:51:28]

LGTM!

https://codereview.chromium.org/2175803002/diff/40001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_detector.h (right):

https://codereview.chromium.org/2175803002/diff/40001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:104: //
secondary_pointer_down_event_.
State that this returns nullptr if the source event isn't current or secondary.

https://codereview.chromium.org/2175803002/diff/40001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:109: int getPointerIndex(const
MotionEvent& event, const int pointer_id);
State that this returns -1 if it can't find it.

[tdresser, 2016-07-25 12:35:08]

https://codereview.chromium.org/2175803002/diff/40001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_detector.cc (right):

https://codereview.chromium.org/2175803002/diff/40001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.cc:588: int
GestureDetector::getPointerIndex(const MotionEvent& event,
This is just MotionEvent::FindPointerIndexOfId(int id).

https://cs.chromium.org/chromium/src/ui/events/gesture_detection/motion_event...

[sahel, 2016-07-25 15:16:33]

https://codereview.chromium.org/2175803002/diff/40001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_detector.h (right):

https://codereview.chromium.org/2175803002/diff/40001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:104: //
secondary_pointer_down_event_.
On 2016/07/22 19:51:28, tdresser wrote:
> State that this returns nullptr if the source event isn't current or
secondary.

Done.

https://codereview.chromium.org/2175803002/diff/40001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_detector.h:109: int getPointerIndex(const
MotionEvent& event, const int pointer_id);
On 2016/07/22 19:51:28, tdresser wrote:
> State that this returns -1 if it can't find it.

Done.

[sahel, 2016-07-25 21:17:46]

Description was changed from

==========
The helper functions for slop region check and subtraction are modified.

The unit test crashes in the old version and passes in the new patch.

BUG=629278
TEST=GestureProviderTest.ScrollStartWithSecondaryPointer
==========

to

==========
The helper functions for slop region check and subtraction are modified.

The unit test crashes in the old version and passes in the new patch.

BUG=629278
TEST=GestureProviderTest.ScrollStartWithSecondaryPointer
==========

[sahel, 2016-07-25 21:17:46]

sahel@chromium.org changed reviewers:
- brettw@chromium.org

[sahel, 2016-07-25 21:18:56]

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

[sahel, 2016-07-25 21:18:56]

sky@chromium.org: Please review changes in 

ui/events/test/motion_event_test_utils.h
ui/events/test/motion_event_test_utils.cc

[sky, 2016-07-25 23:48:58]

LGTM

[sahel, 2016-07-26 14:25:14]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 14:25:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 15:06:22]

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

[commit-bot: I haz the power, 2016-07-26 15:06:23]

Dry run: This issue passed the CQ dry run.

[sahel, 2016-07-26 15:30:51]

The CQ bit was checked by sahel@chromium.org

[sahel, 2016-07-26 15:30:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2175803002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-07-26 15:31:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 15:35:57]

Description was changed from

==========
The helper functions for slop region check and subtraction are modified.

The unit test crashes in the old version and passes in the new patch.

BUG=629278
TEST=GestureProviderTest.ScrollStartWithSecondaryPointer
==========

to

==========
The helper functions for slop region check and subtraction are modified.

The unit test crashes in the old version and passes in the new patch.

BUG=629278
TEST=GestureProviderTest.ScrollStartWithSecondaryPointer
==========

[commit-bot: I haz the power, 2016-07-26 15:35:58]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-26 15:37:28]

Description was changed from

==========
The helper functions for slop region check and subtraction are modified.

The unit test crashes in the old version and passes in the new patch.

BUG=629278
TEST=GestureProviderTest.ScrollStartWithSecondaryPointer
==========

to

==========
The helper functions for slop region check and subtraction are modified.

The unit test crashes in the old version and passes in the new patch.

BUG=629278
TEST=GestureProviderTest.ScrollStartWithSecondaryPointer

Committed: https://crrev.com/4479b653205f98f226e7f5d4cf1c6a78897ca97f
Cr-Commit-Position: refs/heads/master@{#407808}
==========

[commit-bot: I haz the power, 2016-07-26 15:37:29]

Patchset 3 (id:??) landed as
https://crrev.com/4479b653205f98f226e7f5d4cf1c6a78897ca97f
Cr-Commit-Position: refs/heads/master@{#407808}