Create Mojo Struct for user information used in login/lock screen.

Create Mojo Struct for user information used in login/lock screen.

Introduce a Mojo struct LoginUserInfo for login users which include
basic information ash::mojom::UserInfo and other login/lock
screen specific information.
UserSelectionScreen will populate LoginUserInfo and send it back to lockscreen views.
Compare to the user info we sent to current UI, LoginUserInfo have almost everything except:
1. user's gaiaId, which doesn't seem to be used by current UI.(user_pod_row.js)
2. public session information which doesn't exist on lockscreen.
We might need it for login screen later and I have added a TODO for this.



BUG=729687
Review-Url: https://codereview.chromium.org/2937553002
Cr-Commit-Position: refs/heads/master@{#481070}
Committed: https://chromium.googlesource.com/chromium/src/+/820778c575fe891e178b0671a92817a2e54ba197

[xiaoyinh(OOO Sep 11-29), 2017-06-12 17:42:26]

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

[commit-bot: I haz the power, 2017-06-12 17:42:54]

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-12 19:30:35]

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

[commit-bot: I haz the power, 2017-06-12 19:30:36]

Dry run: This issue passed the CQ dry run.

[xiaoyinh(OOO Sep 11-29), 2017-06-12 22:45:03]

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

[commit-bot: I haz the power, 2017-06-12 22:45:38]

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-13 00:07:38]

Description was changed from

==========
Create Mojo Struct for user information used in login/lock screen.

BUG=729687
==========

to

==========
Create Mojo Struct for user information used in login/lock screen.

Introduce a Mojo struct LoginUserInfo for login users which include basic
information from ash::mojom::UserInfo and other login/lock screen specific
information.
UserSelectionScreen will populate LoginUserInfo and send it back to lockscreen
views.
Compare to the user info we sent to current UI, LoginUserInfo have almost
everything except:
1. user's gaiaId, which doesn't seems to be used by current UI.(user_pod_row.js)
2. public session information which doesn't exist on lockscreen. We might need
it for login screen later and I have added a TODO for this.



BUG=729687
==========

[xiaoyinh(OOO Sep 11-29), 2017-06-13 00:08:28]

Description was changed from

==========
Create Mojo Struct for user information used in login/lock screen.

Introduce a Mojo struct LoginUserInfo for login users which include basic
information from ash::mojom::UserInfo and other login/lock screen specific
information.
UserSelectionScreen will populate LoginUserInfo and send it back to lockscreen
views.
Compare to the user info we sent to current UI, LoginUserInfo have almost
everything except:
1. user's gaiaId, which doesn't seems to be used by current UI.(user_pod_row.js)
2. public session information which doesn't exist on lockscreen. We might need
it for login screen later and I have added a TODO for this.



BUG=729687
==========

to

==========
Create Mojo Struct for user information used in login/lock screen.

Introduce a Mojo struct LoginUserInfo for login users which include
basic information from ash::mojom::UserInfo and other login/lock
screen specific information.
UserSelectionScreen will populate LoginUserInfo and send it back to lockscreen
views.
Compare to the user info we sent to current UI, LoginUserInfo have almost
everything except:
1. user's gaiaId, which doesn't seems to be used by current UI.(user_pod_row.js)
2. public session information which doesn't exist on lockscreen.
We might need it for login screen later and I have added a TODO for this.



BUG=729687
==========

[commit-bot: I haz the power, 2017-06-13 00:08:56]

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

[commit-bot: I haz the power, 2017-06-13 00:08:57]

Dry run: This issue passed the CQ dry run.

[xiaoyinh(OOO Sep 11-29), 2017-06-13 00:09:56]

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

[xiaoyinh(OOO Sep 11-29), 2017-06-13 00:09:57]

Hi, could you help to review it? Thanks!

[xiaoyinh(OOO Sep 11-29), 2017-06-13 00:10:36]

Description was changed from

==========
Create Mojo Struct for user information used in login/lock screen.

