Tweak the math in ScrollOffsetAnimationCurve::UpdateTarget.

Tweak the math in ScrollOffsetAnimationCurve::UpdateTarget.

The existing division-by-0 guard was not correct, since MaximumDimension can
return negative values.  This led to occasional wild jumps when scrolling up and
down quickly (because new_velocity had the wrong sign).

This patch addresses the sign issue and improves handling of cases where we have
a large current velocity and a small delta to the new target.

BUG=575
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/a78fdc918ae09ee12096d99dc75a42b949f6b5de
Cr-Commit-Position: refs/heads/master@{#367200}

[skobes, 2015-12-30 01:16:13]

Description was changed from

==========
Velocity-based duration bound.

BUG=
==========

to

==========
Velocity-based duration bound.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[skobes, 2015-12-30 01:26:14]

Description was changed from

==========
Velocity-based duration bound.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Tweak the math in ScrollOffsetAnimationCurve::UpdateTarget.

The existing division-by-0 guard was not correct, since MaximumDimension can
return negative values.  This led to wild jumps when scrolling up and down
quickly (because new_velocity had the wrong sign).

This patch addresses the sign issue and improves handling of cases where we have
a large current velocity and a small delta to the new target.
==========

[skobes, 2015-12-30 01:29:16]

Description was changed from

==========
Tweak the math in ScrollOffsetAnimationCurve::UpdateTarget.

The existing division-by-0 guard was not correct, since MaximumDimension can
return negative values.  This led to wild jumps when scrolling up and down
quickly (because new_velocity had the wrong sign).

This patch addresses the sign issue and improves handling of cases where we have
a large current velocity and a small delta to the new target.
==========

to

==========
Tweak the math in ScrollOffsetAnimationCurve::UpdateTarget.

The existing division-by-0 guard was not correct, since MaximumDimension can
return negative values.  This led to occasional wild jumps when scrolling up and
down quickly (because new_velocity had the wrong sign).

This patch addresses the sign issue and improves handling of cases where we have
a large current velocity and a small delta to the new target.
==========

[skobes, 2015-12-30 01:29:36]

Description was changed from

==========
Tweak the math in ScrollOffsetAnimationCurve::UpdateTarget.

The existing division-by-0 guard was not correct, since MaximumDimension can
return negative values.  This led to occasional wild jumps when scrolling up and
down quickly (because new_velocity had the wrong sign).

This patch addresses the sign issue and improves handling of cases where we have
a large current velocity and a small delta to the new target.
==========

to

==========
Tweak the math in ScrollOffsetAnimationCurve::UpdateTarget.

The existing division-by-0 guard was not correct, since MaximumDimension can
return negative values.  This led to occasional wild jumps when scrolling up and
down quickly (because new_velocity had the wrong sign).

This patch addresses the sign issue and improves handling of cases where we have
a large current velocity and a small delta to the new target.

BUG=575
==========

[skobes, 2015-12-30 01:31:26]

skobes@chromium.org changed reviewers:
+ ymalik@chromium.org

[ymalik, 2015-12-30 19:30:18]

lgtm with questions

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:150: old_delta_max_dimension *
2.5f;
I am trying to understand how this gives us the estimate mentioned in the
comment. Should this not simply be new_delta_max_dimension / old_velocity? What
are the other params for?

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:179: total_animation_duration_ =
base::TimeDelta::FromSecondsD(t);
I know that the premise of this condition is that we are either at or close to
the new target, but should it also update the last_retarget_ and others like at
the end of the function in case UpdateTarget gets called again in time <
epsilon?

[ymalik, 2015-12-30 19:30:18]

lgtm with questions

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:150: old_delta_max_dimension *
2.5f;
I am trying to understand how this gives us the estimate mentioned in the
comment. Should this not simply be new_delta_max_dimension / old_velocity? What
are the other params for?

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:179: total_animation_duration_ =
base::TimeDelta::FromSecondsD(t);
I know that the premise of this condition is that we are either at or close to
the new target, but should it also update the last_retarget_ and others like at
the end of the function in case UpdateTarget gets called again in time <
epsilon?

[skobes, 2015-12-30 19:44:20]

skobes@chromium.org changed reviewers:
+ ajuma@chromium.org

[skobes, 2015-12-30 19:44:20]

+ajuma for OWNERS

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:150: old_delta_max_dimension *
2.5f;
On 2015/12/30 19:30:18, ymalik1 wrote:
> I am trying to understand how this gives us the estimate mentioned in the
> comment. Should this not simply be new_delta_max_dimension / old_velocity?
What
> are the other params for? 

Sorry it's a little confusing.  The old_velocity is actually the slope on the
curve normalized to (0,0)-(1,1), so we have to multiply it by (old_delta /
old_duration) to get a "true" px/sec velocity.

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:179: total_animation_duration_ =
base::TimeDelta::FromSecondsD(t);
On 2015/12/30 19:30:18, ymalik1 wrote:
> I know that the premise of this condition is that we are either at or close to
> the new target, but should it also update the last_retarget_ and others like
at
> the end of the function in case UpdateTarget gets called again in time <
> epsilon?

We don't want to treat this as a new segment, since its delta and duration are
both near 0 and we can't make a meaningful timing function.  Instead we are
making the curve report that it has reached the end of the current segment.

[ajuma, 2015-12-30 19:58:23]

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:152: return bound < kEpsilon ?
std::numeric_limits<double>::infinity() : bound;
Should this be returning zero rather than infinity for tiny bounds? (Actually,
do we need a special case at all, given that the caller already handles the case
where the duration is smaller than epsilon?)

[skobes, 2015-12-30 20:06:24]

Description was changed from

==========
Tweak the math in ScrollOffsetAnimationCurve::UpdateTarget.

The existing division-by-0 guard was not correct, since MaximumDimension can
return negative values.  This led to occasional wild jumps when scrolling up and
down quickly (because new_velocity had the wrong sign).

This patch addresses the sign issue and improves handling of cases where we have
a large current velocity and a small delta to the new target.

BUG=575
==========

to

==========
Tweak the math in ScrollOffsetAnimationCurve::UpdateTarget.

The existing division-by-0 guard was not correct, since MaximumDimension can
return negative values.  This led to occasional wild jumps when scrolling up and
down quickly (because new_velocity had the wrong sign).

This patch addresses the sign issue and improves handling of cases where we have
a large current velocity and a small delta to the new target.

BUG=575
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[skobes, 2015-12-30 20:07:30]

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:152: return bound < kEpsilon ?
std::numeric_limits<double>::infinity() : bound;
On 2015/12/30 19:58:23, ajuma wrote:
> Should this be returning zero rather than infinity for tiny bounds? (Actually,
> do we need a special case at all, given that the caller already handles the
case
> where the duration is smaller than epsilon?)

The important thing is really to return infinity when bound is negative, meaning
the current velocity is in the wrong direction.

I think bound will never be (close to) 0 here, since we have already handled
std::abs(new_delta_max_dimension) < kEpsilon, and old_duration will not be 0.

I've updated the check to use 0 instead of kEpsilon for clarity, and added a
comment.

[ajuma, 2015-12-30 20:09:31]

On 2015/12/30 20:07:30, skobes wrote:
>
https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
> File cc/animation/scroll_offset_animation_curve.cc (right):
> 
>
https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
> cc/animation/scroll_offset_animation_curve.cc:152: return bound < kEpsilon ?
> std::numeric_limits<double>::infinity() : bound;
> On 2015/12/30 19:58:23, ajuma wrote:
> > Should this be returning zero rather than infinity for tiny bounds?
(Actually,
> > do we need a special case at all, given that the caller already handles the
> case
> > where the duration is smaller than epsilon?)
> 
> The important thing is really to return infinity when bound is negative,
meaning
> the current velocity is in the wrong direction.
> 
> I think bound will never be (close to) 0 here, since we have already handled
> std::abs(new_delta_max_dimension) < kEpsilon, and old_duration will not be 0.
> 
> I've updated the check to use 0 instead of kEpsilon for clarity, and added a
> comment.

Thanks, lgtm.

[skobes, 2015-12-30 20:09:43]

The CQ bit was checked by skobes@chromium.org

[skobes, 2015-12-30 20:09:44]

The patchset sent to the CQ was uploaded after l-g-t-m from ymalik@chromium.org
Link to the patchset: https://codereview.chromium.org/1553603003/#ps40001
(title: "address review comment")

[ymalik, 2015-12-30 20:10:00]

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:150: old_delta_max_dimension *
2.5f;
On 2015/12/30 19:44:20, skobes wrote:
> On 2015/12/30 19:30:18, ymalik1 wrote:
> > I am trying to understand how this gives us the estimate mentioned in the
> > comment. Should this not simply be new_delta_max_dimension / old_velocity?
> What
> > are the other params for? 
> 
> Sorry it's a little confusing.  The old_velocity is actually the slope on the
> curve normalized to (0,0)-(1,1), so we have to multiply it by (old_delta /
> old_duration) to get a "true" px/sec velocity.

Ah I see. Perhaps putting it in parentheses would make that more clear?
new_delta / (old_velocity * old_delta / old_duration). 
Or maybe renaming old_velocity = normalized_velocity * old_delta / old_duration
to make that even more clear. 

Up to you though :)

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:179: total_animation_duration_ =
base::TimeDelta::FromSecondsD(t);
On 2015/12/30 19:44:20, skobes wrote:
> On 2015/12/30 19:30:18, ymalik1 wrote:
> > I know that the premise of this condition is that we are either at or close
to
> > the new target, but should it also update the last_retarget_ and others like
> at
> > the end of the function in case UpdateTarget gets called again in time <
> > epsilon?
> 
> We don't want to treat this as a new segment, since its delta and duration are
> both near 0 and we can't make a meaningful timing function.  Instead we are
> making the curve report that it has reached the end of the current segment.

