Minimal support for OOBE/login for chrome-athena

Minimal support for OOBE/login for chrome-athena

BUG=405859
TEST=manual

Committed: https://crrev.com/04e5b80957ca36f912f1e89fe42b3c1d373a5a79
Cr-Commit-Position: refs/heads/master@{#294305}

[Dmitry Polukhin, 2014-09-04 10:13:23]

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

[Dmitry Polukhin, 2014-09-04 10:13:24]

PTAL

It is very first version that can show login screen with command line switch
--login-manager, login with GAIA and start session. Virtual keyboard is not
working on login screen, I'm investigating it. Sent fore review to get early
feedback.

[oshima, 2014-09-04 16:14:27]

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

[oshima, 2014-09-04 16:14:28]

+bshe for wallpaper/VK questions.

Regarding how to deal with ash vs athena code, we may not want to use
ash::Shell::GetInstance(), as it may hide a real
issue. let me think about it a bit more.

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/DEPS (right):

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/DEPS:4: "+athena",
"+athena/extensions/public",
"+athena/main",

instead of allowing whole athena/

(I'll add athena/main/public, but you may depend on athena/main for the time
being)

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:300:
base::ThreadRestrictions::ScopedAllowIO allow_io;
I think we need to move VK initialization to env.

bshe@, how are we dealing with VK in login screen in touch view mode now?
Are we using login profile first then switch to user profile somehow?

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

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:338: // Athena
doesn't use wallpaper manager so don't wait for wallpaper loading.
I believe we want to use it, and if so, we should have TODO with a bug. bshe@?

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1069:
container_params);
this container should be deleted when exiting login screen.

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/ui/startu...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/ui/startu...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:522: //
TODO(dpolukhin): make session restore for Athena.
StartAthenaSessionWithContext should take care of restoring.

[oshima, 2014-09-04 18:00:56]

On 2014/09/04 16:14:28, oshima wrote:
> +bshe for wallpaper/VK questions.
> 
> Regarding how to deal with ash vs athena code, we may not want to use
> ash::Shell::GetInstance(), as it may hide a real
> issue. let me think about it a bit more.

I pondered on this and discussed with a couple of folks, and we agreed that
it's better to use ifdef USE_ATHENA instead of using ash::Shell::GetInstnace()
for following reasons.

1) It may hide regression like moving shutdown code after ash destruction.
2) Checking it at compile time is better than at runtime.
3) It give us an option to remove ash dependency from athena on chrome.
 My current impression that we'll never do this, and should componentize and
remove chrome
 dependency from athena, but you never know.

> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/login/DEPS (right):
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/DEPS:4: "+athena",
> "+athena/extensions/public",
> "+athena/main",
> 
> instead of allowing whole athena/
> 
> (I'll add athena/main/public, but you may depend on athena/main for the time
> being)
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/login/login_utils.cc (right):
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/login_utils.cc:300:
> base::ThreadRestrictions::ScopedAllowIO allow_io;
> I think we need to move VK initialization to env.
> 
> bshe@, how are we dealing with VK in login screen in touch view mode now?
> Are we using login profile first then switch to user profile somehow?
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/ui/login_display_host_impl.cc:338: // Athena
> doesn't use wallpaper manager so don't wait for wallpaper loading.
> I believe we want to use it, and if so, we should have TODO with a bug. bshe@?
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1069:
> container_params);
> this container should be deleted when exiting login screen.
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/ui/startu...
> File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/ui/startu...
> chrome/browser/ui/startup/startup_browser_creator_impl.cc:522: //
> TODO(dpolukhin): make session restore for Athena.
> StartAthenaSessionWithContext should take care of restoring.

[bshe, 2014-09-04 18:36:53]

move myself to cc list since I dont know much about athena

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:300:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2014/09/04 16:14:28, oshima wrote:
> I think we need to move VK initialization to env.
> 
> bshe@, how are we dealing with VK in login screen in touch view mode now?
> Are we using login profile first then switch to user profile somehow?