Introduce a Mojo struct LoginUserInfo for login users which include
basic information from ash::mojom::UserInfo and other login/lock
screen specific information.
UserSelectionScreen will populate LoginUserInfo and send it back to lockscreen
views.
Compare to the user info we sent to current UI, LoginUserInfo have almost
everything except:
1. user's gaiaId, which doesn't seems to be used by current UI.(user_pod_row.js)
2. public session information which doesn't exist on lockscreen.
We might need it for login screen later and I have added a TODO for this.



BUG=729687
==========

to

==========
Create Mojo Struct for user information used in login/lock screen.

Introduce a Mojo struct LoginUserInfo for login users which include
basic information ash::mojom::UserInfo and other login/lock
screen specific information.
UserSelectionScreen will populate LoginUserInfo and send it back to lockscreen
views.
Compare to the user info we sent to current UI, LoginUserInfo have almost
everything except:
1. user's gaiaId, which doesn't seems to be used by current UI.(user_pod_row.js)
2. public session information which doesn't exist on lockscreen.
We might need it for login screen later and I have added a TODO for this.



BUG=729687
==========

[xiaoyinh(OOO Sep 11-29), 2017-06-13 00:11:13]

Description was changed from

==========
Create Mojo Struct for user information used in login/lock screen.

Introduce a Mojo struct LoginUserInfo for login users which include
basic information ash::mojom::UserInfo and other login/lock
screen specific information.
UserSelectionScreen will populate LoginUserInfo and send it back to lockscreen
views.
Compare to the user info we sent to current UI, LoginUserInfo have almost
everything except:
1. user's gaiaId, which doesn't seems to be used by current UI.(user_pod_row.js)
2. public session information which doesn't exist on lockscreen.
We might need it for login screen later and I have added a TODO for this.



BUG=729687
==========

to

==========
Create Mojo Struct for user information used in login/lock screen.

Introduce a Mojo struct LoginUserInfo for login users which include
basic information ash::mojom::UserInfo and other login/lock
screen specific information.
UserSelectionScreen will populate LoginUserInfo and send it back to lockscreen
views.
Compare to the user info we sent to current UI, LoginUserInfo have almost
everything except:
1. user's gaiaId, which doesn't seem to be used by current UI.(user_pod_row.js)
2. public session information which doesn't exist on lockscreen.
We might need it for login screen later and I have added a TODO for this.



BUG=729687
==========

[jdufault, 2017-06-13 17:41:54]

https://codereview.chromium.org/2937553002/diff/20001/ash/public/interfaces/l...
File ash/public/interfaces/login_user_info.mojom (right):

https://codereview.chromium.org/2937553002/diff/20001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:34: bool is_desktop_user;
It'd be good to document all of these members, but especially is_desktop_user,
auth_type, is_multiprofile_allowed, and multiprofile_policy, since those are not
immediately obvious.

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

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/views_screen_locker.cc:81:
user_selection_screen_->PrepareUserListToMojo(), false /* show_guests */);
nit: ToMojo -> ForMojo

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

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:458:
FillMultiProfileUserPrefsInMojo(user, user_info, is_signin_to_add);
I think the existing code is structured like this (ie, FillFoo, FillBar, ...),
but FillMultiProfilePrefsInMojo is unconditionally called once, which makes it a
great candidate for removal :)

