Lock screen for Chrome-Athena

Lock screen for Chrome-Athena

Screen lock activates with keyboard accelerator Ctrl+Shift+L and bower button.

BUG=413926
TEST=manual

Committed: https://crrev.com/5b584d515a38c01b3d9d72d823688b67173c97cf
Cr-Commit-Position: refs/heads/master@{#299878}

[Dmitry Polukhin, 2014-10-01 12:05:01]

dpolukhin@chromium.org changed reviewers:
+ nkostylev@chromium.org, oshima@chromium.org

[Dmitry Polukhin, 2014-10-01 12:05:01]

Ohsima,

This CL makes lock screen works but it cannot be committed as is because it has
wrong dependencies. I need your advise how to initiate lock from Athena code.
One solution is to create lock screen interface in src/components or src/athena
and put keep implementation in src/chrome for now. Alternative solution is to
create some kind of AthenaDelegate that Chrome will implement. I need your
advise what do you prefer.

Also I'm going to initiate lock from PowerButtonController and add Alt+Shift+L
shortcut. As for shortcuts it seems that Athena doesn't have system level
accelerator so it should be power manager accelerator or should I create system
level accelerators?

    Thanks,
    Dmitry

[Dmitry Polukhin, 2014-10-09 14:00:04]

PTAL

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

lgtm

https://codereview.chromium.org/620663005/diff/40001/athena/screen_lock/chrom...
File athena/screen_lock/chrome/DEPS (right):

https://codereview.chromium.org/620663005/diff/40001/athena/screen_lock/chrom...
athena/screen_lock/chrome/DEPS:4: "+chrome/browser/chromeos/login/lock",
I guess this is against current rules that athena is not allowed to depend on
chrome directly.

If this is a temporary dependency, at least add comment with a bug #.

https://codereview.chromium.org/620663005/diff/40001/athena/screen_lock/chrom...
File athena/screen_lock/chrome/screen_lock_manager_impl.cc (right):

https://codereview.chromium.org/620663005/diff/40001/athena/screen_lock/chrom...
athena/screen_lock/chrome/screen_lock_manager_impl.cc:50: instance = NULL;
DCHECK(instance == this)

https://codereview.chromium.org/620663005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/lock_window_aura.cc (right):

https://codereview.chromium.org/620663005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/lock_window_aura.cc:68: // Don't set
TRANSLUCENT_WINDOW because we don't have wallpaper manager yet.
But currently login screen works just fine being translucent i.e. wallpaper is
shown.

You mean there's no way to move that wallpaper above all sessions windows when
screen lock is active?

Please file a bug for this functionality.

[Dmitry Polukhin, 2014-10-10 11:30:01]

Oshima, please take a look.

https://codereview.chromium.org/620663005/diff/40001/athena/screen_lock/chrom...
File athena/screen_lock/chrome/DEPS (right):

https://codereview.chromium.org/620663005/diff/40001/athena/screen_lock/chrom...
athena/screen_lock/chrome/DEPS:4: "+chrome/browser/chromeos/login/lock",
On 2014/10/10 10:53:06, Nikita Kostylev wrote:
> I guess this is against current rules that athena is not allowed to depend on
> chrome directly.
> 
> If this is a temporary dependency, at least add comment with a bug #.

Done.

https://codereview.chromium.org/620663005/diff/40001/athena/screen_lock/chrom...
File athena/screen_lock/chrome/screen_lock_manager_impl.cc (right):

https://codereview.chromium.org/620663005/diff/40001/athena/screen_lock/chrom...
athena/screen_lock/chrome/screen_lock_manager_impl.cc:50: instance = NULL;
On 2014/10/10 10:53:06, Nikita Kostylev wrote:
> DCHECK(instance == this)

Done.

https://codereview.chromium.org/620663005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/ui/lock_window_aura.cc (right):

https://codereview.chromium.org/620663005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/lock_window_aura.cc:68: // Don't set
TRANSLUCENT_WINDOW because we don't have wallpaper manager yet.
On 2014/10/10 10:53:06, Nikita Kostylev wrote:
> But currently login screen works just fine being translucent i.e. wallpaper is
> shown.
> 
> You mean there's no way to move that wallpaper above all sessions windows when
> screen lock is active?
> 
> Please file a bug for this functionality.

Added link to the bug about move wallpaper manager to components.

[Dmitry Polukhin, 2014-10-13 07:26:15]

dpolukhin@chromium.org changed reviewers:
+ mukai@chromium.org

[Dmitry Polukhin, 2014-10-13 07:26:16]

+mukai@ for Athena review, PTAL

[Jun Mukai, 2014-10-13 18:20:25]

https://codereview.chromium.org/620663005/diff/170001/athena/main/athena_main...
File athena/main/athena_main.gyp (right):

https://codereview.chromium.org/620663005/diff/170001/athena/main/athena_main...
athena/main/athena_main.gyp:63: 'lock_manager_stub.cc',
Usually this kind of file should be in athena/screen_lock/shell, and use it in
athena_app_shell_lib vs athena_chrome_lib (see athena.gyp and athena/extensions
files)

https://codereview.chromium.org/620663005/diff/170001/athena/system/power_but...
File athena/system/power_button_controller.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/system/power_but...
athena/system/power_button_controller.cc:50:
athena::ScreenLockManager::Get()->LockScreen();
Return here?  Otherwise normal shutdown process will start, right?

https://codereview.chromium.org/620663005/diff/170001/athena/wm/window_overvi...
File athena/wm/window_overview_mode.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/wm/window_overvi...
athena/wm/window_overview_mode.cc:434: target ? wm::GetTransientParent(target) :
NULL;
Not sure how it's related to the lock screen.  Could you explain a bit?

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:363: #if
!defined(USE_ATHENA)
Please note a crbug.com issue for the missing feature (crbug.com/413926 would be
fine)

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/webui_screen_locker.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:72:
ash::Shell::GetInstance()->delegate()->AddVirtualKeyboardStateObserver(this);
Athena also has virtual keyboards.  Do we need similar thing for athena?

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1306: #if
!defined(USE_ATHENA)
bshe is working for wallpapers IIRC. Can you contact him?

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/display_info_provider_chromeos.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/display_info_provider_chromeos.cc:362: //
TODO(dpolukhin): put something reasonable to the unit without ash::Shell.
Please note crbug.com/416961
Also I'm not sure why this is involved to screen locks.  Possibly better to
separate into another CL?

[Jun Mukai, 2014-10-13 18:21:36]

+bshe for wallpaper

[oshima, 2014-10-13 18:52:20]

https://codereview.chromium.org/620663005/diff/170001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/620663005/diff/170001/athena/athena.gyp#newco...
athena/athena.gyp:195: ],
this is static library, so you don't need this.

