cros: Merge SystemTrayDelegate::GetUserLoginStatus

cros: Merge SystemTrayDelegate::GetUserLoginStatus

Merge SystemTrayDelegate::GetUserLoginStatus into
SessionController::login_status to avoid inconsistent
states in ash code.

BUG=718385
Review-Url: https://codereview.chromium.org/2864663002
Cr-Commit-Position: refs/heads/master@{#469771}
Committed: https://chromium.googlesource.com/chromium/src/+/6779a04e21ebe7424ed2edae270f3303894104dc

[xiyuan, 2017-05-05 16:07:29]

Description was changed from

==========
cros: Remove SystemTrayDelegate::GetUserLoginStatus

Consolidate SystemTrayDelegate::GetUserLoginStatus into
SessionController::login_status to avoid inconsistencies
states.

BUG=718385
==========

to

==========
cros: Merge SystemTrayDelegate::GetUserLoginStatus

Merge SystemTrayDelegate::GetUserLoginStatus into
SessionController::login_status to avoid inconsistent
states in ash code.

BUG=718385
==========

[xiyuan, 2017-05-05 16:09:02]

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

[commit-bot: I haz the power, 2017-05-05 16:09:30]

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-05 16:40:42]

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

[commit-bot: I haz the power, 2017-05-05 16:40:43]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-05-05 17:35:10]

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

[xiyuan, 2017-05-05 17:35:11]

Not urgent so take your time. :)

[James Cook, 2017-05-05 17:51:54]

LGTM with some ideas for follow up. Hooray! I'm so glad to see this going away.

https://codereview.chromium.org/2864663002/diff/1/ash/root_window_controller_...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/2864663002/diff/1/ash/root_window_controller_...
ash/root_window_controller_unittest.cc:390:
Shell::Get()->session_controller()->login_status());
How about switching some of these to their non-LoginStatus equivalents, like
IsActiveUserSessionStarted() or IsScreenLocked()?

Isn't the long-term plan to get rid of LoginStatus?

https://codereview.chromium.org/2864663002/diff/1/ash/root_window_controller_...
ash/root_window_controller_unittest.cc:451: EXPECT_EQ(LoginStatus::USER,
session_controller->login_status());
nit: could remove this one

https://codereview.chromium.org/2864663002/diff/1/ash/system/overview/overvie...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/2864663002/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray.cc:109:
session_controller->login_status() != LoginStatus::KIOSK_APP &&
optional: WDYT of adding either SessionController::IsKiosk() or
SessionController::GetActiveUserType()?

not necessarily for this CL

https://codereview.chromium.org/2864663002/diff/1/ash/system/palette/palette_...
File ash/system/palette/palette_tray.cc (right):

https://codereview.chromium.org/2864663002/diff/1/ash/system/palette/palette_...
ash/system/palette/palette_tray.cc:20: #include
"ash/system/tray/system_tray_delegate.h"
remove?

https://codereview.chromium.org/2864663002/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2864663002/diff/1/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:38: #include
"ash/system/tray/system_tray_delegate.h"
remove?

[xiyuan, 2017-05-05 18:36:34]

https://codereview.chromium.org/2864663002/diff/1/ash/root_window_controller_...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/2864663002/diff/1/ash/root_window_controller_...
ash/root_window_controller_unittest.cc:390:
Shell::Get()->session_controller()->login_status());
On 2017/05/05 17:51:53, James Cook (OOO) wrote:
> How about switching some of these to their non-LoginStatus equivalents, like
> IsActiveUserSessionStarted() or IsScreenLocked()?
> 
> Isn't the long-term plan to get rid of LoginStatus?

Right. Replaced LoginStatus with SessionController calls here an other places. I
was lazy and just doing mechanical changes. :)

https://codereview.chromium.org/2864663002/diff/1/ash/root_window_controller_...
ash/root_window_controller_unittest.cc:451: EXPECT_EQ(LoginStatus::USER,
session_controller->login_status());
On 2017/05/05 17:51:53, James Cook (OOO) wrote:
> nit: could remove this one

Done.

