Fix scrollbar fade animation scheduling.

Fix scrollbar fade animation scheduling.

In r195531, most of the uses of base::TimeTicks::Now() were replaced by
current frame time, but this is incorrect in the case of scrollbar
fade scheduling.  Although the fade itself is animated within draw
frames, the delay until it starts is managed outside frames (intentionally
so -- we want to avoid spamming animation frames before the fade has
actually begun).  Revert that part of r195531 and add more test
coverage.

NOTRY=true
BUG=236351
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206532

[aelias_OOO_until_Jul13, 2013-06-14 08:33:57]

Hi Dana, PTAL.  The only substantive change is to switch the times back to
base::TimeTicks::Now(), the other changes are for better testability.

[danakj, 2013-06-14 17:40:39]

https://codereview.chromium.org/16925007/diff/1/cc/trees/layer_tree_host_impl...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/16925007/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl_unittest.cc:1059:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(25));
This timing stuff is going to be a disaster on xp bots :/

Instead of this, can we hook into the animation controller to look for or listen
to events, so test things are working correctly that way?

One thought is that LayerImpl::DidBecomeActive is virtual so you could override
it and create a subclass of the linear fade animation controller that lets you
listen to things being called on it and respond.

You may see other ways to just make use of the LinearFade controller without
subclassing it to avoid these timing issues?

[aelias_OOO_until_Jul13, 2013-06-14 18:03:35]

There are already tests for LinearFadeAnimationController.  But the problem was
caused specifically by switching to a time value other than
base::TimeTicks::Now(), which is not mockable, so it's hard to test this
particular regression without involving physical time (although I was also
concerned about potential flakiness).  What would you think about making an
intermediate mockable version of base::TimeTicks::Now() instead?

[danakj, 2013-06-14 18:05:52]

On Fri, Jun 14, 2013 at 2:03 PM, <aelias@chromium.org> wrote:

> There are already tests for LinearFadeAnimationController.  But the
> problem was
> caused specifically by switching to a time value other than
> base::TimeTicks::Now(), which is not mockable, so it's hard to test this
> particular regression without involving physical time (although I was also
> concerned about potential flakiness).  What would you think about making an
> intermediate mockable version of base::TimeTicks::Now() instead?
>

Maybe add a function to LTHI that returns Now() always?


>
>
https://codereview.chromium.**org/16925007/<https://codereview.chromium.org/1...
>

[aelias_OOO_until_Jul13, 2013-06-14 18:33:48]

On 2013/06/14 18:05:52, danakj wrote:
> On Fri, Jun 14, 2013 at 2:03 PM, <mailto:aelias@chromium.org> wrote:
> 
> > There are already tests for LinearFadeAnimationController.  But the
> > problem was
> > caused specifically by switching to a time value other than
> > base::TimeTicks::Now(), which is not mockable, so it's hard to test this
> > particular regression without involving physical time (although I was also
> > concerned about potential flakiness).  What would you think about making an
> > intermediate mockable version of base::TimeTicks::Now() instead?
> >
> 
> Maybe add a function to LTHI that returns Now() always?

Done, and got rid of all the sleeps.  PTAL.

[danakj, 2013-06-14 18:35:10]

https://codereview.chromium.org/16925007/diff/5002/cc/trees/layer_tree_host_i...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16925007/diff/5002/cc/trees/layer_tree_host_i...
cc/trees/layer_tree_host_impl.cc:2263: base::TimeTicks
LayerTreeHostImpl::CurrentPhysicalTimeTicks() const {
Think I'd prefer this is virtual instead of adding test data to the prod class.

[danakj, 2013-06-14 18:36:42]

https://codereview.chromium.org/16925007/diff/5002/cc/trees/layer_tree_host_i...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/16925007/diff/5002/cc/trees/layer_tree_host_i...
cc/trees/layer_tree_host_impl_unittest.cc:1041: base::TimeTicks fake_time =
base::TimeTicks::Now();
nit: fake_now ?

https://codereview.chromium.org/16925007/diff/5002/cc/trees/layer_tree_host_i...
cc/trees/layer_tree_host_impl_unittest.cc:1051: // (check greater than 10ms to
avoid flakiness).
this comment out of date

[aelias_OOO_until_Jul13, 2013-06-14 21:16:37]

Good point.  All done, PTAL.

[danakj, 2013-06-14 21:25:23]

LGTM thanks for the test!

https://codereview.chromium.org/16925007/diff/16001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16925007/diff/16001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:2244: void
LayerTreeHostImpl::UpdateCurrentFrameTime(base::TimeTicks* ticks,
nit: just pass in the LTHI and you don't have to add it to class etc.

https://codereview.chromium.org/16925007/diff/16001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/16925007/diff/16001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl_unittest.cc:1027:
LayerTreeHostImplOverridePhysicalTime* host_impl_override_time =
nit: you could save the raw pointer before making the scoped_ptr and avoid
static casting anything, if you like.

[aelias_OOO_until_Jul13, 2013-06-14 22:19:02]

https://codereview.chromium.org/16925007/diff/16001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/16925007/diff/16001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:2244: void
LayerTreeHostImpl::UpdateCurrentFrameTime(base::TimeTicks* ticks,
On 2013/06/14 21:25:23, danakj wrote:
> nit: just pass in the LTHI and you don't have to add it to class etc.

I think it makes more sense to add it to the class since the this pointer is the
natural way to do this, so leaving this as is.

https://codereview.chromium.org/16925007/diff/16001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/16925007/diff/16001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl_unittest.cc:1027:
LayerTreeHostImplOverridePhysicalTime* host_impl_override_time =
On 2013/06/14 21:25:23, danakj wrote:
> nit: you could save the raw pointer before making the scoped_ptr and avoid
> static casting anything, if you like.

Done.

[commit-bot: I haz the power, 2013-06-15 01:19:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/16925007/29001

[commit-bot: I haz the power, 2013-06-15 01:20:06]

Change committed as 206532