Please inline FillMultiProfileUserPrefsInMojo. Feel free to put a comment above
the inlined code, ie,

  // Fill multi-profile data.

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:771:
user_auth_type_map_.clear();
:(, it is a bit unexpected that this method changes internal state.

Please either add a function level comment or rename the method; I'm not sure of
a great name, maybe UpdateAndReturnUserListForMojo? Up to you.

[xiyuan, 2017-06-13 18:00:18]

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

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:182: bool
CanRemoveUser(bool is_single_user, user_manager::User* user) {
nit: const user_manager::User*

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:199: void
GetMultiProfilePolicy(user_manager::User* user,
nit: const user_manager::User*

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:205:
MultiProfileUserController::UserAllowedInSessionReason isUserAllowedReason;
nit: isUserAllowedReason -> is_user_allowed_reason

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:766: bool
single_user = users_.size() == 1;
nit: Declare it as "const" since the rest of the function would not change it.
Same goes for |owner| and |is_signin_to_add|.

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_selection_screen.h (right):

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.h:123: static void
FillUserMojoStruct(user_manager::User* user,
nit: user_manager::User* -> const user_manager::User*

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.h:127:
ash::mojom::LoginUserInfo& user_info);
nit: For function to initialize a struct, the prevailing style is either a
create function that returns the struct or an update function that takes
struct*.

That is,
  ash::mojom::LoginUserInfoPtr CreateUserMojoStruct(...);
or
  void FillUserMojoStruct(..., ash::mojom::LoginUserInfo* user_info);

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.h:130:
user_manager::User* user,
nit: const user_manager::User*

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.h:131:
ash::mojom::LoginUserInfo& user_info,
nit: ash::mojom::LoginUserInfo& -> ash::mojom::LoginUserInfo*

And make it the last in args list as input args should precede output ones.

See
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

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

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

[commit-bot: I haz the power, 2017-06-13 22:19:45]

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-13 22:21:32]

https://codereview.chromium.org/2937553002/diff/20001/ash/public/interfaces/l...
File ash/public/interfaces/login_user_info.mojom (right):

https://codereview.chromium.org/2937553002/diff/20001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:34: bool is_desktop_user;
On 2017/06/13 17:41:54, jdufault wrote:
> It'd be good to document all of these members, but especially is_desktop_user,
> auth_type, is_multiprofile_allowed, and multiprofile_policy, since those are
not
> immediately obvious.

Done.

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

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/views_screen_locker.cc:81:
user_selection_screen_->PrepareUserListToMojo(), false /* show_guests */);
On 2017/06/13 17:41:54, jdufault wrote:
> nit: ToMojo -> ForMojo

Updated.

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

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:182: bool
CanRemoveUser(bool is_single_user, user_manager::User* user) {
On 2017/06/13 18:00:18, xiyuan wrote:
> nit: const user_manager::User*

Done.

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:199: void
GetMultiProfilePolicy(user_manager::User* user,
On 2017/06/13 18:00:18, xiyuan wrote:
> nit: const user_manager::User*

Done.

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:205:
MultiProfileUserController::UserAllowedInSessionReason isUserAllowedReason;
On 2017/06/13 18:00:17, xiyuan wrote:
> nit: isUserAllowedReason -> is_user_allowed_reason

Done.

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:458:
FillMultiProfileUserPrefsInMojo(user, user_info, is_signin_to_add);
On 2017/06/13 17:41:54, jdufault wrote:
> I think the existing code is structured like this (ie, FillFoo, FillBar, ...),
> but FillMultiProfilePrefsInMojo is unconditionally called once, which makes it
a
> great candidate for removal :)
> 
> Please inline FillMultiProfileUserPrefsInMojo. Feel free to put a comment
above
> the inlined code, ie,
> 
>   // Fill multi-profile data.

Done.

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:766: bool
single_user = users_.size() == 1;
On 2017/06/13 18:00:18, xiyuan wrote:
> nit: Declare it as "const" since the rest of the function would not change it.
> Same goes for |owner| and |is_signin_to_add|.

Done.

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:771:
user_auth_type_map_.clear();
On 2017/06/13 17:41:54, jdufault wrote:
> :(, it is a bit unexpected that this method changes internal state.
> 
> Please either add a function level comment or rename the method; I'm not sure
of
> a great name, maybe UpdateAndReturnUserListForMojo? Up to you.

Thanks! Both this and PrepareUserList will update internal states. I've renamed
both.

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_selection_screen.h (right):

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.h:123: static void
FillUserMojoStruct(user_manager::User* user,
On 2017/06/13 18:00:18, xiyuan wrote:
> nit: user_manager::User* -> const user_manager::User*

Done.

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.h:127:
ash::mojom::LoginUserInfo& user_info);
On 2017/06/13 18:00:18, xiyuan wrote:
> nit: For function to initialize a struct, the prevailing style is either a
> create function that returns the struct or an update function that takes
> struct*.
> 
> That is,
>   ash::mojom::LoginUserInfoPtr CreateUserMojoStruct(...);
> or
>   void FillUserMojoStruct(..., ash::mojom::LoginUserInfo* user_info);

Done.

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.h:130:
user_manager::User* user,
On 2017/06/13 18:00:18, xiyuan wrote:
> nit: const user_manager::User*

This method is inlined in FillUserMojoStruct.

https://codereview.chromium.org/2937553002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.h:131:
ash::mojom::LoginUserInfo& user_info,
On 2017/06/13 18:00:18, xiyuan wrote:
> nit: ash::mojom::LoginUserInfo& -> ash::mojom::LoginUserInfo*
> 
> And make it the last in args list as input args should precede output ones.
> 
> See
> https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

Thanks for the information! 
This method is inlined in FillUserMojoStruct.

[commit-bot: I haz the power, 2017-06-13 23:59:41]

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

[commit-bot: I haz the power, 2017-06-13 23:59:42]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-06-14 02:20:42]

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

https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:36: // True if this user is a
desktop user.
What is a desktop user? My best guess is that we're running in a
Windows/macOS/Linux build? In which case I think we should eliminate the
variable.

https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:39: // Supported authentication type
for this user.
What about

  // What method the user can use to sign in.

https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:46: bool is_owner;
is_owner => is_device_owner

https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:55: string multiprofile_policy;
Do we know what values this can be? Can it be an enum instead of a string?

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

https://codereview.chromium.org/2937553002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:209: case
MultiProfileUserController::NOT_ALLOWED_OWNER_AS_SECONDARY:
An if/else is probably simpler.

https://codereview.chromium.org/2937553002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:213: *out_policy
= multi_profile_user_controller->GetCachedValue(user_id);
nit: break

https://codereview.chromium.org/2937553002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:465:
user_info->is_multiprofile_allowed = is_user_allowed;
Instead of using temporaries just write directly to the result.

  GetMultiProfilePolicy(user, &user_info->is_multiprofile_allowed,
&user_info->multiprofile_policy);

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

https://codereview.chromium.org/2937553002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.h:129:
std::unique_ptr<base::ListValue> UpdateAndReturnUserList();
Nit: since you already renamed this what about UpdateAndReturnUserListForWebUI?

[xiaoyinh(OOO Sep 11-29), 2017-06-15 18:38:53]

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

[commit-bot: I haz the power, 2017-06-15 18:39:31]

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-15 18:39:38]

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

https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:36: // True if this user is a
desktop user.
On 2017/06/14 02:20:42, jdufault wrote:
> What is a desktop user? My best guess is that we're running in a
> Windows/macOS/Linux build? In which case I think we should eliminate the
> variable.

Removed, thanks!

https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:39: // Supported authentication type
for this user.
On 2017/06/14 02:20:42, jdufault wrote:
> What about
> 
>   // What method the user can use to sign in.

Changed. Thanks!

https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:46: bool is_owner;
On 2017/06/14 02:20:42, jdufault wrote:
> is_owner => is_device_owner

Done.

https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:55: string multiprofile_policy;
On 2017/06/14 02:20:42, jdufault wrote:
> Do we know what values this can be? Can it be an enum instead of a string?

The values seem to be harded coded here:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/mult...
Change it to an enum should be easy, I can do it in another CL if this is
desired. What do you think?

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

https://codereview.chromium.org/2937553002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:209: case
MultiProfileUserController::NOT_ALLOWED_OWNER_AS_SECONDARY:
On 2017/06/14 02:20:42, jdufault wrote:
> An if/else is probably simpler.

Done.

https://codereview.chromium.org/2937553002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:213: *out_policy
= multi_profile_user_controller->GetCachedValue(user_id);
On 2017/06/14 02:20:42, jdufault wrote:
> nit: break

Changed to if/else.

https://codereview.chromium.org/2937553002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:465:
user_info->is_multiprofile_allowed = is_user_allowed;
On 2017/06/14 02:20:42, jdufault wrote:
> Instead of using temporaries just write directly to the result.
> 
>   GetMultiProfilePolicy(user, &user_info->is_multiprofile_allowed,
> &user_info->multiprofile_policy);

Done.

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

https://codereview.chromium.org/2937553002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.h:129:
std::unique_ptr<base::ListValue> UpdateAndReturnUserList();
On 2017/06/14 02:20:42, jdufault wrote:
> Nit: since you already renamed this what about
UpdateAndReturnUserListForWebUI?

Done.

[jdufault, 2017-06-15 19:00:02]

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

https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:55: string multiprofile_policy;
On 2017/06/15 18:39:37, xiaoyinh wrote:
> On 2017/06/14 02:20:42, jdufault wrote:
> > Do we know what values this can be? Can it be an enum instead of a string?
> 
> The values seem to be harded coded here:
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/mult...
> Change it to an enum should be easy, I can do it in another CL if this is
> desired. What do you think?

Yes, an enum is much better. Can you refactor that code and land it as a CL
before landing this one? Then the first check-in is using the right approach.

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

On 2017/06/15 19:00:02, jdufault wrote:
>
https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
> File ash/public/interfaces/login_user_info.mojom (right):
> 
>
https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
> ash/public/interfaces/login_user_info.mojom:55: string multiprofile_policy;
> On 2017/06/15 18:39:37, xiaoyinh wrote:
> > On 2017/06/14 02:20:42, jdufault wrote:
> > > Do we know what values this can be? Can it be an enum instead of a string?
> > 
> > The values seem to be harded coded here:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/mult...
> > Change it to an enum should be easy, I can do it in another CL if this is
> > desired. What do you think?
> 
> Yes, an enum is much better. Can you refactor that code and land it as a CL
> before landing this one? Then the first check-in is using the right approach.

Will do. Thanks!

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

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

[commit-bot: I haz the power, 2017-06-15 19:49: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-16 21:01:31]

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

[commit-bot: I haz the power, 2017-06-16 21:01:53]

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-16 21:03:38]