Makes sense. Thanks!

[commit-bot: I haz the power, 2015-12-30 20:10:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1553603003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1553603003/40001

[skobes, 2015-12-30 20:10:35]

The CQ bit was unchecked by skobes@chromium.org

[skobes, 2015-12-30 20:19:43]

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/1553603003/diff/20001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:150: old_delta_max_dimension *
2.5f;
On 2015/12/30 20:09:59, ymalik1 wrote:
> On 2015/12/30 19:44:20, skobes wrote:
> > On 2015/12/30 19:30:18, ymalik1 wrote:
> > > I am trying to understand how this gives us the estimate mentioned in the
> > > comment. Should this not simply be new_delta_max_dimension / old_velocity?
> > What
> > > are the other params for? 
> > 
> > Sorry it's a little confusing.  The old_velocity is actually the slope on
the
> > curve normalized to (0,0)-(1,1), so we have to multiply it by (old_delta /
> > old_duration) to get a "true" px/sec velocity.
> 
> Ah I see. Perhaps putting it in parentheses would make that more clear?
> new_delta / (old_velocity * old_delta / old_duration). 
> Or maybe renaming old_velocity = normalized_velocity * old_delta /
old_duration
> to make that even more clear. 
> 
> Up to you though :)

Done.

[skobes, 2015-12-30 20:19:46]

