[CRD iOS] Viewport fling animation

[CRD iOS] Viewport fling animation

This CL implements the viewport fling animation for iOS client. This
re-impements Android client's FlingAnimationJob and impements a simplified
version of Android's Scroller in C++.

BUG=714193
Review-Url: https://codereview.chromium.org/2869723007
Cr-Commit-Position: refs/heads/master@{#471135}
Committed: https://chromium.googlesource.com/chromium/src/+/4e32ad08870d1abc5752c1bfe8912a1f4c69dd57

[Yuwei, 2017-05-10 06:36:15]

Patchset #1 (id:1) has been deleted

[Yuwei, 2017-05-10 06:36:23]

Patchset #1 (id:20001) has been deleted

[Yuwei, 2017-05-10 06:36:30]

Patchset #1 (id:40001) has been deleted

[Yuwei, 2017-05-10 06:36:40]

Patchset #1 (id:60001) has been deleted

[Yuwei, 2017-05-10 06:36:46]

Patchset #1 (id:80001) has been deleted

[Yuwei, 2017-05-10 06:36:53]

Patchset #1 (id:100001) has been deleted

[Yuwei, 2017-05-10 06:47:07]

Description was changed from

==========
[CRD iOS] Viewport fling animation
==========

to

==========
[CRD iOS] Viewport fling animation

This CL implements the viewport fling animation for iOS client. This
re-impements Android client's FlingAnimationJob and impements a simplified
version of Android SDK's Scroller in C++.

BUG=714193
==========

[Yuwei, 2017-05-10 06:49:24]

yuweih@chromium.org changed reviewers:
+ nicholss@chromium.org

[Yuwei, 2017-05-10 06:49:24]

PTAL Thanks!

[Yuwei, 2017-05-10 06:52:55]

Description was changed from

==========
[CRD iOS] Viewport fling animation

This CL implements the viewport fling animation for iOS client. This
re-impements Android client's FlingAnimationJob and impements a simplified
version of Android SDK's Scroller in C++.

BUG=714193
==========

to

==========
[CRD iOS] Viewport fling animation

This CL implements the viewport fling animation for iOS client. This
re-impements Android client's FlingAnimationJob and impements a simplified
version of Android's Scroller in C++.

BUG=714193
==========

[Yuwei, 2017-05-10 06:53:03]

https://codereview.chromium.org/2869723007/diff/120001/remoting/client/ios/cl...
File remoting/client/ios/client_gestures.mm (right):

https://codereview.chromium.org/2869723007/diff/120001/remoting/client/ios/cl...
remoting/client/ios/client_gestures.mm:132: if ([sender state] ==
UIGestureRecognizerStateChanged) {
Removed the |numberOfTouches| check since it seems to work totally fine when the
user makes a pinch-and-pan gesture.

https://codereview.chromium.org/2869723007/diff/120001/remoting/client/ios/se...
File remoting/client/ios/session/remoting_client.mm (left):

https://codereview.chromium.org/2869723007/diff/120001/remoting/client/ios/se...
remoting/client/ios/session/remoting_client.mm:233: if (_gestureInterpreter) {
Removed the check since canvasSizeChanged will only be called after
|_gestureInterpreter| is created.

[joedow, 2017-05-10 15:14:03]

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

[joedow, 2017-05-10 15:14:04]

drive-by comment: Can we get some unit tests for the new functionality.

[Yuwei, 2017-05-10 18:21:45]

On 2017/05/10 15:14:04, joedow wrote:
> drive-by comment: Can we get some unit tests for the new functionality.

I'll add unit tests for FlingTracker/FlingAnimation. Thanks for point this out
:)

[nicholss, 2017-05-10 19:34:05]

I have a couple questions and comments that would be easier in person.

Lets have a quick whiteboard chat!

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
File remoting/client/fling_animation.cc (right):

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.cc:9: FlingAnimation::FlingAnimation(float
time_constant,
It seems like FlingTracker could be just a core class inside of FlingAnimation,
what is the value of having them separate?

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.cc:11: : fling_tracker_(time_constant),
fling_callback_(fling_callback) {}
I am not sure this would compile, how is a float assigned to a fling_tracker?

RE: fling_tracker_(time_constant)

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.cc:28:
fling_tracker_.TrackPositions(base::TimeTicks::Now(), &x, &y);
If this returned deltas you would not need to know about the previous position.

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
File remoting/client/fling_animation.h (right):

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.h:17: using FlingCallback =
base::Callback<void(float, float)>;
Can you document the callback please

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.h:20: //     positions at previous tick.
running git cl format leaves these spaces?

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.h:25: void Reset(float velocity_x, float
velocity_y);
rename to setVelocity() ?

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.h:38: FlingTracker fling_tracker_;
Forward declare rather than include?

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
File remoting/client/fling_tracker.cc (right):

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_tracker.cc:66: initial_speed_rate_ *= kAmplituteFactor;
Why start with attenuation?

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
File remoting/client/fling_tracker.h (right):

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_tracker.h:33: bool TrackPositions(base::TimeTicks time,
float* x, float* y);
Might be simpler if you just return deltaX, deltaY

[Yuwei, 2017-05-10 23:28:24]

PTAL Thanks

As discussed offline, I view the animation as interpolation of user's gesture
while the input strategies decide how gestures should be translated to
viewport/cursor movements.

If this doesn't work for Trackpad mode, I can still easily move the animation
code inside the input strategies. It's just a few lines of code to be moved
though.

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
File remoting/client/fling_animation.cc (right):

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.cc:9: FlingAnimation::FlingAnimation(float
time_constant,
On 2017/05/10 19:34:05, nicholss wrote:
> It seems like FlingTracker could be just a core class inside of
FlingAnimation,
> what is the value of having them separate?

TBH I'm not sure :)
I'm just slightly against putting together modules that are not so dependent
with each other. By separating them, we will be able to swap for another
FlingTracker or unittest them independently, although we are not doing either or
them now...

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.cc:11: : fling_tracker_(time_constant),
fling_callback_(fling_callback) {}
On 2017/05/10 19:34:05, nicholss wrote:
> I am not sure this would compile, how is a float assigned to a fling_tracker?
> 
> RE: fling_tracker_(time_constant)

This is essentially calling the FlingTracker(time_constant) constructor.

However you did remind me that I forgot to mark the constructor as `explicit` :P
Without `explicit` I can do conversion like:

FlingTracker tracker_ = 100.f;

Which doesn't make sense :)

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.cc:28:
fling_tracker_.TrackPositions(base::TimeTicks::Now(), &x, &y);
On 2017/05/10 19:34:05, nicholss wrote:
> If this returned deltas you would not need to know about the previous
position. 

Done.

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
File remoting/client/fling_animation.h (right):

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.h:17: using FlingCallback =
base::Callback<void(float, float)>;
On 2017/05/10 19:34:05, nicholss wrote:
> Can you document the callback please

Done.

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.h:20: //     positions at previous tick.
On 2017/05/10 19:34:05, nicholss wrote:
> running git cl format leaves these spaces?

I was trying to follow the four-space indenting rule. Not working well though.

I've moved the comment to the callback.

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.h:25: void Reset(float velocity_x, float
velocity_y);
On 2017/05/10 19:34:05, nicholss wrote:
> rename to setVelocity() ?

Done.

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_animation.h:38: FlingTracker fling_tracker_;
On 2017/05/10 19:34:05, nicholss wrote:
> Forward declare rather than include?

This is not a pointer nor a reference so forward declaration doesn't work in
this case.

If you insist, I can change this to a unique_ptr.

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
File remoting/client/fling_tracker.cc (right):

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_tracker.cc:66: initial_speed_rate_ *= kAmplituteFactor;
On 2017/05/10 19:34:05, nicholss wrote:
> Why start with attenuation?

I was basically inspired by this article:

https://ariya.io/2013/11/javascript-kinetic-scrolling-part-2

It multiplies the velocity with 0.8 before flinging.

The viewport feels a little bit light so I multiplied it with 0.75.

Anyway tweaking these numbers feel like black magic and the feeling is totally
subjective. Let's change it back to one and later see how we can optimize it.

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
File remoting/client/fling_tracker.h (right):

https://codereview.chromium.org/2869723007/diff/140001/remoting/client/fling_...
remoting/client/fling_tracker.h:33: bool TrackPositions(base::TimeTicks time,
float* x, float* y);
On 2017/05/10 19:34:05, nicholss wrote:
> Might be simpler if you just return deltaX, deltaY

Done.

I was looking at the Android Scroller, which returns positions rather than
deltas...

[nicholss, 2017-05-11 15:13:50]

LGTM

It is a great start to fling animations and momentum tracking.

https://codereview.chromium.org/2869723007/diff/200001/remoting/client/fling_...
File remoting/client/fling_animation.cc (right):

https://codereview.chromium.org/2869723007/diff/200001/remoting/client/fling_...
remoting/client/fling_animation.cc:31: clock_->NowTicks() - fling_start_time_,
&dx, &dy);
I think this looks much cleaner, I hope you agree.

https://codereview.chromium.org/2869723007/diff/200001/remoting/client/fling_...
File remoting/client/fling_tracker.cc (right):

https://codereview.chromium.org/2869723007/diff/200001/remoting/client/fling_...
remoting/client/fling_tracker.cc:100: *dx = position_x - previous_position_x_;
It seems to me you also do not need previous_position_* if you have the
time_delta. I don't think you need to change this at the moment, I think it can
work out, but we might want to change this up when we integrate with scroll
velocity which only acts in one direction.

[Yuwei, 2017-05-11 18:26:27]

Thanks! Checking-in

https://codereview.chromium.org/2869723007/diff/200001/remoting/client/fling_...
File remoting/client/fling_animation.cc (right):

https://codereview.chromium.org/2869723007/diff/200001/remoting/client/fling_...
remoting/client/fling_animation.cc:31: clock_->NowTicks() - fling_start_time_,
&dx, &dy);
On 2017/05/11 15:13:50, nicholss wrote:
> I think this looks much cleaner, I hope you agree. 

Yep, and it's also for testability.

https://codereview.chromium.org/2869723007/diff/200001/remoting/client/fling_...
File remoting/client/fling_tracker.cc (right):

https://codereview.chromium.org/2869723007/diff/200001/remoting/client/fling_...
remoting/client/fling_tracker.cc:100: *dx = position_x - previous_position_x_;
On 2017/05/11 15:13:50, nicholss wrote:
> It seems to me you also do not need previous_position_* if you have the
> time_delta. I don't think you need to change this at the moment, I think it
can
> work out, but we might want to change this up when we integrate with scroll
> velocity which only acts in one direction.

I just renamed |time_delta| to |time_elapsed|. It means the time elapsed since
the animation has started...

I don't feel like we can use time_delta to calculate the change in position
directly given that f(t)-f(t-time_delta) here is dependent on t... Lets see
whether we can simplify this when we get to that

[Yuwei, 2017-05-11 18:26:34]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2017-05-11 18:26:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
nicholss@chromium.org
Link to the patchset: https://codereview.chromium.org/2869723007/#ps220001
(title: "Rename time_delta to time_elapsed")

[commit-bot: I haz the power, 2017-05-11 18:27:30]

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, 2017-05-11 19:59:01]

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

[commit-bot: I haz the power, 2017-05-11 19:59:02]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Yuwei, 2017-05-11 20:08:55]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2017-05-11 20:09:03]