On 2017/06/15 19:07:30, xiaoyinh wrote:
> On 2017/06/15 19:00:02, jdufault wrote:
> >
>
https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
> > File ash/public/interfaces/login_user_info.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2937553002/diff/40001/ash/public/interfaces/l...
> > ash/public/interfaces/login_user_info.mojom:55: string multiprofile_policy;
> > On 2017/06/15 18:39:37, xiaoyinh wrote:
> > > On 2017/06/14 02:20:42, jdufault wrote:
> > > > Do we know what values this can be? Can it be an enum instead of a
string?
> > > 
> > > The values seem to be harded coded here:
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/mult...
> > > Change it to an enum should be easy, I can do it in another CL if this is
> > > desired. What do you think?
> > 
> > Yes, an enum is much better. Can you refactor that code and land it as a CL
> > before landing this one? Then the first check-in is using the right
approach.
> 
> Will do. Thanks!

Done.

[jdufault, 2017-06-16 21:07:23]

https://codereview.chromium.org/2937553002/diff/80001/ash/public/interfaces/l...
File ash/public/interfaces/login_user_info.mojom (right):

https://codereview.chromium.org/2937553002/diff/80001/ash/public/interfaces/l...
ash/public/interfaces/login_user_info.mojom:22: // Keep in sync with the enum in
multi_profile_user_controller.h
Can we replace the enum in multi_profile_user_controller.h with this one?

