Take MT jank into account when animating the scroll offset on CC

Take MT jank into account when animating the scroll offset on CC

This CL calculates the jank by subtracting base::TimeTicks::Now from
the event platform time when GestureScrollUpdate is called.

BUG=616995
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[ymalik, 2016-06-03 20:54:18]

Description was changed from

==========
[WIP]Take MT jank into account when animating the scroll offset on CC

BUG=616995
==========

to

==========
[WIP]Take MT jank into account when animating the scroll offset on CC

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

[skobes, 2016-06-08 22:47:27]

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

[skobes, 2016-06-08 22:47:28]

https://codereview.chromium.org/2040543002/diff/1/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2040543002/diff/1/cc/input/input_handler.h#ne...
cc/input/input_handler.h:135: virtual void AdjustDuration(base::TimeDelta) = 0;
I don't think we should ever adjust the duration after the fact. We should just
set it correctly in the first place.

https://codereview.chromium.org/2040543002/diff/1/ui/events/blink/input_handl...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2040543002/diff/1/ui/events/blink/input_handl...
ui/events/blink/input_handler_proxy.cc:673: lc.second.event_time -
o_component.event_time);
I don't really understand what this is doing, is it equivalent to using
WebInputEvent::timeStampSeconds?

[ymalik, 2016-06-21 23:42:47]

Description was changed from

==========
[WIP]Take MT jank into account when animating the scroll offset on CC

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

to

==========
Take MT jank into account when animating the scroll offset on CC

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

[ymalik, 2016-06-21 23:46:02]

@skobes, can you please do an initial round of review (and play with it) before
I write tests and update a bunch of unit tests due to the plumbing work? 

I am basically pluming the original event time from LatencyInfo to where we
create the animation and subtract it from ::Now to get the jank. Note that this
platform time may be a little inaccurate on some platforms (to an upperbound of
16ms), which is probably okay?

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

https://codereview.chromium.org/2040543002/diff/40001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:76: return base::TimeDelta();
@skobes, If the animation duration is < 83ms, this will just do an instant
scroll. I picked 5 as the min duration constant as a starter, reasonable?

https://codereview.chromium.org/2040543002/diff/40001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:224: 
Reasoning: I think updating an animation which had a small duration will have a
really high velocity match anyway, so might as well just do this whole animation
instantly? Is this reasonable?

[ymalik, 2016-06-21 23:48:24]

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

[ymalik, 2016-06-21 23:48:24]

+tdresser to comment on the use of
LatencyComponentType::INPUT_EVENT_LATENCY_ORIGINAL_COMPONENT and subtracting it
from base::TimeTicks::Now to calculate jank.

[skobes, 2016-06-23 01:33:42]

I tried to verify this change with a test page at
https://output.jsbin.com/deheze/quiet but something weird is going on.  Wheel
events that come in while the main thread is janked seem to (sometimes but not
always) produce immediate scrolls instead of smooth scrolls after the jank ends,
even though we assign a non-zero duration in
ScrollOffsetAnimationCurve::SetInitialValue.

This isn't caused by your change, but it prevents me from testing it properly -
when the jank adjustment reduces the duration by any significant amount, the
scroll seems to be instant (instead of merely sped up).  SegmentDuration returns
non-zero values, so it's not coming from your kMinDuration check.

We should get a better understanding of what's going on before landing this.

[skobes, 2016-06-23 01:53:32]

On 2016/06/23 01:33:42, skobes wrote:
> I tried to verify this change with a test page at
> https://output.jsbin.com/deheze/quiet but something weird is going on.  Wheel
> events that come in while the main thread is janked seem to (sometimes but not
> always) produce immediate scrolls instead of smooth scrolls after the jank
ends,
> even though we assign a non-zero duration in
> ScrollOffsetAnimationCurve::SetInitialValue.
> 
> This isn't caused by your change, but it prevents me from testing it properly
-
> when the jank adjustment reduces the duration by any significant amount, the
> scroll seems to be instant (instead of merely sped up).  SegmentDuration
returns
> non-zero values, so it's not coming from your kMinDuration check.
> 
> We should get a better understanding of what's going on before landing this.

(Filed crbug.com/622553 for this.)

[ymalik, 2016-08-24 19:24:48]

Description was changed from

==========
Take MT jank into account when animating the scroll offset on CC

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

to

==========
Take MT jank into account when animating the scroll offset on CC