https://codereview.chromium.org/620663005/diff/170001/athena/main/athena_laun...
File athena/main/athena_launcher.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/main/athena_laun...
athena/main/athena_launcher.cc:157: athena::ScreenLockManager::Shutdown();
please keep the reverse order of the creation order.

https://codereview.chromium.org/620663005/diff/170001/athena/main/athena_main...
File athena/main/athena_main.gyp (right):

https://codereview.chromium.org/620663005/diff/170001/athena/main/athena_main...
athena/main/athena_main.gyp:63: 'lock_manager_stub.cc',
On 2014/10/13 18:20:24, Jun Mukai wrote:
> Usually this kind of file should be in athena/screen_lock/shell, and use it in
> athena_app_shell_lib vs athena_chrome_lib (see athena.gyp and
athena/extensions
> files)
+1

https://codereview.chromium.org/620663005/diff/170001/athena/screen/screen_ma...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/screen/screen_ma...
athena/screen/screen_manager_impl.cc:333: AthenaEventTargeter*
athena_event_targeter =
scoped_ptr< >

then pass it.

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:465: // TOOD(dpolukhin): we
need to to something similar for Athena.
we probably should simply make containers in between invisible (by setting 0
opacity) instead of updating structure.

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/webui_screen_locker.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:72:
ash::Shell::GetInstance()->delegate()->AddVirtualKeyboardStateObserver(this);
On 2014/10/13 18:20:24, Jun Mukai wrote:
> Athena also has virtual keyboards.  Do we need similar thing for athena?

