Add LockLayoutManager responsible for lock container (login/lock).

Add LockLayoutManager responsible for lock container (login/lock).

Previously WorkspaceManager was used for this container which resulted in various
issues when login screen window was not full screen and was supposed to be resized
only by virtual keyboard container. Otherwise panels and shelf were taken into account
even though they are stacked below the lock container.

This CL improves previous fix
https://codereview.chromium.org/231123002

Disable this layout manager with --ash-disable-lock-layout-manager

Verified that existing out-of-box/login/multi-profiles login/lock* virtual keyboard
overscroll/non-overscroll configurations work fine.

Non-overscroll lock screen configuration is updated to use the same behavior as
login in https://codereview.chromium.org/320523003

BUG=375666
TEST=LockLayoutManager.*, existing tests

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

[Nikita (slow), 2014-06-03 17:30:34]

Please review, I'll be working on adding tests in this CL.

A bit more context:

Currently LockScreenContainer uses WorkspaceLayoutManager which is not the right
one since it uses work area definition from the user session i.e. takes docked
windows and shelfs into consideration.

Everything worked fine when all windows in this container were fullscreen. This
has changed with the virtual keyboard support since windows become maximized
(login and multi-profiles login but not lock screen).

Now when you bring up multi-profiles login and you have shelf + docked window
work area was applied to the window resulting in incorrect UI (should be
fullscreen till VK is shown).

This has been fixed for login screen (but not for multi-profiles login) with
hacks in ShelfLayoutManager.

[oshima, 2014-06-03 22:36:44]

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.cc (right):

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:65: 
you should be able to just

wm::GetWindowState(child)->SetStateObject(lock_state)

and discard the old state. There should be no need to restore to old one. Then
you can eliminate most of memory management code.

I actually think you can just do the same in WorkspaceLayoutManager (for the
lock container), and it should work.

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.h (right):

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
2014
nuke (c)

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.h:14: #include "base/basictypes.h"
macros.h

[Nikita (slow), 2014-06-04 04:17:00]

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.cc (right):

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:65: 
On 2014/06/03 22:36:44, oshima wrote:
> you should be able to just
> 
> wm::GetWindowState(child)->SetStateObject(lock_state)
> 
> and discard the old state. There should be no need to restore to old one. Then
> you can eliminate most of memory management code. 

Ok, I'll do that.

> I actually think you can just do the same in WorkspaceLayoutManager (for the
> lock container), and it should work.

I would like to avoid unnecessary logic in WorkspaceLayoutManager like

WorkspaceLayoutManager::OnDisplayWorkAreaInsetsChanged()
using incorrect work area definition.

And things like
UpdateFullscreenState();
UpdateShelfVisibility();

At the same time I would like to add a custom logic like tracking keyboard
container bounds so that LockContainer is not dependent indirectly on
ShelfLayoutManager changes etc.

[oshima, 2014-06-04 04:34:37]

On 2014/06/04 04:17:00, Nikita Kostylev wrote:
>
https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
> File ash/wm/lock_layout_manager.cc (right):
> 
>
https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
> ash/wm/lock_layout_manager.cc:65: 
> On 2014/06/03 22:36:44, oshima wrote:
> > you should be able to just
> > 
> > wm::GetWindowState(child)->SetStateObject(lock_state)
> > 
> > and discard the old state. There should be no need to restore to old one.
Then
> > you can eliminate most of memory management code. 
> 
> Ok, I'll do that.
> 
> > I actually think you can just do the same in WorkspaceLayoutManager (for the
> > lock container), and it should work.
> 
> I would like to avoid unnecessary logic in WorkspaceLayoutManager like
> 
> WorkspaceLayoutManager::OnDisplayWorkAreaInsetsChanged()
> using incorrect work area definition.

Work are is correct. The issue is that work are should not change when locked,
and windows in lock screen should not depend on it.

> 
> And things like
> UpdateFullscreenState();
> UpdateShelfVisibility();
> 
> At the same time I would like to add a custom logic like tracking keyboard
> container bounds so that LockContainer is not dependent indirectly on
> ShelfLayoutManager changes etc.

