Add new animations in second implementation.

Added new lock/shutdown implementations in session_state_controller_impl2/session_state_animator.

This CL is a follow-up for https://codereview.chromium.org/11230050/.

BUG=138171, 139461
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163790

[Daniel Erat, 2012-10-22 17:45:48]

https://codereview.chromium.org/11220002/diff/5001/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/11220002/diff/5001/ash/ash_switches.h#newcode29
ash/ash_switches.h:29: ASH_EXPORT extern const char
kAshNewLockAnimationsDisabled[];
why two flags?  i'd expect you to just add kAshNewLockAnimationsEnabled and use
that for testing until you're ready to turn it on by default.

https://codereview.chromium.org/11220002/diff/5001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/11220002/diff/5001/ash/root_window_controller...
ash/root_window_controller.cc:568:
CreateContainer(kShellWindowId_TopmostContainer,
give this a better name than "topmost".  "PowerButtonAnimationContainer"?

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
File ash/wm/session_state_animator.cc (right):

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:13: #include
"ui/compositor/layer_animation_observer.h"
keep this list alphabetized

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:110: // Fades |window| to a |target_opacity|
for a |lengthMs| milliseconds.
s/lengthMs/length_ms/

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:111: void
StartPartialFadeAnimation(aura::Window* window, float target_opacity,
one argument per line

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:112: int length_ms,
use base::TimeDelta instead of int

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:148: void LiftUpWindow(aura::Window* window,
int length_ms) {
pass in base::TimeDelta here instead of int

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:158: void PutDownWindow(aura::Window* window,
int length_ms) {
ditto

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:181: //Drop foreground once animation is over.
add space between "//" and "Drop"

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:188: //Drop foreground once animation is over.
same here

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:420: LOG(ERROR) <<
"SessionStateAnimator::CreateForeground()";
delete this

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:421: if (!foreground_.get()) {
nit: little bit cleaner to just do:

  if (foreground_.get())
    return;

  ...

so you don't need to indent the whole body

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:426: foreground_.reset(new
ColoredWindowController(window, "WhiteForeground"));
"SessionStateAnimatorForeground"?

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
File ash/wm/session_state_animator.h (right):

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.h:14: #include
"ash/wm/workspace/colored_window_controller.h"
keep this list alphabetized

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.h:41: ANIMATION_PUT_DOWN,
maybe just ANIMATION_RAISE and ANIMATION_LOWER for these?

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_anima...
ash/wm/session_state_animator.h:143: scoped_ptr<ColoredWindowController>
foreground_;
add a comment describing what this is

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_contr...
File ash/wm/session_state_controller_impl2.cc (right):

https://codereview.chromium.org/11220002/diff/5001/ash/wm/session_state_contr...
ash/wm/session_state_controller_impl2.cc:5: #include
"ash/wm/session_state_controller_impl2.h"
i didn't review this yet.  do you mind moving it to a second changelist, so
you're just refactoring the existing code in the first change and adding the new
implementation in the second?

[Daniel Erat, 2012-10-23 16:02:11]

LGTM with comments

http://codereview.chromium.org/11220002/diff/11001/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

http://codereview.chromium.org/11220002/diff/11001/ash/shell_window_ids.h#new...
ash/shell_window_ids.h:107: // The topmost container, it is used for power off
animation.
nit: s/it is used/used/

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
File ash/wm/session_state_animator.cc (right):

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:110: // Fades |window| to a |target_opacity|
for a |lengthMs| milliseconds.
nit: s/to a/to/, s/for a/over/, s/lengthMs/length_ms/

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:113: int length_ms,
use base::TimeDelta instead

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:149: void RaiseWindow(aura::Window* window, int
length_ms) {
base::TimeDelta

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:159: void LowerWindow(aura::Window* window, int
length_ms) {
base::TimeDelta

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:169: // Animation observer that would drop
animated Foreground once animation is
nit: s/would/will/, s/Foreground/foreground/

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
File ash/wm/session_state_animator.h (right):

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
ash/wm/session_state_animator.h:117: void CreateForeground();
nit: rename this to ShowForeground() to match ShowBlackLayer

also, please add a comment similar to the one that the *DropLayer() methods have

[Denis Kuznetsov (DE-MUC), 2012-10-23 16:07:30]

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
File ash/wm/session_state_animator.cc (right):

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:113: int length_ms,
On 2012/10/23 16:02:11, Daniel Erat wrote:
> use base::TimeDelta instead
What is the reason for that?
It will require base::TimeDelta::FromMilliseconds(...)) at every call to this
method, but I don't see benefits.

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
File ash/wm/session_state_animator.h (right):

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
ash/wm/session_state_animator.h:117: void CreateForeground();
On 2012/10/23 16:02:11, Daniel Erat wrote:
> nit: rename this to ShowForeground() to match ShowBlackLayer
I wanted to indicate that calling this method does not result in Foreground
being displayed. It actually creates foreground, but explicitly sets 0 opacity
to layer that contains it.

[Daniel Erat, 2012-10-23 16:13:02]

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
File ash/wm/session_state_animator.cc (right):

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
ash/wm/session_state_animator.cc:113: int length_ms,
On 2012/10/23 16:07:30, Denis Kuznetsov wrote:
> On 2012/10/23 16:02:11, Daniel Erat wrote:
> > use base::TimeDelta instead
> What is the reason for that?
> It will require base::TimeDelta::FromMilliseconds(...)) at every call to this
> method, but I don't see benefits.

It makes the units obvious to anyone who's reading the code.

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
File ash/wm/session_state_animator.h (right):

http://codereview.chromium.org/11220002/diff/11001/ash/wm/session_state_anima...
ash/wm/session_state_animator.h:117: void CreateForeground();
On 2012/10/23 16:07:30, Denis Kuznetsov wrote:
> On 2012/10/23 16:02:11, Daniel Erat wrote:
> > nit: rename this to ShowForeground() to match ShowBlackLayer
> I wanted to indicate that calling this method does not result in Foreground
> being displayed. It actually creates foreground, but explicitly sets 0 opacity
> to layer that contains it.

Sounds reasonable.  Please document this in a comment, then:

"Create |foreground_| layer if it doesn't already exist, but makes it completely
transparent."

[commit-bot: I haz the power, 2012-10-24 00:50:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/11220002/15001

[commit-bot: I haz the power, 2012-10-24 01:52:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-24 06:30:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/11220002/15001

[commit-bot: I haz the power, 2012-10-24 06:38:44]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-24 06:53:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/11220002/15001

[commit-bot: I haz the power, 2012-10-24 08:23:09]

Change committed as 163790