The CQ bit was checked by skobes@chromium.org

[skobes, 2015-12-30 20:19:46]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
ymalik@chromium.org
Link to the patchset: https://codereview.chromium.org/1553603003/#ps60001
(title: "address ymalik's comment")

[commit-bot: I haz the power, 2015-12-30 20:20:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1553603003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1553603003/60001

[commit-bot: I haz the power, 2015-12-30 21:56:34]

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

[commit-bot: I haz the power, 2015-12-30 21:56:35]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[skobes, 2015-12-30 22:02:18]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2015-12-30 22:02:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1553603003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1553603003/60001

[commit-bot: I haz the power, 2015-12-30 23:03:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-30 23:04:33]

Description was changed from

==========
Tweak the math in ScrollOffsetAnimationCurve::UpdateTarget.

The existing division-by-0 guard was not correct, since MaximumDimension can
return negative values.  This led to occasional wild jumps when scrolling up and
down quickly (because new_velocity had the wrong sign).

This patch addresses the sign issue and improves handling of cases where we have
a large current velocity and a small delta to the new target.

BUG=575
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Tweak the math in ScrollOffsetAnimationCurve::UpdateTarget.

The existing division-by-0 guard was not correct, since MaximumDimension can
return negative values.  This led to occasional wild jumps when scrolling up and
down quickly (because new_velocity had the wrong sign).

This patch addresses the sign issue and improves handling of cases where we have
a large current velocity and a small delta to the new target.

BUG=575
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2015-12-30 23:04:34]

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