[Android] Port Scroller.java to C++ and use for fling animations

[Android] Port Scroller.java to C++ and use for fling animations

Port the Android platform's Scroller.java to C++, and wire the code up to
FlingAnimatorImpl. This has several benefits, including:

1) JNI hop and garbage elimination.
2) Fling updates in DIPs.
3) Animation timing independent of the Android view system.
4) Easy access to X and Y velocity components.

This may also prove useful for non-fling related scroll animations, e.g., smooth
scrolling from mousewheel and/or keyboard inputs on the compositor thread.

BUG=344370
TBR=jochen@chromium.org

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

[jdduke (slow), 2014-02-21 00:21:47]

aelias@: PTAL.  If you think these sensible, I'll get owners for ui/gfx and
webkit/.  Thanks.

[aelias_OOO_until_Jul13, 2014-02-21 02:42:35]

Broadly lgtm.

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller.cc
File ui/gfx/android/scroller.cc (right):

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.cc:14: const base::TimeDelta kDefaultDuration =
base::TimeDelta::FromMilliseconds(250);
Hmm, I'm concerned these will end up as static initializers which are
discouraged.  Could you make this and the deceleration rate member variables
initialized in the constructor?

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.cc:21: const float kEpsilon = 1e-5;
1e-5f instead?

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.cc:72: if (std::abs(tx - alpha) < 1E-5)
kEpsilon?

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.cc:87: if (std::abs(dy - alpha) < 1E-5)
kEpsilon?

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller.h
File ui/gfx/android/scroller.h (right):

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.h:45: float veloctiy_x,
typo

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.h:106: // TODO(jdduke): Convert coordinates to float
using proper geometry types.
On one hand this would make this code more concise, on the other hand it also
makes sense to say let's keep it the same as the original Java to make diffs
more obvious.  Let's see what ui/gfx OWNERS think about this I guess.

https://codereview.chromium.org/172933004/diff/70001/webkit/child/fling_anima...
File webkit/child/fling_animator_impl_android.cc (right):

https://codereview.chromium.org/172933004/diff/70001/webkit/child/fling_anima...
webkit/child/fling_animator_impl_android.cc:52: // TODO(jdduke): Use the
provided |time|, resolving why time deltas are more
Wouldn't this be a preexisting problem?  Are you addressing a bug here in
addition to refactoring?

[jdduke (slow), 2014-02-21 23:33:02]

PTAL for owner review.  Thanks.

yfriedman@: ui/gfx/android, content/app/android
jamesr@: webkit/child

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller.cc
File ui/gfx/android/scroller.cc (right):

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.cc:14: const base::TimeDelta kDefaultDuration =
base::TimeDelta::FromMilliseconds(250);
On 2014/02/21 02:42:35, aelias wrote:
> Hmm, I'm concerned these will end up as static initializers which are
> discouraged.  Could you make this and the deceleration rate member variables
> initialized in the constructor?

I'll just compute the constant in kDecelerationRate, and use an int milliseconds
value for kDefaultDuration.

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.cc:21: const float kEpsilon = 1e-5;
On 2014/02/21 02:42:35, aelias wrote:
> 1e-5f instead?

Done.

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.cc:72: if (std::abs(tx - alpha) < 1E-5)
On 2014/02/21 02:42:35, aelias wrote:
> kEpsilon?

Done.

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.cc:87: if (std::abs(dy - alpha) < 1E-5)
On 2014/02/21 02:42:35, aelias wrote:
> kEpsilon?

Done.

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller.h
File ui/gfx/android/scroller.h (right):

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.h:45: float veloctiy_x,
On 2014/02/21 02:42:35, aelias wrote:
> typo

Done.

https://codereview.chromium.org/172933004/diff/70001/ui/gfx/android/scroller....
ui/gfx/android/scroller.h:106: // TODO(jdduke): Convert coordinates to float
using proper geometry types.
On 2014/02/21 02:42:35, aelias wrote:
> On one hand this would make this code more concise, on the other hand it also
> makes sense to say let's keep it the same as the original Java to make diffs
> more obvious.  Let's see what ui/gfx OWNERS think about this I guess.

I'll remove the TODO for now, it's probably not high enough a priority to make
the change.

https://codereview.chromium.org/172933004/diff/70001/webkit/child/fling_anima...
File webkit/child/fling_animator_impl_android.cc (right):

https://codereview.chromium.org/172933004/diff/70001/webkit/child/fling_anima...
webkit/child/fling_animator_impl_android.cc:52: // TODO(jdduke): Use the
provided |time|, resolving why time deltas are more
On 2014/02/21 02:42:35, aelias wrote:
> Wouldn't this be a preexisting problem?  Are you addressing a bug here in
> addition to refactoring?

Yeah, definitely pre-existing, I'll make that more clear in the comment.

[jamesr, 2014-02-21 23:41:13]

webkit/child looks good in concept, needs a few tweaks though. I assume with
this change we're OK with the fling curve not matching the system curves
whenever the system curve changes?

