cc: Split animating and drawing into separate actions

cc: Split animating and drawing into separate actions

Split impl-side animating and drawing into separate actions. This is
needed to allow for the possibility of a commit between animating and
drawing so that the main thread gets a chance to consume the new
animation state. In particular this allows the main thread to
synchronize with a fling animation using an onscroll handler.

BUG=347366, 368064
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267338

[Sami, 2014-03-20 20:51:21]

Can you do a sanity check on the idea? I realized that without something like
this it's not possible to for the main thread to see the results of a fling
animation until the next frame.

Tests TODO.

[danakj, 2014-03-20 21:45:53]

On 2014/03/20 20:51:21, Sami wrote:
> Can you do a sanity check on the idea? I realized that without something like
> this it's not possible to for the main thread to see the results of a fling
> animation until the next frame.
> 
> Tests TODO.

Perhaps this could open the door to having the scheduler to background animation
ticking instead of LTHI setting up a timer callback on its own.

[brianderson, 2014-03-21 10:49:31]

I would love to see us animate before we send the BeginMainFrame. I had tried
doing something similar in the original scheduler refactoring, by updating the
animation time at the start of the BeginImplFrame, but ran into flaky threaded
animation layout tests.

I didn't understand the root cause at the time, but I think the symptom was that
certain key frames of animation were getting skipped. I got the impression that
the layout tests were being too strict, but maybe I just had a bug in my
implementation? Can you see any situation where this patch might not give the
layout tests what they are expecting?

It would be really nice to get this in, because it would also enable seamless
CSS animation chaining if blink is operating in the low latency mode.

+ajuma, +vollick, +tansell for fyi.

[brianderson, 2014-03-21 12:30:49]

Actually +ajuma, +vollick, +mithro for fyi.

[ajuma, 2014-03-21 15:05:13]

I'd also love to see us animate before sending BeginMainFrame.

One thing left to fix is dealing with the situation where new animations are
added by a commit that occurs between animation and drawing. As it stands, these
won't get ticked till the following frame. We should probably change
LayerAnimationController::PushAnimationUpdatesTo so that newly-pushed animations
get ticked immediately (using last_tick_time_) when we're between calls to
Animate and UpdateAnimationState.

https://codereview.chromium.org/206793003/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/206793003/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:1062:
impl().layer_tree_host_impl->pending_tree()->UpdateDrawProperties();
We should move this down by a line so that it happens after animating.

[Sami, 2014-03-31 18:50:36]

While this is churning on the bots, would mind having a look Brian -- in
particular whether I missed something in how
SchedulerStateMachine::ShouldAnimate() is wired up? The contract is that
SetNeedsAnimate() implies a SetNeedsRedraw() after the animation step because
that's what the current clients seem to assume.

I did not see any flaky animation layout tests while running this locally, but I
may have to do a few more runs tomorrow to be sure.

On 2014/03/21 15:05:13, ajuma wrote:
> One thing left to fix is dealing with the situation where new animations are
> added by a commit that occurs between animation and drawing. As it stands,
these
> won't get ticked till the following frame. We should probably change
> LayerAnimationController::PushAnimationUpdatesTo so that newly-pushed
animations
> get ticked immediately (using last_tick_time_) when we're between calls to
> Animate and UpdateAnimationState.

Good point. It feels like it might be better to do that in a follow-up patch to
avoid rocking the boat too much.

[brianderson, 2014-04-01 16:05:10]

https://codereview.chromium.org/206793003/diff/40001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/206793003/diff/40001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine_unittest.cc:1971: state.SetNeedsAnimate();
Can you add a test that verifies what happens when SetNeedsAnimate is called
after the animation has already occurred, but before we draw?

https://codereview.chromium.org/206793003/diff/40001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine_unittest.cc:1975:
state.OnBeginImplFrame(BeginFrameArgs::CreateForTesting());
Can you verify that no actions are triggered before this point?

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:469: void
LayerTreeHostImpl::ScheduleAnimation() { SetNeedsAnimate(); }
Can we get rid of ScheduleAnimation?

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1685:
NotifySwapPromiseMonitorsOfSetNeedsRedraw();
What does NotifySwapPromiseMonitorsOfSetNeedsRedraw do? Do we need it in
SetNeedsAnimate too?

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2667: SetNeedsAnimate();
Can you get rid of the SetNeedsRedraw above?

