MacViews: Disable animations when running views_unittests.

MacViews: Disable animations when running views_unittests.

Disable animations to get TextfieldTest.DragToSelect to pass.
AuraTestHelper disables animations, so they were already disabled on non-Mac.
This change creates ViewsTestHelperMac that disables animation the same way
as ViewsTestHelperAura does.

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

[msw, 2014-07-07 23:11:44]

ScopedAnimationDurationScaleMode instances are more commonly owned by
*TestHelper classes, more specific test classes, or tests themselves. Perhaps
this belongs in ViewsTestHelper (like AshTestHelper, AuraTestHelper),
TextfieldTest, or TextfieldTest.DragToSelect (like
WidgetTest.CloseWidgetWhileAnimating).

[tapted, 2014-07-07 23:25:53]

I think adding a ViewsTestHelperMac would be most consistent -- there's already
a ViewsTestHelper::Create function, so you'd just have to change the guard for
the stub in views_test_helper.cc to include mac

[Andre, 2014-07-08 00:08:32]

On 2014/07/07 23:25:53, tapted wrote:
> I think adding a ViewsTestHelperMac would be most consistent -- there's
already
> a ViewsTestHelper::Create function, so you'd just have to change the guard for
> the stub in views_test_helper.cc to include mac

I'm not sure about that. This is not platform specific code and I think we want
this setup to be the same for all platforms.
WDYT about putting this code in ViewsTestHelper?

[tapted, 2014-07-08 00:35:44]

On 2014/07/08 00:08:32, Andre wrote:
> On 2014/07/07 23:25:53, tapted wrote:
> > I think adding a ViewsTestHelperMac would be most consistent -- there's
> already
> > a ViewsTestHelper::Create function, so you'd just have to change the guard
for
> > the stub in views_test_helper.cc to include mac
> 
> I'm not sure about that. This is not platform specific code and I think we
want
> this setup to be the same for all platforms.
> WDYT about putting this code in ViewsTestHelper?

I think there are things in ui/aura that need it to be in AuraTestHelper, since
they can't depend on views (e.g. ui/aura/window_unittest.cc). So (assuming it
works) it might mean a downside of having it in both ViewsTestHelper and
AuraTestHelper (unless removing it from AuraTestHelper results in zero
regressions/flakes, but that sounds risky).

[Andre, 2014-07-08 00:43:09]

On 2014/07/08 00:35:44, tapted wrote:
> On 2014/07/08 00:08:32, Andre wrote:
> > On 2014/07/07 23:25:53, tapted wrote:
> > > I think adding a ViewsTestHelperMac would be most consistent -- there's
> > already
> > > a ViewsTestHelper::Create function, so you'd just have to change the guard
> for
> > > the stub in views_test_helper.cc to include mac
> > 
> > I'm not sure about that. This is not platform specific code and I think we
> want
> > this setup to be the same for all platforms.
> > WDYT about putting this code in ViewsTestHelper?
> 
> I think there are things in ui/aura that need it to be in AuraTestHelper,
since
> they can't depend on views (e.g. ui/aura/window_unittest.cc). So (assuming it
> works) it might mean a downside of having it in both ViewsTestHelper and
> AuraTestHelper (unless removing it from AuraTestHelper results in zero
> regressions/flakes, but that sounds risky).

Yes, I mean having it in both AuraTestHelper and ViewsTestHelper (it works).
There is that downside of redundant setup on aura, but I think it's a better
home for this platform neutral code.

[tapted, 2014-07-08 00:56:58]

