Polish lock animation - launcher should do a fade out.

Polish lock animation - launcher should do a fade out.

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

[Nikita (slow), 2012-09-28 20:33:50]

Polishing lock animation for M23. Now launcher is animated into center of the
screen and that reveals 3px dark bar below it.

[Nikita (slow), 2012-09-28 20:37:24]

On 2012/09/28 20:33:50, Nikita Kostylev wrote:
> Now launcher is animated into center of the
> screen and that reveals 3px dark bar below it.

This change changes launcher animation to hide/restore during lock/unlock.

[Daniel Erat, 2012-09-28 20:48:48]

+sky as stacking expert

http://codereview.chromium.org/10990118/diff/2001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

http://codereview.chromium.org/10990118/diff/2001/ash/root_window_controller....
ash/root_window_controller.cc:272: aura::Window* launcher_container =
CreateContainer(
This is a pretty big stacking change, right?  Does this cause any problems?

[Nikita (slow), 2012-09-28 20:55:31]

http://codereview.chromium.org/10990118/diff/2001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

http://codereview.chromium.org/10990118/diff/2001/ash/root_window_controller....
ash/root_window_controller.cc:272: aura::Window* launcher_container =
CreateContainer(
On 2012/09/28 20:48:48, Daniel Erat wrote:
> This is a pretty big stacking change, right?  Does this cause any problems?

I haven't noticed any in my testing. What specific test cases would need to be
checked?

However when I first left launcher_container defined where it was before (lines
317-321) but just changed its parent to root_window I've seen an issue that I
could not click tray when I was inside user session. Moving launcher container
definition here fixed that.

[Daniel Erat, 2012-09-28 21:14:05]

I'd be concerned that it'd now be possible to drag certain types of
windows above the launcher, whereas before they would end up
underneath the launcher when dragged.

On Fri, Sep 28, 2012 at 1:55 PM,  <nkostylev@chromium.org> wrote:
>
>
http://codereview.chromium.org/10990118/diff/2001/ash/root_window_controller.cc
> File ash/root_window_controller.cc (right):
>
>
http://codereview.chromium.org/10990118/diff/2001/ash/root_window_controller....
> ash/root_window_controller.cc:272: aura::Window* launcher_container =
> CreateContainer(
> On 2012/09/28 20:48:48, Daniel Erat wrote:
>>
>> This is a pretty big stacking change, right?  Does this cause any
>
> problems?
>
> I haven't noticed any in my testing. What specific test cases would need
> to be checked?
>
> However when I first left launcher_container defined where it was before
> (lines 317-321) but just changed its parent to root_window I've seen an
> issue that I could not click tray when I was inside user session. Moving
> launcher container definition here fixed that.
>
> http://codereview.chromium.org/10990118/

[Nikita (slow), 2012-09-28 22:05:33]

Could you provide few examples of such window types?

[Daniel Erat, 2012-09-28 22:25:14]

On 2012/09/28 22:05:33, Nikita Kostylev wrote:
> Could you provide few examples of such window types?

Just looking at the change, I'd guess that this could be an issue for all of the
windows that are placed in containers that used to be above LauncherContainer
but are now below it.

I think that this includes panels (do we use these at the moment?),
always-on-top windows (from StackingController::GetDefaultParent(), it looks
like we maybe use this container for all NORMAL and POPUP windows that aren't
modal, which seems unexpected, but maybe I'm misinterpreting this), and the
default container.

Just as a basic sanity test, when you drag a regular browser window down, does
it go under or over the launcher icons?

[Daniel Erat, 2012-09-28 22:26:21]

Sorry, typo in last message.  First sentence should've been:

Just looking at the change, I'd guess that this could be an issue for all of the
windows that are placed in containers that used to be below LauncherContainer
but are now above it.

[sky, 2012-09-28 23:20:28]

I can't think of any problems this will cause.

http://codereview.chromium.org/10990118/diff/2001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

http://codereview.chromium.org/10990118/diff/2001/ash/root_window_controller....
ash/root_window_controller.cc:276: SetUsesScreenCoordinates(launcher_container);
You should update the ids in shell_window_ids too, since that roughly matches
z-ordering. You also need to add a comment as to why the order needs to be the
way it is.

I also think this code would be made clearer if you added it after the last
thing is added to the non_lock_screen_containers since that's better matches
visually how all this fits together.

[Daniel Erat, 2012-09-30 01:22:22]

LGTM if/when Scott is fine with it.

[Nikita (slow), 2012-10-01 14:58:57]

Please take another look. I had to explicitly remove launcher from
non_lock_screen_containers.

In M24 Denis may refactor this.

http://codereview.chromium.org/10990118/diff/2001/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

http://codereview.chromium.org/10990118/diff/2001/ash/root_window_controller....
ash/root_window_controller.cc:272: aura::Window* launcher_container =
CreateContainer(
On 2012/09/28 20:48:48, Daniel Erat wrote:
> This is a pretty big stacking change, right?  Does this cause any problems?

In fact it did. What I've changed was:

* background
* non_lock_screen_containers
  ** ...
  ** App List
  ** System modal windows
* Launcher

This introduced bug that app list and modal dialogs were placed below launcher.

http://codereview.chromium.org/10990118/diff/2001/ash/root_window_controller....
ash/root_window_controller.cc:276: SetUsesScreenCoordinates(launcher_container);
On 2012/09/28 23:20:28, sky wrote:
> I also think this code would be made clearer if you added it after the last
> thing is added to the non_lock_screen_containers since that's better matches
> visually how all this fits together.

Due to stacking issues I had to leave root_window_controller.cc unchanged for
this fix.

[Daniel Erat, 2012-10-01 15:01:06]

lgtm

http://codereview.chromium.org/10990118/diff/14001/ash/wm/power_button_contro...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/10990118/diff/14001/ash/wm/power_button_contro...
ash/wm/power_button_controller.cc:188: if (window->id() ==
internal::kShellWindowId_LauncherContainer) {
nit: remove curly brackets

[Nikita (slow), 2012-10-01 15:51:30]

http://codereview.chromium.org/10990118/diff/14001/ash/wm/power_button_contro...
File ash/wm/power_button_controller.cc (right):

http://codereview.chromium.org/10990118/diff/14001/ash/wm/power_button_contro...
ash/wm/power_button_controller.cc:188: if (window->id() ==
internal::kShellWindowId_LauncherContainer) {
On 2012/10/01 15:01:06, Daniel Erat wrote:
> nit: remove curly brackets

Done.