Ok, that make sense. I'm fine with new layout manager then.

[Nikita (slow), 2014-06-04 17:40:39]

Added test + keyboard overscroll logic.

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.cc (right):

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:65: 
On 2014/06/03 22:36:44, oshima wrote:
> you should be able to just
> 
> wm::GetWindowState(child)->SetStateObject(lock_state)
> 
> and discard the old state. There should be no need to restore to old one. Then
> you can eliminate most of memory management code.
> 
> I actually think you can just do the same in WorkspaceLayoutManager (for the
> lock container), and it should work.

Done.

[oshima, 2014-06-04 19:20:13]

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.cc (right):

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:63: windows_.insert(child);
do you need windows_? Isn't this same as window_->children() ?

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:66: window_state_map_[child] = new
LockWindowState(child, this);
the following should work (no need to pass child to LockWindowState).

wm::WindowState* window_state = wm::GetWindowState(child);
window_state->SetStateobject(new LockWindowState);

(with proper scoped_ptr tricks)

and remove window_state_map_

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:147: }
this should be equivalent to 

for (child in window_->children())
   wm::GetWindowState(child)->OnWMEvent(event);

https://codereview.chromium.org/254673004/diff/210001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.cc (right):

https://codereview.chromium.org/254673004/diff/210001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:153: }
looks to me that you're sending the same event twice to the same set of windows.

https://codereview.chromium.org/254673004/diff/210001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.h (right):

https://codereview.chromium.org/254673004/diff/210001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.h:33: 
nuke empty line

https://codereview.chromium.org/254673004/diff/210001/ash/wm/lock_window_state.h
File ash/wm/lock_window_state.h (right):

https://codereview.chromium.org/254673004/diff/210001/ash/wm/lock_window_stat...
ash/wm/lock_window_state.h:47: aura::Window* window_;
I believe you don't need this (you can always get it from
window_state->window())

[Nikita (slow), 2014-06-04 19:37:56]

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.cc (right):

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:66: window_state_map_[child] = new
LockWindowState(child, this);
On 2014/06/04 19:20:14, oshima wrote:
> the following should work (no need to pass child to LockWindowState).
> 
> wm::WindowState* window_state = wm::GetWindowState(child);
> window_state->SetStateobject(new LockWindowState);
> 
> (with proper scoped_ptr tricks)
> 
> and remove window_state_map_

SetStateobject is private. Ok to add LockLayoutManager as a friend instead of
LockWindowState?

[oshima, 2014-06-04 19:48:53]

On 2014/06/04 19:37:56, Nikita Kostylev wrote:
>
https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
> File ash/wm/lock_layout_manager.cc (right):
> 
>
https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
> ash/wm/lock_layout_manager.cc:66: window_state_map_[child] = new
> LockWindowState(child, this);
> On 2014/06/04 19:20:14, oshima wrote:
> > the following should work (no need to pass child to LockWindowState).
> > 
> > wm::WindowState* window_state = wm::GetWindowState(child);
> > window_state->SetStateobject(new LockWindowState);
> > 
> > (with proper scoped_ptr tricks)
> > 
> > and remove window_state_map_
> 
> SetStateobject is private. Ok to add LockLayoutManager as a friend instead of
> LockWindowState?

Can you define static LockWindowState::Creat(aura::Window* window) and set the
state object there?

[Nikita (slow), 2014-06-05 09:27:25]

On 2014/06/04 19:48:53, oshima wrote:
> On 2014/06/04 19:37:56, Nikita Kostylev wrote:
> >
>
https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
> > File ash/wm/lock_layout_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
> > ash/wm/lock_layout_manager.cc:66: window_state_map_[child] = new
> > LockWindowState(child, this);
> > On 2014/06/04 19:20:14, oshima wrote:
> > > the following should work (no need to pass child to LockWindowState).
> > > 
> > > wm::WindowState* window_state = wm::GetWindowState(child);
> > > window_state->SetStateobject(new LockWindowState);
> > > 
> > > (with proper scoped_ptr tricks)
> > > 
> > > and remove window_state_map_
> > 
> > SetStateobject is private. Ok to add LockLayoutManager as a friend instead
of
> > LockWindowState?
> 
> Can you define static LockWindowState::Creat(aura::Window* window) and set the
> state object there?