On 2014/07/08 00:43:09, Andre wrote:
> On 2014/07/08 00:35:44, tapted wrote:
> > On 2014/07/08 00:08:32, Andre wrote:
> > > On 2014/07/07 23:25:53, tapted wrote:
> > > > I think adding a ViewsTestHelperMac would be most consistent -- there's
> > > already
> > > > a ViewsTestHelper::Create function, so you'd just have to change the
guard
> > for
> > > > the stub in views_test_helper.cc to include mac
> > > 
> > > I'm not sure about that. This is not platform specific code and I think we
> > want
> > > this setup to be the same for all platforms.
> > > WDYT about putting this code in ViewsTestHelper?
> > 
> > I think there are things in ui/aura that need it to be in AuraTestHelper,
> since
> > they can't depend on views (e.g. ui/aura/window_unittest.cc). So (assuming
it
> > works) it might mean a downside of having it in both ViewsTestHelper and
> > AuraTestHelper (unless removing it from AuraTestHelper results in zero
> > regressions/flakes, but that sounds risky).
> 
> Yes, I mean having it in both AuraTestHelper and ViewsTestHelper (it works).
> There is that downside of redundant setup on aura, but I think it's a better
> home for this platform neutral code.

sgtm if owners are happy with that downside. Although you may need to take care
to ensure the destruction order is the reverse of the creation order of the
ScopedAnimationDurationScaleMode to avoid potential subtle problems if
animations are triggered during teardown.

[Andre, 2014-07-08 01:07:07]

On 2014/07/08 00:56:58, tapted wrote:
> On 2014/07/08 00:43:09, Andre wrote:
> > On 2014/07/08 00:35:44, tapted wrote:
> > > On 2014/07/08 00:08:32, Andre wrote:
> > > > On 2014/07/07 23:25:53, tapted wrote:
> > > > > I think adding a ViewsTestHelperMac would be most consistent --
there's
> > > > already
> > > > > a ViewsTestHelper::Create function, so you'd just have to change the
> guard
> > > for
> > > > > the stub in views_test_helper.cc to include mac
> > > > 
> > > > I'm not sure about that. This is not platform specific code and I think
we
> > > want
> > > > this setup to be the same for all platforms.
> > > > WDYT about putting this code in ViewsTestHelper?
> > > 
> > > I think there are things in ui/aura that need it to be in AuraTestHelper,
> > since
> > > they can't depend on views (e.g. ui/aura/window_unittest.cc). So (assuming
> it
> > > works) it might mean a downside of having it in both ViewsTestHelper and
> > > AuraTestHelper (unless removing it from AuraTestHelper results in zero
> > > regressions/flakes, but that sounds risky).
> > 
> > Yes, I mean having it in both AuraTestHelper and ViewsTestHelper (it works).
> > There is that downside of redundant setup on aura, but I think it's a better
> > home for this platform neutral code.
> 
> sgtm if owners are happy with that downside. Although you may need to take
care
> to ensure the destruction order is the reverse of the creation order of the
> ScopedAnimationDurationScaleMode to avoid potential subtle problems if
> animations are triggered during teardown.

Good point! The destruction order shouldn't matter in tests, but it feels messy.
I'm now liking more your suggestion of creating ViewsTestHelperMac.

[msw, 2014-07-08 01:19:46]

On 2014/07/08 01:07:07, Andre wrote:
> On 2014/07/08 00:56:58, tapted wrote:
> > On 2014/07/08 00:43:09, Andre wrote:
> > > On 2014/07/08 00:35:44, tapted wrote:
> > > > On 2014/07/08 00:08:32, Andre wrote:
> > > > > On 2014/07/07 23:25:53, tapted wrote:
> > > > > > I think adding a ViewsTestHelperMac would be most consistent --
> there's
> > > > > already
> > > > > > a ViewsTestHelper::Create function, so you'd just have to change the
> > guard
> > > > for
> > > > > > the stub in views_test_helper.cc to include mac
> > > > > 
> > > > > I'm not sure about that. This is not platform specific code and I
think
> we
> > > > want
> > > > > this setup to be the same for all platforms.
> > > > > WDYT about putting this code in ViewsTestHelper?
> > > > 
> > > > I think there are things in ui/aura that need it to be in
AuraTestHelper,
> > > since
> > > > they can't depend on views (e.g. ui/aura/window_unittest.cc). So
(assuming
> > it
> > > > works) it might mean a downside of having it in both ViewsTestHelper and
> > > > AuraTestHelper (unless removing it from AuraTestHelper results in zero
> > > > regressions/flakes, but that sounds risky).
> > > 
> > > Yes, I mean having it in both AuraTestHelper and ViewsTestHelper (it
works).
> > > There is that downside of redundant setup on aura, but I think it's a
better
> > > home for this platform neutral code.
> > 
> > sgtm if owners are happy with that downside. Although you may need to take
> care
> > to ensure the destruction order is the reverse of the creation order of the
> > ScopedAnimationDurationScaleMode to avoid potential subtle problems if
> > animations are triggered during teardown.
> 
> Good point! The destruction order shouldn't matter in tests, but it feels
messy.
> I'm now liking more your suggestion of creating ViewsTestHelperMac.