I believe it's just observing the bounds change.

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1306: #if
!defined(USE_ATHENA)
crbug.com/408734

[Dmitry Polukhin, 2014-10-14 11:20:32]

dpolukhin@chromium.org changed reviewers:
+ mek@chromium.org, thestig@chromium.org

[Dmitry Polukhin, 2014-10-14 11:20:34]

+ mek@ for OWNER review in
chrome/browser/extensions/display_info_provider_chromeos.cc
+ thestig@ for OWNER review +base in DEPS

PTAL

https://codereview.chromium.org/620663005/diff/170001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/620663005/diff/170001/athena/athena.gyp#newco...
athena/athena.gyp:195: ],
On 2014/10/13 18:52:20, oshima wrote:
> this is static library, so you don't need this.

Done.

https://codereview.chromium.org/620663005/diff/170001/athena/main/athena_laun...
File athena/main/athena_launcher.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/main/athena_laun...
athena/main/athena_launcher.cc:157: athena::ScreenLockManager::Shutdown();
On 2014/10/13 18:52:20, oshima wrote:
> please keep the reverse order of the creation order.

Done.

https://codereview.chromium.org/620663005/diff/170001/athena/main/athena_main...
File athena/main/athena_main.gyp (right):

https://codereview.chromium.org/620663005/diff/170001/athena/main/athena_main...
athena/main/athena_main.gyp:63: 'lock_manager_stub.cc',
On 2014/10/13 18:52:20, oshima wrote:
> On 2014/10/13 18:20:24, Jun Mukai wrote:
> > Usually this kind of file should be in athena/screen_lock/shell, and use it
in
> > athena_app_shell_lib vs athena_chrome_lib (see athena.gyp and
> athena/extensions
> > files)
> +1

Done.

https://codereview.chromium.org/620663005/diff/170001/athena/screen/screen_ma...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/screen/screen_ma...
athena/screen/screen_manager_impl.cc:333: AthenaEventTargeter*
athena_event_targeter =
On 2014/10/13 18:52:20, oshima wrote:
> scoped_ptr< >
> 
> then pass it.

I need plain pointer here because in next line I call
athena_event_targeter->SetPreviousEventTargeter passing athena_event_targeter as
scoped_ptr. Therefore depending on order of expression evaluation
athena_event_targeter may become NULL before SetPreviousEventTargeter will be
called. Or I'll have to make a copy in palin pointer before passing it as an
argument.

https://codereview.chromium.org/620663005/diff/170001/athena/system/power_but...
File athena/system/power_button_controller.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/system/power_but...
athena/system/power_button_controller.cc:50:
athena::ScreenLockManager::Get()->LockScreen();
On 2014/10/13 18:20:24, Jun Mukai wrote:
> Return here?  Otherwise normal shutdown process will start, right?

No return here to show dialog that keeping power button pressed will shutdown
machine in 5 sec. So if user press power button less than 1 sec nothing happens,
after 1 second lock screen will be shown, keep holding for 5 will shutdown. If
user release power button before 5 sec. screen will be locked. I would refine
power button logic later with UX for now I just added lock screen here.

https://codereview.chromium.org/620663005/diff/170001/athena/wm/window_overvi...
File athena/wm/window_overview_mode.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/wm/window_overvi...
athena/wm/window_overview_mode.cc:434: target ? wm::GetTransientParent(target) :
NULL;
On 2014/10/13 18:20:24, Jun Mukai wrote:
> Not sure how it's related to the lock screen.  Could you explain a bit?

I see that first keyboard or mouse event after screen lock results in target ==
NULL here that causes crash inside GetTransientParent. Morever according to loop
condition it is legal case when target is NULL (while (target &&
target->parent() != container_)).

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:363: #if
!defined(USE_ATHENA)
On 2014/10/13 18:20:24, Jun Mukai wrote:
> Please note a http://crbug.com issue for the missing feature (crbug.com/413926
would be
> fine)

Done, but I'm not sure what should be do here. In Athena there is no Shell
instance that we should notify about locking.

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:465: // TOOD(dpolukhin): we
need to to something similar for Athena.
On 2014/10/13 18:52:20, oshima wrote:
> we probably should simply make containers in between invisible (by setting 0
> opacity) instead of updating structure.

