Overview Tray Button should not be visible on login screen

Overview Tray Button should not be visible on login screen
Restrict OverviewButtonTray from showing on both the login screen as well
as the locked screen.

TEST=OverviewButtonTrayTest
BUG=354493
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260577

[jonross, 2014-03-26 18:55:20]

Please review.

[flackr, 2014-03-26 21:00:12]

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

https://codereview.chromium.org/212553005/diff/1/ash/system/overview/overview...
ash/system/overview/overview_button_tray.cc:45: SetVisible(false);
Won't this fail when a display is added while in maximize mode?

https://codereview.chromium.org/212553005/diff/1/ash/system/overview/overview...
ash/system/overview/overview_button_tray.cc:111:
ShouldBeVisibleForLoginStatus(status));
Try to avoid repeating logic like this, it makes it easy for someone to
accidentally change it in one place and not the other. I'd suggest a common
UpdateVisibility which (for now) takes the state of maximize mode as an
argument.

https://codereview.chromium.org/212553005/diff/1/ash/system/overview/overview...
ash/system/overview/overview_button_tray.cc:120: status ==
user::LOGGED_IN_LOCALLY_MANAGED;
Can you try to unify this with
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/overview/wi...
? These should use the same condition, probably the same function.

[jonross, 2014-03-28 16:47:04]

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

https://codereview.chromium.org/212553005/diff/1/ash/system/overview/overview...
ash/system/overview/overview_button_tray.cc:111:
ShouldBeVisibleForLoginStatus(status));
On 2014/03/26 21:00:12, flackr wrote:
> Try to avoid repeating logic like this, it makes it easy for someone to
> accidentally change it in one place and not the other. I'd suggest a common
> UpdateVisibility which (for now) takes the state of maximize mode as an
> argument.

Done.

https://codereview.chromium.org/212553005/diff/1/ash/system/overview/overview...
ash/system/overview/overview_button_tray.cc:120: status ==
user::LOGGED_IN_LOCALLY_MANAGED;
On 2014/03/26 21:00:12, flackr wrote:
> Can you try to unify this with
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/overview/wi...
> ? These should use the same condition, probably the same function.

Done.

[flackr, 2014-03-28 16:56:40]

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

https://codereview.chromium.org/212553005/diff/20001/ash/system/overview/over...
ash/system/overview/overview_button_tray.cc:60: SetIconVisibility(true);
nit: Comment what argument is for clarity: i.e.
SetIconVisibility(/* maximize_mode_enabled */ true);

https://codereview.chromium.org/212553005/diff/20001/ash/system/overview/over...
ash/system/overview/overview_button_tray.cc:64: SetIconVisibility(false);
And here.

