Changes sequence of docked animations when evicting windows from dock.

Changes sequence of docked animations when evicting windows from dock. Windows that are restored into docked area are slid from below rather than from above as before. Additionally minimizing animation for the windows that no longer fit in the dock is slowed down in hopes of making it less confusing.
To allow this a new ScopedLayerAnimationSettings::LockTransitionDuration method is introduced which locks the current animation transition duration until the ScopedLayerAnimationSettings object that invoked it goes out of scope.

BUG=323188
TEST=ash_unittests --gtest_filter=WindowAnimationsTest.LockAnimationDuration

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

[varkha, 2013-11-22 03:54:36]

vollick@, Could you please take a look and see if you can suggest an acceptable
way to slow down minimizing animation without creating a custom animation (and
without using a global test-oriented ScopedAnimationDurationScaleMode). I think
I like the exact result of 4x slower sequence. Do you think adding animation
speed multiplier to ScopedLayerAnimationSettings is a right approach? Many stock
animations today are using hard-coded values. Would it be a god idea to add this
ability to all of them?

[Ian Vollick, 2013-11-22 04:07:30]

On 2013/11/22 03:54:36, varkha wrote:
> vollick@, Could you please take a look and see if you can suggest an
acceptable
> way to slow down minimizing animation without creating a custom animation (and
> without using a global test-oriented ScopedAnimationDurationScaleMode). I
think
> I like the exact result of 4x slower sequence. Do you think adding animation
> speed multiplier to ScopedLayerAnimationSettings is a right approach? Many
stock
> animations today are using hard-coded values. Would it be a god idea to add
this
> ability to all of them?

Won't ScopedLayerAnimationSettings::SetTransitionDuration do what you need?

[varkha, 2013-11-22 05:12:20]

> Won't ScopedLayerAnimationSettings::SetTransitionDuration do what you need?

I was hoping it would but minimizing animation (and some other animations)
ignore that. AnimateHideWindow_Minimize uses
kLayerAnimationsForMinimizeDurationMS. I have tried in patch set 2 to make it so
that the scoped SetTransitionDuration will indeed be respected. Please take a
look. I hope there is not much harm in opening access to a const
GetTransitionDuration. It seems a far lesser evil than exposing LayerAnimator to
ash friends such as AddLayerAnimationsForMinimize.
Can you think of any stock animations that install scoped settings that used to
be ignored but now will be erroneously respected for minimize / restore
animations?
Thanks!

[varkha, 2013-11-25 16:06:22]

vollick@, ping to look at proposed changes.

[Ian Vollick, 2013-11-25 21:58:04]

On 2013/11/25 16:06:22, varkha wrote:
> vollick@, ping to look at proposed changes.

So sorry for the delay. I'd be much happier if we'd make it possible to set a
'final' duration on a scoped layer animation settings object.

As I understand it, the problem is that subsequent code will create their own
scoped layer animation setting instance and clobber whatever duration you'd
previously set. It'd be nice if you could lock this value. Would this be a
possibility?

[varkha, 2013-11-27 18:18:02]

vollick@, could you please take another look? I've added ability to lock the
duration. Is this what you had in mind? Do you think it is worthwhile to restore
the previous value of locked_transition_duration_ in destructor rather than just
clearing it? Do you foresee nested scoped locked duration (I thought this would
be an overkill)?

[Ian Vollick, 2013-11-28 00:34:30]

On 2013/11/27 18:18:02, varkha wrote:
> vollick@, could you please take another look? I've added ability to lock the
> duration. Is this what you had in mind? Do you think it is worthwhile to
restore
> the previous value of locked_transition_duration_ in destructor rather than
just
> clearing it? Do you foresee nested scoped locked duration (I thought this
would
> be an overkill)?

No, not quite. It looks like it's still possible to create a scoped layer
animation settings (SLAS) object in two nested scopes that both set the 'locked'
duration.

What I'm suggesting is that you add a boolean to LayerAnimator,
transition_duration_is_locked_ or something. When the transition duration is
locked, it cannot be reset. It'd be great if the setter were only visible to
SLAS via friendship, too, so that it's not accidentally monkeyed with by other
code.

[Ian Vollick, 2013-11-28 00:40:55]

> Do you foresee nested scoped locked duration (I thought this would be an
overkill)?

I would like to be prepared for this, even if it's unlikely.

[varkha, 2013-11-28 16:19:31]

Thanks. Can you please see if this is better? I considered  this approach before
but thought that I would like to be able to unlock the duration and have it
automagically restore to the default that would have been set but ignored
(rather than not set). But I guess this is more inviting for hacky animation
changes mid-stack.

[varkha, 2013-11-28 21:50:42]

https://codereview.chromium.org/82573002/diff/210001/ash/wm/window_animations...
File ash/wm/window_animations_unittest.cc (right):