This CL calculates the jank my subtracting base::TimeTicks::Now from  the event
platform time when GestureScrollUpdate is called.

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

[ymalik, 2016-08-24 19:25:09]

@skobes, @tdresser, PTAL :)

[ymalik, 2016-08-24 19:25:50]

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

[tdresser, 2016-08-25 15:55:54]

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:65: base::TimeDelta td =
base::TimeDelta::FromMicroseconds(
td -> time_delta

https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
'Give as descriptive a name as possible, within reason. Do not worry about
saving horizontal space as it is far more important to make your code
immediately understandable by a new reader. Do not use abbreviations that are
ambiguous or unfamiliar to readers outside your project, and do not abbreviate
by deleting letters within a word.'

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:69: if (td >= base::TimeDelta())
operator> is comparing unsigned integers, so this is always true...

Does
operator-(https://cs.chromium.org/chromium/src/base/time/time.h?rcl=1472109603&l=185)
do what you think it does?

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:131: if (has_set_initial_value_ &&
duration == base::TimeDelta())
duration_.is_zero()

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:209: if (t == 0) {
Should this be combined with the clause above?

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animations_impl.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animations_impl.cc:99: if (delayed_by >
base::TimeDelta())
!delayed_by.is_zero()

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animations_impl.cc:100: trimmed = delayed_by <
trimmed ? trimmed - delayed_by : base::TimeDelta();
I'm not completely clear on what's going on here.

We're trimming time off the front?

I'm not sure how overflow works with TimeDeltas, but we should be sure this
makes sense.

https://codereview.chromium.org/2040543002/diff/140001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/input/input_handler...
cc/input/input_handler.h:133: // the duration of the animation.
More importantly, it determines the start time of the animation, correct?

https://codereview.chromium.org/2040543002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:10063:
base::TimeDelta::FromMilliseconds(100))
Comment on added delay.

https://codereview.chromium.org/2040543002/diff/140001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy.cc:645:
base::TimeDelta::FromSecondsD(gesture_event.timeStampSeconds)));
Math seems correct, though I might add a temporary here to make this prettier.

[tdresser, 2016-08-29 14:31:33]

Sorry, I misread the TimeDelta class...

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:69: if (td >= base::TimeDelta())
On 2016/08/25 15:55:54, tdresser wrote:
> operator> is comparing unsigned integers, so this is always true...
> 
> Does
>
operator-(https://cs.chromium.org/chromium/src/base/time/time.h?rcl=1472109603&l=185)
> do what you think it does?

Whoops, nevermind, this looks fine.

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animations_impl.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animations_impl.cc:100: trimmed = delayed_by <
trimmed ? trimmed - delayed_by : base::TimeDelta();
On 2016/08/25 15:55:54, tdresser wrote:
> I'm not completely clear on what's going on here.
> 
> We're trimming time off the front?
> 
> I'm not sure how overflow works with TimeDeltas, but we should be sure this
> makes sense.

Subtraction is just saturated. This seems reasonable.

[ymalik, 2016-08-29 14:52:06]

@tdresser, PTAL :)

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:65: base::TimeDelta td =
base::TimeDelta::FromMicroseconds(
On 2016/08/25 15:55:54, tdresser wrote:
> td -> time_delta
> 
> https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
> 'Give as descriptive a name as possible, within reason. Do not worry about
> saving horizontal space as it is far more important to make your code
> immediately understandable by a new reader. Do not use abbreviations that are
> ambiguous or unfamiliar to readers outside your project, and do not abbreviate
> by deleting letters within a word.'

Great, thanks!

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:69: if (td >= base::TimeDelta())
On 2016/08/29 14:31:33, tdresser wrote:
> On 2016/08/25 15:55:54, tdresser wrote:
> > operator> is comparing unsigned integers, so this is always true...
> > 
> > Does
> >
>
operator-(https://cs.chromium.org/chromium/src/base/time/time.h?rcl=1472109603&l=185)
> > do what you think it does?
> 
> Whoops, nevermind, this looks fine.

Acknowledged.

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:131: if (has_set_initial_value_ &&
duration == base::TimeDelta())
On 2016/08/25 15:55:54, tdresser wrote:
> duration_.is_zero()

Done.

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:209: if (t == 0) {
On 2016/08/25 15:55:54, tdresser wrote:
> Should this be combined with the clause above?

I think this is more clear in communicating that this case is for when the
animation hasn't started yet, as opposed to the one above, where the new target
very small. WDYT?

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animations_impl.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animations_impl.cc:99: if (delayed_by >
base::TimeDelta())
On 2016/08/25 15:55:54, tdresser wrote:
> !delayed_by.is_zero()
Covered this on chat, but for posterity:
delayed_by is a TimeDelta and can be negative, so this prevents against  
non-sane values.

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animations_impl.cc:100: trimmed = delayed_by <
trimmed ? trimmed - delayed_by : base::TimeDelta();
On 2016/08/29 14:31:33, tdresser wrote:
> On 2016/08/25 15:55:54, tdresser wrote:
> > I'm not completely clear on what's going on here.
> > 
> > We're trimming time off the front?
> > 
> > I'm not sure how overflow works with TimeDeltas, but we should be sure this
> > makes sense.
> 
> Subtraction is just saturated. This seems reasonable.

Yes. We're just trying to use the delay as a signal that this should have really
been re-targeted delayed_by ago.

https://codereview.chromium.org/2040543002/diff/140001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/input/input_handler...
cc/input/input_handler.h:133: // the duration of the animation.
On 2016/08/25 15:55:54, tdresser wrote:
> More importantly, it determines the start time of the animation, correct?

Not quite, since the animation will still start at the same time that it would
have without this delay. It just finishes faster because the duration of the
animation is adjusted by "delayed_by". Does that make sense?

https://codereview.chromium.org/2040543002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:10063:
base::TimeDelta::FromMilliseconds(100))
On 2016/08/25 15:55:54, tdresser wrote:
> Comment on added delay.

Done.

https://codereview.chromium.org/2040543002/diff/140001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy.cc:645:
base::TimeDelta::FromSecondsD(gesture_event.timeStampSeconds)));
On 2016/08/25 15:55:54, tdresser wrote:
> Math seems correct, though I might add a temporary here to make this prettier.