I don't like approach with invisible windows on lock screen. I would change
window tree to make separate root window for lock screen. Anyway it needs to be
refined when Athene will have normal wallpaper manager.

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/webui_screen_locker.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:72:
ash::Shell::GetInstance()->delegate()->AddVirtualKeyboardStateObserver(this);
On 2014/10/13 18:52:20, oshima wrote:
> On 2014/10/13 18:20:24, Jun Mukai wrote:
> > Athena also has virtual keyboards.  Do we need similar thing for athena?
> 
> I believe it's just observing the bounds change.

Yes, it looks like for now there is nothing to do here.

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:1306: #if
!defined(USE_ATHENA)
On 2014/10/13 18:52:20, oshima wrote:
> crbug.com/408734

Done.

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/display_info_provider_chromeos.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/display_info_provider_chromeos.cc:362: //
TODO(dpolukhin): put something reasonable to the unit without ash::Shell.
On 2014/10/13 18:20:25, Jun Mukai wrote:
> Please note crbug.com/416961
> Also I'm not sure why this is involved to screen locks.  Possibly better to
> separate into another CL?

Lock screen triggers crash without this #ifdef. Added link to the bug.

https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp#newco...
athena/athena.gyp:220: 'ATHENA_IMPLEMENTATION',
I had to add this because of error during athena_main build:

/usr/local/google/home/dpolukhin/chrome/src/third_party/binutils/Linux_x64/Release/bin/ld.gold:
warning: hidden symbol 'athena::ScreenLockManager::Get()' in
obj/athena/libathena_app_shell_lib.a(obj/athena/screen_lock/shell/athena_app_shell_lib.lock_manager_stub.o)
is referenced by DSO lib/libathena_lib.so
/usr/local/google/home/dpolukhin/chrome/src/third_party/binutils/Linux_x64/Release/bin/ld.gold:
error: treating warnings as errors
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[bshe, 2014-10-14 13:39:34]

bshe@chromium.org changed reviewers:
+ bshe@chromium.org

[bshe, 2014-10-14 13:39:35]

wallpaper_manager.cc lgtm

[Marijn Kruisselbrink, 2014-10-14 17:34:13]

chrome/browser/extensions/display_info_provider_chromeos.cc lgtm

[Jun Mukai, 2014-10-14 18:27:25]

https://codereview.chromium.org/620663005/diff/170001/athena/system/power_but...
File athena/system/power_button_controller.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/system/power_but...
athena/system/power_button_controller.cc:50:
athena::ScreenLockManager::Get()->LockScreen();
On 2014/10/14 11:20:33, Dmitry Polukhin wrote:
> On 2014/10/13 18:20:24, Jun Mukai wrote:
> > Return here?  Otherwise normal shutdown process will start, right?
> 
> No return here to show dialog that keeping power button pressed will shutdown
> machine in 5 sec. So if user press power button less than 1 sec nothing
happens,
> after 1 second lock screen will be shown, keep holding for 5 will shutdown. If
> user release power button before 5 sec. screen will be locked. I would refine
> power button logic later with UX for now I just added lock screen here.

Ah, I see.

https://codereview.chromium.org/620663005/diff/170001/athena/wm/window_overvi...
File athena/wm/window_overview_mode.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/wm/window_overvi...
athena/wm/window_overview_mode.cc:434: target ? wm::GetTransientParent(target) :
NULL;
On 2014/10/14 11:20:33, Dmitry Polukhin wrote:
> On 2014/10/13 18:20:24, Jun Mukai wrote:
> > Not sure how it's related to the lock screen.  Could you explain a bit?
> 
> I see that first keyboard or mouse event after screen lock results in target
==
> NULL here that causes crash inside GetTransientParent. Morever according to
loop
> condition it is legal case when target is NULL (while (target &&
> target->parent() != container_)).

