Fix focus loss on partial lock.

Fix focus loss on partial lock.

(Add success callback to layer animations, call Window->Hide() upon successful animation)

BUG=162646

[Denis Kuznetsov (DE-MUC), 2012-11-27 17:41:50]

Ian, look at ui/ part.
Daniel, look on ash/wm/ part.

[Daniel Erat, 2012-11-27 18:32:11]

https://codereview.chromium.org/11280188/diff/1/ash/wm/workspace/workspace_an...
File ash/wm/workspace/workspace_animations.cc (right):

https://codereview.chromium.org/11280188/diff/1/ash/wm/workspace/workspace_an...
ash/wm/workspace/workspace_animations.cc:155: base::Closure hide_window_callback
= base::Bind(&HideWindow, window);
add a comment describing why you're doing this

https://codereview.chromium.org/11280188/diff/1/ash/wm/workspace/workspace_an...
ash/wm/workspace/workspace_animations.cc:168: 
nit: remove extra blank line

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator.cc
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.cc:78: int attached_;
nit: rename to num_attached_sequences_ or something similarly descriptive

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.cc:302: properties_to_pause.insert(
pause?

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.cc:315: 
nit: delete extra blank line

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator.h
File ui/compositor/layer_animator.h (right):

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.h:136: void ScheduleSuccessCallbackForProperties(
nit: "Schedule" doesn't make sense to me here, and "Success" seems a bit strange
too.  SetCompletionCallbackForProperties()?

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.h:139: ...);
nit: fix indenting

[Ian Vollick, 2012-11-27 18:35:07]

The implementation in ui/ LGTM.

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator.cc
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.cc:78: int attached_;
DISALLOW_COPY_AND_ASSIGN

[Denis Kuznetsov (DE-MUC), 2012-11-27 18:46:59]

https://codereview.chromium.org/11280188/diff/1/ash/wm/workspace/workspace_an...
File ash/wm/workspace/workspace_animations.cc (right):

https://codereview.chromium.org/11280188/diff/1/ash/wm/workspace/workspace_an...
ash/wm/workspace/workspace_animations.cc:155: base::Closure hide_window_callback
= base::Bind(&HideWindow, window);
On 2012/11/27 18:32:11, Daniel Erat wrote:
> add a comment describing why you're doing this

Done.

https://codereview.chromium.org/11280188/diff/1/ash/wm/workspace/workspace_an...
ash/wm/workspace/workspace_animations.cc:168: 
On 2012/11/27 18:32:11, Daniel Erat wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator.cc
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.cc:78: int attached_;
On 2012/11/27 18:32:11, Daniel Erat wrote:
> nit: rename to num_attached_sequences_ or something similarly descriptive

Done.

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.cc:78: int attached_;
On 2012/11/27 18:35:07, vollick wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.cc:302: properties_to_pause.insert(
On 2012/11/27 18:32:11, Daniel Erat wrote:
> pause?
We create PauseElement (one that does not change any property, but can have
duration), that works as a barrier for set of properties. In this case duration
is 0, so there is no pause actually.

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.cc:315: 
On 2012/11/27 18:32:11, Daniel Erat wrote:
> nit: delete extra blank line

Done.

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator.h
File ui/compositor/layer_animator.h (right):

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.h:136: void ScheduleSuccessCallbackForProperties(
On 2012/11/27 18:32:11, Daniel Erat wrote:
> nit: "Schedule" doesn't make sense to me here, and "Success" seems a bit
strange
> too.  SetCompletionCallbackForProperties()?

Done.

https://codereview.chromium.org/11280188/diff/1/ui/compositor/layer_animator....
ui/compositor/layer_animator.h:139: ...);
On 2012/11/27 18:32:11, Daniel Erat wrote:
> nit: fix indenting

Done.

[Daniel Erat, 2012-11-27 19:03:27]

LGTM, but adding sky@ in case he has any comments about the workspace animation
changes.

Just to double-check, does the focus also get restored correctly when you click
in the omnibox, lock the screen, and then unlock it?

https://codereview.chromium.org/11280188/diff/10002/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_animations.cc (right):

https://codereview.chromium.org/11280188/diff/10002/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_animations.cc:152:
window->layer()->SetVisible(false);
does this get balanced out by a call to SetVisible(true) if the animation is
aborted?

https://codereview.chromium.org/11280188/diff/10002/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_animations.cc:166: );
nit: move this to the end of the previous line

https://codereview.chromium.org/11280188/diff/10002/ui/compositor/layer_anima...
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/11280188/diff/10002/ui/compositor/layer_anima...
ui/compositor/layer_animator.cc:55: virtual void
OnLayerAnimationEnded(LayerAnimationSequence* sequence) {
nit: add OVERRIDE to all overridden methods, along with a comment like this
above them:

// LayerAnimationObserver overrides:

[sky, 2012-11-27 21:19:28]

https://codereview.chromium.org/11280188/diff/10002/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_animations.cc (right):

https://codereview.chromium.org/11280188/diff/10002/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_animations.cc:152:
window->layer()->SetVisible(false);
I don't like that this leaves the workspace window thinking it's visible now.
Seems like we only want this logic for the lock screen. Could we pass in a
boolean indicating if we want this?

[Denis Kuznetsov (DE-MUC), 2012-11-28 15:41:46]

https://chromiumcodereview.appspot.com/11280188/diff/10002/ash/wm/workspace/w...
File ash/wm/workspace/workspace_animations.cc (right):

https://chromiumcodereview.appspot.com/11280188/diff/10002/ash/wm/workspace/w...
ash/wm/workspace/workspace_animations.cc:152:
window->layer()->SetVisible(false);
On 2012/11/27 19:03:27, Daniel Erat wrote:
> does this get balanced out by a call to SetVisible(true) if the animation is
> aborted?
Animation is aborted only by other animation that animates visibility property
of the layer (Callback is created only for VISIBILITY property).

https://chromiumcodereview.appspot.com/11280188/diff/10002/ash/wm/workspace/w...
ash/wm/workspace/workspace_animations.cc:166: );
On 2012/11/27 19:03:27, Daniel Erat wrote:
> nit: move this to the end of the previous line

Done.

https://chromiumcodereview.appspot.com/11280188/diff/10002/ui/compositor/laye...
File ui/compositor/layer_animator.cc (right):

https://chromiumcodereview.appspot.com/11280188/diff/10002/ui/compositor/laye...
ui/compositor/layer_animator.cc:55: virtual void
OnLayerAnimationEnded(LayerAnimationSequence* sequence) {
On 2012/11/27 19:03:27, Daniel Erat wrote:
> nit: add OVERRIDE to all overridden methods, along with a comment like this
> above them:
> 
> // LayerAnimationObserver overrides:

Done.

[sky, 2012-11-28 21:54:40]

LGTM with a name change.

https://codereview.chromium.org/11280188/diff/2005/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_animations.h (right):

https://codereview.chromium.org/11280188/diff/2005/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_animations.h:45: bool can_be_cancelled;
How hide_window_immediately defaulting to true? Also, clearly document what this
is does and what it is intended for.