cros: Move IsUserSupervised and IsUserChild off SystemTrayDelegate

cros: Move IsUserSupervised and IsUserChild off SystemTrayDelegate

Put them on SessionController so they work properly in mustash.

Remove redundant TraySupervisedTest for login crash (the underlying tray
notification code has been refactored so that the crashing code path
no longer exists, and the other test covers the login case well
enough). See crbug.com/275697 for the crash.

Eliminate ScopedInitialLoginStatus, which was only used by the
redundant test.

BUG=712799
TEST=ash_unittests, added to SessionControllerTest, manually testing of
login with supervised users

Review-Url: https://codereview.chromium.org/2829813002
Cr-Commit-Position: refs/heads/master@{#465797}
Committed: https://chromium.googlesource.com/chromium/src/+/0e518b6113db1f4d0c30e2831288a08bc457f639

[James Cook, 2017-04-19 20:31:56]

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

[James Cook, 2017-04-19 20:31:57]

xiyuan, please take a look. I plan to migrate the other methods in a follow-up
CL.

https://codereview.chromium.org/2829813002/diff/1/ash/session/session_control...
File ash/session/session_controller.h (right):

https://codereview.chromium.org/2829813002/diff/1/ash/session/session_control...
ash/session/session_controller.h:93: bool IsUserChild() const;
I just moved the method here because it was simple. However, I wonder if it
would be better to provide a GetActiveUserType() method. That might help reduce
usage of ash::LoginStatus. OTOH, the method would have to return something when
there was no active user, which would mean adding UserType::UNKNOWN = -1 (since
there are histograms on UserType) and updating the mojo serializers. WDYT?

[James Cook, 2017-04-19 20:32:11]

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

[commit-bot: I haz the power, 2017-04-19 20:32:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-04-19 20:58:00]

lgtm

https://codereview.chromium.org/2829813002/diff/1/ash/session/session_control...
File ash/session/session_controller.h (right):

https://codereview.chromium.org/2829813002/diff/1/ash/session/session_control...
ash/session/session_controller.h:93: bool IsUserChild() const;
On 2017/04/19 20:31:57, James Cook wrote:
> I just moved the method here because it was simple. However, I wonder if it
> would be better to provide a GetActiveUserType() method. That might help
reduce
> usage of ash::LoginStatus. OTOH, the method would have to return something
when
> there was no active user, which would mean adding UserType::UNKNOWN = -1
(since
> there are histograms on UserType) and updating the mojo serializers. WDYT?

SGTM. It would be a good starer bug. :p

https://codereview.chromium.org/2829813002/diff/1/ash/system/supervised/tray_...
File ash/system/supervised/tray_supervised_user_unittest.cc (right):

https://codereview.chromium.org/2829813002/diff/1/ash/system/supervised/tray_...
ash/system/supervised/tray_supervised_user_unittest.cc:52:
GetSystemTrayDelegate()->GetUserLoginStatus());
I would vote to merge GetUserLoginStatus with the
SessionController::login_status() in the future. WDYT?

https://codereview.chromium.org/2829813002/diff/1/ash/system/supervised/tray_...
ash/system/supervised/tray_supervised_user_unittest.cc:62:
Shell::Get()->session_controller()->UpdateUserSession(std::move(session));
Make TestSessionControllerClient::AddUserSession to take a user type  and use
that?

https://codereview.chromium.org/2829813002/diff/1/ash/test/test_system_tray_d...
File ash/test/test_system_tray_delegate.cc (right):

https://codereview.chromium.org/2829813002/diff/1/ash/test/test_system_tray_d...
ash/test/test_system_tray_delegate.cc:54: return login_status_;
Possible to drop |login_status_| and just return
session_controller()->login_status() here ?

[commit-bot: I haz the power, 2017-04-19 21:08:58]

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

[commit-bot: I haz the power, 2017-04-19 21:08:59]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-04-19 22:16:38]

Thanks for the quick review.

https://codereview.chromium.org/2829813002/diff/1/ash/session/session_control...
File ash/session/session_controller.h (right):

