Remove unneeded ScopedLayerAnimationSettings::LockTransitionDuration()

This CL
- Introduces views::corewm::SetWindowShowAnimationDuration() and views::corewm::SetWindowHideAnimationDuration(). This allows the animation duration for showing and hiding the window to be different.
- Fixes a memory leak in AddLayerAnimationsForRotate()
- Fixes calls to views::corewm::SetWindowVisibilityAnimationType() and views::corewm::SetWindowVisibilityAnimationDuration(). In particular, the animation type and animation duration were not properly reset. So docking a window would permanently affect the duration of the show and hide animations.
- Removes unneeded ScopedLayerAnimationSettings::LockTransitionDuration(). This API is not currently necessary so it is best for it to not exist at this point in time.

BUG=323791
TEST=None
R=varkha, oshima
TBR=vollick

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

[pkotwicz, 2013-12-23 01:12:06]

Valery, can you please take a look? Once you are happy, I will remove
ScopedLayerAnimationSettings:: LockTransitionDuration().

https://codereview.chromium.org/105673008/diff/80001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_layout_manager.cc (left):

https://codereview.chromium.org/105673008/diff/80001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_layout_manager.cc:770: animation_type =
views::corewm::WINDOW_VISIBILITY_ANIMATION_TYPE_DROP;
WINDOW_VISIBILITY_ANIMATION_TYPE_DROP is the default animation type.

https://codereview.chromium.org/105673008/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/105673008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_display_host_impl.cc:983:
views::corewm::SetWindowHideAnimationDuration(
This call is a no-op. I filed crbug.com/330469

https://codereview.chromium.org/105673008/diff/80001/ui/views/corewm/window_a...
File ui/views/corewm/window_animations.cc (right):

https://codereview.chromium.org/105673008/diff/80001/ui/views/corewm/window_a...
ui/views/corewm/window_animations.cc:387: if (!show) {
Previously, we were leaking memory here.

[varkha, 2014-01-02 20:24:25]

Just a question. So this CL is not changing any visible transition duration or
type, correct?

https://codereview.chromium.org/105673008/diff/80001/ui/views/corewm/window_a...
File ui/views/corewm/window_animations.cc (right):

https://codereview.chromium.org/105673008/diff/80001/ui/views/corewm/window_a...
ui/views/corewm/window_animations.cc:387: if (!show) {
On 2013/12/23 01:12:06, pkotwicz wrote:
> Previously, we were leaking memory here.

Why wasn't the hiding observer deleting itself upon completion of the animation?

[pkotwicz, 2014-01-02 20:29:53]

This CL should not result in any visible changes

[pkotwicz, 2014-01-02 20:30:00]

https://codereview.chromium.org/105673008/diff/80001/ui/views/corewm/window_a...
File ui/views/corewm/window_animations.cc (right):

https://codereview.chromium.org/105673008/diff/80001/ui/views/corewm/window_a...
ui/views/corewm/window_animations.cc:387: if (!show) {
We were never setting the HidingWindowAnimationObserver as an observer of an
animation. Hence, the observer was never getting notified when the animation
terminated

[varkha, 2014-01-02 21:35:27]

https://codereview.chromium.org/105673008/diff/80001/ui/views/corewm/window_a...
File ui/views/corewm/window_animations.cc (right):

https://codereview.chromium.org/105673008/diff/80001/ui/views/corewm/window_a...
ui/views/corewm/window_animations.cc:387: if (!show) {
On 2014/01/02 20:30:00, pkotwicz wrote:
> We were never setting the HidingWindowAnimationObserver as an observer of an
> animation. Hence, the observer was never getting notified when the animation
> terminated

I see. Don't you want to use views::corewm::CreateHidingWindowAnimationObserver
as elsewhere. Same in
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/corewm/wi...

[pkotwicz, 2014-01-02 21:51:51]

Valery, can you please take another look?

[varkha, 2014-01-02 22:26:41]

https://codereview.chromium.org/105673008/diff/350001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/105673008/diff/350001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:470: base::TimeDelta());
nit: Could this be called once after if-else?

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
File ash/wm/window_animations.cc (right):

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
ash/wm/window_animations.cc:141:
base::TimeDelta::FromMilliseconds(kLayerAnimationsForMinimizeDurationMS));
Shouldn't this use GetDefaultWindowVisibilityAnimationDuration(window) now?

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
ash/wm/window_animations.cc:158:
base::TimeDelta::FromMilliseconds(kLayerAnimationsForMinimizeDurationMS));
Shouldn't this use GetDefaultWindowVisibilityAnimationDuration(window) now?

[pkotwicz, 2014-01-02 22:45:51]

https://codereview.chromium.org/105673008/diff/350001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/105673008/diff/350001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:470: base::TimeDelta());
Given that the two durations are different, I do not think that there is much to
gain in doing this.

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
File ash/wm/window_animations.cc (right):

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
ash/wm/window_animations.cc:141:
base::TimeDelta::FromMilliseconds(kLayerAnimationsForMinimizeDurationMS));
The default animation durations are different for each animation type.
views/corewm/window_animations.cc knows nothing about
ash/wm/window_animations.cc I could write a version of
GetDefaultWindowVisibilityAnimationDuration*( which handles the animations in
ash/wm/window_animations.cc but I do not think that doing this is particularly
useful.

[varkha, 2014-01-03 03:47:48]

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
File ash/wm/window_animations.cc (right):

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
ash/wm/window_animations.cc:141:
base::TimeDelta::FromMilliseconds(kLayerAnimationsForMinimizeDurationMS));
On 2014/01/02 22:45:51, pkotwicz wrote:
> The default animation durations are different for each animation type.
> views/corewm/window_animations.cc knows nothing about
> ash/wm/window_animations.cc I could write a version of
> GetDefaultWindowVisibilityAnimationDuration*( which handles the animations in
> ash/wm/window_animations.cc but I do not think that doing this is particularly
> useful.
> 

Doesn't GetDefaultWindowVisibilityAnimationDuration currently return 200 (same
as kLayerAnimationsForMinimizeDurationMS) - forgetting about menu window type
for a second?

[pkotwicz, 2014-01-03 04:15:40]

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
File ash/wm/window_animations.cc (right):

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
ash/wm/window_animations.cc:141:
base::TimeDelta::FromMilliseconds(kLayerAnimationsForMinimizeDurationMS));
But kBrightnessGrayscaleFadeDurationMs != 200

[varkha, 2014-01-03 04:39:18]

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
File ash/wm/window_animations.cc (right):

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
ash/wm/window_animations.cc:141:
base::TimeDelta::FromMilliseconds(kLayerAnimationsForMinimizeDurationMS));
On 2014/01/03 04:15:40, pkotwicz wrote:
> But kBrightnessGrayscaleFadeDurationMs != 200
> 

