Introduce NON_ZERO_DURATION for animation unit tests

Introduce NON_ZERO_DURATION for animation unit tests

Animations are usually disabled in unit tests for performance. However, to
test the animation system itself some tests request "normal" durations.
Under some conditions (remote desktop, animation disabled for accessibility)
the "normal" duration could still be zero.

This was causing test failures and use-after-frees in ash_unittests on the
drmemory bots and for developers using remote desktop.

Introduce NON_ZERO_DURATION to signal that animation must be enabled and the
duration must be non-zero, but other than that can be as short as possible.

BUG=397478, 396969
TEST=ash_unittests on drmemory, Windows and Chrome OS bots

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

[James Cook, 2014-07-26 00:28:25]

oshima, PTAL

[oshima, 2014-07-26 00:53:26]

nice! lgtm with one suggestion.

https://codereview.chromium.org/420013002/diff/1/ui/compositor/layer_animatio...
File ui/compositor/layer_animation_element.cc (right):

https://codereview.chromium.org/420013002/diff/1/ui/compositor/layer_animatio...
ui/compositor/layer_animation_element.cc:28: const int kTestDurationScaleFactor
= 100;

Can we rename to something like XxxDuratinoMultiplier/ XxxDurationDivider?

Having the same value for Slow/Fast is confusing...

[James Cook, 2014-07-26 04:02:32]

ajuma, PTAL for ui/compositor and animation code.

https://codereview.chromium.org/420013002/diff/1/ui/compositor/layer_animatio...
File ui/compositor/layer_animation_element.cc (right):

https://codereview.chromium.org/420013002/diff/1/ui/compositor/layer_animatio...
ui/compositor/layer_animation_element.cc:28: const int kTestDurationScaleFactor
= 100;
On 2014/07/26 00:53:26, oshima wrote:
> 
> Can we rename to something like XxxDuratinoMultiplier/ XxxDurationDivider?
> 
> Having the same value for Slow/Fast is confusing...

Done.

[ajuma, 2014-07-26 14:34:19]

lgtm with one nit:

https://codereview.chromium.org/420013002/diff/20001/ui/compositor/layer_anim...
File ui/compositor/layer_animator_unittest.cc (right):

https://codereview.chromium.org/420013002/diff/20001/ui/compositor/layer_anim...
ui/compositor/layer_animator_unittest.cc:1894:
ScopedAnimationDurationScaleMode::TEST_DURATION);
The rest of the this test assumes that an unscaled duration is used (since it
calls animator->Step() with a time computed using unscaled duration), so we
should continue to use NORMAL_DURATION here.

[James Cook, 2014-07-28 04:42:35]

sky, can I get OWNERS for ui/* ?

https://codereview.chromium.org/420013002/diff/20001/ui/compositor/layer_anim...
File ui/compositor/layer_animator_unittest.cc (right):

https://codereview.chromium.org/420013002/diff/20001/ui/compositor/layer_anim...
ui/compositor/layer_animator_unittest.cc:1894:
ScopedAnimationDurationScaleMode::TEST_DURATION);
On 2014/07/26 14:34:19, ajuma wrote:
> The rest of the this test assumes that an unscaled duration is used (since it
> calls animator->Step() with a time computed using unscaled duration), so we
> should continue to use NORMAL_DURATION here.

Done.

[sky, 2014-07-28 15:35:09]

https://codereview.chromium.org/420013002/diff/60001/ui/compositor/scoped_ani...
File ui/compositor/scoped_animation_duration_scale_mode.h (right):

https://codereview.chromium.org/420013002/diff/60001/ui/compositor/scoped_ani...
ui/compositor/scoped_animation_duration_scale_mode.h:22: TEST_DURATION,
I'm concerned that naming this 'test' implies anyone writing a test should use
this duration, which isn't the case. Tests really want ZERO_DURATION. Is there a
better name we can use?

[James Cook, 2014-07-28 15:47:32]

sky PTAL - How about "non-zero duration" along with the comment about how it's
different from "zero duration"?

[sky, 2014-07-28 15:57:20]

LGTM

[James Cook, 2014-07-28 16:04:40]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2014-07-28 16:04:49]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2014-07-28 16:05:32]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-07-28 16:06:44]

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

[commit-bot: I haz the power, 2014-07-28 20:37:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-28 21:12:52]

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

[commit-bot: I haz the power, 2014-07-28 21:12:53]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2014-07-29 00:11:37]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-07-29 00:13:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamescook@chromium.org/420013002/120001

[commit-bot: I haz the power, 2014-07-29 04:22:13]

Change committed as 286089

[James Cook, 2014-07-29 05:02:48]

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

The reason for reverting is: I suspect this is causing Linux Tests dbg (2)
failures in app_list_unittests:
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29...

Strangely I had green try runs for linux_chromium_rel for app_list_unittests.
.

[Peter Mayo, 2014-07-29 21:44:31]

On 2014/07/29 05:02:48, James Cook wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/413403011/ by mailto:jamescook@chromium.org.
> 
> The reason for reverting is: I suspect this is causing Linux Tests dbg (2)
> failures in app_list_unittests:
>
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29...
> 
> Strangely I had green try runs for linux_chromium_rel for app_list_unittests.
> .

I believe, having tried to reproduce the failure and failed, that the failure
was flake of an unknown origin.

The landed revision in the tree looked good to me.