VK is currently initialized in ash (see
https://code.google.com/p/chromium/codesearch/#chromium/src/ash/shell.cc&rcl=...)

We use the login profile first and once real user login, the vk will reload
(https://code.google.com/p/chromium/codesearch/#chromium/src/ash/shell.cc&rcl=...)

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

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:338: // Athena
doesn't use wallpaper manager so don't wait for wallpaper loading.
On 2014/09/04 16:14:28, oshima wrote:
> I believe we want to use it, and if so, we should have TODO with a bug. bshe@?

Right. I think eventually we want to support changing wallpaper in athena. Not
sure about the timeline though. Leaving a todo sounds fine and can probably
refer to crbug.com/408734 ?

[Dmitry Polukhin, 2014-09-05 15:05:00]

Work in progress not ready for final review after virtual keyboard
re-initialiation. Will ping when it is fully ready.

I'll change all ash::Shell::HasInstance() with defined(USE_ATHENA) but I'm not
sure that it is really better because in general we try to minimize use of
defines and it is very easy to break code for other platforms with defines but
it is your call to decide.

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/DEPS (right):

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/DEPS:4: "+athena",
On 2014/09/04 16:14:27, oshima wrote:
> "+athena/extensions/public",
> "+athena/main",
> 
> instead of allowing whole athena/
> 
> (I'll add athena/main/public, but you may depend on athena/main for the time
> being)

Done.

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:300:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2014/09/04 18:36:52, bshe wrote:
> On 2014/09/04 16:14:28, oshima wrote:
> > I think we need to move VK initialization to env.
> > 
> > bshe@, how are we dealing with VK in login screen in touch view mode now?
> > Are we using login profile first then switch to user profile somehow?
> 
> VK is currently initialized in ash (see
>
https://code.google.com/p/chromium/codesearch/#chromium/src/ash/shell.cc&rcl=...)
> 
> We use the login profile first and once real user login, the vk will reload
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/ash/shell.cc&rcl=...)
> 

It seems that ash doing something different because it doesn't use 
keyboard::InitializeWebUIBindings() that makes file IO. I recreate keyboard
inside session with session profile but it is not enough + found bunch of
circular dependencies in VK destruction, continue investigation.

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

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:338: // Athena
doesn't use wallpaper manager so don't wait for wallpaper loading.
On 2014/09/04 18:36:53, bshe wrote:
> On 2014/09/04 16:14:28, oshima wrote:
> > I believe we want to use it, and if so, we should have TODO with a bug.
bshe@?
> 
> Right. I think eventually we want to support changing wallpaper in athena. Not
> sure about the timeline though. Leaving a todo sounds fine and can probably
> refer to crbug.com/408734 ?

Done.

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/ui/startu...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/ui/startu...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:522: //
TODO(dpolukhin): make session restore for Athena.
On 2014/09/04 16:14:28, oshima wrote:
> StartAthenaSessionWithContext should take care of restoring. 

Done.

[bshe, 2014-09-05 15:15:41]

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

[bshe, 2014-09-05 15:15:42]

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:300:
base::ThreadRestrictions::ScopedAllowIO allow_io;
VK for athena is totally different as athena doesn't have extension available.
But VK is part of an IME extension.
+sadrul who added the webui version of VK.

On 2014/09/05 15:05:00, Dmitry Polukhin wrote:
> On 2014/09/04 18:36:52, bshe wrote:
> > On 2014/09/04 16:14:28, oshima wrote:
> > > I think we need to move VK initialization to env.
> > > 
> > > bshe@, how are we dealing with VK in login screen in touch view mode now?
> > > Are we using login profile first then switch to user profile somehow?
> > 
> > VK is currently initialized in ash (see
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/ash/shell.cc&rcl=...)
> > 
> > We use the login profile first and once real user login, the vk will reload
> >
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/ash/shell.cc&rcl=...)
> > 
> 
> It seems that ash doing something different because it doesn't use 
> keyboard::InitializeWebUIBindings() that makes file IO. I recreate keyboard
> inside session with session profile but it is not enough + found bunch of
> circular dependencies in VK destruction, continue investigation.

[oshima, 2014-09-06 01:23:13]

On 2014/09/05 15:05:00, Dmitry Polukhin wrote:
> Work in progress not ready for final review after virtual keyboard
> re-initialiation. Will ping when it is fully ready.
> 
> I'll change all ash::Shell::HasInstance() with defined(USE_ATHENA) but I'm not
> sure that it is really better because in general we try to minimize use of
> defines and it is very easy to break code for other platforms with defines but
> it is your call to decide.

I agree that we should minimize the use of USE_ATHENA, but it's better than
usnig ash::Shell::GetInstance. Ideally, we should design so that we don't have
to use
USE_ATHENA.

> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/login/DEPS (right):
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/DEPS:4: "+athena",
> On 2014/09/04 16:14:27, oshima wrote:
> > "+athena/extensions/public",
> > "+athena/main",
> > 
> > instead of allowing whole athena/
> > 
> > (I'll add athena/main/public, but you may depend on athena/main for the time
> > being)
> 
> Done.
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/login/login_utils.cc (right):
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/login_utils.cc:300:
> base::ThreadRestrictions::ScopedAllowIO allow_io;
> On 2014/09/04 18:36:52, bshe wrote:
> > On 2014/09/04 16:14:28, oshima wrote:
> > > I think we need to move VK initialization to env.
> > > 
> > > bshe@, how are we dealing with VK in login screen in touch view mode now?
> > > Are we using login profile first then switch to user profile somehow?
> > 
> > VK is currently initialized in ash (see
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/ash/shell.cc&rcl=...)
> > 
> > We use the login profile first and once real user login, the vk will reload
> >
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/ash/shell.cc&rcl=...)
> > 
> 
> It seems that ash doing something different because it doesn't use 
> keyboard::InitializeWebUIBindings() that makes file IO. I recreate keyboard
> inside session with session profile but it is not enough + found bunch of
> circular dependencies in VK destruction, continue investigation.
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/ui/login_display_host_impl.cc:338: // Athena
> doesn't use wallpaper manager so don't wait for wallpaper loading.
> On 2014/09/04 18:36:53, bshe wrote:
> > On 2014/09/04 16:14:28, oshima wrote:
> > > I believe we want to use it, and if so, we should have TODO with a bug.
> bshe@?
> > 
> > Right. I think eventually we want to support changing wallpaper in athena.
Not
> > sure about the timeline though. Leaving a todo sounds fine and can probably
> > refer to crbug.com/408734 ?
> 
> Done.
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/ui/startu...
> File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):
> 
>
https://codereview.chromium.org/542623002/diff/40001/chrome/browser/ui/startu...
> chrome/browser/ui/startup/startup_browser_creator_impl.cc:522: //
> TODO(dpolukhin): make session restore for Athena.
> On 2014/09/04 16:14:28, oshima wrote:
> > StartAthenaSessionWithContext should take care of restoring. 
> 
> Done.