The CQ bit was unchecked by yuweih@chromium.org

[Yuwei, 2017-05-11 20:13:11]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2017-05-11 20:13:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
nicholss@chromium.org
Link to the patchset: https://codereview.chromium.org/2869723007/#ps240001
(title: "Merge branch 'master' into feat-fling-animation")

[commit-bot: I haz the power, 2017-05-11 20:14:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuwei, 2017-05-11 23:15:47]

The CQ bit was unchecked by yuweih@chromium.org

[Yuwei, 2017-05-11 23:16:40]

The CQ bit was checked by yuweih@chromium.org

[commit-bot: I haz the power, 2017-05-11 23:17:49]

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, 2017-05-12 00:16:52]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1494544600424630,
"parent_rev": "d2f276e5c8e098ab9cc16d77315a2475e94a2582", "commit_rev":
"4e32ad08870d1abc5752c1bfe8912a1f4c69dd57"}

[commit-bot: I haz the power, 2017-05-12 00:17:00]

Description was changed from

==========
[CRD iOS] Viewport fling animation

This CL implements the viewport fling animation for iOS client. This
re-impements Android client's FlingAnimationJob and impements a simplified
version of Android's Scroller in C++.

BUG=714193
==========

to

==========
[CRD iOS] Viewport fling animation

This CL implements the viewport fling animation for iOS client. This
re-impements Android client's FlingAnimationJob and impements a simplified
version of Android's Scroller in C++.

BUG=714193

Review-Url: https://codereview.chromium.org/2869723007
Cr-Commit-Position: refs/heads/master@{#471135}
Committed:
https://chromium.googlesource.com/chromium/src/+/4e32ad08870d1abc5752c1bfe891...
==========

[commit-bot: I haz the power, 2017-05-12 00:17:01]

Committed patchset #7 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/4e32ad08870d1abc5752c1bfe891...