Won't work

../../base/memory/scoped_ptr.h: In static member function 'static void
ash::LockWindowState::Create(aura::Window*)':
../../base/memory/scoped_ptr.h:311:3: error: 'scoped_ptr<T,
D>::scoped_ptr(scoped_ptr<T, D>&) [with T = ash::wm::WindowState::State, D =
base::DefaultDeleter<ash::wm::WindowState::State>, scoped_ptr<T, D> =
scoped_ptr<ash::wm::WindowState::State>]' is private
../../ash/wm/lock_window_state.cc:31:55: error: within this context
../../ash/wm/window_state.h:169:21: error:   initializing argument 1 of
'scoped_ptr<ash::wm::WindowState::State>
ash::wm::WindowState::SetStateObject(scoped_ptr<ash::wm::WindowState::State>)'

[Nikita (slow), 2014-06-05 10:51:36]

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.cc (right):

https://codereview.chromium.org/254673004/diff/170001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:147: }
On 2014/06/04 19:20:14, oshima wrote:
> this should be equivalent to 
> 
> for (child in window_->children())
>    wm::GetWindowState(child)->OnWMEvent(event);

Done.

https://codereview.chromium.org/254673004/diff/210001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.h (right):

https://codereview.chromium.org/254673004/diff/210001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.h:33: 
On 2014/06/04 19:20:14, oshima wrote:
> nuke empty line

Done.

https://codereview.chromium.org/254673004/diff/210001/ash/wm/lock_window_state.h
File ash/wm/lock_window_state.h (right):

https://codereview.chromium.org/254673004/diff/210001/ash/wm/lock_window_stat...
ash/wm/lock_window_state.h:47: aura::Window* window_;
On 2014/06/04 19:20:14, oshima wrote:
> I believe you don't need this (you can always get it from
> window_state->window())

Done.

[oshima, 2014-06-05 15:27:26]

> Won't work
>
>../../base/memory/scoped_ptr.h: In static member function 'static void
ash::LockWindowState::Create(aura::Window*)':
>../../base/memory/scoped_ptr.h:311:3: error: 'scoped_ptr<T,
D>::scoped_ptr(scoped_ptr<T, D>&) [with T = ash::wm::WindowState::State, D =
base::DefaultDeleter<ash::wm::WindowState::State>, scoped_ptr<T, D>
=scoped_ptr<ash::wm::WindowState::State>]' is private
>../../ash/wm/lock_window_state.cc:31:55: error: within this context
>../../ash/wm/window_state.h:169:21: error:   initializing argument 1 of
'scoped_ptr<ash::wm::WindowState::State>
ash::wm::WindowState::SetStateObject(scoped_ptr<ash::wm::WindowState::State>)'

https://codereview.chromium.org/311373002 works for me.

https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_stat...
File ash/wm/lock_window_state.cc (right):

https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_stat...
ash/wm/lock_window_state.cc:95: window_state->set_can_be_dragged(false);
you can remove this, and one in DetachState. they're specific to touch view
mode.

https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_state.h
File ash/wm/lock_window_state.h (right):

https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_stat...
ash/wm/lock_window_state.h:13: // states to maximized and is applied only on
lock (login) window container.
This does not match the description for current_state_type_. Are there
normal/minimized window in lock window container?

[Nikita (slow), 2014-06-05 16:08:29]

On 2014/06/05 15:27:26, oshima wrote:
> https://codereview.chromium.org/311373002 works for me.

Thanks. I've probably missed something. Updated.

[Nikita (slow), 2014-06-05 16:08:36]

https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_stat...
File ash/wm/lock_window_state.cc (right):