In the current patch, SetNeedsAnimate eventually implies SetNeedsRedraw in the
scheduler. In the future, it seems like you want the animation system to trigger
a redraw if something has changed. In both cases, I don't think the
SetNeedsRedraw is needed.

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2680: SetNeedsAnimate();
I guess this is the way it already is, but I wonder why this SetNeedsAnimate
isn't needed for the else block?

https://codereview.chromium.org/206793003/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/206793003/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1061:
impl().layer_tree_host_impl->UpdateCurrentFrameTime();
I think we should keep the UpdateCurrentFrameTime() in the BeginImplFrame, so
that even if we animate right before the draw, we are using a timestamp that is
representative of the BeginImplFrame.

https://codereview.chromium.org/206793003/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1345:
impl().layer_tree_host_impl->pending_tree()->UpdateDrawProperties();
Why was this moved here?

[mithro-old, 2014-04-16 07:05:10]

Hi Sami!

Could you give me a status update on this CL?

Thanks!

[Sami, 2014-04-16 09:58:27]

Hi Tim, 

I'm currently working on getting https://codereview.chromium.org/206603002/
landed before returning to this. It doesn't seem like there is anything major
left to do here -- other than Brian's comments, rebasing against a few other
recent refactorings and more testing.

[Sami, 2014-04-17 15:16:33]

Thanks for the comments Brian. All addressed.

yufeng@, could you check the bit about
NotifySwapPromiseMonitorsOfSetNeedsRedraw?

https://codereview.chromium.org/206793003/diff/40001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/206793003/diff/40001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine_unittest.cc:1971: state.SetNeedsAnimate();
On 2014/04/01 16:05:11, brianderson wrote:
> Can you add a test that verifies what happens when SetNeedsAnimate is called
> after the animation has already occurred, but before we draw?

Good idea, done.

https://codereview.chromium.org/206793003/diff/40001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine_unittest.cc:1975:
state.OnBeginImplFrame(BeginFrameArgs::CreateForTesting());
On 2014/04/01 16:05:11, brianderson wrote:
> Can you verify that no actions are triggered before this point?

Done.

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:469: void
LayerTreeHostImpl::ScheduleAnimation() { SetNeedsAnimate(); }
On 2014/04/01 16:05:11, brianderson wrote:
> Can we get rid of ScheduleAnimation?

Looks like we can, done.

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1685:
NotifySwapPromiseMonitorsOfSetNeedsRedraw();
On 2014/04/01 16:05:11, brianderson wrote:
> What does NotifySwapPromiseMonitorsOfSetNeedsRedraw do? Do we need it in
> SetNeedsAnimate too?

As far as I understand it, that call tells the latency tracking system that we
are about to draw and swap soon. A call to SetNeedsAnimate() by itself isn't a
strong enough guarantee of that, because the animation may end up being a no-op,
so I don't think we need the call there.

+miletus@, could you verify my assumptions?

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2667: SetNeedsAnimate();
On 2014/04/01 16:05:11, brianderson wrote:
> Can you get rid of the SetNeedsRedraw above?
> 
> In the current patch, SetNeedsAnimate eventually implies SetNeedsRedraw in the
> scheduler. In the future, it seems like you want the animation system to
trigger
> a redraw if something has changed. In both cases, I don't think the
> SetNeedsRedraw is needed.

I think we still need the SetNeedsRedraw here because AnimatePageScale is the
"animation system" for page scale animations, and really only it knows when a
redraw is needed. I guess an alternative would be to move more of this logic to
input/page_scale_animation.cc, or better yet introduce a new
PageScaleAnimationManager.

You're right that it isn't required in the sense that things still work, but
I've made some of the animation unit tests to enforce the separation between
SetNeedsAnimate and SetNeedsRedraw.

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2680: SetNeedsAnimate();
On 2014/04/01 16:05:11, brianderson wrote:
> I guess this is the way it already is, but I wonder why this SetNeedsAnimate
> isn't needed for the else block?

Good question. I think we should call SetNeedsAnimate in both cases here. It
looks the way this worked is that top_controls_manager_->Animate() ends up
calling DidChangeTopControlsPosition, which in turn calls SetNeedsRedraw. Now
that we can schedule animation without relying on that callback I think we can
just remove this special case.

https://codereview.chromium.org/206793003/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/206793003/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1061:
impl().layer_tree_host_impl->UpdateCurrentFrameTime();
On 2014/04/01 16:05:11, brianderson wrote:
> I think we should keep the UpdateCurrentFrameTime() in the BeginImplFrame, so
> that even if we animate right before the draw, we are using a timestamp that
is
> representative of the BeginImplFrame.