https://codereview.chromium.org/2864663002/diff/1/ash/system/overview/overvie...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/2864663002/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray.cc:109:
session_controller->login_status() != LoginStatus::KIOSK_APP &&
On 2017/05/05 17:51:53, James Cook (OOO) wrote:
> optional: WDYT of adding either SessionController::IsKiosk() or
> SessionController::GetActiveUserType()?
> 
> not necessarily for this CL

Added a SessionController::IsKioskSession() and updated call sites like this to
use the call.

https://codereview.chromium.org/2864663002/diff/1/ash/system/palette/palette_...
File ash/system/palette/palette_tray.cc (right):

https://codereview.chromium.org/2864663002/diff/1/ash/system/palette/palette_...
ash/system/palette/palette_tray.cc:20: #include
"ash/system/tray/system_tray_delegate.h"
On 2017/05/05 17:51:53, James Cook (OOO) wrote:
> remove?

Done.

https://codereview.chromium.org/2864663002/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2864663002/diff/1/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:38: #include
"ash/system/tray/system_tray_delegate.h"
On 2017/05/05 17:51:53, James Cook (OOO) wrote:
> remove?

This is still in use, around Line 279
  delegate->CreateRotationLockTrayItem(..)

[xiyuan, 2017-05-05 18:36:39]

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

[commit-bot: I haz the power, 2017-05-05 18:37:10]

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-05 19:29:55]

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

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

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-05-05 20:23:23]

still lgtm

https://codereview.chromium.org/2864663002/diff/20001/ash/system/palette/pale...
File ash/system/palette/palette_tray.cc (right):

https://codereview.chromium.org/2864663002/diff/20001/ash/system/palette/pale...
ash/system/palette/palette_tray.cc:67: session_controller->login_status() !=
LoginStatus::KIOSK_APP;
Aside: I wonder if it is intentional that ARC kiosk apps can show the stylus
tools.

[xiyuan, 2017-05-05 20:54:46]

https://codereview.chromium.org/2864663002/diff/20001/ash/system/palette/pale...
File ash/system/palette/palette_tray.cc (right):

https://codereview.chromium.org/2864663002/diff/20001/ash/system/palette/pale...
ash/system/palette/palette_tray.cc:67: session_controller->login_status() !=
LoginStatus::KIOSK_APP;
On 2017/05/05 20:23:23, James Cook (OOO) wrote:
> Aside: I wonder if it is intentional that ARC kiosk apps can show the stylus
> tools.

I doubt that it is intentional since ARC kiosk is full screen and does not show
shelf. Changed to use IsKioskSession. And we will find out.

[xiyuan, 2017-05-05 20:54:59]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-05-05 20:55:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2864663002/#ps40001
(title: "update PaletteTray to exclude ARC kiosk")

[commit-bot: I haz the power, 2017-05-05 20:55:49]

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-05 21:27:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494017699686470,
"parent_rev": "935ffc18282202b8b51036252ecfb9a4001dae2a", "commit_rev":
"6779a04e21ebe7424ed2edae270f3303894104dc"}

[commit-bot: I haz the power, 2017-05-05 21:28:51]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494017699686470,
"parent_rev": "935ffc18282202b8b51036252ecfb9a4001dae2a", "commit_rev":
"6779a04e21ebe7424ed2edae270f3303894104dc"}

[commit-bot: I haz the power, 2017-05-05 21:28:54]

Description was changed from

==========
cros: Merge SystemTrayDelegate::GetUserLoginStatus

Merge SystemTrayDelegate::GetUserLoginStatus into
SessionController::login_status to avoid inconsistent
states in ash code.

BUG=718385
==========

to

==========
cros: Merge SystemTrayDelegate::GetUserLoginStatus

Merge SystemTrayDelegate::GetUserLoginStatus into
SessionController::login_status to avoid inconsistent
states in ash code.

BUG=718385

Review-Url: https://codereview.chromium.org/2864663002
Cr-Commit-Position: refs/heads/master@{#469771}
Committed:
https://chromium.googlesource.com/chromium/src/+/6779a04e21ebe7424ed2edae270f...
==========

[commit-bot: I haz the power, 2017-05-05 21:28:55]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6779a04e21ebe7424ed2edae270f...