Stop the OverviewButton from appearing on the Add-User screen

Stop the OverviewButton from appearing on the Add-User screen

OverviewButtonTray was showing up on the Add-User screen. It should only be
available when a user is logged in and in the active state.

Update OverviewButtonTray to observe SessionState changes to determine
visibility.

Update WindowSelectorController to not allow overview mode while on the
add-user screen.

TEST=OverviewButtonTrayTest.VisibilityChangesForLoginStatus
BUG=488464, 482591
Committed: https://crrev.com/cd872eea1f6a0571d94796046bf690274254db2d
Cr-Commit-Position: refs/heads/master@{#332872}

[jonross, 2015-05-26 20:56:12]

jonross@chromium.org changed reviewers:
+ flackr@chromium.org

[jonross, 2015-05-26 20:56:13]

Hey Rob,

Could you PTAL and provide owner review for ash/wm?

Thanks,
Jon

[flackr, 2015-05-28 15:20:38]

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

https://codereview.chromium.org/1149833006/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray.cc:71: UpdateIconVisibility();
I think this is getting brittle, the logic / responsibility for determining when
we can enter overview being split between window_selector_controller to query
and any class which wants to know about when this state changes to observe all
possible events which cause the state to change.

Can you move the necessary observers to determine when CanSelect changes to
window_selector_controller and make it so that this class only needs to observe
something on window_selector_controller? (And still observe maximize mode
because that is independent of whether CanSelect is true)

[jonross, 2015-06-01 17:00:37]

+tdanderson@ FYI as you are working on a related patch.

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

https://codereview.chromium.org/1149833006/diff/1/ash/system/overview/overvie...
ash/system/overview/overview_button_tray.cc:71: UpdateIconVisibility();
On 2015/05/28 15:20:37, flackr wrote:
> I think this is getting brittle, the logic / responsibility for determining
when
> we can enter overview being split between window_selector_controller to query
> and any class which wants to know about when this state changes to observe all
> possible events which cause the state to change.
> 
> Can you move the necessary observers to determine when CanSelect changes to
> window_selector_controller and make it so that this class only needs to
observe
> something on window_selector_controller? (And still observe maximize mode
> because that is independent of whether CanSelect is true)

I've decided to decouple OverviewButton's visibility from WindowSelectors
ability to activate.

As seen in:
https://codereview.chromium.org/1114383002/
there is another case where the two are separate.

I've updated OverviewButtonTray to track the aspects of user sessions to
determine when to show.

[flackr, 2015-06-01 17:05:32]

https://codereview.chromium.org/1149833006/diff/20001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1149833006/diff/20001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:58: SetVisible(false);
What guarantee is there that the visibility won't be later updated not taking
into account this state? Seems like it would be better to save the loginstatus
if you can't query for LOGGED_IN_KIOSK_APP.

[jonross, 2015-06-01 17:20:37]

https://codereview.chromium.org/1149833006/diff/20001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1149833006/diff/20001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:58: SetVisible(false);
On 2015/06/01 17:05:32, flackr wrote:
> What guarantee is there that the visibility won't be later updated not taking
> into account this state? Seems like it would be better to save the loginstatus
> if you can't query for LOGGED_IN_KIOSK_APP.

Good point.

Done.

[flackr, 2015-06-01 17:50:01]

In the original change you had also added a check for
SessionStateDelegate::SESSION_STATE_ACTIVE to CanSelect. Do we need to prevent
window selection as well as hiding the overview button while in the add user
screen?

[jonross, 2015-06-01 18:10:24]

On 2015/06/01 17:50:01, flackr wrote:
> In the original change you had also added a check for
> SessionStateDelegate::SESSION_STATE_ACTIVE to CanSelect. Do we need to prevent
> window selection as well as hiding the overview button while in the add user
> screen?

The button, while present on the Add User Screen, was non functional. Other
requirements for actually starting Overview mode were failing. So the extra
check is unnecessary in CanSelect.

I've confirmed that this change also fixes: 482591, so I've included the unit
test that tdanderson@ wrote for that fix.

[flackr, 2015-06-01 18:16:33]

lgtm with nits

https://codereview.chromium.org/1149833006/diff/60001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1149833006/diff/60001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:138: user::LOGGED_IN_KIOSK_APP);
nit: Lots of Shell::GetInstance()'s here, recommend using a local variable (e.g.
"shell") even if just for readability.

https://codereview.chromium.org/1149833006/diff/60001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray_unittest.cc (right):

