This CL replaces e-mail with AccountId in easy signin code.

This CL replaces e-mail with AccountId in easy unlock/signin code.

This is part of transition to AccountId.

BUG=462823
TEST=manual

Committed: https://crrev.com/546380c369281ef7d2a1ca704b0343acfde9507c
Cr-Commit-Position: refs/heads/master@{#364223}

[Alexander Alekseev, 2015-12-03 14:34:10]

Description was changed from

==========
This CL replaces e-mail with AccountId in easy signin code.

This is part of transition to AccountId.

BUG=462823
TEST=manual
==========

to

==========
This CL replaces e-mail with AccountId in easy unlock/signin code.

This is part of transition to AccountId.

BUG=462823
TEST=manual
==========

[Alexander Alekseev, 2015-12-03 14:34:21]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-03 14:34:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494153002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494153002/1

[commit-bot: I haz the power, 2015-12-03 14:38:32]

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

[commit-bot: I haz the power, 2015-12-03 14:38:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2015-12-03 15:28:15]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-03 15:29:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494153002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494153002/20001

[commit-bot: I haz the power, 2015-12-03 15:35:15]

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

[commit-bot: I haz the power, 2015-12-03 15:35:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Alexander Alekseev, 2015-12-03 15:55:41]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-03 15:56:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494153002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494153002/40001

[commit-bot: I haz the power, 2015-12-03 16:08:13]

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

[commit-bot: I haz the power, 2015-12-03 16:08:14]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Alexander Alekseev, 2015-12-03 16:56:07]

alemate@chromium.org changed reviewers:
+ achuith@chromium.org, rogerta@chromium.org, tengs@chromium.org

[Alexander Alekseev, 2015-12-03 16:56:07]

This CL is rather large, but most of the changes are around */easy_unlock/* .

Please review:

rogerta@ :
  c/b/signin/*
  c/b/ui/webui/signin/*
  */DEPS  (as dependency owner)

tengs@ :
  c/b/c/login/easy_unlock/*
  c/b/extensions/api/easy_unlock_private/
  components/proximity_auth/
  extensions/api/screenlock_private/

achuith@ : all the rest, namely:
  c/b/c/login/app_launch_signin_screen.cc
  c/b/c/login/app_launch_signin_screen.h
  c/b/c/login/auth/chrome_login_performer.cc
  c/b/c/login/auth/chrome_login_performer.h
  c/b/c/login/existing_user_controller.cc
  c/b/c/login/existing_user_controller.h
  c/b/c/login/existing_user_controller_browsertest.cc
  c/b/c/login/lock/webui_screen_locker.cc
  c/b/c/login/lock/webui_screen_locker.h
  c/b/c/login/screens/user_selection_screen.cc
  c/b/c/login/screens/user_selection_screen.h
  c/b/c/login/session/user_session_manager.cc
  c/b/c/login/supervised/supervised_user_creation_controller_new.cc
  c/b/c/login/supervised/supervised_user_creation_controller_new.h
  c/b/c/login/supervised/supervised_user_creation_flow.cc
  c/b/c/login/supervised/supervised_user_creation_flow.h
  c/b/c/login/supervised/supervised_user_creation_screen.cc
  c/b/c/login/supervised/supervised_user_creation_screen.h
  c/b/c/login/supervised/supervised_user_login_flow.cc
  c/b/c/login/supervised/supervised_user_login_flow.h
  c/b/c/login/ui/login_display.h
  c/b/c/login/ui/models/user_board_model.h
  c/b/c/login/ui/webui_login_display.cc
  c/b/c/login/ui/webui_login_display.h
  c/b/c/login/user_flow.cc
  c/b/c/login/user_flow.h
  c/b/c/login/users/chrome_user_manager_impl.cc
  c/b/c/login/users/chrome_user_manager_impl.h
  c/b/ui/webui/chromeos/login/gaia_screen_handler.cc
  c/b/ui/webui/chromeos/login/signin_screen_handler.cc
  c/b/ui/webui/chromeos/login/signin_screen_handler.h
  c/b/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc
  c/b/ui/webui/chromeos/login/supervised_user_creation_screen_handler.h
  c.os/login/auth/login_performer.cc
  c.os/login/auth/login_performer.h

[Roger Tawa OOO till Jul 10th, 2015-12-03 18:23:03]

lgtm
  c/b/signin/*
  c/b/ui/webui/signin/*
  */DEPS