Still don't get it. Rather than handling here, should we cancel the window
overview mode when entering to lock screen?  This code path won't be reached in
the lock screen.

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/webui_screen_locker.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.cc:72:
ash::Shell::GetInstance()->delegate()->AddVirtualKeyboardStateObserver(this);
On 2014/10/14 11:20:34, Dmitry Polukhin wrote:
> On 2014/10/13 18:52:20, oshima wrote:
> > On 2014/10/13 18:20:24, Jun Mukai wrote:
> > > Athena also has virtual keyboards.  Do we need similar thing for athena?
> > 
> > I believe it's just observing the bounds change.
> 
> Yes, it looks like for now there is nothing to do here.

Ah, okay, and it's already observing in Athena so nothing is necessary. Thanks.

https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp#newco...
athena/athena.gyp:220: 'ATHENA_IMPLEMENTATION',
On 2014/10/14 11:20:34, Dmitry Polukhin wrote:
> I had to add this because of error during athena_main build:
> 
>
/usr/local/google/home/dpolukhin/chrome/src/third_party/binutils/Linux_x64/Release/bin/ld.gold:
> warning: hidden symbol 'athena::ScreenLockManager::Get()' in
>
obj/athena/libathena_app_shell_lib.a(obj/athena/screen_lock/shell/athena_app_shell_lib.lock_manager_stub.o)
> is referenced by DSO lib/libathena_lib.so
>
/usr/local/google/home/dpolukhin/chrome/src/third_party/binutils/Linux_x64/Release/bin/ld.gold:
> error: treating warnings as errors
> clang: error: linker command failed with exit code 1 (use -v to see
invocation)

I see.  However, I personally prefer not to have this as far as possible.  Can
you follow the way of athena/extensions/extensions_delegate?  It avoids this
linker trouble by the following way:
- Get() and Shutdown() is common, therefore it's in athena_content_lib.  It has
a global pointer of |instance|
- once an instance is created, it will remember itself as global |instance|
- Create...() static method is different among shell/ and chrome/.  In your
case, Create() doing nothing would be fine (therefore |instance| is always NULL
in athena_main)

[oshima, 2014-10-14 19:05:15]

https://codereview.chromium.org/620663005/diff/170001/athena/screen/screen_ma...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/screen/screen_ma...
athena/screen/screen_manager_impl.cc:333: AthenaEventTargeter*
athena_event_targeter =
On 2014/10/14 11:20:33, Dmitry Polukhin wrote:
> On 2014/10/13 18:52:20, oshima wrote:
> > scoped_ptr< >
> > 
> > then pass it.
> 
> I need plain pointer here because in next line I call
> athena_event_targeter->SetPreviousEventTargeter passing athena_event_targeter
as
> scoped_ptr. Therefore depending on order of expression evaluation
> athena_event_targeter may become NULL before SetPreviousEventTargeter will be
> called. Or I'll have to make a copy in palin pointer before passing it as an
> argument.

Acknowledged.

https://codereview.chromium.org/620663005/diff/170001/athena/wm/window_overvi...
File athena/wm/window_overview_mode.cc (right):

https://codereview.chromium.org/620663005/diff/170001/athena/wm/window_overvi...
athena/wm/window_overview_mode.cc:434: target ? wm::GetTransientParent(target) :
NULL;
On 2014/10/14 18:27:25, Jun Mukai wrote:
> On 2014/10/14 11:20:33, Dmitry Polukhin wrote:
> > On 2014/10/13 18:20:24, Jun Mukai wrote:
> > > Not sure how it's related to the lock screen.  Could you explain a bit?
> > 
> > I see that first keyboard or mouse event after screen lock results in target
> ==
> > NULL here that causes crash inside GetTransientParent. Morever according to
> loop
> > condition it is legal case when target is NULL (while (target &&
> > target->parent() != container_)).
> 
> Still don't get it. Rather than handling here, should we cancel the window
> overview mode when entering to lock screen?  This code path won't be reached
in
> the lock screen.

I think this is a regression I introduced. I will create separate CL with a
test, but
I'm ok to land this in this CL (if my CL isn't ready).

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/620663005/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:465: // TOOD(dpolukhin): we
need to to something similar for Athena.
On 2014/10/14 11:20:33, Dmitry Polukhin wrote:
> On 2014/10/13 18:52:20, oshima wrote:
> > we probably should simply make containers in between invisible (by setting 0
> > opacity) instead of updating structure.
> 
> I don't like approach with invisible windows on lock screen. I would change
> window tree to make separate root window for lock screen. Anyway it needs to
be
> refined when Athene will have normal wallpaper manager.