https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_stat...
ash/wm/lock_window_state.cc:95: window_state->set_can_be_dragged(false);
On 2014/06/05 15:27:26, oshima wrote:
> you can remove this, and one in DetachState. they're specific to touch view
> mode.

Done.

https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_state.h
File ash/wm/lock_window_state.h (right):

https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_stat...
ash/wm/lock_window_state.h:13: // states to maximized and is applied only on
lock (login) window container.
On 2014/06/05 15:27:26, oshima wrote:
> This does not match the description for current_state_type_. Are there
> normal/minimized window in lock window container?

Updated.

[oshima, 2014-06-05 16:15:09]

On 2014/06/05 16:08:36, Nikita Kostylev wrote:
>
https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_stat...
> File ash/wm/lock_window_state.cc (right):
> 
>
https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_stat...
> ash/wm/lock_window_state.cc:95: window_state->set_can_be_dragged(false);
> On 2014/06/05 15:27:26, oshima wrote:
> > you can remove this, and one in DetachState. they're specific to touch view
> > mode.
> 
> Done.
> 
>
https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_state.h
> File ash/wm/lock_window_state.h (right):
> 
>
https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_stat...
> ash/wm/lock_window_state.h:13: // states to maximized and is applied only on
> lock (login) window container.
> On 2014/06/05 15:27:26, oshima wrote:
> > This does not match the description for current_state_type_. Are there
> > normal/minimized window in lock window container?
> 
> Updated.

You probably missed this change in the latest patch?

[Nikita (slow), 2014-06-05 16:26:31]

On 2014/06/05 16:15:09, oshima wrote:
>
https://codereview.chromium.org/254673004/diff/290001/ash/wm/lock_window_stat...
> > ash/wm/lock_window_state.h:13: // states to maximized and is applied only on
> > lock (login) window container.
> > On 2014/06/05 15:27:26, oshima wrote:
> > > This does not match the description for current_state_type_. Are there
> > > normal/minimized window in lock window container?
> > 
> > Updated.
> 
> You probably missed this change in the latest patch?

Right, fixed.

[oshima, 2014-06-05 17:09:05]

lgtm if you address the following comments.

https://codereview.chromium.org/254673004/diff/320001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.cc (right):

https://codereview.chromium.org/254673004/diff/320001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:18: using aura::Window;
nit: you're using both Window and aura::Window. please be consistent. I
generally avoid using using for aura:: though, because it's short enough that it
doesn't bother me.

https://codereview.chromium.org/254673004/diff/320001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:102: }
can now this be just

window->RemoveObserver(this);
if (root_window_ == window)
  root_window_ = NULL;

?

[Nikita (slow), 2014-06-06 04:31:30]

https://codereview.chromium.org/254673004/diff/320001/ash/wm/lock_layout_mana...
File ash/wm/lock_layout_manager.cc (right):

https://codereview.chromium.org/254673004/diff/320001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:18: using aura::Window;
On 2014/06/05 17:09:05, oshima wrote:
> nit: you're using both Window and aura::Window. please be consistent. I
> generally avoid using using for aura:: though, because it's short enough that
it
> doesn't bother me.

Done.

https://codereview.chromium.org/254673004/diff/320001/ash/wm/lock_layout_mana...
ash/wm/lock_layout_manager.cc:102: }
On 2014/06/05 17:09:04, oshima wrote:
> can now this be just
> 
> window->RemoveObserver(this);
> if (root_window_ == window)
>   root_window_ = NULL;
> 
> ?

Done.

[Nikita (slow), 2014-06-06 08:34:10]

The CQ bit was checked by nkostylev@chromium.org

[Nikita (slow), 2014-06-06 08:34:19]

The CQ bit was unchecked by nkostylev@chromium.org

[Nikita (slow), 2014-06-06 10:47:54]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-06-06 10:49:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/254673004/380001

[commit-bot: I haz the power, 2014-06-06 12:44:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-06 13:16:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 13:16:35]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[Nikita (slow), 2014-06-06 13:18:26]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-06-06 13:19:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/254673004/380001

[commit-bot: I haz the power, 2014-06-06 13:51:16]

Change committed as 275431