https://codereview.chromium.org/82573002/diff/210001/ash/wm/window_animations...
ash/wm/window_animations_unittest.cc:139: TEST_F(WindowAnimationsTest,
LockAnimationDuration) {
I thought a test for the new ability would be useful too.

[Ian Vollick, 2013-11-29 04:33:31]

I really like this. Thanks for making these changes. I just have a few small
concerns, but this seems very close.

https://codereview.chromium.org/82573002/diff/210001/ui/compositor/scoped_lay...
File ui/compositor/scoped_layer_animation_settings.cc (right):

https://codereview.chromium.org/82573002/diff/210001/ui/compositor/scoped_lay...
ui/compositor/scoped_layer_animation_settings.cc:125:
is_transition_duration_locked_ = is_locked;
What is SLAS::is_transition_duration_locked_ used for? (Can it be deleted?)

https://codereview.chromium.org/82573002/diff/210001/ui/compositor/scoped_lay...
File ui/compositor/scoped_layer_animation_settings.h (right):

https://codereview.chromium.org/82573002/diff/210001/ui/compositor/scoped_lay...
ui/compositor/scoped_layer_animation_settings.h:42: void
SetTransitionDurationLocked(bool is_locked);
How would you feel about changing this to void LockTransitionDuration()? It
seems scary to me that at an inner scope we might just unlock the locked
duration. It seems safer to me if the only thing that could unlock the duration
is the SLAS destructor.

[varkha, 2013-11-29 06:05:53]

Thanks! I like it more indeed with less corner cases to worry about.

https://codereview.chromium.org/82573002/diff/210001/ui/compositor/scoped_lay...
File ui/compositor/scoped_layer_animation_settings.cc (right):

https://codereview.chromium.org/82573002/diff/210001/ui/compositor/scoped_lay...
ui/compositor/scoped_layer_animation_settings.cc:125:
is_transition_duration_locked_ = is_locked;
On 2013/11/29 04:33:31, Ian Vollick wrote:
> What is SLAS::is_transition_duration_locked_ used for? (Can it be deleted?)

Done.

https://codereview.chromium.org/82573002/diff/210001/ui/compositor/scoped_lay...
File ui/compositor/scoped_layer_animation_settings.h (right):

https://codereview.chromium.org/82573002/diff/210001/ui/compositor/scoped_lay...
ui/compositor/scoped_layer_animation_settings.h:42: void
SetTransitionDurationLocked(bool is_locked);
On 2013/11/29 04:33:31, Ian Vollick wrote:
> How would you feel about changing this to void LockTransitionDuration()? It
> seems scary to me that at an inner scope we might just unlock the locked
> duration. It seems safer to me if the only thing that could unlock the
duration
> is the SLAS destructor.

Done. I like it - less things to test.

[varkha, 2013-11-29 06:20:15]

+jamescook for OWNERS review in ash/wm. This changes the sequence of animations
following the last UI review of docking behaviour. Thanks!

[varkha, 2013-12-02 16:31:36]

Pinging this issue. Thanks!

[Ian Vollick, 2013-12-02 17:03:48]

On 2013/12/02 16:31:36, varkha wrote:
> Pinging this issue. Thanks!

ui/compositor lgtm.

[James Cook, 2013-12-02 18:33:50]

ash/wm LGTM with a few nits

https://codereview.chromium.org/82573002/diff/290009/ash/wm/window_animations...
File ash/wm/window_animations_unittest.cc (right):

https://codereview.chromium.org/82573002/diff/290009/ash/wm/window_animations...
ash/wm/window_animations_unittest.cc:140: ui::ScopedAnimationDurationScaleMode
long_duration(
Did you mean normal_duration? Or is this long relative to something else?

https://codereview.chromium.org/82573002/diff/290009/ash/wm/window_animations...
ash/wm/window_animations_unittest.cc:189: class MinimizeAnimationObserver :
public ui::LayerAnimationObserver {
optional nit: It's unusual to see classes declared in the middle of functions in
Chrome. There's no style guide entry about it, so you're welcome to keep it
here, it just seemed a bit odd to me as a reader.

https://codereview.chromium.org/82573002/diff/290009/ash/wm/window_animations...
ash/wm/window_animations_unittest.cc:191:
MinimizeAnimationObserver(ui::LayerAnimator* animator)
nit: explicit

https://codereview.chromium.org/82573002/diff/290009/ash/wm/window_animations...
ash/wm/window_animations_unittest.cc:205: animator_->RemoveObserver(this);
Do you need to remove the observer in two places?

[varkha, 2013-12-02 20:18:08]

Thanks for the review!

https://codereview.chromium.org/82573002/diff/290009/ash/wm/window_animations...
File ash/wm/window_animations_unittest.cc (right):

https://codereview.chromium.org/82573002/diff/290009/ash/wm/window_animations...
ash/wm/window_animations_unittest.cc:140: ui::ScopedAnimationDurationScaleMode
long_duration(
On 2013/12/02 18:33:51, James Cook wrote:
> Did you mean normal_duration? Or is this long relative to something else?

I was meaning to do something else and ended up leaving it intact but did not
rename the variable back. Nice catch. I think we want to leave the animation
timing at "NORMAL" for this test.

https://codereview.chromium.org/82573002/diff/290009/ash/wm/window_animations...
ash/wm/window_animations_unittest.cc:189: class MinimizeAnimationObserver :
public ui::LayerAnimationObserver {
On 2013/12/02 18:33:51, James Cook wrote:
> optional nit: It's unusual to see classes declared in the middle of functions
in
> Chrome. There's no style guide entry about it, so you're welcome to keep it
> here, it just seemed a bit odd to me as a reader.

Done.

https://codereview.chromium.org/82573002/diff/290009/ash/wm/window_animations...
ash/wm/window_animations_unittest.cc:191:
MinimizeAnimationObserver(ui::LayerAnimator* animator)
On 2013/12/02 18:33:51, James Cook wrote:
> nit: explicit

Done.

https://codereview.chromium.org/82573002/diff/290009/ash/wm/window_animations...
ash/wm/window_animations_unittest.cc:205: animator_->RemoveObserver(this);
On 2013/12/02 18:33:51, James Cook wrote:
> Do you need to remove the observer in two places?

I wanted to add an assert about !HasObserver in the destructor but we don't have
that method here. RemoveObserver is safe here to be called more than once so
there is no harm either way. I will remove the call in destructor and add a
comment instead.

[commit-bot: I haz the power, 2013-12-02 20:20:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/82573002/330001

[commit-bot: I haz the power, 2013-12-02 22:35:53]

Change committed as 238212