https://codereview.chromium.org/172933004/diff/320001/ui/gfx/android/scroller.h
File ui/gfx/android/scroller.h (right):

https://codereview.chromium.org/172933004/diff/320001/ui/gfx/android/scroller...
ui/gfx/android/scroller.h:28: Scroller(const Config& config);
explicit

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
File webkit/child/fling_animator_impl_android.cc (right):

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
webkit/child/fling_animator_impl_android.cc:52: // TODO(jdduke): Use
|base::TimeTicks()| upon resolution of crbug.com/345459.
it looks like this code *is* using base::TimeTicks::Now(), so I'm not sure what
this comment is supposed to mean.

why isn't this using gfx::FrameTime ?

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
webkit/child/fling_animator_impl_android.cc:68: // TODO(jdduke): Use |time| upon
resolution of crbug.com/345459.
this comment seems to say the opposite of the earlier comment. which one's
right?

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
webkit/child/fling_animator_impl_android.cc:69: base::TimeTicks timeticks =
base::TimeTicks::Now();
why not gfx::FrameTime ?

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
File webkit/child/fling_animator_impl_android.h (right):

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
webkit/child/fling_animator_impl_android.h:42: scoped_ptr<gfx::Scroller>
scroller_;
why heap allocate this object? it's always constructed, seems like it'd be
better to have it be a member instead of a pointer

[jdduke (slow), 2014-02-22 00:06:57]

On 2014/02/21 23:41:13, jamesr wrote:
> webkit/child looks good in concept, needs a few tweaks though. I assume with
> this change we're OK with the fling curve not matching the system curves
> whenever the system curve changes?

Yes, though we do have the flexibility to accommodate system curve changes
should the need arise.  The current Android fling curve has not changed in
nearly 2 years.

https://codereview.chromium.org/172933004/diff/320001/ui/gfx/android/scroller.h
File ui/gfx/android/scroller.h (right):

https://codereview.chromium.org/172933004/diff/320001/ui/gfx/android/scroller...
ui/gfx/android/scroller.h:28: Scroller(const Config& config);
On 2014/02/21 23:41:13, jamesr wrote:
> explicit

Done.

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
File webkit/child/fling_animator_impl_android.cc (right):

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
webkit/child/fling_animator_impl_android.cc:52: // TODO(jdduke): Use
|base::TimeTicks()| upon resolution of crbug.com/345459.
On 2014/02/21 23:41:13, jamesr wrote:
> it looks like this code *is* using base::TimeTicks::Now(), so I'm not sure
what
> this comment is supposed to mean.

It's using Now(), not an empty |base::TimeTicks()|.  I'll clear up confusion in
the comment...

> 
> why isn't this using gfx::FrameTime ?

No reason, I'll switch (though I believe on Android they're equivalent?).

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
webkit/child/fling_animator_impl_android.cc:68: // TODO(jdduke): Use |time| upon
resolution of crbug.com/345459.
On 2014/02/21 23:41:13, jamesr wrote:
> this comment seems to say the opposite of the earlier comment. which one's
> right?

We can either zero-initialize the fling start time (base::TimeTicks()) and use
the monotonic time from |apply()|, or we can initialize the fling at
TimeTicks::Now() and use Now() on each update.  We're doing the latter, with a
bug to switch to the former.

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
webkit/child/fling_animator_impl_android.cc:69: base::TimeTicks timeticks =
base::TimeTicks::Now();
On 2014/02/21 23:41:13, jamesr wrote:
> why not gfx::FrameTime ?

Done.

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
File webkit/child/fling_animator_impl_android.h (right):

https://codereview.chromium.org/172933004/diff/320001/webkit/child/fling_anim...
webkit/child/fling_animator_impl_android.h:42: scoped_ptr<gfx::Scroller>
scroller_;
On 2014/02/21 23:41:13, jamesr wrote:
> why heap allocate this object? it's always constructed, seems like it'd be
> better to have it be a member instead of a pointer

No reason at all, done.

[jamesr, 2014-02-22 00:17:41]

lgtm

https://codereview.chromium.org/172933004/diff/380001/webkit/child/fling_anim...
File webkit/child/fling_animator_impl_android.h (right):

https://codereview.chromium.org/172933004/diff/380001/webkit/child/fling_anim...
webkit/child/fling_animator_impl_android.h:21: class Scroller;
don't need the forward decl now

[Yaron, 2014-02-22 00:38:28]

rs lgtm

I think aelias review is much better for this than what I could provide.

Also, given that you've written most of the code in ui/gfx/android please me a
separate CL to take ownership of it. Consider adding one or two others (aelias,
skyostil, sievers come to mind as candidates). I think I'm an OWNER largely for
historical/upstreaming reasons and you guys can offer better guidance.

[jdduke (slow), 2014-02-22 01:30:26]

jochen@: Owner review for content/child? Thanks.

[jdduke (slow), 2014-02-22 01:30:43]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-02-22 01:40:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/172933004/520001

[commit-bot: I haz the power, 2014-02-22 08:41:28]

Change committed as 252789