Yes, agreed. I've changed this back the way it was.

https://codereview.chromium.org/206793003/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1345:
impl().layer_tree_host_impl->pending_tree()->UpdateDrawProperties();
On 2014/04/01 16:05:11, brianderson wrote:
> Why was this moved here?

Hmm, I was trying to keep it in DrawSwapReadbackInternal. Not sure how it ended
up here. In any case, I think updating the draw properties should happen right
before we draw instead as a part of animation because the potential consumer of
the animation stage, i.e., the main thread, doesn't care about draw properties.

[danakj, 2014-04-17 15:18:09]

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/206793003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1685:
NotifySwapPromiseMonitorsOfSetNeedsRedraw();
On 2014/04/17 15:16:33, Sami wrote:
> On 2014/04/01 16:05:11, brianderson wrote:
> > What does NotifySwapPromiseMonitorsOfSetNeedsRedraw do? Do we need it in
> > SetNeedsAnimate too?
> 
> As far as I understand it, that call tells the latency tracking system that we
> are about to draw and swap soon. A call to SetNeedsAnimate() by itself isn't a
> strong enough guarantee of that, because the animation may end up being a
no-op,
> so I don't think we need the call there.
> 
> +miletus@, could you verify my assumptions?

It is meant to tell the monitor that we might draw and swap and cause it to
insert a LatencyInfo. It can have false positives. The LatencyInfo should later
be aborted if we don't end up drawing as a result.

[Sami, 2014-04-17 15:27:41]

On 2014/04/17 15:18:09, danakj wrote:
> It is meant to tell the monitor that we might draw and swap and cause it to
> insert a LatencyInfo. It can have false positives. The LatencyInfo should
later
> be aborted if we don't end up drawing as a result.

Are there requirements that NotifySwapPromiseMonitorsOfSetNeedsRedraw needs to
be called within a certain call stack, e.g., during input event processing? My
patch could break that assumption, for instance, if for some reason the first
frame of an animation is a no-op and doesn't cause a call to SetNeedsRedraw.

[danakj, 2014-04-17 15:31:11]

On Thu, Apr 17, 2014 at 11:27 AM, <skyostil@chromium.org> wrote:

> On 2014/04/17 15:18:09, danakj wrote:
>
>> It is meant to tell the monitor that we might draw and swap and cause it
>> to
>> insert a LatencyInfo. It can have false positives. The LatencyInfo should
>>
> later
>
>> be aborted if we don't end up drawing as a result.
>>
>
> Are there requirements that NotifySwapPromiseMonitorsOfSetNeedsRedraw
> needs to
> be called within a certain call stack, e.g., during input event
> processing? My
>

Right. We want to know the latency from an input event to the frame that it
generates. If no frame results from that input event (or a frame results
later), then that isn't something we want to track.


> patch could break that assumption, for instance, if for some reason the
> first
> frame of an animation is a no-op and doesn't cause a call to
> SetNeedsRedraw.
>

So I think that is okay, as long as we are telling it about SetNeedsRedraws
that occur *right then* as a result of the input event/state changes.

miletus@ can you confirm?

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

[Sami, 2014-04-17 15:48:28]

On 2014/04/17 15:31:11, danakj wrote:
> So I think that is okay, as long as we are telling it about SetNeedsRedraws
> that occur *right then* as a result of the input event/state changes.

Yes, that part doesn't change.

[Yufeng Shen (Slow to review), 2014-04-17 18:42:35]

On 2014/04/17 15:48:28, Sami wrote:
> On 2014/04/17 15:31:11, danakj wrote:
> > So I think that is okay, as long as we are telling it about SetNeedsRedraws
> > that occur *right then* as a result of the input event/state changes.
> 
> Yes, that part doesn't change.

hmm, if there is case SetNeedsAnimate() does not lead to a call to
SetNeedsRedraws()
or when the SetNeedsRedraws() is triggered it is already out of the call stack
of input event processing,
then I think we should also call NotifySwapPromiseMonitorsOfSetNeedsRedraw() in
SetNeedsAnimate().

The point here is we want to associate input event to the frame and if
SetNeedsAnimate() is
called during input processing. If the animation is a no-op and no drawing is
needed, then
the swap will be aborted and so are the LatencyInfo.

[brianderson, 2014-04-18 01:10:54]

https://codereview.chromium.org/206793003/diff/60001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/206793003/diff/60001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:858: if (needs_animate_)
If the BeginFrame is needed to advance the animation, should we rename this
method to just BeginFrameNeeded?