Maybe ScopedAnimationDurationScaleMode should no-op (and not restore anything on
destruction) if the setting is already correct. +CC sky, ajuma for potential
advice.

[ajuma, 2014-07-08 14:04:41]

On 2014/07/08 01:19:46, msw wrote:
> On 2014/07/08 01:07:07, Andre wrote:
> > On 2014/07/08 00:56:58, tapted wrote:
> > > On 2014/07/08 00:43:09, Andre wrote:
> > > > On 2014/07/08 00:35:44, tapted wrote:
> > > > > On 2014/07/08 00:08:32, Andre wrote:
> > > > > > On 2014/07/07 23:25:53, tapted wrote:
> > > > > > > I think adding a ViewsTestHelperMac would be most consistent --
> > there's
> > > > > > already
> > > > > > > a ViewsTestHelper::Create function, so you'd just have to change
the
> > > guard
> > > > > for
> > > > > > > the stub in views_test_helper.cc to include mac
> > > > > > 
> > > > > > I'm not sure about that. This is not platform specific code and I
> think
> > we
> > > > > want
> > > > > > this setup to be the same for all platforms.
> > > > > > WDYT about putting this code in ViewsTestHelper?
> > > > > 
> > > > > I think there are things in ui/aura that need it to be in
> AuraTestHelper,
> > > > since
> > > > > they can't depend on views (e.g. ui/aura/window_unittest.cc). So
> (assuming
> > > it
> > > > > works) it might mean a downside of having it in both ViewsTestHelper
and
> > > > > AuraTestHelper (unless removing it from AuraTestHelper results in zero
> > > > > regressions/flakes, but that sounds risky).
> > > > 
> > > > Yes, I mean having it in both AuraTestHelper and ViewsTestHelper (it
> works).
> > > > There is that downside of redundant setup on aura, but I think it's a
> better
> > > > home for this platform neutral code.
> > > 
> > > sgtm if owners are happy with that downside. Although you may need to take
> > care
> > > to ensure the destruction order is the reverse of the creation order of
the
> > > ScopedAnimationDurationScaleMode to avoid potential subtle problems if
> > > animations are triggered during teardown.
> > 
> > Good point! The destruction order shouldn't matter in tests, but it feels
> messy.
> > I'm now liking more your suggestion of creating ViewsTestHelperMac.
> 
> Maybe ScopedAnimationDurationScaleMode should no-op (and not restore anything
on
> destruction) if the setting is already correct. +CC sky, ajuma for potential
> advice.

This still seems to create dependency on destruction order: if the first
ScopedAnimationDurationScaleMode is destroyed before the second (no-op) one, it
will restore non-zero animation duration when destroyed.

Instead, to make it easier to identify when we have a destruction order problem,
we could add a DCHECK to the destructor that verifies that duration_scale_mode_
hasn't changed from the value it was set to (we'd need to keep a copy of this
value).

[Andre, 2014-07-08 17:47:49]

I've created ViewsTestHelperMac to disable the animation (patch #2 ).

[msw, 2014-07-08 19:15:03]

LGTM; I agree with "The destruction order shouldn't matter in tests, but it
feels messy."

[Andre, 2014-07-08 20:15:35]

Sadrul, can you review as owner?

[sadrul, 2014-07-10 14:21:56]

lgtm

[Andre, 2014-07-10 16:11:20]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-07-10 16:13:59]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andresantoso@chromium.org/378663002/40001

[commit-bot: I haz the power, 2014-07-10 18:34:06]

Change committed as 282385