[sadrul, 2014-09-06 02:58:12]

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.cc:300:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2014/09/05 15:15:42, bshe wrote:
> VK for athena is totally different as athena doesn't have extension available.
> But VK is part of an IME extension.
> +sadrul who added the webui version of VK.

For athena_main, the keyboard is initialized before the UI message-loop runs,
and so it's not necessary to use ScopedAllowIO. There may be other code in the
athena initialization that expects that the message-loop isn't running yet. So
it would be good if we can initialize athena from inside chrome before the
message-loop starts running.

[Dmitry Polukhin, 2014-09-08 09:10:44]

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

[Dmitry Polukhin, 2014-09-08 09:10:45]

PTAL, code is ready for review now. Virtual keyboard is working ob both login
screen and inside user session but use login profile inside user session. It
seems that it is right approach until we switch to component extension based
virtual keyboard instead of webui version that we use in Athena.

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

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1069:
container_params);
On 2014/09/04 16:14:28, oshima wrote:
> this container should be deleted when exiting login screen.

Done.

[Nikita (slow), 2014-09-08 09:53:17]

lgtm

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

https://codereview.chromium.org/542623002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1069:
container_params);
On 2014/09/04 16:14:28, oshima wrote:
> this container should be deleted when exiting login screen.

Are there plans to have single place (like RootWindow in Ash) that manages such
containers?

Lock screen may reuse this container and lock screen is not handled by
LoginDisplayHost so it seems that having one place that creates this container
just once would be a better approach.

https://codereview.chromium.org/542623002/diff/120001/athena/common/container...
File athena/common/container_priorities.h (right):