[achuithb, 2015-12-04 10:12:54]

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/bootstrap_manager.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/bootstrap_manager.cc:47: std::string
current_user;
let's make this current_user_email too

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/bootstrap_manager.cc:66:
current_user_email, std::string()));
add comment /* gaia_id */

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_challenge_wrapper.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_challenge_wrapper.h:63:
const AccountId account_id_;
Should this be a const pointer or is a copy correct?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:276:
std::string key = GetPublicTpmKey(account_id);
const

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:132:
AccountId account_id_;
should this be const?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/supervised/supervised_user_creation_controller_new.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_creation_controller_new.h:123:
AccountId manager_id = EmptyAccountId();
Why do this? the default ctor does the right thing, right?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc:238: bool
was_incomplete = auth->HasIncompleteKey(account_id().GetUserEmail());
const

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_login_flow.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_login_flow.h:54: bool
data_loaded_;
Could you please do in-class member initialization while you're here?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_login_flow.h:56:
Profile* profile_;
this should be set to nullptr - it is not being initialized correctly at the
moment.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/models/user_board_model.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/models/user_board_model.h:31: void
PrepareToShow() override{};
shouldn't there be a space between override and {}?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_display.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_display.h:92: bool show_guest_;
could you please initialize these here?

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

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_flow.cc:15: void UnregisterFlow(const
AccountId& account_id) {
This function seems unnecessary?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_flow.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_flow.h:14: class AccountId;
don't think you need this?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_flow.h:89: AccountId account_id_;
const?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc:130:
const AccountId& /* account_id */) {}
personally I don't think the /* */ is necessary since the fn body is empty

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/screenlock_private/screenlock_private_api.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/screenlock_private/screenlock_private_api.h:70: //
proximity_auth::ScreenlockBridge::Observer
let's make these private?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_auth_attempt.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_auth_attempt.h:78: AccountId account_id_;
const?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc:200: AccountId
account_id_;
const

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_screenlock_state_handler.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_screenlock_state_handler.h:98: AccountId
account_id_;
const

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_screenlock_state_handler.h:103: bool
hardlock_ui_shown_;
= false, here and below

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.h:102: virtual const AccountId
GetAccountId() const = 0;
AccountId? Don't think const makes sense unless it's a const reference

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.h:80: const AccountId
GetAccountId() const override;
AccountId?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:105:
account_id_(EmptyAccountId()),
Doesn't the default ctor do the right thing?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:123:
std::map<AccountId, UserData*>::const_iterator it =
auto?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:343: bool
should_update_app_state =
const

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:383:
std::map<AccountId, UserData*>::iterator it = user_data_.find(account_id_);
auto?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:471: return NULL;
nullptr

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:473:
std::map<AccountId, UserData*>::const_iterator it =
auto?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:476: return NULL;
nullptr. Could you please replace all instances in this file?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.h:82: const AccountId
GetAccountId() const override;
AccountId

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.h:140: bool
allow_cryptohome_backoff_;
in class member initialization please

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:408: const
std::string& account_identifier) {
should we rename to user_email?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:411:
account_identifier, std::string())))
/* gaia_id */

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.h:35:
virtual void AuthenticateManager(const AccountId& manager_id,
manager_account_id?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:356:
UserAuthTypeMap::const_iterator it =
auto

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:369:
base::FilePath path = profiles::GetPathOfProfileWithEmail(
const

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.h:47: void
GetLocalizedValues(base::DictionaryValue* localized_strings);
Make this public and everything else private?

https://codereview.chromium.org/1494153002/diff/40001/components/proximity_au...
File components/proximity_auth/proximity_auth_system.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/components/proximity_au...
components/proximity_auth/proximity_auth_system.cc:52: void
ProximityAuthSystem::OnAuthAttempted(const AccountId& account_id) {
/* account_id */

https://codereview.chromium.org/1494153002/diff/40001/components/proximity_au...
components/proximity_auth/proximity_auth_system.cc:53: // TODO(tengs): There is
no reason to pass the |user_id| argument anymore.
update comment

https://codereview.chromium.org/1494153002/diff/40001/components/proximity_au...
File components/proximity_auth/screenlock_bridge.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/components/proximity_au...
components/proximity_auth/screenlock_bridge.cc:179: 
default ctor does the right thing, right?

[Alexander Alekseev, 2015-12-04 12:44:07]

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/bootstrap_manager.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/bootstrap_manager.cc:47: std::string
current_user;
On 2015/12/04 10:12:52, achuithb wrote:
> let's make this current_user_email too

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/bootstrap_manager.cc:66:
current_user_email, std::string()));
On 2015/12/04 10:12:52, achuithb wrote:
> add comment /* gaia_id */

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_challenge_wrapper.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_challenge_wrapper.h:63:
const AccountId account_id_;
On 2015/12/04 10:12:52, achuithb wrote:
> Should this be a const pointer or is a copy correct?

It is created as a copy of a temporary object. So it would be tricky to make
this a reference.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:276:
std::string key = GetPublicTpmKey(account_id);
On 2015/12/04 10:12:52, achuithb wrote:
> const

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:132:
AccountId account_id_;
On 2015/12/04 10:12:52, achuithb wrote:
> should this be const?

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/supervised/supervised_user_creation_controller_new.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_creation_controller_new.h:123:
AccountId manager_id = EmptyAccountId();
On 2015/12/04 10:12:52, achuithb wrote:
> Why do this? the default ctor does the right thing, right?

We cannot make AccountId default-constructible until migration is done.
The purpose is to annotate all places, where we might have empty account id
objects.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_login_flow.cc:238: bool
was_incomplete = auth->HasIncompleteKey(account_id().GetUserEmail());
On 2015/12/04 10:12:52, achuithb wrote:
> const

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_login_flow.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_login_flow.h:54: bool
data_loaded_;
On 2015/12/04 10:12:52, achuithb wrote:
> Could you please do in-class member initialization while you're here?

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_login_flow.h:56:
Profile* profile_;
On 2015/12/04 10:12:52, achuithb wrote:
> this should be set to nullptr - it is not being initialized correctly at the
> moment.

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/models/user_board_model.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/models/user_board_model.h:31: void
PrepareToShow() override{};
On 2015/12/04 10:12:52, achuithb wrote:
> shouldn't there be a space between override and {}?

Done.

(This looks like a bug in cl-format as it cuts space here.)

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_display.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_display.h:92: bool show_guest_;
On 2015/12/04 10:12:52, achuithb wrote:
> could you please initialize these here?

Done.

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

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_flow.cc:15: void UnregisterFlow(const
AccountId& account_id) {
On 2015/12/04 10:12:52, achuithb wrote:
> This function seems unnecessary? 

This is used in ExtendedUserFlow::UnregisterFlowSoon(). So it seems necessary.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_flow.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_flow.h:14: class AccountId;
On 2015/12/04 10:12:52, achuithb wrote:
> don't think you need this?

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_flow.h:89: AccountId account_id_;
On 2015/12/04 10:12:52, achuithb wrote:
> const?

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc:130:
const AccountId& /* account_id */) {}
On 2015/12/04 10:12:52, achuithb wrote:
> personally I don't think the /* */ is necessary since the fn body is empty

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/screenlock_private/screenlock_private_api.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/screenlock_private/screenlock_private_api.h:70: //
proximity_auth::ScreenlockBridge::Observer
On 2015/12/04 10:12:52, achuithb wrote:
> let's make these private?

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_auth_attempt.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_auth_attempt.h:78: AccountId account_id_;
On 2015/12/04 10:12:53, achuithb wrote:
> const?

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_auth_attempt_unittest.cc:200: AccountId
account_id_;
On 2015/12/04 10:12:53, achuithb wrote:
> const

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_screenlock_state_handler.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_screenlock_state_handler.h:98: AccountId
account_id_;
On 2015/12/04 10:12:53, achuithb wrote:
> const

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_screenlock_state_handler.h:103: bool
hardlock_ui_shown_;
On 2015/12/04 10:12:53, achuithb wrote:
> = false, here and below

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.h:102: virtual const AccountId
GetAccountId() const = 0;
On 2015/12/04 10:12:53, achuithb wrote:
> AccountId? Don't think const makes sense unless it's a const reference

Done.

It should be a constant reference, but it is not possible now (in this CL).

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.h:80: const AccountId
GetAccountId() const override;
On 2015/12/04 10:12:53, achuithb wrote:
> AccountId?

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:105:
account_id_(EmptyAccountId()),
On 2015/12/04 10:12:53, achuithb wrote:
> Doesn't the default ctor do the right thing?

No.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:123:
std::map<AccountId, UserData*>::const_iterator it =
On 2015/12/04 10:12:53, achuithb wrote:
> auto?

We know the exact type here, so we can use const_iterator instead of iterator.
So let's leave as it is.

Or, if it looks too long, we can make a shorter typedef for this map.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:343: bool
should_update_app_state =
On 2015/12/04 10:12:53, achuithb wrote:
> const

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:383:
std::map<AccountId, UserData*>::iterator it = user_data_.find(account_id_);
On 2015/12/04 10:12:53, achuithb wrote:
> auto?

Let's add const here ;)

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:471: return NULL;
On 2015/12/04 10:12:53, achuithb wrote:
> nullptr

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:473:
std::map<AccountId, UserData*>::const_iterator it =
On 2015/12/04 10:12:53, achuithb wrote:
> auto?

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:476: return NULL;
On 2015/12/04 10:12:53, achuithb wrote:
> nullptr. Could you please replace all instances in this file?

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.h:82: const AccountId
GetAccountId() const override;
On 2015/12/04 10:12:53, achuithb wrote:
> AccountId

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.h:140: bool
allow_cryptohome_backoff_;
On 2015/12/04 10:12:53, achuithb wrote:
> in class member initialization please

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:408: const
std::string& account_identifier) {
On 2015/12/04 10:12:53, achuithb wrote:
> should we rename to user_email?

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:411:
account_identifier, std::string())))
On 2015/12/04 10:12:53, achuithb wrote:
> /* gaia_id */

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.h
(right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.h:35:
virtual void AuthenticateManager(const AccountId& manager_id,
On 2015/12/04 10:12:53, achuithb wrote:
> manager_account_id?

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:356:
UserAuthTypeMap::const_iterator it =
On 2015/12/04 10:12:53, achuithb wrote:
> auto

Why? This doesn't look too long.
Personally I think auto makes code less readable. So I would better leave this
as it is.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:369:
base::FilePath path = profiles::GetPathOfProfileWithEmail(
On 2015/12/04 10:12:53, achuithb wrote:
> const

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.h (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.h:47: void
GetLocalizedValues(base::DictionaryValue* localized_strings);
On 2015/12/04 10:12:53, achuithb wrote:
> Make this public and everything else private?

Done.

https://codereview.chromium.org/1494153002/diff/40001/components/proximity_au...
File components/proximity_auth/proximity_auth_system.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/components/proximity_au...
components/proximity_auth/proximity_auth_system.cc:52: void
ProximityAuthSystem::OnAuthAttempted(const AccountId& account_id) {
On 2015/12/04 10:12:53, achuithb wrote:
> /* account_id */

Done.

https://codereview.chromium.org/1494153002/diff/40001/components/proximity_au...
components/proximity_auth/proximity_auth_system.cc:53: // TODO(tengs): There is
no reason to pass the |user_id| argument anymore.
On 2015/12/04 10:12:53, achuithb wrote:
> update comment

Done.

https://codereview.chromium.org/1494153002/diff/40001/components/proximity_au...
File components/proximity_auth/screenlock_bridge.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/components/proximity_au...
components/proximity_auth/screenlock_bridge.cc:179: 
On 2015/12/04 10:12:53, achuithb wrote:
> default ctor does the right thing, right?

AccountId is not default-constructible.

[Alexander Alekseev, 2015-12-04 12:45:13]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-04 12:45:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494153002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494153002/60001

[commit-bot: I haz the power, 2015-12-04 13:29:18]

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

[commit-bot: I haz the power, 2015-12-04 13:29:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[achuithb, 2015-12-04 20:16:57]

If you're really against use of auto, that's fine, but I believe the style guide
is clear on this particular case (return type of std::map::find()). In all these
cases, the type of the iterator it is clear from the map that you're using.

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

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_flow.cc:15: void UnregisterFlow(const
AccountId& account_id) {
On 2015/12/04 12:44:06, Alexander Alekseev wrote:
> On 2015/12/04 10:12:52, achuithb wrote:
> > This function seems unnecessary? 
> 
> This is used in ExtendedUserFlow::UnregisterFlowSoon(). So it seems necessary.

No, I meant that in UnregisterFlowSoon, you could do something like:
PostTask(FROM_HERE, base::Bind(base::Unretained(ChromeUserManager::Get()),
&ChromeUserManager::ResetUserFlow, account_id());

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:123:
std::map<AccountId, UserData*>::const_iterator it =
On 2015/12/04 12:44:07, Alexander Alekseev wrote:
> On 2015/12/04 10:12:53, achuithb wrote:
> > auto?
> 
> We know the exact type here, so we can use const_iterator instead of iterator.
> So let's leave as it is.
> 
> Or, if it looks too long, we can make a shorter typedef for this map.

If you look at the style guide, I think the idea is to use auto for precisely
this case:
https://google.github.io/styleguide/cppguide.html#auto

It is to improve readability when you have a long type name that doesn't add any
value.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:383:
std::map<AccountId, UserData*>::iterator it = user_data_.find(account_id_);
On 2015/12/04 12:44:07, Alexander Alekseev wrote:
> On 2015/12/04 10:12:53, achuithb wrote:
> > auto?
> 
> Let's add const here ;)

I really believe auto is recommended in this case.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:356:
UserAuthTypeMap::const_iterator it =
On 2015/12/04 12:44:07, Alexander Alekseev wrote:
> On 2015/12/04 10:12:53, achuithb wrote:
> > auto
> 
> Why? This doesn't look too long.
> Personally I think auto makes code less readable. So I would better leave this
> as it is.

It would fit in one line with auto, and I honestly don't believe it hurts
readability since the type is clear from user_auth_type_map_

[achuithb, 2015-12-04 20:23:39]

lgtm.

Thanks for making the suggested changes!

I hope you'll reconsider auto :)

Please fix the nits below to conform with the style guide recommendations.

https://codereview.chromium.org/1494153002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/screenlock_private/screenlock_private_api.h
(right):

https://codereview.chromium.org/1494153002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/screenlock_private/screenlock_private_api.h:79:
friend class BrowserContextKeyedAPIFactory<ScreenlockPrivateEventRouter>;
Sorry, I think we need to move this to right after private:

https://codereview.chromium.org/1494153002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.h (right):

https://codereview.chromium.org/1494153002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.h:76: class
ProfileUpdateObserver;
Let's move this and the comment to right after private:

[Alexander Alekseev, 2015-12-05 05:20:08]

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

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_flow.cc:15: void UnregisterFlow(const
AccountId& account_id) {
On 2015/12/04 20:16:56, achuithb wrote:
> On 2015/12/04 12:44:06, Alexander Alekseev wrote:
> > On 2015/12/04 10:12:52, achuithb wrote:
> > > This function seems unnecessary? 
> > 
> > This is used in ExtendedUserFlow::UnregisterFlowSoon(). So it seems
necessary.
> 
> No, I meant that in UnregisterFlowSoon, you could do something like:
> PostTask(FROM_HERE, base::Bind(base::Unretained(ChromeUserManager::Get()),
> &ChromeUserManager::ResetUserFlow, account_id());

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:123:
std::map<AccountId, UserData*>::const_iterator it =
On 2015/12/04 20:16:56, achuithb wrote:
> On 2015/12/04 12:44:07, Alexander Alekseev wrote:
> > On 2015/12/04 10:12:53, achuithb wrote:
> > > auto?
> > 
> > We know the exact type here, so we can use const_iterator instead of
iterator.
> > So let's leave as it is.
> > 
> > Or, if it looks too long, we can make a shorter typedef for this map.
> 
> If you look at the style guide, I think the idea is to use auto for precisely
> this case:
> https://google.github.io/styleguide/cppguide.html#auto
> 
> It is to improve readability when you have a long type name that doesn't add
any
> value.

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:383:
std::map<AccountId, UserData*>::iterator it = user_data_.find(account_id_);
On 2015/12/04 20:16:56, achuithb wrote:
> On 2015/12/04 12:44:07, Alexander Alekseev wrote:
> > On 2015/12/04 10:12:53, achuithb wrote:
> > > auto?
> > 
> > Let's add const here ;)
> 
> I really believe auto is recommended in this case.

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:473:
std::map<AccountId, UserData*>::const_iterator it =
On 2015/12/04 12:44:07, Alexander Alekseev wrote:
> On 2015/12/04 10:12:53, achuithb wrote:
> > auto?

Done.

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1494153002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:356:
UserAuthTypeMap::const_iterator it =
On 2015/12/04 20:16:56, achuithb wrote:
> On 2015/12/04 12:44:07, Alexander Alekseev wrote:
> > On 2015/12/04 10:12:53, achuithb wrote:
> > > auto
> > 
> > Why? This doesn't look too long.
> > Personally I think auto makes code less readable. So I would better leave
this
> > as it is.
> 
> It would fit in one line with auto, and I honestly don't believe it hurts
> readability since the type is clear from user_auth_type_map_

Done.

https://codereview.chromium.org/1494153002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/screenlock_private/screenlock_private_api.h
(right):

https://codereview.chromium.org/1494153002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/screenlock_private/screenlock_private_api.h:79:
friend class BrowserContextKeyedAPIFactory<ScreenlockPrivateEventRouter>;
On 2015/12/04 20:23:38, achuithb wrote:
> Sorry, I think we need to move this to right after private:

Done.

https://codereview.chromium.org/1494153002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.h (right):

https://codereview.chromium.org/1494153002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.h:76: class
ProfileUpdateObserver;
On 2015/12/04 20:23:39, achuithb wrote:
> Let's move this and the comment to right after private:

Done.

[achuithb, 2015-12-05 05:47:43]

lgtm

[Alexander Alekseev, 2015-12-05 06:05:04]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-05 06:05:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494153002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494153002/80001

[commit-bot: I haz the power, 2015-12-05 06:48:54]

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

[commit-bot: I haz the power, 2015-12-05 06:48:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2015-12-05 10:21:02]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-05 10:21:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494153002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494153002/100001

[commit-bot: I haz the power, 2015-12-05 12:22:04]

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

[commit-bot: I haz the power, 2015-12-05 12:22:05]

Dry run: Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[Alexander Alekseev, 2015-12-08 03:04:32]

tengs@ , friendly ping.

[Tim Song, 2015-12-08 06:51:16]

Sorry for the delay. LGTM.

[Alexander Alekseev, 2015-12-08 07:29:48]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-12-08 07:29:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/1494153002/#ps100001
(title: "Bugfix.")

[commit-bot: I haz the power, 2015-12-08 07:30:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494153002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494153002/100001

[commit-bot: I haz the power, 2015-12-08 07:36:21]

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

[commit-bot: I haz the power, 2015-12-08 07:36:25]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Alexander Alekseev, 2015-12-08 19:51:28]

rogerta@ - Please look at new note.

https://codereview.chromium.org/1494153002/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (left):

https://codereview.chromium.org/1494153002/diff/100001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:426: if (user_id !=
user_id || devices.empty())
Roger, doesn't this line looks like a bug? Should I also fix this?

[Alexander Alekseev, 2015-12-09 02:46:26]

alemate@chromium.org changed reviewers:
+ asargent@chromium.org

[Alexander Alekseev, 2015-12-09 02:46:27]

Adding asargent@ as OWNER for:

chrome/browser/extensions/api/screenlock_private/screenlock_private_api.cc
    chrome/browser/extensions/api/screenlock_private/screenlock_private_api.h
   
chrome/browser/extensions/api/screenlock_private/screenlock_private_apitest.cc


Please review.

[Alexander Alekseev, 2015-12-09 02:46:56]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 02:48:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494153002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494153002/120001

[Alexander Alekseev, 2015-12-09 02:49:15]

tengs@ , what do you think about this strange line?

https://codereview.chromium.org/1494153002/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (left):

https://codereview.chromium.org/1494153002/diff/100001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:426: if (user_id !=
user_id || devices.empty())
On 2015/12/08 19:51:27, Alexander Alekseev wrote:
> Roger, doesn't this line looks like a bug? Should I also fix this?

Tim, what do you think?

[Tim Song, 2015-12-09 03:16:42]

https://codereview.chromium.org/1494153002/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (left):

https://codereview.chromium.org/1494153002/diff/100001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:426: if (user_id !=
user_id || devices.empty())
On 2015/12/09 02:49:15, Alexander Alekseev wrote:
> On 2015/12/08 19:51:27, Alexander Alekseev wrote:
> > Roger, doesn't this line looks like a bug? Should I also fix this?
> 
> Tim, what do you think?

Yes this is definitely a bug. |user_id| should definitely be compared against
|user_id_| (the member variable). Thanks for catching this.

[Tim Song, 2015-12-09 03:52:48]

https://codereview.chromium.org/1494153002/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (left):

https://codereview.chromium.org/1494153002/diff/100001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:426: if (user_id !=
user_id || devices.empty())
On 2015/12/09 03:16:42, Tim Song wrote:
> On 2015/12/09 02:49:15, Alexander Alekseev wrote:
> > On 2015/12/08 19:51:27, Alexander Alekseev wrote:
> > > Roger, doesn't this line looks like a bug? Should I also fix this?
> > 
> > Tim, what do you think?
> 
> Yes this is definitely a bug. |user_id| should definitely be compared against
> |user_id_| (the member variable). Thanks for catching this.

Actually, I think the proper fix is just to remove this expression, since that
preserves the existing logic.

[Alexander Alekseev, 2015-12-09 04:48:22]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 04:48:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494153002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494153002/140001

[Alexander Alekseev, 2015-12-09 04:49:24]

https://codereview.chromium.org/1494153002/diff/100001/chrome/browser/signin/...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (left):

https://codereview.chromium.org/1494153002/diff/100001/chrome/browser/signin/...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:426: if (user_id !=
user_id || devices.empty())
On 2015/12/09 03:52:48, Tim Song wrote:
> On 2015/12/09 03:16:42, Tim Song wrote:
> > On 2015/12/09 02:49:15, Alexander Alekseev wrote:
> > > On 2015/12/08 19:51:27, Alexander Alekseev wrote:
> > > > Roger, doesn't this line looks like a bug? Should I also fix this?
> > > 
> > > Tim, what do you think?
> > 
> > Yes this is definitely a bug. |user_id| should definitely be compared
against
> > |user_id_| (the member variable). Thanks for catching this.
> 
> Actually, I think the proper fix is just to remove this expression, since that
> preserves the existing logic.

Done.

[commit-bot: I haz the power, 2015-12-09 06:03:46]

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

[commit-bot: I haz the power, 2015-12-09 06:03:47]

Dry run: This issue passed the CQ dry run.

[asargent_no_longer_on_chrome, 2015-12-09 17:20:39]

chrome/browser/extensions/api/screenlock_private/* lgtm

[Alexander Alekseev, 2015-12-09 23:44:46]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-12-09 23:44:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, tengs@chromium.org, rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1494153002/#ps140001
(title: "Bugfix in original easy unlock code'")

[commit-bot: I haz the power, 2015-12-09 23:47:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494153002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494153002/140001

[commit-bot: I haz the power, 2015-12-09 23:54:53]

Description was changed from

==========
This CL replaces e-mail with AccountId in easy unlock/signin code.

This is part of transition to AccountId.

BUG=462823
TEST=manual
==========

to

==========
This CL replaces e-mail with AccountId in easy unlock/signin code.

This is part of transition to AccountId.

BUG=462823
TEST=manual
==========

[commit-bot: I haz the power, 2015-12-09 23:54:54]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-09 23:55:34]

Description was changed from

==========
This CL replaces e-mail with AccountId in easy unlock/signin code.

This is part of transition to AccountId.

BUG=462823
TEST=manual
==========

to

==========
This CL replaces e-mail with AccountId in easy unlock/signin code.

This is part of transition to AccountId.

BUG=462823
TEST=manual

Committed: https://crrev.com/546380c369281ef7d2a1ca704b0343acfde9507c
Cr-Commit-Position: refs/heads/master@{#364223}
==========

[commit-bot: I haz the power, 2015-12-09 23:55:36]

Patchset 8 (id:??) landed as
https://crrev.com/546380c369281ef7d2a1ca704b0343acfde9507c
Cr-Commit-Position: refs/heads/master@{#364223}