Good tip, looks much prettier!

[ymalik, 2016-08-29 14:52:06]

@tdresser, PTAL :)

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:65: base::TimeDelta td =
base::TimeDelta::FromMicroseconds(
On 2016/08/25 15:55:54, tdresser wrote:
> td -> time_delta
> 
> https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
> 'Give as descriptive a name as possible, within reason. Do not worry about
> saving horizontal space as it is far more important to make your code
> immediately understandable by a new reader. Do not use abbreviations that are
> ambiguous or unfamiliar to readers outside your project, and do not abbreviate
> by deleting letters within a word.'

Great, thanks!

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:69: if (td >= base::TimeDelta())
On 2016/08/29 14:31:33, tdresser wrote:
> On 2016/08/25 15:55:54, tdresser wrote:
> > operator> is comparing unsigned integers, so this is always true...
> > 
> > Does
> >
>
operator-(https://cs.chromium.org/chromium/src/base/time/time.h?rcl=1472109603&l=185)
> > do what you think it does?
> 
> Whoops, nevermind, this looks fine.

Acknowledged.

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:131: if (has_set_initial_value_ &&
duration == base::TimeDelta())
On 2016/08/25 15:55:54, tdresser wrote:
> duration_.is_zero()

Done.

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:209: if (t == 0) {
On 2016/08/25 15:55:54, tdresser wrote:
> Should this be combined with the clause above?

I think this is more clear in communicating that this case is for when the
animation hasn't started yet, as opposed to the one above, where the new target
very small. WDYT?

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animations_impl.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animations_impl.cc:99: if (delayed_by >
base::TimeDelta())
On 2016/08/25 15:55:54, tdresser wrote:
> !delayed_by.is_zero()
Covered this on chat, but for posterity:
delayed_by is a TimeDelta and can be negative, so this prevents against  
non-sane values.

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animations_impl.cc:100: trimmed = delayed_by <
trimmed ? trimmed - delayed_by : base::TimeDelta();
On 2016/08/29 14:31:33, tdresser wrote:
> On 2016/08/25 15:55:54, tdresser wrote:
> > I'm not completely clear on what's going on here.
> > 
> > We're trimming time off the front?
> > 
> > I'm not sure how overflow works with TimeDeltas, but we should be sure this
> > makes sense.
> 
> Subtraction is just saturated. This seems reasonable.

Yes. We're just trying to use the delay as a signal that this should have really
been re-targeted delayed_by ago.

https://codereview.chromium.org/2040543002/diff/140001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/input/input_handler...
cc/input/input_handler.h:133: // the duration of the animation.
On 2016/08/25 15:55:54, tdresser wrote:
> More importantly, it determines the start time of the animation, correct?

Not quite, since the animation will still start at the same time that it would
have without this delay. It just finishes faster because the duration of the
animation is adjusted by "delayed_by". Does that make sense?

https://codereview.chromium.org/2040543002/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:10063:
base::TimeDelta::FromMilliseconds(100))
On 2016/08/25 15:55:54, tdresser wrote:
> Comment on added delay.

Done.

https://codereview.chromium.org/2040543002/diff/140001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy.cc:645:
base::TimeDelta::FromSecondsD(gesture_event.timeStampSeconds)));
On 2016/08/25 15:55:54, tdresser wrote:
> Math seems correct, though I might add a temporary here to make this prettier.