https://codereview.chromium.org/542623002/diff/120001/athena/common/container...
athena/common/container_priorities.h:16: CP_VIRTUAL_KEYBOARD,
Please create an issue to track bug with select popup menu being shown below
login screen container.

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/helper.cc (right):

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/helper.cc:46: // TODO(dpolukhin): temporary always
enable overscroll mode for Athena,
Please file a work item and assign it to me - to figure out proper loging VK
implementation in Athena.

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils.cc:311:
app_list::StartPageService::Get(profile);
May be placed under #else block.

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/preferences.cc (left):

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/preferences.cc:532: if (user_is_active) {
I think user_is_active check was incorrectly removed.

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.cc
(right):

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.cc:33: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
This seems to cover https://code.google.com/p/chromium/issues/detail?id=387610
for now.

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:275: const
std::string app_locale = g_browser_process->GetApplicationLocale();
I think we should have a bug filed to get this language change working.

[Dmitry Polukhin, 2014-09-08 13:28:19]

https://codereview.chromium.org/542623002/diff/120001/athena/common/container...
File athena/common/container_priorities.h (right):

https://codereview.chromium.org/542623002/diff/120001/athena/common/container...
athena/common/container_priorities.h:16: CP_VIRTUAL_KEYBOARD,
On 2014/09/08 09:53:17, Nikita Kostylev wrote:
> Please create an issue to track bug with select popup menu being shown below
> login screen container.

Done, crbug.com/411878

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/helper.cc (right):

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/helper.cc:46: // TODO(dpolukhin): temporary always
enable overscroll mode for Athena,
On 2014/09/08 09:53:17, Nikita Kostylev wrote:
> Please file a work item and assign it to me - to figure out proper loging VK
> implementation in Athena.

Done.

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils.cc:311:
app_list::StartPageService::Get(profile);
On 2014/09/08 09:53:17, Nikita Kostylev wrote:
> May be placed under #else block.

Done.

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/preferences.cc (left):

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/preferences.cc:532: if (user_is_active) {
On 2014/09/08 09:53:17, Nikita Kostylev wrote:
> I think user_is_active check was incorrectly removed.

Done. Good catch!

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/542623002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:275: const
std::string app_locale = g_browser_process->GetApplicationLocale();
On 2014/09/08 09:53:17, Nikita Kostylev wrote:
> I think we should have a bug filed to get this language change working.

Done, crbug.com/411884

[Dmitry Polukhin, 2014-09-09 04:49:10]

oshima@ please take a look

[oshima, 2014-09-09 06:32:51]

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:862: #if
defined(USE_ASH) && !defined(USE_ATHENA)
can you add crbug.com/408733 ?

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:710: //
crbug.com/408733, crbug.com/408734.
can you rebase ?

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/locale_change_guard.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/locale_change_guard.cc:180: #if !defined(USE_ATHENA)
can you file a bug to support locale change in athena?

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/DEPS (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/DEPS:6: 
remove new line

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:110: #if
!defined(USE_ATHENA)
can you add crbug.com/408752 ? (not exactly same, but related)

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:213: #if
!defined(USE_ATHENA)
ditto

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/input_events_blocker.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/input_events_blocker.cc:15:
ash::Shell::GetInstance()->PrependPreTargetHandler(this);
I assume we eventually need this?

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:327:
chrome::IncrementKeepAliveCount();
If someone else may call decrease, you can keep this call.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:339: // Athena
doesn't use wallpaper manager so don't wait for wallpaper loading,
Athena will use wallpaper manager, so please update the comment and just say
this is a temporal solution.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1067:
container_params.can_activate_children = true;
container_params.grab_inputs = true?

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1177: #if
!defined(USE_ATHENA)
crbug.com/407579

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:315: #if
!defined(USE_ATHENA)
please add accessibility bug

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/l10n_util.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/l10n_util.cc:62: #if !defined(USE_ATHENA)
ime bug

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/l10n_util.cc:417: #if
!defined(USE_ATHENA)
ditto

[Dmitry Polukhin, 2014-09-10 20:30:24]

PTAL

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:862: #if
defined(USE_ASH) && !defined(USE_ATHENA)
On 2014/09/09 06:32:50, oshima wrote:
> can you add crbug.com/408733 ?

Done.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:710: //
crbug.com/408733, crbug.com/408734.
On 2014/09/09 06:32:50, oshima wrote:
> can you rebase ?

Done.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/locale_change_guard.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/locale_change_guard.cc:180: #if !defined(USE_ATHENA)
On 2014/09/09 06:32:50, oshima wrote:
> can you file a bug to support locale change in athena?

Done. Linked to existing but about local change crbug.com/411884.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/DEPS (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/DEPS:6: 
On 2014/09/09 06:32:50, oshima wrote:
> remove new line

Done.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:110: #if
!defined(USE_ATHENA)
On 2014/09/09 06:32:50, oshima wrote:
> can you add crbug.com/408752 ? (not exactly same, but related)

Done.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:213: #if
!defined(USE_ATHENA)
On 2014/09/09 06:32:50, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/input_events_blocker.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/input_events_blocker.cc:15:
ash::Shell::GetInstance()->PrependPreTargetHandler(this);
On 2014/09/09 06:32:50, oshima wrote:
> I assume we eventually need this?

It is part of supporting local switching, added link to crbug.com/411884

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:327:
chrome::IncrementKeepAliveCount();
On 2014/09/09 06:32:50, oshima wrote:
> If someone else may call decrease, you can keep this call.

Done.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:339: // Athena
doesn't use wallpaper manager so don't wait for wallpaper loading,
On 2014/09/09 06:32:50, oshima wrote:
> Athena will use wallpaper manager, so please update the comment and just say
> this is a temporal solution.

Done.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1067:
container_params.can_activate_children = true;
On 2014/09/09 06:32:51, oshima wrote:
> container_params.grab_inputs = true?

It is not required now because other Athena windows don't exists at this point
but later we may need it for lock screen.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1177: #if
!defined(USE_ATHENA)
On 2014/09/09 06:32:50, oshima wrote:
> crbug.com/407579

Done.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:315: #if
!defined(USE_ATHENA)
On 2014/09/09 06:32:51, oshima wrote:
> please add accessibility bug

Done. crbug.com/412891

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/l10n_util.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/l10n_util.cc:62: #if !defined(USE_ATHENA)
On 2014/09/09 06:32:51, oshima wrote:
> ime bug

Done.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/l10n_util.cc:417: #if
!defined(USE_ATHENA)
On 2014/09/09 06:32:51, oshima wrote:
> ditto

Done.

[oshima, 2014-09-10 20:42:27]

lgtm with a nit.

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/542623002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1067:
container_params.can_activate_children = true;
On 2014/09/10 20:30:24, Dmitry Polukhin wrote:
> On 2014/09/09 06:32:51, oshima wrote:
> > container_params.grab_inputs = true?
> 
> It is not required now because other Athena windows don't exists at this point
> but later we may need it for lock screen.

Acknowledged.

https://codereview.chromium.org/542623002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter_controller.cc (right):

https://codereview.chromium.org/542623002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter_controller.cc:23: // TODO(oshima):
Support accessibility on athena. crbug.com/408733.
you should be able to revert this change now (It's already excluded)

[Dmitry Polukhin, 2014-09-10 21:28:41]

Thank you for review!

https://codereview.chromium.org/542623002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter_controller.cc (right):

https://codereview.chromium.org/542623002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter_controller.cc:23: // TODO(oshima):
Support accessibility on athena. crbug.com/408733.
On 2014/09/10 20:42:27, oshima wrote:
> you should be able to revert this change now (It's already excluded)

Done.

[Dmitry Polukhin, 2014-09-10 22:12:18]

The CQ bit was checked by dpolukhin@chromium.org

[commit-bot: I haz the power, 2014-09-10 22:15:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/542623002/180001

[commit-bot: I haz the power, 2014-09-10 23:49:08]

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

[commit-bot: I haz the power, 2014-09-10 23:49:10]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54320)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/59252)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Dmitry Polukhin, 2014-09-10 23:55:46]

The CQ bit was checked by dpolukhin@chromium.org

[commit-bot: I haz the power, 2014-09-10 23:57:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/542623002/200001

[commit-bot: I haz the power, 2014-09-11 02:11:02]

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

[commit-bot: I haz the power, 2014-09-11 02:11:03]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Dmitry Polukhin, 2014-09-11 02:26:04]

The CQ bit was checked by dpolukhin@chromium.org

[commit-bot: I haz the power, 2014-09-11 02:30:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/542623002/200001

[commit-bot: I haz the power, 2014-09-11 02:58:26]

Committed patchset #11 (id:200001) as 3336124bd854a2ef9c6d2e6a9b07cb65a41c9412

[commit-bot: I haz the power, 2014-09-11 03:09:32]

Patchset 11 (id:??) landed as
https://crrev.com/04e5b80957ca36f912f1e89fe42b3c1d373a5a79
Cr-Commit-Position: refs/heads/master@{#294305}