Added login screen mode for adding users into session.

Added login screen mode for adding users into session.

Created class UserAddingScreen controlling new mode.
Type of login UI explicitly set in hashtag after URL.

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

[dzhioev (left Google), 2013-05-27 17:50:38]

Stefan FYI. I think observing UserAddingScreen will be useful for ASH part.

[Nikita (slow), 2013-05-28 11:34:21]

Mostly nits.

Please think about creating browser test for this (could be in next CL). Smth
like
0. Set up user list (several users)
1. Start browser_tests (i.e. runs inside user session)
2. Bring up add user screen
3. Emulate adding another user

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_display_host.h (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_display_host.h:85: // Starts screen for
adding user into session.
nit: Add comment on |completion_callback|.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_display_host_impl.cc:413: if
(waiting_for_wallpaper_load_ && !initialize_webui_hidden_) {
Please remove this as I think that is not needed for this type of UI. They only
guard boot animation.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_display_host_impl.cc:420: }
nit: drop {}

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_display_host_impl.cc:634: if
(!completion_callback_.is_null()) {
nit: drop {}

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_display_host_impl.h (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_display_host_impl.h:205:
RESTORE_USER_ADDING,
nit: Let's rename it to something more specific like
Add user to session

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_adding_screen.cc (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.cc:81: {}
nit: Should be placed on separate lines:
 ... {
}

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_adding_screen.h (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
To make name more consistent this could be renamed to
add_user_to_session_screen.
Will that work?

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.h:12: // An interface that
defines screen for adding users into multiprofile session.
nit: multi-profile

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.h:13: // Current implementation
is singleton.
nit: Please add TODO to came up with a better design for this and ideally get
rid of singleton.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.h:13: // Current implementation
is singleton.
nit: is a

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.h:17: virtual void
OnUserAddingStarted() {}
Body should go to .cc

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1422: if
(delegate_) {
nit: drop {}

[dzhioev (left Google), 2013-05-29 12:58:21]

On 2013/05/28 11:34:21, Nikita Kostylev wrote:
> Mostly nits.
> 
> Please think about creating browser test for this (could be in next CL). Smth
> like
> 0. Set up user list (several users)
> 1. Start browser_tests (i.e. runs inside user session)
> 2. Bring up add user screen
> 3. Emulate adding another user
> 
I will add browser test in next CL.

[dzhioev (left Google), 2013-05-29 13:05:03]

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_display_host.h (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_display_host.h:85: // Starts screen for
adding user into session.
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> nit: Add comment on |completion_callback|.

Done.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_display_host_impl.cc:413: if
(waiting_for_wallpaper_load_ && !initialize_webui_hidden_) {
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> Please remove this as I think that is not needed for this type of UI. They
only
> guard boot animation.

Done.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_display_host_impl.cc:420: }
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> nit: drop {}

Done.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_display_host_impl.cc:634: if
(!completion_callback_.is_null()) {
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> nit: drop {}

Done.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_display_host_impl.h (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_display_host_impl.h:205:
RESTORE_USER_ADDING,
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> nit: Let's rename it to something more specific like
> Add user to session

Done.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_adding_screen.cc (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.cc:81: {}
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> nit: Should be placed on separate lines:
>  ... {
> }

Done.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_adding_screen.h (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> To make name more consistent this could be renamed to
> add_user_to_session_screen.
> Will that work?
Decided to leave current name.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.h:12: // An interface that
defines screen for adding users into multiprofile session.
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> nit: multi-profile

Done.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.h:13: // Current implementation
is singleton.
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> nit: is a

Done.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.h:13: // Current implementation
is singleton.
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> nit: Please add TODO to came up with a better design for this and ideally get
> rid of singleton.

Done.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.h:17: virtual void
OnUserAddingStarted() {}
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> Body should go to .cc
I think it is OK to leave empty definitions of Observer's methods in header. I
saw such thing in many other places: PowerManagerClient::Observer,
BluetoothAdapter::Observer, etc.

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1422: if
(delegate_) {
On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> nit: drop {}

Done.

[Nikita (slow), 2013-05-29 13:29:37]

lgtm

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_adding_screen.h (right):

https://codereview.chromium.org/16002004/diff/2007/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_adding_screen.h:17: virtual void
OnUserAddingStarted() {}
On 2013/05/29 13:05:04, dzhioev wrote:
> On 2013/05/28 11:34:22, Nikita Kostylev wrote:
> > Body should go to .cc
> I think it is OK to leave empty definitions of Observer's methods in header. I
> saw such thing in many other places: PowerManagerClient::Observer,
> BluetoothAdapter::Observer, etc.

I think that violates
http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

[commit-bot: I haz the power, 2013-05-29 14:23:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dzhioev@chromium.org/16002004/23001

[commit-bot: I haz the power, 2013-05-29 17:23:49]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-29 18:01:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dzhioev@chromium.org/16002004/27002

[commit-bot: I haz the power, 2013-05-29 19:15:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dzhioev@chromium.org/16002004/27002

[commit-bot: I haz the power, 2013-05-29 23:01:53]

Change committed as 202996