Moving container (or window) triggers a lot of observer executions, and that
used to cause other issues.

I'm going to create an API to hide containers (so that you don't have to worry
about how), so we can revisit later when it's ready.

https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp#newco...
athena/athena.gyp:220: 'ATHENA_IMPLEMENTATION',
On 2014/10/14 11:20:34, Dmitry Polukhin wrote:
> I had to add this because of error during athena_main build:
> 
>
/usr/local/google/home/dpolukhin/chrome/src/third_party/binutils/Linux_x64/Release/bin/ld.gold:
> warning: hidden symbol 'athena::ScreenLockManager::Get()' in
>
obj/athena/libathena_app_shell_lib.a(obj/athena/screen_lock/shell/athena_app_shell_lib.lock_manager_stub.o)
> is referenced by DSO lib/libathena_lib.so
>
/usr/local/google/home/dpolukhin/chrome/src/third_party/binutils/Linux_x64/Release/bin/ld.gold:
> error: treating warnings as errors
> clang: error: linker command failed with exit code 1 (use -v to see
invocation)

Oh, that is because you have header file in component lib but impl in
static_library. Can you move the header to content_lib ?

https://codereview.chromium.org/620663005/diff/350001/athena/screen/screen_ma...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/620663005/diff/350001/athena/screen/screen_ma...
athena/screen/screen_manager_impl.cc:338: }
FYI: This will be removed/replaced with new code. I'll cc to you.

[Lei Zhang, 2014-10-14 22:14:58]

Incoming base/ DEPS addition LGTM.

[Dmitry Polukhin, 2014-10-15 10:14:26]

PTAL

https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp#newco...
athena/athena.gyp:220: 'ATHENA_IMPLEMENTATION',
On 2014/10/14 19:05:15, oshima wrote:
> On 2014/10/14 11:20:34, Dmitry Polukhin wrote:
> > I had to add this because of error during athena_main build:
> > 
> >
>
/usr/local/google/home/dpolukhin/chrome/src/third_party/binutils/Linux_x64/Release/bin/ld.gold:
> > warning: hidden symbol 'athena::ScreenLockManager::Get()' in
> >
>
obj/athena/libathena_app_shell_lib.a(obj/athena/screen_lock/shell/athena_app_shell_lib.lock_manager_stub.o)
> > is referenced by DSO lib/libathena_lib.so
> >
>
/usr/local/google/home/dpolukhin/chrome/src/third_party/binutils/Linux_x64/Release/bin/ld.gold:
> > error: treating warnings as errors
> > clang: error: linker command failed with exit code 1 (use -v to see
> invocation)
> 
> Oh, that is because you have header file in component lib but impl in
> static_library. Can you move the header to content_lib ?
> 

I did it and surprisingly it works. I thought that .h files are basically
ignored.

https://codereview.chromium.org/620663005/diff/350001/athena/screen/screen_ma...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/620663005/diff/350001/athena/screen/screen_ma...
athena/screen/screen_manager_impl.cc:338: }
On 2014/10/14 19:05:15, oshima wrote:
> FYI: This will be removed/replaced with new code. I'll cc to you.

Acknowledged.

[oshima, 2014-10-15 10:36:34]

lgtm

[Dmitry Polukhin, 2014-10-15 12:08:53]

PTAL

https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp#newco...
athena/athena.gyp:220: 'ATHENA_IMPLEMENTATION',
On 2014/10/15 10:14:26, Dmitry Polukhin wrote:
> I did it and surprisingly it works. I thought that .h files are basically
> ignored.

Ooops... I was wrong I forgot to add 'use_athena=1' when I was building. So
there is no magic here, moving header to another build target won't change
anything. As suggested Jun I created ScreenLockManagerBase but I have to put it
into athena_lib to use it from libathena_lib.so. If I put it into
athena_content_lib, linker reports error because libathena_lib.so has referenc
to ScreenLockManager::Get from power_button_controller.cc. I also need export
because Chrome won't be able to use it.

[oshima, 2014-10-15 18:05:44]