https://codereview.chromium.org/1149833006/diff/60001/ash/system/overview/ove...
ash/system/overview/overview_button_tray_unittest.cc:77: void
OverviewButtonTrayTest::UpdateSessionsState() {
nit: This doesn't look like it updates the session state - perhaps call it
NotifySessionStateChanged?

[flackr, 2015-06-01 18:18:55]

https://codereview.chromium.org/1149833006/diff/60001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1149833006/diff/60001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:127: SetVisible(
nit: You should add a comment to the effect of why this is intentionally
different than WindowSelectorController::CanSelect

[jonross, 2015-06-01 18:27:43]

https://codereview.chromium.org/1149833006/diff/60001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1149833006/diff/60001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:127: SetVisible(
On 2015/06/01 18:18:55, flackr wrote:
> nit: You should add a comment to the effect of why this is intentionally
> different than WindowSelectorController::CanSelect

Done.

https://codereview.chromium.org/1149833006/diff/60001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:138: user::LOGGED_IN_KIOSK_APP);
On 2015/06/01 18:16:33, flackr wrote:
> nit: Lots of Shell::GetInstance()'s here, recommend using a local variable
(e.g.
> "shell") even if just for readability.

Done.

https://codereview.chromium.org/1149833006/diff/60001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray_unittest.cc (right):

https://codereview.chromium.org/1149833006/diff/60001/ash/system/overview/ove...
ash/system/overview/overview_button_tray_unittest.cc:77: void
OverviewButtonTrayTest::UpdateSessionsState() {
On 2015/06/01 18:16:33, flackr wrote:
> nit: This doesn't look like it updates the session state - perhaps call it
> NotifySessionStateChanged?

It did at one point... name/functionality fell out of sync in one branch..

Done.

[jonross, 2015-06-01 18:28:09]

jonross@chromium.org changed reviewers:
+ sadrul@chromium.org

[jonross, 2015-06-01 18:28:10]

Hey Sadrul,

Could you provide an owners review of this change?

Thanks,
Jon

[sadrul, 2015-06-03 19:08:36]

lgtm

https://codereview.chromium.org/1149833006/diff/80001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1149833006/diff/80001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:140:
SessionStateDelegate::SESSION_STATE_ACTIVE &&
This ... is a bit complex. Can this be made better? e.g. have a separate state
for locked screen, so that just checking SESSION_STATE_ACTIVE here is
sufficient? and/or, update IsActiveUserSessionStarted() so that it does the
checking for screen-lock and session-state? At this current state, this API
seems fairly unusable for someone not already closely familiar with it.

[jonross, 2015-06-04 17:38:23]

https://codereview.chromium.org/1149833006/diff/80001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1149833006/diff/80001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:140:
SessionStateDelegate::SESSION_STATE_ACTIVE &&
On 2015/06/03 19:08:36, sadrul wrote:
> This ... is a bit complex. Can this be made better? e.g. have a separate state
> for locked screen, so that just checking SESSION_STATE_ACTIVE here is
> sufficient? and/or, update IsActiveUserSessionStarted() so that it does the
> checking for screen-lock and session-state? At this current state, this API
> seems fairly unusable for someone not already closely familiar with it.

The "Active Session" term has become a bit overloaded.

Right now:
  - IsActiveUserSessionStarted tracks if a user login status, can precede
completion of UI creation. Some usage current expects it to be separated from UI
  - GetSessionState is a login UI flow state
  - IsScreenLocked is currently separated from GetSessionState

I really think that this state tracking should be merged in the
SessionStateDelegate. Then code using them to be updated.

The fact that the type of user is tracked through the SystemTrayDelegate is also
a bit confusing.

[jonross, 2015-06-04 17:55:55]

https://codereview.chromium.org/1149833006/diff/80001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/1149833006/diff/80001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:140:
SessionStateDelegate::SESSION_STATE_ACTIVE &&
On 2015/06/04 17:38:22, jonross wrote:
> On 2015/06/03 19:08:36, sadrul wrote:
> > This ... is a bit complex. Can this be made better? e.g. have a separate
state
> > for locked screen, so that just checking SESSION_STATE_ACTIVE here is
> > sufficient? and/or, update IsActiveUserSessionStarted() so that it does the
> > checking for screen-lock and session-state? At this current state, this API
> > seems fairly unusable for someone not already closely familiar with it.
> 
> The "Active Session" term has become a bit overloaded.
> 
> Right now:
>   - IsActiveUserSessionStarted tracks if a user login status, can precede
> completion of UI creation. Some usage current expects it to be separated from
UI
>   - GetSessionState is a login UI flow state
>   - IsScreenLocked is currently separated from GetSessionState
> 
> I really think that this state tracking should be merged in the
> SessionStateDelegate. Then code using them to be updated.
> 
> The fact that the type of user is tracked through the SystemTrayDelegate is
also
> a bit confusing.

Filed crbug.com/496761 to cover a refactor of SessionStateDelegate

[jonross, 2015-06-04 17:55:58]

The CQ bit was checked by jonross@chromium.org

[jonross, 2015-06-04 17:55:59]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org
Link to the patchset: https://codereview.chromium.org/1149833006/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-06-04 17:57:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149833006/80001

[commit-bot: I haz the power, 2015-06-04 19:20:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-04 19:21:34]

Patchset 5 (id:??) landed as
https://crrev.com/cd872eea1f6a0571d94796046bf690274254db2d
Cr-Commit-Position: refs/heads/master@{#332872}