Adding mojo calls for easy unlock service

Adding mojo calls for easy unlock service

BUG=721524
Review-Url: https://codereview.chromium.org/2903353003
Cr-Commit-Position: refs/heads/master@{#477722}
Committed: https://chromium.googlesource.com/chromium/src/+/9f6fa0ed40b236032707e7af67009ce53817e627

[xiaoyinh(OOO Sep 11-29), 2017-05-25 23:39:03]

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

[commit-bot: I haz the power, 2017-05-25 23:39:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiaoyinh(OOO Sep 11-29), 2017-05-25 23:57:53]

xiaoyinh@chromium.org changed reviewers:
+ jdufault@chromium.org

[xiaoyinh(OOO Sep 11-29), 2017-05-25 23:57:54]

jdufault@, could you take a look? Thanks!

[commit-bot: I haz the power, 2017-05-26 00:17:28]

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

[commit-bot: I haz the power, 2017-05-26 00:17:29]

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

[xiaoyinh(OOO Sep 11-29), 2017-05-26 16:46:08]

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

[commit-bot: I haz the power, 2017-05-26 16:46:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 18:21:38]

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

[commit-bot: I haz the power, 2017-05-26 18:21:39]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-05-26 18:59:10]

https://codereview.chromium.org/2903353003/diff/20001/ash/login/lock_screen_c...
File ash/login/lock_screen_controller_unittest.cc (right):

