ChromeOS login webui refactoring : Simplify login methods.

ChromeOS login webui refactoring : Simplify login methods.


BUG=373314
R=nkostylev@chromium.org

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

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

[Nikita (slow), 2014-06-06 14:49:48]

lgtm

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

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/app_launch_signin_screen.h:60: //  virtual void
LoginAsRetailModeUser() OVERRIDE;
These are no longer used, may be removed.

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

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/existing_user_controller.cc:390: if
(user_context.GetUserType() == User::USER_TYPE_PUBLIC_ACCOUNT) {
nit: else if here and below.

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
File chrome/browser/chromeos/login/lock/webui_screen_locker.h (right):

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/lock/webui_screen_locker.h:83: //  virtual void
LoginAsRetailModeUser() OVERRIDE;
May be removed.

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
File chrome/browser/chromeos/login/signin_specifics.h (right):

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/signin_specifics.h:10: #include
"chrome/browser/chromeos/login/auth/key.h"
nit: Both includes are not needed.

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/signin_specifics.h:20: std::string guest_mode_url;
Were you planning to use guest_mode_url in this CL or later?

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
File chrome/browser/chromeos/login/ui/login_display.h (right):

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/ui/login_display.h:42: // Ignore password change,
remove existing cryptohome and
Are you planning to leave this (LoginDisplay) interface or not?

If you planning to leave it, makes sense to sort methods in
webui_login_display.h

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/ui/login_display.h:55: // Sign in as a retail mode
user.
nit: Remove commented code.

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
File chrome/browser/chromeos/login/ui/webui_login_display.h (right):

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/ui/webui_login_display.h:61: //  virtual void
LoginAsRetailModeUser() OVERRIDE;
No longer used, may be removed.

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

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:136: //
--------------- Account creation methods.
nit: should be updated

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:156: //
--------------- Account creation methods.
nit: should be updated

[Denis Kuznetsov (DE-MUC), 2014-06-06 15:18:43]

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

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/app_launch_signin_screen.h:60: //  virtual void
LoginAsRetailModeUser() OVERRIDE;
On 2014/06/06 14:49:47, Nikita Kostylev wrote:
> These are no longer used, may be removed.

Done.

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

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/existing_user_controller.cc:390: if
(user_context.GetUserType() == User::USER_TYPE_PUBLIC_ACCOUNT) {
On 2014/06/06 14:49:47, Nikita Kostylev wrote:
> nit: else if here and below.

Done.

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
File chrome/browser/chromeos/login/lock/webui_screen_locker.h (right):

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/lock/webui_screen_locker.h:83: //  virtual void
LoginAsRetailModeUser() OVERRIDE;
On 2014/06/06 14:49:47, Nikita Kostylev wrote:
> May be removed.

Done.

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
File chrome/browser/chromeos/login/signin_specifics.h (right):

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/signin_specifics.h:10: #include
"chrome/browser/chromeos/login/auth/key.h"
On 2014/06/06 14:49:47, Nikita Kostylev wrote:
> nit: Both includes are not needed.

Done.

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/signin_specifics.h:20: std::string guest_mode_url;
On 2014/06/06 14:49:47, Nikita Kostylev wrote:
> Were you planning to use guest_mode_url in this CL or later?
Added accepting parameters.
I will set this parameter in future CLs to get rid of CreateAccount() method.

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
File chrome/browser/chromeos/login/ui/login_display.h (right):

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/ui/login_display.h:42: // Ignore password change,
remove existing cryptohome and
On 2014/06/06 14:49:47, Nikita Kostylev wrote:
> Are you planning to leave this (LoginDisplay) interface or not?
> 
> If you planning to leave it, makes sense to sort methods in
> webui_login_display.h
It will live in some way, but it's most likely to be split to 1-2-3 separate
interfaces.
I will do another pass to ensure ordering of the methods after that.

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/ui/login_display.h:55: // Sign in as a retail mode
user.
On 2014/06/06 14:49:48, Nikita Kostylev wrote:
> nit: Remove commented code.

Done.

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
File chrome/browser/chromeos/login/ui/webui_login_display.h (right):

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/chrome...
chrome/browser/chromeos/login/ui/webui_login_display.h:61: //  virtual void
LoginAsRetailModeUser() OVERRIDE;
On 2014/06/06 14:49:48, Nikita Kostylev wrote:
> No longer used, may be removed.

Done.

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

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:136: //
--------------- Account creation methods.
On 2014/06/06 14:49:48, Nikita Kostylev wrote:
> nit: should be updated

Done.

https://chromiumcodereview.appspot.com/324463003/diff/1/chrome/browser/ui/web...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:156: //
--------------- Account creation methods.
On 2014/06/06 14:49:48, Nikita Kostylev wrote:
> nit: should be updated

Done.

[Denis Kuznetsov (DE-MUC), 2014-06-06 15:19:11]

The CQ bit was checked by antrim@chromium.org

[commit-bot: I haz the power, 2014-06-06 15:20:13]

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

[commit-bot: I haz the power, 2014-06-06 18:59:36]

Change committed as 275492

[Denis Kuznetsov (DE-MUC), 2014-06-11 12:13:26]

This is the right one to look at.

[Denis Kuznetsov (DE-MUC), 2014-06-11 12:13:28]

This is the right one to look at.

[Nikita (slow), 2014-06-11 14:05:14]

lgtm

https://codereview.chromium.org/324463003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/auth/user_context.cc (right):

https://codereview.chromium.org/324463003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/auth/user_context.cc:41: user_type_ ==
User::USER_TYPE_KIOSK_APP) {
nit: I think SUPERVISED user should be here as well.

How about just

if (user_type_ == USER_TYPE_REGULAR)
  user_id_ = login::CanonicalizeUserID(user_id);
else
  user_id_ = user_id;

[Denis Kuznetsov (DE-MUC), 2014-06-16 12:14:38]

The CQ bit was checked by antrim@chromium.org

[commit-bot: I haz the power, 2014-06-16 12:15:42]

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

[commit-bot: I haz the power, 2014-06-16 15:57:44]

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

[commit-bot: I haz the power, 2014-06-16 15:57:46]

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

[Nikita (slow), 2014-06-16 16:27:47]

Rebase needed

../../chrome/browser/chromeos/login/existing_user_controller.cc:390:79: error:
too few arguments to function 'GURL google_util::AppendGoogleLocaleParam(const
GURL&, const string&)'
../../chrome/browser/google/google_util.h:37:6: note: declared here

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

Please execute linux_chromium_chromeos_dbg manually before committing.

[Denis Kuznetsov (DE-MUC), 2014-06-24 13:12:58]

The CQ bit was checked by antrim@chromium.org

[commit-bot: I haz the power, 2014-06-24 13:13:47]

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

[commit-bot: I haz the power, 2014-06-24 15:14:02]

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

[commit-bot: I haz the power, 2014-06-24 15:57:42]

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

[commit-bot: I haz the power, 2014-06-24 15:57:43]

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...)

[Denis Kuznetsov (DE-MUC), 2014-06-24 17:52:49]

The CQ bit was checked by antrim@chromium.org

[commit-bot: I haz the power, 2014-06-24 17:53:38]

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

[commit-bot: I haz the power, 2014-06-24 17:55:34]

Change committed as 279438