[xiaoyinh(OOO Sep 11-29), 2017-06-16 21:57:25]

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

[commit-bot: I haz the power, 2017-06-16 21:57:44]

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-16 21:58:24]

On 2017/06/16 21:07:23, jdufault wrote:
>
https://codereview.chromium.org/2937553002/diff/80001/ash/public/interfaces/l...
> File ash/public/interfaces/login_user_info.mojom (right):
> 
>
https://codereview.chromium.org/2937553002/diff/80001/ash/public/interfaces/l...
> ash/public/interfaces/login_user_info.mojom:22: // Keep in sync with the enum
in
> multi_profile_user_controller.h
> Can we replace the enum in multi_profile_user_controller.h with this one?

Updated, thanks!

[jdufault, 2017-06-16 22:18:09]

lgtm

https://codereview.chromium.org/2937553002/diff/100001/ash/public/interfaces/...
File ash/public/interfaces/login_user_info.mojom (right):

https://codereview.chromium.org/2937553002/diff/100001/ash/public/interfaces/...
ash/public/interfaces/login_user_info.mojom:24: UNRESTRICTED,
Does mojo allow us to explicitly give values here, ie, = 0, = 1?

https://codereview.chromium.org/2937553002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/multi_profile_user_controller.h
(right):

https://codereview.chromium.org/2937553002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/multi_profile_user_controller.h:24: using
MultiProfileUserBehavior = ash::mojom::MultiProfileUserBehavior;
It doesn't look like this saves a lot of typing so let's just remove it.