Good tip, looks much prettier!

[tdresser, 2016-08-29 15:28:28]

LGTM.

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/2040543002/diff/140001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:209: if (t == 0) {
On 2016/08/29 14:52:05, ymalik wrote:
> On 2016/08/25 15:55:54, tdresser wrote:
> > Should this be combined with the clause above?
> 
> I think this is more clear in communicating that this case is for when the
> animation hasn't started yet, as opposed to the one above, where the new
target
> very small. WDYT?

Acknowledged.

https://codereview.chromium.org/2040543002/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2040543002/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.h:175: base::TimeDelta delay_in_second =
base::TimeDelta()) override;
delayed_by?

[ajuma, 2016-08-29 21:48:25]

Description was changed from

==========
Take MT jank into account when animating the scroll offset on CC

This CL calculates the jank my subtracting base::TimeTicks::Now from  the event
platform time when GestureScrollUpdate is called.

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

to

==========
Take MT jank into account when animating the scroll offset on CC

This CL calculates the jank by subtracting base::TimeTicks::Now from
the event platform time when GestureScrollUpdate is called.

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

[ajuma, 2016-08-29 22:01:07]

lgtm too

[skobes, 2016-08-29 22:13:12]

https://codereview.chromium.org/2040543002/diff/160001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/2040543002/diff/160001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:131: if (has_set_initial_value_ &&
duration.is_zero())
Does it ever make sense to call GetValue if has_set_initial_value_ is false? 
Maybe that should be an assertion?

https://codereview.chromium.org/2040543002/diff/160001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:210: // Just update the target
since the animation hasn't started yet.
Remind me why the early return is here?  Shouldn't we recompute the duration if
the target has changed?

[ymalik, 2016-08-29 23:44:06]

@skobes, PTAL :)

https://codereview.chromium.org/2040543002/diff/160001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/2040543002/diff/160001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:131: if (has_set_initial_value_ &&
duration.is_zero())
On 2016/08/29 22:13:11, skobes wrote:
> Does it ever make sense to call GetValue if has_set_initial_value_ is false? 
> Maybe that should be an assertion?

You're right, we never call GetValue without calling SetInitialValue.

https://codereview.chromium.org/2040543002/diff/160001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:210: // Just update the target
since the animation hasn't started yet.
On 2016/08/29 22:13:11, skobes wrote:
> Remind me why the early return is here?  Shouldn't we recompute the duration
if
> the target has changed?

This is an optimization for the case where we get an UpdateTarget call right
after we create the animation. Because the animation hasn't started yet, we can
early out by just updating the target.

https://codereview.chromium.org/2040543002/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2040543002/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.h:175: base::TimeDelta delay_in_second =
base::TimeDelta()) override;
On 2016/08/29 15:28:28, tdresser wrote:
> delayed_by?

Thanks!

[skobes, 2016-08-29 23:49:44]

https://codereview.chromium.org/2040543002/diff/160001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/2040543002/diff/160001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:210: // Just update the target
since the animation hasn't started yet.
On 2016/08/29 23:44:05, ymalik wrote:
> This is an optimization for the case where we get an UpdateTarget call right
> after we create the animation. Because the animation hasn't started yet, we
can
> early out by just updating the target.

But the duration depends on the target, so this seems wrong.

[ymalik, 2016-09-01 15:31:13]

@skobes, PTAL :)

https://codereview.chromium.org/2040543002/diff/160001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/2040543002/diff/160001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:210: // Just update the target
since the animation hasn't started yet.
On 2016/08/29 23:49:43, skobes wrote:
> On 2016/08/29 23:44:05, ymalik wrote:
> > This is an optimization for the case where we get an UpdateTarget call right
> > after we create the animation. Because the animation hasn't started yet, we
> can
> > early out by just updating the target.
> 
> But the duration depends on the target, so this seems wrong.