True, but I am not suggesting changing line 187, only here and in hide where you
seem to be using a 200ms constant.

[varkha, 2014-01-03 05:40:54]

lgtm

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
File ash/wm/window_animations.cc (right):

https://codereview.chromium.org/105673008/diff/350001/ash/wm/window_animation...
ash/wm/window_animations.cc:141:
base::TimeDelta::FromMilliseconds(kLayerAnimationsForMinimizeDurationMS));
On 2014/01/03 04:39:19, varkha wrote:
> On 2014/01/03 04:15:40, pkotwicz wrote:
> > But kBrightnessGrayscaleFadeDurationMs != 200
> > 
> 
> True, but I am not suggesting changing line 187, only here and in hide where
you
> seem to be using a 200ms constant.

I agree that this may be an overkill at this stage and the constants are not
necessarily same. Let's keep ash constants separate for now.

[pkotwicz, 2014-01-04 01:26:55]

Oshima, can you please take a look?

[pkotwicz, 2014-01-04 01:29:43]

The changes in ui/compositor/ are a partial revert of
https://codereview.chromium.org/82573002

[oshima, 2014-01-08 00:29:12]

lgtm

https://codereview.chromium.org/105673008/diff/550001/ui/compositor/layer_ani...
File ui/compositor/layer_animator.h (right):

https://codereview.chromium.org/105673008/diff/550001/ui/compositor/layer_ani...
ui/compositor/layer_animator.h:299: base::TimeDelta GetTransitionDuration()
const;
maybe we should rename this (and below) to {Get|Set}DefaultTransitionDuration()?
(separate CL is fine)

[commit-bot: I haz the power, 2014-01-15 00:53:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/105673008/650001

[commit-bot: I haz the power, 2014-01-15 14:06:48]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_unittests, unit_tests,
views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-15 19:16:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/105673008/680002

[commit-bot: I haz the power, 2014-01-16 00:22:58]

Change committed as 245031

[pkotwicz, 2014-01-19 00:02:50]

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

The reason for reverting is: Experimentally reverting this CL in case it caused
the crash in crbug.com/335068.

[pkotwicz, 2014-01-19 00:41:22]

Note: The attempt to revert the CL was unsuccessful because of merge conflicts.
It is worth tracking down the root cause properly