Make it possible to cancel commits following an animation

Make it possible to cancel commits following an animation

If we didn't invalidate anything when running main thread animations, there is
no need to continue with the commit.

BUG=345584, 347255, 348433
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254558

[Sami, 2014-02-24 17:26:33]

This is a tentative patch and I'm hoping you could tell me why it's a
non-starter. I noticed we haven't allowed animation commits to be canceled all
the way from https://codereview.chromium.org/178123003/, but I'm not sure why.

[enne (OOO), 2014-02-24 17:54:15]

I can't think of any reason why we shouldn't do this.  Sounds good to me.

Can you add an animation-only commit aborting unit test that tests that also
sets animate during update (to test the other code block you moved?)  There
should be some LayerTreeHost aborting and animation unit tests to crib from.

[Sami, 2014-02-24 18:05:51]

Thanks for the encouraging words. Looks like this breaks
LayerTreeHostAnimationTestContinuousAnimate so I'll figure out why first.

[Sami, 2014-02-24 18:39:29]

LayerTreeHostAnimationTestContinuousAnimate was failing because it wasn't
actually painting any new content during Animate(), so the commit was correctly
getting canceled.

I've fixed that and added the test you asked for. PTAL, thanks!

[danakj, 2014-02-24 18:47:21]

This reminded me of https://codereview.chromium.org/133263004/ which I guess was
reverted. If this can stick it might make that easier to land.

The revert was here: https://codereview.chromium.org/141833002

[enne (OOO), 2014-02-24 18:47:57]

lgtm.  Thanks for noticing this, and thanks for the test.  :)

[enne (OOO), 2014-02-24 18:48:00]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-02-24 18:49:36]

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

[commit-bot: I haz the power, 2014-02-24 20:27:46]

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

[commit-bot: I haz the power, 2014-02-24 21:11:12]

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

[commit-bot: I haz the power, 2014-02-24 23:52:05]

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

[commit-bot: I haz the power, 2014-02-24 23:52:06]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Sami, 2014-02-25 14:44:44]

On 2014/02/24 23:52:06, I haz the power (commit-bot) wrote:
> Retried try job too often on mac_rel for step(s) telemetry_unittests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Looks like the telemetry tab screenshot test failed for a similar reason as the
other unit test earlier: it requests an animation but doesn't actually provide
any new content. I've fixed this by making LayerTreeHost lie that there is new
content when it wants to force a redraw. Dana, Enne, any issues with that?

[danakj, 2014-02-25 16:36:59]

I think that LGTM.

[danakj, 2014-02-25 16:38:11]

On 2014/02/25 16:36:59, danakj wrote:
> I think that LGTM.

A quick unit test that would fail without that would be nice. Much faster
turnaround on noticing problems than breaking a telemetry perf test.

[Sami, 2014-02-25 17:16:47]

On 2014/02/25 16:38:11, danakj wrote:
> A quick unit test that would fail without that would be nice. Much faster
> turnaround on noticing problems than breaking a telemetry perf test.

Agreed and added :)

[Sami, 2014-02-25 17:16:52]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-02-25 17:17:21]

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

[danakj, 2014-02-25 18:11:36]

On Tue, Feb 25, 2014 at 12:16 PM, <skyostil@chromium.org> wrote:

> On 2014/02/25 16:38:11, danakj wrote:
>
>> A quick unit test that would fail without that would be nice. Much faster
>> turnaround on noticing problems than breaking a telemetry perf test.
>>
>
> Agreed and added :)
>

thanks!


>
> https://codereview.chromium.org/178123003/
>

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

[commit-bot: I haz the power, 2014-02-25 21:48:00]

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

[commit-bot: I haz the power, 2014-02-25 22:12:40]

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

[commit-bot: I haz the power, 2014-02-25 23:16:19]

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

[commit-bot: I haz the power, 2014-02-26 00:03:11]

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

[Paweł Hajdan Jr., 2014-02-26 05:34:28]

The CQ bit was unchecked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:34:48]

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

[Paweł Hajdan Jr., 2014-02-26 05:42:40]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:50:36]

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

[commit-bot: I haz the power, 2014-02-26 14:31:18]

Change committed as 253439

[waffles, 2014-02-26 18:24:22]

CL reverted in https://codereview.chromium.org/181383005

[Sami, 2014-02-26 18:39:50]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-02-26 18:41:04]

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

[commit-bot: I haz the power, 2014-02-26 19:06:16]

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

[commit-bot: I haz the power, 2014-02-26 19:06:16]

Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg

[danakj, 2014-02-26 19:09:04]

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