Hmm yeah you're right. Fixed.

[skobes, 2016-09-01 18:20:09]

lgtm

[ymalik, 2016-09-01 18:22:13]

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

[commit-bot: I haz the power, 2016-09-01 18:22:29]

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-09-01 18:22:31]

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

[commit-bot: I haz the power, 2016-09-01 18:22:32]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[ymalik, 2016-09-01 18:24:18]

Description was changed from

==========
Take MT jank into account when animating the scroll offset on CC

This CL calculates the jank by subtracting base::TimeTicks::Now from
the event platform time when GestureScrollUpdate is called.

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

to

==========
Take MT jank into account when animating the scroll offset on CC

This CL calculates the jank by subtracting base::TimeTicks::Now from
the event platform time when GestureScrollUpdate is called.

BUG=616995
==========

[ymalik, 2016-09-01 18:24:26]

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

[commit-bot: I haz the power, 2016-09-01 18:25:00]

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-09-01 19:15:39]

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

[commit-bot: I haz the power, 2016-09-01 19:15:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ymalik, 2016-09-01 21:56:42]

Description was changed from

==========
Take MT jank into account when animating the scroll offset on CC

This CL calculates the jank by subtracting base::TimeTicks::Now from
the event platform time when GestureScrollUpdate is called.

BUG=616995
==========

to

==========
Take MT jank into account when animating the scroll offset on CC

This CL calculates the jank by subtracting base::TimeTicks::Now from
the event platform time when GestureScrollUpdate is called.

BUG=616995
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[ymalik, 2016-09-01 22:04:39]

@skobes, PTAL

I had missed two cases. The first is that give that we are re-targeting back in
time, it could be that we're trying to re-target before "last_retarget_". We
should probably clamp to the max of the two?

Second, if the duration of the animation is 0, it probably makes sense to keep
the duration 0 when UpdateTarget is called, because it doesn't make sense to do
all the velocity calculations in UpdateTarget for that case.

WDYT?

[ymalik, 2016-09-01 22:06:10]

https://codereview.chromium.org/2040543002/diff/240001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/2040543002/diff/240001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:219: (t -
last_retarget_.InSecondsF()) / old_duration);
The Velocity function expects a non-negative value, and t - last_target_ may not
be non-negative if we don't clamp above.

[skobes, 2016-09-01 23:05:26]

On 2016/09/01 22:04:39, ymalik wrote:
> @skobes, PTAL
> 
> I had missed two cases. The first is that give that we are re-targeting back
in
> time, it could be that we're trying to re-target before "last_retarget_". We
> should probably clamp to the max of the two?
> 
> Second, if the duration of the animation is 0, it probably makes sense to keep
> the duration 0 when UpdateTarget is called, because it doesn't make sense to
do
> all the velocity calculations in UpdateTarget for that case.
> 
> WDYT?

It makes sense for UpdateTarget to clamp t to the range [last_retarget_,
total_animation_duration_].

If t is 0 (after this clamping), it makes sense for UpdateTarget to skip the
velocity calculations, but in that case it should set total_animation_duration_
to the output of SegmentDuration, similar to what SetInitialValue does.

How does that sound?

[skobes, 2016-09-01 23:10:59]

On 2016/09/01 23:05:26, skobes wrote:
> If t is 0 (after this clamping), it makes sense for UpdateTarget to skip the
> velocity calculations, but in that case it should set
total_animation_duration_
> to the output of SegmentDuration, similar to what SetInitialValue does.

Wait, maybe I am wrong here.  If we have queued up a bunch of wheel events near
the start of a big jank, and the first one creates an animation with a duration
of 0 (due to the jank adjustment), will the second event call UpdateTarget and
reset the duration as if it had come in at the end of the jank?

If so, ignore what I said and do what you said. :)

[ymalik, 2016-09-01 23:20:50]

On 2016/09/01 23:10:59, skobes wrote:
> On 2016/09/01 23:05:26, skobes wrote:
> > If t is 0 (after this clamping), it makes sense for UpdateTarget to skip the
> > velocity calculations, but in that case it should set
> total_animation_duration_
> > to the output of SegmentDuration, similar to what SetInitialValue does.
> 
> Wait, maybe I am wrong here.  If we have queued up a bunch of wheel events
near
> the start of a big jank, and the first one creates an animation with a
duration
> of 0 (due to the jank adjustment), will the second event call UpdateTarget and
> reset the duration as if it had come in at the end of the jank?
> 
> If so, ignore what I said and do what you said. :)

