Don't create zero-velocity GestureFlingStarts.

Don't create zero-velocity GestureFlingStarts.

Currently trackpad initiated GestureFlingStarts on CrOS can have zero-
velocity, but this causes crashes in the renderer. This CL converts
these events to be GestureFlingCancels (just in case they are issued
in the middle of ongoing fling).

As it is hard to find the sources of such events after the fact, a
DCHECK has been added to catch new pathways if they issue such events.

BUG=627227
Committed: https://crrev.com/e80690ff28bf6db9392ab9b96a31105e1a6d4205
Cr-Commit-Position: refs/heads/master@{#406678}

[wjmaclean, 2016-07-20 20:08:24]

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

[wjmaclean, 2016-07-20 20:08:37]

Description was changed from

==========
Don't create zero-velocity GestureFlingStarts.

Currently trackpad initiated GestureFlingStarts on CrOS can have zero-
velocity, but this causes crashes in the renderer. This CL converts
these events to be GestureFlingCancels (just in case they are issued
in the middle of ongoing fling).

As it is hard to find the sources of such events after the fact, a
DCHECK has been added to catch new pathways if they issue such events.

BUG=627227
==========

to

==========
Don't create zero-velocity GestureFlingStarts.

Currently trackpad initiated GestureFlingStarts on CrOS can have zero-
velocity, but this causes crashes in the renderer. This CL converts
these events to be GestureFlingCancels (just in case they are issued
in the middle of ongoing fling).

As it is hard to find the sources of such events after the fact, a
DCHECK has been added to catch new pathways if they issue such events.

BUG=627227
==========

[wjmaclean, 2016-07-20 20:08:37]

wjmaclean@chromium.org changed reviewers:
+ spang@chromium.org

[commit-bot: I haz the power, 2016-07-20 20:08:51]

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

[wjmaclean, 2016-07-20 20:09:26]

wjmaclean@chromium.org changed reviewers:
+ nasko@chromium.org

[wjmaclean, 2016-07-20 20:09:56]

spang@ - does this look ok?

[spang, 2016-07-20 20:26:59]

lgtm

[Charlie Reis, 2016-07-20 21:25:47]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2016-07-20 21:25:47]

content/ LGTM.  Did you want this to be a CHECK to catch it in the wild, or just
a DCHECK for developer builds?

[wjmaclean, 2016-07-20 21:28:50]

On 2016/07/20 21:25:47, Charlie Reis (OOO til 7-20) wrote:
> content/ LGTM.  Did you want this to be a CHECK to catch it in the wild, or
just
> a DCHECK for developer builds?

Just a DCHECK for devs ... a CHECK would crash the browser unnecessarily; in the
wild this will cause a renderer crash only if the condition is violated.

[nasko, 2016-07-20 21:29:46]

lgtm

Lgtm

[commit-bot: I haz the power, 2016-07-20 21:32:58]

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

[commit-bot: I haz the power, 2016-07-20 21:32:59]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2016-07-20 21:33:10]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2016-07-20 21:33:47]

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-20 21:40:40]

Description was changed from

==========
Don't create zero-velocity GestureFlingStarts.

Currently trackpad initiated GestureFlingStarts on CrOS can have zero-
velocity, but this causes crashes in the renderer. This CL converts
these events to be GestureFlingCancels (just in case they are issued
in the middle of ongoing fling).

As it is hard to find the sources of such events after the fact, a
DCHECK has been added to catch new pathways if they issue such events.

BUG=627227
==========

to

==========
Don't create zero-velocity GestureFlingStarts.

Currently trackpad initiated GestureFlingStarts on CrOS can have zero-
velocity, but this causes crashes in the renderer. This CL converts
these events to be GestureFlingCancels (just in case they are issued
in the middle of ongoing fling).

As it is hard to find the sources of such events after the fact, a
DCHECK has been added to catch new pathways if they issue such events.

BUG=627227
==========

[commit-bot: I haz the power, 2016-07-20 21:40:42]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-20 21:42:44]

Description was changed from