https://codereview.chromium.org/178123003/diff/300001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:892: EXPECT_GT(num_draw_layers_,
0);
The first commit will always draw. Do you want this to be >= 1?

https://codereview.chromium.org/178123003/diff/300001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:892: EXPECT_GT(num_draw_layers_,
0);
The first commit will always draw, maybe you want this to be > 1, and do the
EndTest on the 2nd draw?

[Sami, 2014-02-27 12:35:58]

On 2014/02/26 19:09:04, danakj wrote:
>
https://codereview.chromium.org/178123003/diff/300001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_unittest_animation.cc:892:
EXPECT_GT(num_draw_layers_,
> 0);
> The first commit will always draw, maybe you want this to be > 1, and do the
> EndTest on the 2nd draw?

Thanks, I knew I missed something. I've now made it stop end the test after two
draws and only animate twice, so I think I can check that num_draws == 2 here.
(I also fixed the same animate-but-no-new-content bug in cc_perftests.) Could
you do another sanity check please?

[danakj, 2014-02-27 17:43:28]

LGTM

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

https://codereview.chromium.org/178123003/diff/320001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:893: EXPECT_EQ(num_draw_layers_,
2);
order should be: expected, actual

[Sami, 2014-02-27 17:52:19]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-02-27 17:52:55]

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

[enne (OOO), 2014-02-27 17:53:34]

lgtm

[commit-bot: I haz the power, 2014-02-27 20:00:10]

Change committed as 253910

[nduca, 2014-03-02 22:26:09]

Hey sami, this broke the rasterize_and_record_micro benchmark in telemetry:
https://code.google.com/p/chromium/issues/detail?id=348433

Im going to revert it to see if it fixes things.

[Sami, 2014-03-03 16:35:40]

Third time's the charm :) The problem was that calling SetNeedsAnimate() after
SetNeedsCommit() would reset the can_cancel_commit flag to true, causing the
commit to possibly get canceled.

I've fixed this by not touching that flag at all in SetNeedsAnimate() and
instead relying on the commit to set it to true. There's a new test for this,
and one of the original tests needed a tweak to not depend on this.

Thanks for the reviews so far! One more look?

[danakj, 2014-03-03 16:41:46]

Thanks for the new test, LGTM

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

https://codereview.chromium.org/178123003/diff/360001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:930: EXPECT_EQ(3, num_animate_);
Can you test >= 2 instead of == 3 here? If the test ended and didn't do a 3rd
commit it should still pass?

[Sami, 2014-03-03 17:17:36]

On 2014/03/03 16:41:46, danakj wrote:
> Thanks for the new test, LGTM
> 
>
https://codereview.chromium.org/178123003/diff/360001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_unittest_animation.cc (right):
> 
>
https://codereview.chromium.org/178123003/diff/360001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_unittest_animation.cc:930: EXPECT_EQ(3,
num_animate_);
> Can you test >= 2 instead of == 3 here? If the test ended and didn't do a 3rd
> commit it should still pass?

Ah, good idea, done. It's a little unfortunate the test harness is
non-deterministic in this way.

[Sami, 2014-03-03 17:17:44]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-03-03 17:17:58]

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

[commit-bot: I haz the power, 2014-03-03 17:35:38]

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

[danakj, 2014-03-03 17:47:17]

On Mon, Mar 3, 2014 at 12:17 PM, <skyostil@chromium.org> wrote:

> On 2014/03/03 16:41:46, danakj wrote:
>
>> Thanks for the new test, LGTM
>>
>
>
> https://codereview.chromium.org/178123003/diff/360001/cc/
> trees/layer_tree_host_unittest_animation.cc
>
>> File cc/trees/layer_tree_host_unittest_animation.cc (right):
>>
>
>
> https://codereview.chromium.org/178123003/diff/360001/cc/
> trees/layer_tree_host_unittest_animation.cc#newcode930
>
>> cc/trees/layer_tree_host_unittest_animation.cc:930: EXPECT_EQ(3,
>>
> num_animate_);
>
>> Can you test >= 2 instead of == 3 here? If the test ended and didn't do a
>> 3rd
>> commit it should still pass?
>>
>
> Ah, good idea, done. It's a little unfortunate the test harness is
> non-deterministic in this way.
>

It actually should be deterministic now, this was a huge source of flake in
the past, but maybe we change it one day, who knows :)


>
> https://codereview.chromium.org/178123003/
>

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

[commit-bot: I haz the power, 2014-03-03 18:26:52]

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

[commit-bot: I haz the power, 2014-03-03 19:54:47]

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

[commit-bot: I haz the power, 2014-03-03 21:03:41]

Change committed as 254558