[xiyuan, 2017-06-16 23:05:41]

lgtm

https://codereview.chromium.org/2937553002/diff/100001/ash/public/interfaces/...
File ash/public/interfaces/login_user_info.mojom (right):

https://codereview.chromium.org/2937553002/diff/100001/ash/public/interfaces/...
ash/public/interfaces/login_user_info.mojom:24: UNRESTRICTED,
On 2017/06/16 22:18:09, jdufault wrote:
> Does mojo allow us to explicitly give values here, ie, = 0, = 1?

Yes.

See
https://chromium.googlesource.com/chromium/src/+/master/mojo/public/tools/bin...

https://codereview.chromium.org/2937553002/diff/100001/ash/public/interfaces/...
ash/public/interfaces/login_user_info.mojom:27: OWNER_PRIMARY_ONLY,
nit: wrong indent

https://codereview.chromium.org/2937553002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/multi_profile_user_controller.h
(right):

https://codereview.chromium.org/2937553002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/multi_profile_user_controller.h:24: using
MultiProfileUserBehavior = ash::mojom::MultiProfileUserBehavior;
On 2017/06/16 22:18:09, jdufault wrote:
> It doesn't look like this saves a lot of typing so let's just remove it.

+1

[commit-bot: I haz the power, 2017-06-16 23:37:18]

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

[commit-bot: I haz the power, 2017-06-16 23:37:20]

Dry run: This issue passed the CQ dry run.

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

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

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

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-19 17:29:49]

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

[xiaoyinh(OOO Sep 11-29), 2017-06-19 17:29:50]

+jamescook@ for ash/public/ and chrome/browser/ui/ash
+rsesek@ for lock_screen.mojom and login_user_info.mojom
Please take a look. Thanks!

https://codereview.chromium.org/2937553002/diff/100001/ash/public/interfaces/...
File ash/public/interfaces/login_user_info.mojom (right):

https://codereview.chromium.org/2937553002/diff/100001/ash/public/interfaces/...
ash/public/interfaces/login_user_info.mojom:24: UNRESTRICTED,
On 2017/06/16 23:05:41, xiyuan wrote:
> On 2017/06/16 22:18:09, jdufault wrote:
> > Does mojo allow us to explicitly give values here, ie, = 0, = 1?
> 
> Yes.
> 
> See
>
https://chromium.googlesource.com/chromium/src/+/master/mojo/public/tools/bin...