https://codereview.chromium.org/212553005/diff/20001/ash/system/overview/over...
ash/system/overview/overview_button_tray.cc:107: void
OverviewButtonTray::SetIconVisibility(bool maximize_mode_enabled) {
nit: Since this doesn't just use maximize_mode_enabled, call this
UpdateIconVisibility.

https://codereview.chromium.org/212553005/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/212553005/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_controller.cc:31: IsActiveUserSessionStarted()
&&
Does this work in guest mode too?

[jonross, 2014-03-28 19:39:00]

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

https://codereview.chromium.org/212553005/diff/20001/ash/system/overview/over...
ash/system/overview/overview_button_tray.cc:60: SetIconVisibility(true);
On 2014/03/28 16:56:40, flackr wrote:
> nit: Comment what argument is for clarity: i.e.
> SetIconVisibility(/* maximize_mode_enabled */ true);

Done.

https://codereview.chromium.org/212553005/diff/20001/ash/system/overview/over...
ash/system/overview/overview_button_tray.cc:64: SetIconVisibility(false);
On 2014/03/28 16:56:40, flackr wrote:
> And here.

Done.

https://codereview.chromium.org/212553005/diff/20001/ash/system/overview/over...
ash/system/overview/overview_button_tray.cc:107: void
OverviewButtonTray::SetIconVisibility(bool maximize_mode_enabled) {
On 2014/03/28 16:56:40, flackr wrote:
> nit: Since this doesn't just use maximize_mode_enabled, call this
> UpdateIconVisibility.

Done.

https://codereview.chromium.org/212553005/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/212553005/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_controller.cc:31: IsActiveUserSessionStarted()
&&
On 2014/03/28 16:56:40, flackr wrote:
> Does this work in guest mode too?

Yes it does. Verified on device.

[flackr, 2014-03-28 19:43:00]

LGTM

[jonross, 2014-03-28 19:45:04]

sadrul@chromium.org: Please review changes in Overview and status_area_widget

[sadrul, 2014-03-28 21:23:35]

LGTM with the nits addressed:

https://codereview.chromium.org/212553005/diff/40001/ash/system/overview/over...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/212553005/diff/40001/ash/system/overview/over...
ash/system/overview/overview_button_tray.cc:61: UpdateIconVisibility(/*
maximize_mode_enabled */ true);
In some parts of the code, an enum is used instead of bools so you don't need to
add comments like this.

https://codereview.chromium.org/212553005/diff/40001/ash/system/overview/over...
ash/system/overview/overview_button_tray.cc:117: }
The function order in .cc should match the order in .h

https://codereview.chromium.org/212553005/diff/40001/ash/system/overview/over...
File ash/system/overview/overview_button_tray.h (right):

https://codereview.chromium.org/212553005/diff/40001/ash/system/overview/over...
ash/system/overview/overview_button_tray.h:45: virtual void
UpdateAfterLoginStatusChange(user::LoginStatus status);
This doesn't need to be virtual?

Also, you should move this above the overrides.

https://codereview.chromium.org/212553005/diff/40001/ash/system/status_area_w...
File ash/system/status_area_widget.cc (right):

https://codereview.chromium.org/212553005/diff/40001/ash/system/status_area_w...
ash/system/status_area_widget.cc:191:
overview_button_tray_->UpdateAfterLoginStatusChange(login_status);
Should UpdateAfterLoginStatusChange() be added to TrayBackgroundView interface?
That would ensure new implementations of tray-icons provide a proper
implementation for this case. (if you do this, you should do that in a
subsequent CL. This looks good for this CL)

[jonross, 2014-03-31 15:09:01]

Cleanup to address nits.

https://codereview.chromium.org/212553005/diff/40001/ash/system/overview/over...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/212553005/diff/40001/ash/system/overview/over...
ash/system/overview/overview_button_tray.cc:61: UpdateIconVisibility(/*
maximize_mode_enabled */ true);
On 2014/03/28 21:23:35, sadrul wrote:
> In some parts of the code, an enum is used instead of bools so you don't need
to
> add comments like this.

This is only here until a refactor of MaximizeMode is done. We want everywhere
listening for maximize mode to just be able to check the state in Shell.
Currently it doesn't match. I'll add another TODO here.

https://codereview.chromium.org/212553005/diff/40001/ash/system/overview/over...
ash/system/overview/overview_button_tray.cc:117: }
On 2014/03/28 21:23:35, sadrul wrote:
> The function order in .cc should match the order in .h

Done.

https://codereview.chromium.org/212553005/diff/40001/ash/system/overview/over...
File ash/system/overview/overview_button_tray.h (right):

https://codereview.chromium.org/212553005/diff/40001/ash/system/overview/over...
ash/system/overview/overview_button_tray.h:45: virtual void
UpdateAfterLoginStatusChange(user::LoginStatus status);
On 2014/03/28 21:23:35, sadrul wrote:
> This doesn't need to be virtual?
> 
> Also, you should move this above the overrides.

Currently no. Each tray in the StatusAreaWidget just implemented this method
themselves. I can move it up to their common ancestor though.

https://codereview.chromium.org/212553005/diff/40001/ash/system/overview/over...
ash/system/overview/overview_button_tray.h:45: virtual void
UpdateAfterLoginStatusChange(user::LoginStatus status);
On 2014/03/28 21:23:35, sadrul wrote:
> This doesn't need to be virtual?
> 
> Also, you should move this above the overrides.

Done.

https://codereview.chromium.org/212553005/diff/40001/ash/system/status_area_w...
File ash/system/status_area_widget.cc (right):

https://codereview.chromium.org/212553005/diff/40001/ash/system/status_area_w...
ash/system/status_area_widget.cc:191:
overview_button_tray_->UpdateAfterLoginStatusChange(login_status);
On 2014/03/28 21:23:35, sadrul wrote:
> Should UpdateAfterLoginStatusChange() be added to TrayBackgroundView
interface?
> That would ensure new implementations of tray-icons provide a proper
> implementation for this case. (if you do this, you should do that in a
> subsequent CL. This looks good for this CL)

Yeah, each of these trays implements it. It should definitely be moved up into
TrayBackgroundView. I'll do that on a subsequent CL.

[jonross, 2014-03-31 15:09:10]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-03-31 15:09:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jonross@chromium.org/212553005/60001

[commit-bot: I haz the power, 2014-03-31 17:08:32]

Change committed as 260577