On 2014/10/15 12:08:53, Dmitry Polukhin wrote:
> PTAL
> 
> https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp
> File athena/athena.gyp (right):
> 
>
https://codereview.chromium.org/620663005/diff/350001/athena/athena.gyp#newco...
> athena/athena.gyp:220: 'ATHENA_IMPLEMENTATION',
> On 2014/10/15 10:14:26, Dmitry Polukhin wrote:
> > I did it and surprisingly it works. I thought that .h files are basically
> > ignored.
> 
> Ooops... I was wrong I forgot to add 'use_athena=1' when I was building. So
> there is no magic here, moving header to another build target won't change
> anything. As suggested Jun I created ScreenLockManagerBase but I have to put
it
> into athena_lib to use it from libathena_lib.so. If I put it into
> athena_content_lib, linker reports error because libathena_lib.so has referenc
> to ScreenLockManager::Get from power_button_controller.cc. I also need export
> because Chrome won't be able to use it.
Ah, I understand the issue correctly now. (Sorry, I should've taken closer look)

The problem was the dependency violation: system_ui, which is shared lib, canoot
depend on
screen_lock, which is static. I think the better way is to create a power button
observer and
let screen locker observe it.

I'll create this API today so that screen_lock can use it.
Please feel free to ping me when you saw me online.
I can review quickly tonight.

[Dmitry Polukhin, 2014-10-16 09:26:00]

PTAL, added PowerButtonObserver, moved ScreenLockManager to athena_content_lib
and removed export. Will rebase and land only after
https://codereview.chromium.org/654343002/ reland.

[oshima, 2014-10-16 10:52:19]

lgtm with nits

https://codereview.chromium.org/620663005/diff/430001/athena/screen_lock/chro...
File athena/screen_lock/chrome/chrome_screen_lock_manager.cc (right):

https://codereview.chromium.org/620663005/diff/430001/athena/screen_lock/chro...
athena/screen_lock/chrome/chrome_screen_lock_manager.cc:81: if (state ==
PRESSED)
LONG_PRESSED ? (not sure which one you wanted, so I'll leave it to you)

https://codereview.chromium.org/620663005/diff/430001/athena/screen_lock/scre...
File athena/screen_lock/screen_lock_manager_base.h (right):

https://codereview.chromium.org/620663005/diff/430001/athena/screen_lock/scre...
athena/screen_lock/screen_lock_manager_base.h:20: virtual
~ScreenLockManagerBase();
nit: DISALLOW...

[Dmitry Polukhin, 2014-10-16 11:25:41]

Thank you for the review!

https://codereview.chromium.org/620663005/diff/430001/athena/screen_lock/chro...
File athena/screen_lock/chrome/chrome_screen_lock_manager.cc (right):

https://codereview.chromium.org/620663005/diff/430001/athena/screen_lock/chro...
athena/screen_lock/chrome/chrome_screen_lock_manager.cc:81: if (state ==
PRESSED)
On 2014/10/16 10:52:18, oshima wrote:
> LONG_PRESSED ? (not sure which one you wanted, so I'll leave it to you)

I decided to change it to PRESSED to lock sooner. Anyway it seems that we will
need to refine UX for power button later.

https://codereview.chromium.org/620663005/diff/430001/athena/screen_lock/scre...
File athena/screen_lock/screen_lock_manager_base.h (right):

https://codereview.chromium.org/620663005/diff/430001/athena/screen_lock/scre...
athena/screen_lock/screen_lock_manager_base.h:20: virtual
~ScreenLockManagerBase();
On 2014/10/16 10:52:18, oshima wrote:
> nit: DISALLOW...

Done.

[Dmitry Polukhin, 2014-10-16 11:29:53]

The CQ bit was checked by dpolukhin@chromium.org

[commit-bot: I haz the power, 2014-10-16 11:30:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/620663005/470001

[commit-bot: I haz the power, 2014-10-16 11:58:26]

Committed patchset #12 (id:470001)

[commit-bot: I haz the power, 2014-10-16 11:58:58]

Patchset 12 (id:??) landed as
https://crrev.com/5b584d515a38c01b3d9d72d823688b67173c97cf
Cr-Commit-Position: refs/heads/master@{#299878}