If it's not, I feel like this if should go into ProactiveBeginFrameWanted.

[Sami, 2014-04-22 10:34:33]

+jdduke@ for content/renderer/input.

On 2014/04/17 18:42:35, Yufeng Shen wrote:
> hmm, if there is case SetNeedsAnimate() does not lead to a call to
> SetNeedsRedraws()
> or when the SetNeedsRedraws() is triggered it is already out of the call stack
> of input event processing,
> then I think we should also call NotifySwapPromiseMonitorsOfSetNeedsRedraw()
in
> SetNeedsAnimate().

Ah, got it, we need to make sure the association happens while we're still in
the input handling call stack. I've added the call to
NotifySwapPromiseMonitorsOfSetNeedsRedraw to SetNeedsAnimate. Thanks for the
explanation.

https://codereview.chromium.org/206793003/diff/60001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/206793003/diff/60001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:858: if (needs_animate_)
On 2014/04/18 01:10:54, brianderson wrote:
> If the BeginFrame is needed to advance the animation, should we rename this
> method to just BeginFrameNeeded?
> 
> If it's not, I feel like this if should go into ProactiveBeginFrameWanted.

It feels like an animation request is a stronger signal that we're about to draw
than what we currently have in ProactiveBeginFrameWanted. Also, I think we need
to have the check here anyway for synchronous compositing.

How about BeginFrameNeededToAnimateOrDraw -- since we already have a more
generic BeginFrameNeeded?

[jdduke (slow), 2014-04-22 14:50:01]

content/renderer/input lgtm.

[brianderson, 2014-04-22 17:53:57]

lgtm

[Sami, 2014-04-22 18:05:38]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-04-22 18:06:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/206793003/100001

[commit-bot: I haz the power, 2014-04-22 18:33:18]

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

[commit-bot: I haz the power, 2014-04-22 18:33:18]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Sami, 2014-04-25 19:10:49]

Turns out the telemetry smoke bots found a bug here: if we have already animated
during the current frame and we do a commit that introduces a new animation, the
newly added animation finishes immediately at the next BeginMainFrame.

This happens because LayerAnimationController::UpdateState() assumes all
animations have been ticked prior before it is called. When this isn't the case,
it uses a zero last_tick_time_ to set the starting point for all new animations.
When the main thread gets the resulting animation started notification with the
zero start time, it concludes that the animation started at the beginning of
time so it must have finished already.

This bug happens fairly often with
tools/telemetry/unittest_data/interaction_enabled_page.html. The effect is that
the red sidebar snaps into place instead of animating smoothly.

ajuma@/mithro@: My candidate fix is to make
LayerAnimationController::UpdateState() not start animations without a valid
starting point and make LTHI::UpdateAnimationState() schedule an animation so we
get around to starting the animation at the next frame. Please have a look and
suggest a better fix if you have one.

[ajuma, 2014-04-25 19:26:30]

On 2014/04/25 19:10:49, Sami wrote:
> Turns out the telemetry smoke bots found a bug here: if we have already
animated
> during the current frame and we do a commit that introduces a new animation,
the
> newly added animation finishes immediately at the next BeginMainFrame.
> 
> This happens because LayerAnimationController::UpdateState() assumes all
> animations have been ticked prior before it is called. When this isn't the
case,
> it uses a zero last_tick_time_ to set the starting point for all new
animations.
> When the main thread gets the resulting animation started notification with
the
> zero start time, it concludes that the animation started at the beginning of
> time so it must have finished already.
> 
> This bug happens fairly often with
> tools/telemetry/unittest_data/interaction_enabled_page.html. The effect is
that
> the red sidebar snaps into place instead of animating smoothly.
> 
> ajuma@/mithro@: My candidate fix is to make
> LayerAnimationController::UpdateState() not start animations without a valid
> starting point and make LTHI::UpdateAnimationState() schedule an animation so
we
> get around to starting the animation at the next frame. Please have a look and
> suggest a better fix if you have one.

We can't rely on last_tick_time_ being zero, since if the controller has ever
been ticked in the past (say it had an animation that finished a while ago), it
will have a non-zero last_tick_time_. The cleanest solution I can think of is to
make sure that whenever we have a mid-frame commit, we always also animate
again. That way, when we get to LayerAnimationController::UpdateState, we'll be
in a consistent state.

[Sami, 2014-04-25 20:15:42]