Yes that's correct. The call to UpdateTarget will happen right away and if we go
with what you suggested, we will do a smooth scroll as if it had come in at the
end of the jank.

I don't think t will ever be > total_animation_duration_.

Does this patch still LGTY? :)

[skobes, 2016-09-01 23:38:56]

On 2016/09/01 23:20:50, ymalik wrote:
> Does this patch still LGTY? :)

Something feels wrong about preserving a total duration of 0.

If UpdateTarget skips the velocity calculations, the correct duration should be
(current frame time - animation start time - jank adjustment + delta based
segment duration), or 0 if that value is less than 0.

I think that means ScrollOffsetAnimationsImpl::ScrollAnimationUpdateTarget needs
to subtract delayed_by even if the result is < 0.  Then UpdateTarget can compute
the correct duration with t + SegmentDuration(...) when
total_animation_duration_ is 0.

[ymalik, 2016-09-02 01:09:12]

On 2016/09/01 23:38:56, skobes wrote:
> On 2016/09/01 23:20:50, ymalik wrote:
> > Does this patch still LGTY? :)
> 
> Something feels wrong about preserving a total duration of 0.
> 
> If UpdateTarget skips the velocity calculations, the correct duration should
be
> (current frame time - animation start time - jank adjustment + delta based
> segment duration), or 0 if that value is less than 0.
> 
> I think that means ScrollOffsetAnimationsImpl::ScrollAnimationUpdateTarget
needs
> to subtract delayed_by even if the result is < 0.  Then UpdateTarget can
compute
> the correct duration with t + SegmentDuration(...) when
> total_animation_duration_ is 0.

This makes sense! PTAL.

Thanks for the back and fourth on this! I will update the tests once you stamp
the logic :)

[skobes, 2016-09-02 01:27:56]

Sorry, just one more tweak that's occurred to me. Thanks for your patience. :)

https://codereview.chromium.org/2040543002/diff/260001/cc/animation/scroll_of...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/2040543002/diff/260001/cc/animation/scroll_of...
cc/animation/scroll_offset_animation_curve.cc:221: t = std::max(t,
last_retarget_.InSecondsF());
Any change in t produced by this line represents jank that we're failing to
account for.  Could we save the difference and pass it as delayed_by to
SegmentDuration on the line that assigns new_duration?

Once that's in place, I think we have a guaranteed invariant that we will never
schedule an animation to end later than the event's platform timestamp + the
"ideal" duration, which is pretty cool.

[ymalik, 2016-09-02 14:50:30]

On 2016/09/02 01:27:56, skobes wrote:
> Sorry, just one more tweak that's occurred to me. Thanks for your patience. :)
> 
>
https://codereview.chromium.org/2040543002/diff/260001/cc/animation/scroll_of...
> File cc/animation/scroll_offset_animation_curve.cc (right):
> 
>
https://codereview.chromium.org/2040543002/diff/260001/cc/animation/scroll_of...
> cc/animation/scroll_offset_animation_curve.cc:221: t = std::max(t,
> last_retarget_.InSecondsF());
> Any change in t produced by this line represents jank that we're failing to
> account for.  Could we save the difference and pass it as delayed_by to
> SegmentDuration on the line that assigns new_duration?
> 
> Once that's in place, I think we have a guaranteed invariant that we will
never
> schedule an animation to end later than the event's platform timestamp + the
> "ideal" duration, which is pretty cool.

I agree with that tweak. Thanks! PTAL :)

[skobes, 2016-09-02 16:34:21]

lgtm

[ymalik, 2016-09-05 17:35:54]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-09-05 17:35:55]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2040543002/#ps280001
(title: "add test + apply suggested improvement")

[commit-bot: I haz the power, 2016-09-05 17:36:06]

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-09-05 18:40:55]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-09-05 18:42:09]

Description was changed from

==========
Take MT jank into account when animating the scroll offset on CC

This CL calculates the jank by subtracting base::TimeTicks::Now from
the event platform time when GestureScrollUpdate is called.

BUG=616995
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Take MT jank into account when animating the scroll offset on CC

This CL calculates the jank by subtracting base::TimeTicks::Now from
the event platform time when GestureScrollUpdate is called.

BUG=616995
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[commit-bot: I haz the power, 2016-09-05 18:42:12]

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