Scroll offset animation curve retargeting.

Scroll offset animation curve retargeting.

Adds support for updating the target of an in-progress input-triggered impl-side scroll offset animation, keeping the velocity function continuous.

BUG=575
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284228

[danakj, 2014-07-16 18:19:24]

At what point do we rethink scrolling animation and just implement an animation
type with a callback that lets the embedder apply any transform it wants during
each tick of the animation instead of adding more and more scrolling-specific
stuff to cc?

[skobes, 2014-07-16 20:00:34]

On 2014/07/16 18:19:24, danakj wrote:
> At what point do we rethink scrolling animation and just implement an
animation
> type with a callback that lets the embedder apply any transform it wants
during
> each tick of the animation instead of adding more and more scrolling-specific
> stuff to cc?

That seems out of scope for this change but it's a good question, I don't know
the answer.  Can you file a bug?

[danakj, 2014-07-16 20:05:42]

On Wed, Jul 16, 2014 at 4:00 PM, <skobes@chromium.org> wrote:

> On 2014/07/16 18:19:24, danakj wrote:
>
>> At what point do we rethink scrolling animation and just implement an
>>
> animation
>
>> type with a callback that lets the embedder apply any transform it wants
>>
> during
>
>> each tick of the animation instead of adding more and more
>> scrolling-specific
>> stuff to cc?
>>
>
> That seems out of scope for this change but it's a good question, I don't
> know
> the answer.  Can you file a bug?
>

Filed 394485.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ajuma, 2014-07-16 20:35:23]

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

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:20: static float
maxDelta(gfx::Vector2dF delta) {
MaximumDimension? Or InfinityNorm maybe?

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:24: static base::TimeDelta
durationFromDelta(gfx::Vector2dF delta) {
Nit: function names should start with a capital letter

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:35: static
scoped_ptr<TimingFunction> easeOutWithInitialVelocity(double velocity) {
EaseOutWithInitialVelocity

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:118: (maxDelta(old_delta) /
maxDelta(new_delta));
Please add a comment explaining why the new velocity is chosen this way.

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve_unittest.cc (right):

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve_unittest.cc:138: EXPECT_EQ(9900.0,
curve->GetValue(2.0).y());
Please add one more re-targeting here (to verify that an already-retargeted
curve gets re-targeted again correctly).

[skobes, 2014-07-16 21:10:49]

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

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:20: static float
maxDelta(gfx::Vector2dF delta) {
On 2014/07/16 20:35:23, ajuma wrote:
> MaximumDimension? Or InfinityNorm maybe?

Done.

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:24: static base::TimeDelta
durationFromDelta(gfx::Vector2dF delta) {
On 2014/07/16 20:35:23, ajuma wrote:
> Nit: function names should start with a capital letter

Done.

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:35: static
scoped_ptr<TimingFunction> easeOutWithInitialVelocity(double velocity) {
On 2014/07/16 20:35:23, ajuma wrote:
> EaseOutWithInitialVelocity

Done.

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:118: (maxDelta(old_delta) /
maxDelta(new_delta));
On 2014/07/16 20:35:23, ajuma wrote:
> Please add a comment explaining why the new velocity is chosen this way.

Done.

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve_unittest.cc (right):

https://codereview.chromium.org/393713002/diff/150001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve_unittest.cc:138: EXPECT_EQ(9900.0,
curve->GetValue(2.0).y());
On 2014/07/16 20:35:23, ajuma wrote:
> Please add one more re-targeting here (to verify that an already-retargeted
> curve gets re-targeted again correctly).

Done.

[ajuma, 2014-07-16 21:26:03]

Thanks, just one more thing (sorry for missing this the first time): please also
update the ScrollAnimated test in layer_tree_host_impl_unittest.cc to include
updating the target of an in-progress scroll.

[skobes, 2014-07-16 22:06:50]

On 2014/07/16 21:26:03, ajuma wrote:
> Thanks, just one more thing (sorry for missing this the first time): please
also
> update the ScrollAnimated test in layer_tree_host_impl_unittest.cc to include
> updating the target of an in-progress scroll.

Done.

[ajuma, 2014-07-16 23:05:04]

lgtm

[skobes, 2014-07-16 23:06:25]

Thanks!

@danakj, can you approve for ui/gfx?

[danakj, 2014-07-17 15:24:01]

rsLGTM

[skobes, 2014-07-18 00:20:38]

@ajuma: linux release bots hit a clang bug, which I filed as http://b/16376945. 
I've added a workaround in scroll_offset_animation_curve.cc, can you lgtm again?

[ajuma, 2014-07-18 01:18:52]

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

https://codereview.chromium.org/393713002/diff/210001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:28: #endif
Rather than adding this #if, let's just use the workaround approach
unconditionally for now (and add a brief comment explaining why we need this;
the http://b/ link on its own isn't really enough since that's not
publicly-accessible).

[skobes, 2014-07-18 16:48:03]

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

https://codereview.chromium.org/393713002/diff/210001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:28: #endif
On 2014/07/18 01:18:51, ajuma wrote:
> Rather than adding this #if, let's just use the workaround approach
> unconditionally for now (and add a brief comment explaining why we need this;
> the http://b/ link on its own isn't really enough since that's not
> publicly-accessible). 

The offending compiler optimization has been disabled in
http://crrev.com/393193003, so I've removed the workaround here.

[skobes, 2014-07-18 16:48:09]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-07-18 16:48:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skobes@chromium.org/393713002/230001

[commit-bot: I haz the power, 2014-07-18 20:24:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-18 22:37:35]

Change committed as 284228