ChromeOS login webui refactoring: split user selection/gaia login screens.

ChromeOS login webui refactoring: split user selection/gaia login screens.
Phase two : move user list processing to UserSelectionScreen

BUG=373314
R=nkostylev@chromium.org

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

[Nikita (slow), 2014-05-23 15:52:15]

https://codereview.chromium.org/296823009/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/app_launch_signin_screen.cc (right):

https://codereview.chromium.org/296823009/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/app_launch_signin_screen.cc:209: base::ListValue
users_list;
Why this screen should implement this? It should just have an empty body here.

https://codereview.chromium.org/296823009/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/screens/user_selection_screen.h (right):

https://codereview.chromium.org/296823009/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/screens/user_selection_screen.h:65: bool
show_guest_;
nit: Copy comment as well.

https://codereview.chromium.org/296823009/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/296823009/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1233: void
SigninScreenHandler::FillUserDictionary(User* user,
You can delete this method.

[Denis Kuznetsov (DE-MUC), 2014-05-27 14:22:37]

Nikita, please take another look.
Tim, please take a look on app_launch_signin_screen.

[Nikita (slow), 2014-05-27 17:03:24]

https://codereview.chromium.org/296823009/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/296823009/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1233: void
SigninScreenHandler::FillUserDictionary(User* user,
On 2014/05/23 15:52:15, Nikita Kostylev wrote:
> You can delete this method.

What about this method?

[Denis Kuznetsov (DE-MUC), 2014-05-27 17:35:28]

https://chromiumcodereview.appspot.com/296823009/diff/1/chrome/browser/chrome...
File chrome/browser/chromeos/login/app_launch_signin_screen.cc (right):

https://chromiumcodereview.appspot.com/296823009/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/app_launch_signin_screen.cc:209: base::ListValue
users_list;
On 2014/05/23 15:52:15, Nikita Kostylev wrote:
> Why this screen should implement this? It should just have an empty body here.

Because it uses user pods (with just one user).

https://chromiumcodereview.appspot.com/296823009/diff/1/chrome/browser/chrome...
File chrome/browser/chromeos/login/screens/user_selection_screen.h (right):

https://chromiumcodereview.appspot.com/296823009/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/screens/user_selection_screen.h:65: bool
show_guest_;
On 2014/05/23 15:52:15, Nikita Kostylev wrote:
> nit: Copy comment as well.

Done.

https://chromiumcodereview.appspot.com/296823009/diff/1/chrome/browser/ui/web...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://chromiumcodereview.appspot.com/296823009/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1233: void
SigninScreenHandler::FillUserDictionary(User* user,
On 2014/05/27 17:03:24, Nikita Kostylev wrote:
> On 2014/05/23 15:52:15, Nikita Kostylev wrote:
> > You can delete this method.
> 
> What about this method?

Done.

[Nikita (slow), 2014-05-28 04:08:59]

lgtm

https://codereview.chromium.org/296823009/diff/60001/chrome/browser/ui/webui/...
File
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.cc
(right):

https://codereview.chromium.org/296823009/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.cc:223:
UserSelectionScreen::FillUserDictionary(
Now that this utility method is used in several screens please consider moving
it into some common place later.

[Tim Song, 2014-05-28 18:16:07]

I'm not too familiar with the refactoring you're doing, but the
AppLaunchSignInScreen is functionally just a specialized UserSelectionScreen in
kiosk mode. I had it implement the large SigninScreenHandlerDelegate interface
to get it to work with the rest of the login UI. 

Now that you're refactoring the login code, I think it might be a good idea to
revisit this design. For example, would it make sense to make
UserSelectionScreen an interface that AppLaunchSigninScreen can implement?

https://chromiumcodereview.appspot.com/296823009/diff/80001/chrome/browser/ui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):

https://chromiumcodereview.appspot.com/296823009/diff/80001/chrome/browser/ui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:102: // An
interface for SigninScreenHandler to call WebUILoginDisplay.
This is not strictly true as AppLaunchSigninScreen also implements this
interface.

[Denis Kuznetsov (DE-MUC), 2014-05-29 13:46:04]

On 2014/05/28 18:16:07, Tim Song wrote:
> I'm not too familiar with the refactoring you're doing, but the
> AppLaunchSignInScreen is functionally just a specialized UserSelectionScreen
in
> kiosk mode. I had it implement the large SigninScreenHandlerDelegate interface
> to get it to work with the rest of the login UI. 
> 
> Now that you're refactoring the login code, I think it might be a good idea to
> revisit this design. For example, would it make sense to make
> UserSelectionScreen an interface that AppLaunchSigninScreen can implement?
> 
>
https://chromiumcodereview.appspot.com/296823009/diff/80001/chrome/browser/ui...
> File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right):
> 
>
https://chromiumcodereview.appspot.com/296823009/diff/80001/chrome/browser/ui...
> chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:102: // An
> interface for SigninScreenHandler to call WebUILoginDisplay.
> This is not strictly true as AppLaunchSigninScreen also implements this
> interface.


Well, key idea of refactoring is to generalize idea of the "Screen", and make
them modular.
At current phase I'm working on separating interfaces for GAIA-based login
screen and UserPod-based screen.

I will split SigninScreenHandlerDelegate in upcoming CLs - it's on the way, it
is bloated, and most implementations have part of methods marked as not
implemented, that's a bad sign too.

I also want to move as much logic as possible from JS side and Handler side, so
that it can be easily tested/reused.

[Tim Song, 2014-05-29 16:57:18]

Thanks for the explanation about the refactoring. LGTM for these changes. Please
consider refactoring AppLaunchSigninScreen as a modular "Screen" when you get to
that point.

[Denis Kuznetsov (DE-MUC), 2014-05-30 15:17:09]

The CQ bit was checked by antrim@chromium.org

[commit-bot: I haz the power, 2014-05-30 15:18:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/antrim@chromium.org/296823009/80001

[commit-bot: I haz the power, 2014-05-31 01:01:03]

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

[commit-bot: I haz the power, 2014-05-31 02:02:50]

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

[commit-bot: I haz the power, 2014-05-31 02:02:50]

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

[Denis Kuznetsov (DE-MUC), 2014-06-02 12:45:38]

The CQ bit was checked by antrim@chromium.org

[commit-bot: I haz the power, 2014-06-02 12:45:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/antrim@chromium.org/296823009/80001

[commit-bot: I haz the power, 2014-06-02 13:56:22]

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

[commit-bot: I haz the power, 2014-06-02 17:18:40]

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

[commit-bot: I haz the power, 2014-06-02 17:18:41]

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

[Denis Kuznetsov (DE-MUC), 2014-06-04 19:26:17]

The CQ bit was checked by antrim@chromium.org

[commit-bot: I haz the power, 2014-06-04 19:27:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/antrim@chromium.org/296823009/120001

[commit-bot: I haz the power, 2014-06-04 22:31:03]

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

[commit-bot: I haz the power, 2014-06-04 22:36:52]

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

[commit-bot: I haz the power, 2014-06-04 22:36:52]

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

[Denis Kuznetsov (DE-MUC), 2014-06-05 08:48:35]

The CQ bit was checked by antrim@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/antrim@chromium.org/296823009/120001

[commit-bot: I haz the power, 2014-06-05 09:26:13]

Change committed as 275046