https://codereview.chromium.org/2903353003/diff/20001/ash/login/lock_screen_c...
ash/login/lock_screen_controller_unittest.cc:84:
TEST_F(LockScreenControllerTest, RequestEasyUnlock) {
This will need to rebased to use the mock

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:224:
view_screen_locker_->OnLockScreenReady();
Instead of calling directly into view_screen_locker_, we could make it a session
state observer? A TODO is good enough for now.

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/view_screen_locker.cc (right):

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.cc:19: std::string
display_type = kLockDisplay;
const

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/view_screen_locker.h (right):

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.h:16: // ViewScreenLocker
helps ScreenLocker to call into the views-based lock screen
I think this comment is a little misleading, since this class also calls into
ash.

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.h:19: class
ViewScreenLocker : public LockScreenClient::Delegate,
If I understand correctly:

  LockScreenClient::Delegate -> calls coming from ash into chrome
  ScreenLocker::Delegate -> calls from chrome into ash

Can you document this?

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/lock_screen_client.h (right):

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/lock_screen_client.h:22: class Delegate {
Add comment - what is the purpose of this delegate?

[xiaoyinh(OOO Sep 11-29), 2017-05-30 22:05:52]

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

[commit-bot: I haz the power, 2017-05-30 22:06:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiaoyinh(OOO Sep 11-29), 2017-05-30 22:23:49]

xiaoyinh@chromium.org changed reviewers:
+ xiyuan@chromium.org

[xiaoyinh(OOO Sep 11-29), 2017-05-30 22:23:50]

+xiyuan@, could you also take a look?

https://codereview.chromium.org/2903353003/diff/20001/ash/login/lock_screen_c...
File ash/login/lock_screen_controller_unittest.cc (right):

https://codereview.chromium.org/2903353003/diff/20001/ash/login/lock_screen_c...
ash/login/lock_screen_controller_unittest.cc:84:
TEST_F(LockScreenControllerTest, RequestEasyUnlock) {
On 2017/05/26 18:59:10, jdufault wrote:
> This will need to rebased to use the mock

Done.

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:224:
view_screen_locker_->OnLockScreenReady();
On 2017/05/26 18:59:10, jdufault wrote:
> Instead of calling directly into view_screen_locker_, we could make it a
session
> state observer? A TODO is good enough for now.

ViewScreenLocker::OnLockScreenReady will call into ScreenLocker::ScreenLockReady
where it sets the session state to LOCKED.
The above line is probably not needed? 

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/lock/scree...

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/view_screen_locker.cc (right):

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.cc:19: std::string
display_type = kLockDisplay;
On 2017/05/26 18:59:10, jdufault wrote:
> const

Thanks. Changed to use kLockDisplay directly.

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/view_screen_locker.h (right):

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.h:19: class
ViewScreenLocker : public LockScreenClient::Delegate,
On 2017/05/26 18:59:10, jdufault wrote:
> If I understand correctly:
> 
>   LockScreenClient::Delegate -> calls coming from ash into chrome
>   ScreenLocker::Delegate -> calls from chrome into ash
> 
> Can you document this?
Thanks! Please let me know if the updated comment looks good.

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/lock_screen_client.h (right):

https://codereview.chromium.org/2903353003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/lock_screen_client.h:22: class Delegate {
On 2017/05/26 18:59:10, jdufault wrote:
> Add comment - what is the purpose of this delegate?

Done. Let me know if this is clear to you.

[commit-bot: I haz the power, 2017-05-31 01:17:19]

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

[commit-bot: I haz the power, 2017-05-31 01:17:20]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-05-31 17:51:19]

https://codereview.chromium.org/2903353003/diff/40001/ash/login/lock_screen_c...
File ash/login/lock_screen_controller.h (right):

https://codereview.chromium.org/2903353003/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.h:36: void
LoadUsers(std::unique_ptr<base::ListValue> users,
nit: Follow the declaration order in mojom

https://codereview.chromium.org/2903353003/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.h:38: 
nit: no empty line to break the overridden interface block.

https://codereview.chromium.org/2903353003/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.h:47: // Wrappers around the
mojom::SystemTrayClient interface.
nit: This comment looks wrong.

https://codereview.chromium.org/2903353003/diff/40001/ash/public/interfaces/l...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2903353003/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:34: mojo.common.mojom.DictionaryValue
icon);
Can we create mojo struct for ScreenlockBridge::UserPodCustomIconOptions and use
it here? We are do it in a separate prep CL.

https://codereview.chromium.org/2903353003/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:45: int32 auth_type,
Create a mojo enum and use it here?

https://codereview.chromium.org/2903353003/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:51:
LoadUsers(mojo.common.mojom.ListValue users, bool show_guest);
Similar here, prefer to have a array<some_struct> for users.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:213:
view_screen_locker_.reset(new ViewScreenLocker(this));
nit: view_screen_locker_ = base::MakeUnique<ViewScreenLocker>(this);

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/view_screen_locker.cc (right):

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.cc:13: const char
kLockDisplay[] = "lock";
nit: const -> constexpr

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.cc:77:
user_selection_screen_->HandleGetUsers();
Prefer to split UserSelectionScreen::SendUserList to get an explicit method to
create a user list and pass to LockScreenClient instead of depending on
HandleGetUsers() -> SendUserList().

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/view_screen_locker.h (right):

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.h:20: class
ViewScreenLocker : public LockScreenClient::Delegate,
nit: ViewScreenLocker -> ViewsScreenLocker if the name means a views based
screen locker. "views" means the views UI library.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.h:23:
ViewScreenLocker(ScreenLocker* screen_locker);
nit: explicit

Use "explicit" keyword for single arg ctor
https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.h:52: ScreenLocker*
screen_locker_ = nullptr;
nit: ScreenLocker* const screen_locker_;

since |screen_locker_| should never change during the lifetime of this class.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/lock_screen_client.h (right):

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/lock_screen_client.h:17: public chromeos::UserBoardView {
Can we have a separate UserBoardView impl/proxy instead of piggybacking on
LockScreenClient? It is fine to have ShowUserPodCustomIcon etc calls on
LockScreenClient but it feels strange to be a UserBoardView.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/lock_screen_client.h:64: // Wrappers around the
mojom::SystemTray interface.
nit: This comment looks wrong.

[jdufault, 2017-05-31 22:54:01]

https://codereview.chromium.org/2903353003/diff/40001/ash/public/interfaces/l...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2903353003/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:51:
LoadUsers(mojo.common.mojom.ListValue users, bool show_guest);
On 2017/05/31 17:51:18, xiyuan wrote:
> Similar here, prefer to have a array<some_struct> for users.

ie, ash.mojom.UserInfo

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

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:548:
LockScreenClient::Get()->LoadUsers(users_list, show_guest_);
FYI, https://codereview.chromium.org/2811383002/. Might make the mojom interface
a bit simpler since it won't be proxying through a dict.

[xiaoyinh(OOO Sep 11-29), 2017-06-02 23:01:10]

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

[commit-bot: I haz the power, 2017-06-02 23:01:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 23:06:05]

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

[commit-bot: I haz the power, 2017-06-02 23:06:06]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[xiaoyinh(OOO Sep 11-29), 2017-06-02 23:42:29]

Sorry, the new patch set is big. I could split it into ash side and chrome side
if that's desired.
I'm not sure why all the android and mac trybots are failing?

https://codereview.chromium.org/2903353003/diff/40001/ash/login/lock_screen_c...
File ash/login/lock_screen_controller.h (right):

https://codereview.chromium.org/2903353003/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.h:36: void
LoadUsers(std::unique_ptr<base::ListValue> users,
On 2017/05/31 17:51:18, xiyuan wrote:
> nit: Follow the declaration order in mojom

Done.

https://codereview.chromium.org/2903353003/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.h:38: 
On 2017/05/31 17:51:18, xiyuan wrote:
> nit: no empty line to break the overridden interface block.

Done.

https://codereview.chromium.org/2903353003/diff/40001/ash/login/lock_screen_c...
ash/login/lock_screen_controller.h:47: // Wrappers around the
mojom::SystemTrayClient interface.
On 2017/05/31 17:51:18, xiyuan wrote:
> nit: This comment looks wrong.

Oops, changed.

https://codereview.chromium.org/2903353003/diff/40001/ash/public/interfaces/l...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2903353003/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:34: mojo.common.mojom.DictionaryValue
icon);
On 2017/05/31 17:51:18, xiyuan wrote:
> Can we create mojo struct for ScreenlockBridge::UserPodCustomIconOptions and
use
> it here? We are do it in a separate prep CL.

Done.

https://codereview.chromium.org/2903353003/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:45: int32 auth_type,
On 2017/05/31 17:51:18, xiyuan wrote:
> Create a mojo enum and use it here?

Done.

https://codereview.chromium.org/2903353003/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:51:
LoadUsers(mojo.common.mojom.ListValue users, bool show_guest);
On 2017/05/31 17:51:18, xiyuan wrote:
> Similar here, prefer to have a array<some_struct> for users.

Thanks for the suggestion. Each user in |users| has about 20+ fields of
information which I'm not sure if these are all applicable for the new
view-based UI. I've added a TODO to convert it to a mojom struct later.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:213:
view_screen_locker_.reset(new ViewScreenLocker(this));
On 2017/05/31 17:51:18, xiyuan wrote:
> nit: view_screen_locker_ = base::MakeUnique<ViewScreenLocker>(this);

Done.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/view_screen_locker.cc (right):

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.cc:13: const char
kLockDisplay[] = "lock";
On 2017/05/31 17:51:18, xiyuan wrote:
> nit: const -> constexpr

Done.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.cc:77:
user_selection_screen_->HandleGetUsers();
On 2017/05/31 17:51:18, xiyuan wrote:
> Prefer to split UserSelectionScreen::SendUserList to get an explicit method to
> create a user list and pass to LockScreenClient instead of depending on
> HandleGetUsers() -> SendUserList().

Done.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/view_screen_locker.h (right):

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.h:20: class
ViewScreenLocker : public LockScreenClient::Delegate,
On 2017/05/31 17:51:18, xiyuan wrote:
> nit: ViewScreenLocker -> ViewsScreenLocker if the name means a views based
> screen locker. "views" means the views UI library.

Done.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.h:23:
ViewScreenLocker(ScreenLocker* screen_locker);
On 2017/05/31 17:51:18, xiyuan wrote:
> nit: explicit
> 
> Use "explicit" keyword for single arg ctor
> https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

Thanks for the information. Done.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/view_screen_locker.h:52: ScreenLocker*
screen_locker_ = nullptr;
On 2017/05/31 17:51:18, xiyuan wrote:
> nit: ScreenLocker* const screen_locker_;
> 
> since |screen_locker_| should never change during the lifetime of this class.

Done.

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

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:548:
LockScreenClient::Get()->LoadUsers(users_list, show_guest_);
On 2017/05/31 22:54:00, jdufault wrote:
> FYI, https://codereview.chromium.org/2811383002/. Might make the mojom
interface
> a bit simpler since it won't be proxying through a dict.

Thanks for the information.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/lock_screen_client.h (right):

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/lock_screen_client.h:17: public chromeos::UserBoardView {
On 2017/05/31 17:51:19, xiyuan wrote:
> Can we have a separate UserBoardView impl/proxy instead of piggybacking on
> LockScreenClient? It is fine to have ShowUserPodCustomIcon etc calls on
> LockScreenClient but it feels strange to be a UserBoardView.

Thanks! I added UserSelectionScreenProxy as a separate UserBoardView impl.
Please let me know if this makes sense.

https://codereview.chromium.org/2903353003/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/lock_screen_client.h:64: // Wrappers around the
mojom::SystemTray interface.
On 2017/05/31 17:51:19, xiyuan wrote:
> nit: This comment looks wrong.

Thanks! Changed.

[jdufault, 2017-06-03 01:00:45]

lgtm

https://codereview.chromium.org/2903353003/diff/60001/ash/public/interfaces/l...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2903353003/diff/60001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:71: // TODO: create a mojo struct for
|users|, this contains user information
Please file a bug and reference it here.

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

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:631:
std::unique_ptr<base::ListValue> users_list(new base::ListValue());
nit: prefer MakeUnique

[xiyuan, 2017-06-05 17:30:40]

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/views_screen_locker.cc (right):

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/views_screen_locker.cc:80:
std::unique_ptr<base::ListValue> users_list =
nit: get rid of |users_list| and use it in-place in the call below.

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/views_screen_locker.cc:82:
LockScreenClient::Get()->LoadUsers(std::move(users_list),
nit: #include <utility> for std::move

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

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:487:
handler_->LoadUsers(users_to_send_, *users_list);
Can we clean up LoadUsers API to only take the ListValue? We can do it in a prep
CL before this CL. It seems useless since the ListValue is a superset of it.

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_selection_screen_proxy.cc (right):

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_selection_screen_proxy.cc:23:
LockScreenClient::Get()->ShowUserPodCustomIcon(account_id, std::move(icon));
nit: #include <utility>

https://codereview.chromium.org/2903353003/diff/60001/components/proximity_au...
File components/proximity_auth/BUILD.gn (right):

https://codereview.chromium.org/2903353003/diff/60001/components/proximity_au...
components/proximity_auth/BUILD.gn:55: "//ash/public/cpp:ash_public_cpp",
Not sure whether proximity_auth owner would be happen about this. We can put
ToUserPodCustomIconOptionsPtr in UserSelectionScreenProxy where it is needed to
avoid this deps for now.

The build failures might be caused by this since proximity_auth is available on
win/mac etc platforms but ash is chromeos only.

[xiaoyinh(OOO Sep 11-29), 2017-06-05 17:48:31]

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

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:487:
handler_->LoadUsers(users_to_send_, *users_list);
On 2017/06/05 17:30:40, xiyuan wrote:
> Can we clean up LoadUsers API to only take the ListValue? We can do it in a
prep
> CL before this CL. It seems useless since the ListValue is a superset of it.

I think this parameter (user_manager::UserList& users) was recently added for
pin for signin feature?
+jdufault@ for more context.

[jdufault, 2017-06-05 19:31:14]

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

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:487:
handler_->LoadUsers(users_to_send_, *users_list);
On 2017/06/05 17:48:31, xiaoyinh wrote:
> On 2017/06/05 17:30:40, xiyuan wrote:
> > Can we clean up LoadUsers API to only take the ListValue? We can do it in a
> prep
> > CL before this CL. It seems useless since the ListValue is a superset of it.
> 
> I think this parameter (user_manager::UserList& users) was recently added for
> pin for signin feature?
> +jdufault@ for more context.

Yes, we need it for pin sign-in.

[xiyuan, 2017-06-05 20:04:28]

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

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:487:
handler_->LoadUsers(users_to_send_, *users_list);
On 2017/06/05 19:31:14, jdufault wrote:
> On 2017/06/05 17:48:31, xiaoyinh wrote:
> > On 2017/06/05 17:30:40, xiyuan wrote:
> > > Can we clean up LoadUsers API to only take the ListValue? We can do it in
a
> > prep
> > > CL before this CL. It seems useless since the ListValue is a superset of
it.
> > 
> > I think this parameter (user_manager::UserList& users) was recently added
for
> > pin for signin feature?
> > +jdufault@ for more context.
> 
> Yes, we need it for pin sign-in.

Okay. Ignore this comment then.

[xiaoyinh(OOO Sep 11-29), 2017-06-05 20:29:43]

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

[commit-bot: I haz the power, 2017-06-05 20:30:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 21:52:23]

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

[commit-bot: I haz the power, 2017-06-05 21:52:24]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[xiaoyinh(OOO Sep 11-29), 2017-06-06 16:36:05]

https://codereview.chromium.org/2903353003/diff/60001/ash/public/interfaces/l...
File ash/public/interfaces/lock_screen.mojom (right):

https://codereview.chromium.org/2903353003/diff/60001/ash/public/interfaces/l...
ash/public/interfaces/lock_screen.mojom:71: // TODO: create a mojo struct for
|users|, this contains user information
On 2017/06/03 01:00:45, jdufault wrote:
> Please file a bug and reference it here.

Done.

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/views_screen_locker.cc (right):

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/views_screen_locker.cc:80:
std::unique_ptr<base::ListValue> users_list =
On 2017/06/05 17:30:40, xiyuan wrote:
> nit: get rid of |users_list| and use it in-place in the call below.

Done.

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/views_screen_locker.cc:82:
LockScreenClient::Get()->LoadUsers(std::move(users_list),
On 2017/06/05 17:30:40, xiyuan wrote:
> nit: #include <utility> for std::move

Thanks for your comment! This is no longer needed after I incorporate the above
comment.

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

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:631:
std::unique_ptr<base::ListValue> users_list(new base::ListValue());
On 2017/06/03 01:00:45, jdufault wrote:
> nit: prefer MakeUnique

Done.

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_selection_screen_proxy.cc (right):

https://codereview.chromium.org/2903353003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_selection_screen_proxy.cc:23:
LockScreenClient::Get()->ShowUserPodCustomIcon(account_id, std::move(icon));
On 2017/06/05 17:30:40, xiyuan wrote:
> nit: #include <utility>

Done.

[xiyuan, 2017-06-06 19:27:53]

lgtm

https://codereview.chromium.org/2903353003/diff/80001/components/proximity_au...
File components/proximity_auth/screenlock_bridge.h (right):

https://codereview.chromium.org/2903353003/diff/80001/components/proximity_au...
components/proximity_auth/screenlock_bridge.h:75: base::string16 tooltip() const
{ return tooltip_; }
nit: const base::string16& to avoid copying

same goes for aria_label().

[xiaoyinh(OOO Sep 11-29), 2017-06-06 20:23:22]

xiaoyinh@chromium.org changed reviewers:
+ rsesek@chromium.org, stevenjb@chromium.org

[xiaoyinh(OOO Sep 11-29), 2017-06-06 20:23:23]

+rsesek@ for ash/public/interfaces/lock_screen.mojom
+stevenjb@ for chrome/browser/ui/ash/

Could you help to take a look? Thanks!

[stevenjb, 2017-06-06 21:36:53]

c/b/ui/ash lgtm

[Robert Sesek, 2017-06-07 17:09:05]

mojom lgtm

[xiaoyinh(OOO Sep 11-29), 2017-06-07 17:34:54]

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

[commit-bot: I haz the power, 2017-06-07 17:35:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiaoyinh(OOO Sep 11-29), 2017-06-07 17:35:25]

https://codereview.chromium.org/2903353003/diff/80001/components/proximity_au...
File components/proximity_auth/screenlock_bridge.h (right):

https://codereview.chromium.org/2903353003/diff/80001/components/proximity_au...
components/proximity_auth/screenlock_bridge.h:75: base::string16 tooltip() const
{ return tooltip_; }
On 2017/06/06 19:27:53, xiyuan wrote:
> nit: const base::string16& to avoid copying
> 
> same goes for aria_label().

Done.

[commit-bot: I haz the power, 2017-06-07 19:15:07]

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

[commit-bot: I haz the power, 2017-06-07 19:15:08]

Dry run: This issue passed the CQ dry run.

[xiaoyinh(OOO Sep 11-29), 2017-06-07 19:15:45]

The CQ bit was checked by xiaoyinh@chromium.org

[xiaoyinh(OOO Sep 11-29), 2017-06-07 19:15:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdufault@chromium.org, rsesek@chromium.org, xiyuan@chromium.org,
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2903353003/#ps100001
(title: "comments and rebase")

[commit-bot: I haz the power, 2017-06-07 19:16:26]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 19:22:36]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496862945001670,
"parent_rev": "f33b08933cff6791612633f2e4a191917bb9d567", "commit_rev":
"9f6fa0ed40b236032707e7af67009ce53817e627"}

[commit-bot: I haz the power, 2017-06-07 19:22:48]

Description was changed from

==========
Adding mojo calls for easy unlock service

BUG=721524
==========

to

==========
Adding mojo calls for easy unlock service

BUG=721524

Review-Url: https://codereview.chromium.org/2903353003
Cr-Commit-Position: refs/heads/master@{#477722}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f6fa0ed40b236032707e7af6700...
==========

[commit-bot: I haz the power, 2017-06-07 19:22:50]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9f6fa0ed40b236032707e7af6700...