https://codereview.chromium.org/2829813002/diff/1/ash/session/session_control...
ash/session/session_controller.h:93: bool IsUserChild() const;
On 2017/04/19 20:57:59, xiyuan wrote:
> On 2017/04/19 20:31:57, James Cook wrote:
> > I just moved the method here because it was simple. However, I wonder if it
> > would be better to provide a GetActiveUserType() method. That might help
> reduce
> > usage of ash::LoginStatus. OTOH, the method would have to return something
> when
> > there was no active user, which would mean adding UserType::UNKNOWN = -1
> (since
> > there are histograms on UserType) and updating the mojo serializers. WDYT?
> 
> SGTM. It would be a good starer bug. :p

Filed crbug.com/713379

https://codereview.chromium.org/2829813002/diff/1/ash/system/supervised/tray_...
File ash/system/supervised/tray_supervised_user_unittest.cc (right):

https://codereview.chromium.org/2829813002/diff/1/ash/system/supervised/tray_...
ash/system/supervised/tray_supervised_user_unittest.cc:52:
GetSystemTrayDelegate()->GetUserLoginStatus());
On 2017/04/19 20:58:00, xiyuan wrote:
> I would vote to merge GetUserLoginStatus with the
> SessionController::login_status() in the future. WDYT?

I think that's a good idea, but we need to eliminate ash::LoginStatus::OWNER,
because the two don't match for that case. Filed crbug.com/713380

https://codereview.chromium.org/2829813002/diff/1/ash/system/supervised/tray_...
ash/system/supervised/tray_supervised_user_unittest.cc:62:
Shell::Get()->session_controller()->UpdateUserSession(std::move(session));
On 2017/04/19 20:58:00, xiyuan wrote:
> Make TestSessionControllerClient::AddUserSession to take a user type  and use
> that?

Done.

https://codereview.chromium.org/2829813002/diff/1/ash/test/test_system_tray_d...
File ash/test/test_system_tray_delegate.cc (right):

https://codereview.chromium.org/2829813002/diff/1/ash/test/test_system_tray_d...
ash/test/test_system_tray_delegate.cc:54: return login_status_;
On 2017/04/19 20:58:00, xiyuan wrote:
> Possible to drop |login_status_| and just return
> session_controller()->login_status() here ?

I'll do this in a follow-up CL because I need to fix UserMetricsRecorderTest to
do that, and that is best done by rewriting part of UserMetricsRecorder.

[James Cook, 2017-04-19 22:16:48]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2017-04-19 22:16:48]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2829813002/#ps20001
(title: "review comments")

[commit-bot: I haz the power, 2017-04-19 22:17:39]

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-04-19 23:00:22]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492640208550320,
"parent_rev": "a022d09fac4eae6cadf496a3f743488bbb60dafd", "commit_rev":
"0e518b6113db1f4d0c30e2831288a08bc457f639"}

[commit-bot: I haz the power, 2017-04-19 23:00:37]

Description was changed from

==========
cros: Move IsUserSupervised and IsUserChild off SystemTrayDelegate

Put them on SessionController so they work properly in mustash.

Remove redundant TraySupervisedTest for login crash (the underlying tray
notification code has been refactored so that the crashing code path
no longer exists, and the other test covers the login case well
enough). See crbug.com/275697 for the crash.

Eliminate ScopedInitialLoginStatus, which was only used by the
redundant test.

BUG=712799
TEST=ash_unittests, added to SessionControllerTest, manually testing of
login with supervised users
==========

to

==========
cros: Move IsUserSupervised and IsUserChild off SystemTrayDelegate

Put them on SessionController so they work properly in mustash.

Remove redundant TraySupervisedTest for login crash (the underlying tray
notification code has been refactored so that the crashing code path
no longer exists, and the other test covers the login case well
enough). See crbug.com/275697 for the crash.

Eliminate ScopedInitialLoginStatus, which was only used by the
redundant test.

BUG=712799
TEST=ash_unittests, added to SessionControllerTest, manually testing of
login with supervised users

Review-Url: https://codereview.chromium.org/2829813002
Cr-Commit-Position: refs/heads/master@{#465797}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e518b6113db1f4d0c30e2831288...
==========

[commit-bot: I haz the power, 2017-04-19 23:00:38]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0e518b6113db1f4d0c30e2831288...