Done.

https://codereview.chromium.org/2937553002/diff/100001/ash/public/interfaces/...
ash/public/interfaces/login_user_info.mojom:27: OWNER_PRIMARY_ONLY,
On 2017/06/16 23:05:41, xiyuan wrote:
> nit: wrong indent

Done.

https://codereview.chromium.org/2937553002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/multi_profile_user_controller.h
(right):

https://codereview.chromium.org/2937553002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/multi_profile_user_controller.h:24: using
MultiProfileUserBehavior = ash::mojom::MultiProfileUserBehavior;
On 2017/06/16 23:05:41, xiyuan wrote:
> On 2017/06/16 22:18:09, jdufault wrote:
> > It doesn't look like this saves a lot of typing so let's just remove it.
> 
> +1

Done.

[James Cook, 2017-06-19 18:04:52]

https://codereview.chromium.org/2937553002/diff/120001/ash/public/interfaces/...
File ash/public/interfaces/login_user_info.mojom (right):

https://codereview.chromium.org/2937553002/diff/120001/ash/public/interfaces/...
ash/public/interfaces/login_user_info.mojom:12: enum AuthType {
We should share this enum between ash and components/proximity_auth. Right now
there is a hidden dependency between ash and proximity_auth that is not
expressed in the code.

Please introduce a new mojom file in components/proximity_auth/public/interfaces
and use it both from ash and to replace the enum in screenlocker_bridge.h

optional, would be a nice follow-up CL: Figure out how to use mojom JS bindings.
Use the enum in JS code and eliminate the copy in user_pod_row.js.

https://codereview.chromium.org/2937553002/diff/120001/ash/public/interfaces/...
ash/public/interfaces/login_user_info.mojom:18: FORCE_OFFLINE_PASSWORD,
These all need to be documented.

[commit-bot: I haz the power, 2017-06-19 19:05:42]

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

[commit-bot: I haz the power, 2017-06-19 19:05:43]

Dry run: This issue passed the CQ dry run.

[xiaoyinh(OOO Sep 11-29), 2017-06-20 16:55:52]

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

[commit-bot: I haz the power, 2017-06-20 16:56:02]

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-20 17:02:08]

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

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

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-20 17:09:23]

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

[commit-bot: I haz the power, 2017-06-20 17:09:24]

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

[xiaoyinh(OOO Sep 11-29), 2017-06-20 17:14:58]

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

[commit-bot: I haz the power, 2017-06-20 17:15:09]

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-20 17:21:54]

https://codereview.chromium.org/2937553002/diff/120001/ash/public/interfaces/...
File ash/public/interfaces/login_user_info.mojom (right):

https://codereview.chromium.org/2937553002/diff/120001/ash/public/interfaces/...
ash/public/interfaces/login_user_info.mojom:12: enum AuthType {
On 2017/06/19 18:04:52, James Cook wrote:
> We should share this enum between ash and components/proximity_auth. Right now
> there is a hidden dependency between ash and proximity_auth that is not
> expressed in the code.
> 
> Please introduce a new mojom file in
components/proximity_auth/public/interfaces
> and use it both from ash and to replace the enum in screenlocker_bridge.h
> 
> optional, would be a nice follow-up CL: Figure out how to use mojom JS
bindings.
> Use the enum in JS code and eliminate the copy in user_pod_row.js.

I've moved this enum to auth_type.mojom. Could you take a look and let me know?
Since this enum is widely used in the easy unlock code, multiple files need to
be changed in the new patchset.

+xiyuan@, could you also take a look for the enum change?

https://codereview.chromium.org/2937553002/diff/120001/ash/public/interfaces/...
ash/public/interfaces/login_user_info.mojom:18: FORCE_OFFLINE_PASSWORD,
On 2017/06/19 18:04:52, James Cook wrote:
> These all need to be documented.

Done.

[commit-bot: I haz the power, 2017-06-20 17:29:13]

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

[commit-bot: I haz the power, 2017-06-20 17:29:14]

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

[James Cook, 2017-06-20 17:37:46]

ash and chrome/browser/ui/ash LGTM, assuming the bot failures are not anything
serious

https://codereview.chromium.org/2937553002/diff/180001/components/proximity_a...
File components/proximity_auth/public/interfaces/auth_type.mojom (right):

https://codereview.chromium.org/2937553002/diff/180001/components/proximity_a...
components/proximity_auth/public/interfaces/auth_type.mojom:28: };
Nice docs.