On 2014/04/25 19:26:30, ajuma wrote:
> We can't rely on last_tick_time_ being zero, since if the controller has ever
> been ticked in the past (say it had an animation that finished a while ago),
it
> will have a non-zero last_tick_time_. The cleanest solution I can think of is
to
> make sure that whenever we have a mid-frame commit, we always also animate
> again. That way, when we get to LayerAnimationController::UpdateState, we'll
be
> in a consistent state.

Oh right, I forgot the controller can be reused for multiple animations. Thanks
for the suggestion, it seems to work nicely.

I can't seem to trigger this in an LTHI test without mucking with the scheduler
deadline, so this is a little tricky to write a test for. Maybe it's time to
write those thread proxy tests...

[Sami, 2014-04-28 12:45:11]

Since it looks like Simon is already working on thread proxy tests, I've added a
standalone test for the animation issue here. Ali, could you check it as well as
the corresponding fix in ThreadProxy (did_commit_after_animating)? Thanks.

[ajuma, 2014-04-28 13:33:42]

lgtm

https://codereview.chromium.org/206793003/diff/180001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/206793003/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1284: ASSERT_EQ(2u, copy.size());
EXPECT_EQ

https://codereview.chromium.org/206793003/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1292:
ASSERT_GT(anim->start_time(), 0);
EXPECT_GT

https://codereview.chromium.org/206793003/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1295:
ASSERT_GT(anim->start_time(), 0);
EXPECT_GT

https://codereview.chromium.org/206793003/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1308:
MULTI_THREAD_TEST_F(LayerTreeHostAnimationTestAddAnimationAfterAnimating);
Can this be SINGLE_AND_MULTI_THREAD_TEST_F?

[Sami, 2014-04-28 16:02:04]

Thanks Ali.

https://codereview.chromium.org/206793003/diff/180001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/206793003/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1284: ASSERT_EQ(2u, copy.size());
On 2014/04/28 13:33:42, ajuma wrote:
> EXPECT_EQ

D'oh, done.

https://codereview.chromium.org/206793003/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1292:
ASSERT_GT(anim->start_time(), 0);
On 2014/04/28 13:33:42, ajuma wrote:
> EXPECT_GT

Done.

https://codereview.chromium.org/206793003/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1295:
ASSERT_GT(anim->start_time(), 0);
On 2014/04/28 13:33:42, ajuma wrote:
> EXPECT_GT

Done.

https://codereview.chromium.org/206793003/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:1308:
MULTI_THREAD_TEST_F(LayerTreeHostAnimationTestAddAnimationAfterAnimating);
On 2014/04/28 13:33:42, ajuma wrote:
> Can this be SINGLE_AND_MULTI_THREAD_TEST_F?

Indeed it can.

[Sami, 2014-04-28 16:02:31]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-04-28 16:02:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/206793003/200001

[commit-bot: I haz the power, 2014-04-28 16:40:32]

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

[commit-bot: I haz the power, 2014-04-28 16:40:33]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[brianderson, 2014-04-28 19:05:36]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:08:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/206793003/200001

[commit-bot: I haz the power, 2014-04-28 19:16:44]

Change committed as 266624

[Sami, 2014-04-29 12:14:23]

A revert of this CL has been created in
https://codereview.chromium.org/256233006/ by skyostil@chromium.org.

The reason for reverting is: Broke some downstream unit tests.

BUG=368064.

[Sami, 2014-04-29 14:09:46]

A revert of this CL has been created in
https://codereview.chromium.org/260383003/ by skyostil@chromium.org.

The reason for reverting is: The reason for reverting is: Broke some downstream
unit tests.

BUG=368064..

[Sami, 2014-04-29 15:51:51]

FYI, ended up reverting manually with https://codereview.chromium.org/254883004

[Sami, 2014-04-30 15:48:43]

Turns out we were just missing a call to SetNeedsAnimate() in
LTHI::DidChangeTopControlsPosition(). We need to *both* draw to show the new top
controls offset and animate to tick any possible top controls animation for the
next frame. Fixed and added a test here too.

I'll reland with the fix.

[Sami, 2014-04-30 15:49:40]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-04-30 15:50:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/206793003/240001

[commit-bot: I haz the power, 2014-04-30 15:57:20]

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

[commit-bot: I haz the power, 2014-04-30 15:57:21]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Sami, 2014-04-30 16:18:46]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-04-30 16:18:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/206793003/260001

[commit-bot: I haz the power, 2014-04-30 21:24:36]

Change committed as 267338