==========
Don't create zero-velocity GestureFlingStarts.

Currently trackpad initiated GestureFlingStarts on CrOS can have zero-
velocity, but this causes crashes in the renderer. This CL converts
these events to be GestureFlingCancels (just in case they are issued
in the middle of ongoing fling).

As it is hard to find the sources of such events after the fact, a
DCHECK has been added to catch new pathways if they issue such events.

BUG=627227
==========

to

==========
Don't create zero-velocity GestureFlingStarts.

Currently trackpad initiated GestureFlingStarts on CrOS can have zero-
velocity, but this causes crashes in the renderer. This CL converts
these events to be GestureFlingCancels (just in case they are issued
in the middle of ongoing fling).

As it is hard to find the sources of such events after the fact, a
DCHECK has been added to catch new pathways if they issue such events.

BUG=627227

Committed: https://crrev.com/e80690ff28bf6db9392ab9b96a31105e1a6d4205
Cr-Commit-Position: refs/heads/master@{#406678}
==========

[commit-bot: I haz the power, 2016-07-20 21:42:45]

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

[khmel, 2016-07-21 19:55:31]

On 2016/07/20 21:42:45, commit-bot: I haz the power wrote:
> Patchset 1 (id:??) landed as
> https://crrev.com/e80690ff28bf6db9392ab9b96a31105e1a6d4205
> Cr-Commit-Position: refs/heads/master@{#406678}

I think we have to revert this CL. More details in the bug.

[wjmaclean, 2016-07-21 20:16:18]

On 2016/07/21 19:55:31, khmel wrote:
> On 2016/07/20 21:42:45, commit-bot: I haz the power wrote:
> > Patchset 1 (id:??) landed as
> > https://crrev.com/e80690ff28bf6db9392ab9b96a31105e1a6d4205
> > Cr-Commit-Position: refs/heads/master@{#406678}
> 
> I think we have to revert this CL. More details in the bug.

I'm ok with reverting this CL, but only if the hanging animation is *worse* than
having the renderer possibly crash on every scroll. In my mind that's true if
(1) two-finger navigation swiping is very common (I don't know, though I'd be
surprised if it's more common than scrolling), and (2) if the resulting hung
animation leaves the user stuck, ie. they can't recover in some reasonable way
like repeating the gesture.

Can you provide some more feedback?

[spang, 2016-07-21 20:26:51]

On 2016/07/21 20:16:18, wjmaclean wrote:
> On 2016/07/21 19:55:31, khmel wrote:
> > On 2016/07/20 21:42:45, commit-bot: I haz the power wrote:
> > > Patchset 1 (id:??) landed as
> > > https://crrev.com/e80690ff28bf6db9392ab9b96a31105e1a6d4205
> > > Cr-Commit-Position: refs/heads/master@{#406678}
> > 
> > I think we have to revert this CL. More details in the bug.
> 
> I'm ok with reverting this CL, but only if the hanging animation is *worse*
than
> having the renderer possibly crash on every scroll. In my mind that's true if
> (1) two-finger navigation swiping is very common (I don't know, though I'd be
> surprised if it's more common than scrolling), and (2) if the resulting hung
> animation leaves the user stuck, ie. they can't recover in some reasonable way
> like repeating the gesture.
> 
> Can you provide some more feedback?

Why is renderer crashing on zero velocity the right behavior? That really seems
like a poor choice to me. 

Floating point calculations are not exact and not-zero could still zero in some
innocent middle layer code.

[wjmaclean, 2016-07-21 20:40:41]

On 2016/07/21 20:26:51, spang wrote:
> On 2016/07/21 20:16:18, wjmaclean wrote:
> > On 2016/07/21 19:55:31, khmel wrote:
> > > On 2016/07/20 21:42:45, commit-bot: I haz the power wrote:
> > > > Patchset 1 (id:??) landed as
> > > > https://crrev.com/e80690ff28bf6db9392ab9b96a31105e1a6d4205
> > > > Cr-Commit-Position: refs/heads/master@{#406678}
> > > 
> > > I think we have to revert this CL. More details in the bug.
> > 
> > I'm ok with reverting this CL, but only if the hanging animation is *worse*
> than
> > having the renderer possibly crash on every scroll. In my mind that's true
if
> > (1) two-finger navigation swiping is very common (I don't know, though I'd
be
> > surprised if it's more common than scrolling), and (2) if the resulting hung
> > animation leaves the user stuck, ie. they can't recover in some reasonable
way
> > like repeating the gesture.
> > 
> > Can you provide some more feedback?
> 
> Why is renderer crashing on zero velocity the right behavior? That really
seems
> like a poor choice to me. 
> 
> Floating point calculations are not exact and not-zero could still zero in
some
> innocent middle layer code.

The renderer code creates a standard curve with amplitude scaling ... in order
to do the scaling it needs to invert the initial velocity.

So far as I know, the renderer never alters the velocity value, rather it just
passes it along and then reads it when creating the curve, so I don't see much
opportunity to have it go from non-zero to zero after it arrives in the
renderer.

Ideally, and this has always been the case, is that for touch-screen based
flings we just don't pass zero-velocity flings to the renderer. It seems also
reasonable that we should try and do similar things for the touchpad as well.

Two-finger navigation swipe works for touchscreen as well, and to the best of my
knowledge we don't send zero-velocity flings there. And I cannot get the
animation to hang for touchscreen with this change in place. (I've now been able
to reproduce the touchpad hang, but it's easily recovered by just putting the
fingers down and finishing the swipe.) Why aren't the two the same?

[tdresser, 2016-07-21 20:45:29]

On 2016/07/21 20:40:41, wjmaclean wrote:
> On 2016/07/21 20:26:51, spang wrote:
> > On 2016/07/21 20:16:18, wjmaclean wrote:
> > > On 2016/07/21 19:55:31, khmel wrote:
> > > > On 2016/07/20 21:42:45, commit-bot: I haz the power wrote:
> > > > > Patchset 1 (id:??) landed as
> > > > > https://crrev.com/e80690ff28bf6db9392ab9b96a31105e1a6d4205
> > > > > Cr-Commit-Position: refs/heads/master@{#406678}
> > > > 
> > > > I think we have to revert this CL. More details in the bug.
> > > 
> > > I'm ok with reverting this CL, but only if the hanging animation is
*worse*
> > than
> > > having the renderer possibly crash on every scroll. In my mind that's true
> if
> > > (1) two-finger navigation swiping is very common (I don't know, though I'd
> be
> > > surprised if it's more common than scrolling), and (2) if the resulting
hung
> > > animation leaves the user stuck, ie. they can't recover in some reasonable
> way
> > > like repeating the gesture.
> > > 
> > > Can you provide some more feedback?
> > 
> > Why is renderer crashing on zero velocity the right behavior? That really
> seems
> > like a poor choice to me. 
> > 
> > Floating point calculations are not exact and not-zero could still zero in
> some
> > innocent middle layer code.
> 
> The renderer code creates a standard curve with amplitude scaling ... in order
> to do the scaling it needs to invert the initial velocity.
> 
> So far as I know, the renderer never alters the velocity value, rather it just
> passes it along and then reads it when creating the curve, so I don't see much
> opportunity to have it go from non-zero to zero after it arrives in the
> renderer.
> 
> Ideally, and this has always been the case, is that for touch-screen based
> flings we just don't pass zero-velocity flings to the renderer. It seems also
> reasonable that we should try and do similar things for the touchpad as well.
> 
> Two-finger navigation swipe works for touchscreen as well, and to the best of
my
> knowledge we don't send zero-velocity flings there. And I cannot get the
> animation to hang for touchscreen with this change in place. (I've now been
able
> to reproduce the touchpad hang, but it's easily recovered by just putting the
> fingers down and finishing the swipe.) Why aren't the two the same?

I think the two are different because for touch we get events when the fingers
are lifted, but we don't for touchpad (or at least they aren't plumbed through
all the way).