[xiyuan, 2017-06-20 17:52:59]

enum auth_type mojo and proximity_auth lgtm

Please fix bots compile.

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

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

[commit-bot: I haz the power, 2017-06-20 19:05:56]

Dry run: CQ is trying da patch.

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

[Robert Sesek, 2017-06-20 20:05:06]

mojom lgtm

[commit-bot: I haz the power, 2017-06-20 21:09:34]

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

[commit-bot: I haz the power, 2017-06-20 21:09:35]

Dry run: This issue passed the CQ dry run.

[xiaoyinh(OOO Sep 11-29), 2017-06-20 21:13:10]

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

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

Trybots are all green now:)

+reillyg for chrome/browser/extensions/api/screenlock_private/*

reillyg@, could you help to take a look? The change is to move AuthType enum to
auth_type.mojom file and screenlock private api is one of the places that use
this enum.

[Reilly Grant (use Gerrit), 2017-06-20 23:00:58]

chrome/browser/extensions lgtm

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

The CQ bit was checked by xiaoyinh@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
jdufault@chromium.org, xiyuan@chromium.org, jamescook@chromium.org,
rsesek@chromium.org, reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2937553002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-20 23:10:46]

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-21 01:42:54]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1498000210260010,
"parent_rev": "ee841c26ab77ca0aebd65ad839167b2178b7ecde", "commit_rev":
"99339af745731786e79e5ff5e6c8ec606e9509b5"}

[commit-bot: I haz the power, 2017-06-21 01:43:09]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1498000210260010,
"parent_rev": "23f56b7b5e6ce93edce36088fd1beb8d373abd7d", "commit_rev":
"820778c575fe891e178b0671a92817a2e54ba197"}

[commit-bot: I haz the power, 2017-06-21 01:43:24]

Description was changed from

==========
Create Mojo Struct for user information used in login/lock screen.

Introduce a Mojo struct LoginUserInfo for login users which include
basic information ash::mojom::UserInfo and other login/lock
screen specific information.
UserSelectionScreen will populate LoginUserInfo and send it back to lockscreen
views.
Compare to the user info we sent to current UI, LoginUserInfo have almost
everything except:
1. user's gaiaId, which doesn't seem to be used by current UI.(user_pod_row.js)
2. public session information which doesn't exist on lockscreen.
We might need it for login screen later and I have added a TODO for this.



BUG=729687
==========

to

==========
Create Mojo Struct for user information used in login/lock screen.

Introduce a Mojo struct LoginUserInfo for login users which include
basic information ash::mojom::UserInfo and other login/lock
screen specific information.
UserSelectionScreen will populate LoginUserInfo and send it back to lockscreen
views.
Compare to the user info we sent to current UI, LoginUserInfo have almost
everything except:
1. user's gaiaId, which doesn't seem to be used by current UI.(user_pod_row.js)
2. public session information which doesn't exist on lockscreen.
We might need it for login screen later and I have added a TODO for this.



BUG=729687

Review-Url: https://codereview.chromium.org/2937553002
Cr-Commit-Position: refs/heads/master@{#481070}
Committed:
https://chromium.googlesource.com/chromium/src/+/820778c575fe891e178b0671a928...
==========

[commit-bot: I haz the power, 2017-06-21 01:43:26]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/820778c575fe891e178b0671a928...