Tray elements behave appropriately on the multiple signin screen (more like lock screen)

Tray elements behave appropriately on the multiple signin screen (more like lock screen)

BUG=385623
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288245

[Roman Sorokin (ftl), 2014-07-01 11:01:21]

skuhne@, please take a look.

[Roman Sorokin (ftl), 2014-07-07 07:32:58]

The CQ bit was checked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2014-07-07 07:32:58]

The CQ bit was unchecked by rsorokin@chromium.org

[Mr4D (OOO till 08-26), 2014-07-08 15:32:07]

Sorry for the delay - was OOO the last days.

Once addressed, lgtm

https://codereview.chromium.org/357323002/diff/1/ash/session/session_state_de...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/357323002/diff/1/ash/session/session_state_de...
ash/session/session_state_delegate.h:137: // Hides multiple sign-in screen in
case it is opened.
What about "Hides multi user sign-in screen .. "?

https://codereview.chromium.org/357323002/diff/1/ash/session/session_state_de...
ash/session/session_state_delegate.h:138: virtual void
HideMultipleSigninScreen() = 0;
Can you name this HideMultiUserSigninScreen()?

[Roman Sorokin (ftl), 2014-07-09 13:04:15]

oshima@, please take a look.

https://codereview.chromium.org/357323002/diff/1/ash/session/session_state_de...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/357323002/diff/1/ash/session/session_state_de...
ash/session/session_state_delegate.h:137: // Hides multiple sign-in screen in
case it is opened.
On 2014/07/08 15:32:07, Mr4D wrote:
> What about "Hides multi user sign-in screen .. "?

Done.

https://codereview.chromium.org/357323002/diff/1/ash/session/session_state_de...
ash/session/session_state_delegate.h:138: virtual void
HideMultipleSigninScreen() = 0;
On 2014/07/08 15:32:07, Mr4D wrote:
> Can you name this HideMultiUserSigninScreen()? 

Done.

[oshima, 2014-07-09 17:54:35]

I thought we want not to show the setting item in multiple signin screen?

[Roman Sorokin (ftl), 2014-07-16 10:18:40]

On 2014/07/09 17:54:35, oshima wrote:
> I thought we want not to show the setting item in multiple signin screen?

Actually now it is different issue. It works for all tray elements, not only for
settings.

[oshima, 2014-07-16 15:55:08]

On 2014/07/16 10:18:40, Roman Sorokin wrote:
> On 2014/07/09 17:54:35, oshima wrote:
> > I thought we want not to show the setting item in multiple signin screen?
> 
> Actually now it is different issue. It works for all tray elements, not only
for
> settings.

I mean, do the same as lock screen. Is there good reason to do it differently?

[Roman Sorokin (ftl), 2014-07-16 18:35:39]

On 2014/07/16 15:55:08, oshima wrote:
> On 2014/07/16 10:18:40, Roman Sorokin wrote:
> > On 2014/07/09 17:54:35, oshima wrote:
> > > I thought we want not to show the setting item in multiple signin screen?
> > 
> > Actually now it is different issue. It works for all tray elements, not only
> for
> > settings.
> 
> I mean, do the same as lock screen. Is there good reason to do it differently?

We does not need to enter password to get rid of multiple signin screen. Just
press cancel.

[oshima, 2014-07-16 20:43:31]

> does not need to enter password to get rid of multiple >signin screen. Just
press cancel.

Yes, exactly. You can just cancel to access tray menu.

I'm actually fine with either approach. I just want to make sure UX or PM are
aware of it.
Did you consult with any one from UX/PM folks? 

And not all tray elements should close the sign in screen (and I do believe
you're not planning to do so). Can you update the bug description?

https://codereview.chromium.org/357323002/diff/20001/ash/session/session_stat...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/357323002/diff/20001/ash/session/session_stat...
ash/session/session_state_delegate.h:138: virtual void
HideMultiUserSigninScreen() = 0;
This should be either "CloseMultiUser..." or "CancelMultiUser" as this actually
cancels the sign in screen.

https://codereview.chromium.org/357323002/diff/20001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/357323002/diff/20001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:218: if
(this->GetSessionState() ==
remove "this->"

[Roman Sorokin (ftl), 2014-07-31 13:58:17]

oshima@, can you please take another look. We've decided to make tray look
similar to lock screen tray.

[oshima, 2014-07-31 18:28:31]

can you update the CL description?

bots seems to have failures. Not sure if it's related, but you may want to
double check.

https://codereview.chromium.org/357323002/diff/100001/ash/system/bluetooth/tr...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/357323002/diff/100001/ash/system/bluetooth/tr...
ash/system/bluetooth/tray_bluetooth.cc:309:
ash::SessionStateDelegate::SESSION_STATE_LOGIN_SECONDARY)
Am I correct that many checks are basically "if a user is primary, unlocked"? If
so, can you add a predicate method in Shell and use it instead of repeating this
everywhere?

https://codereview.chromium.org/357323002/diff/100001/ash/system/bluetooth/tr...
ash/system/bluetooth/tray_bluetooth.cc:310: return;
you need {} in this case

[Roman Sorokin (ftl), 2014-07-31 18:51:32]

https://codereview.chromium.org/357323002/diff/100001/ash/system/bluetooth/tr...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/357323002/diff/100001/ash/system/bluetooth/tr...
ash/system/bluetooth/tray_bluetooth.cc:309:
ash::SessionStateDelegate::SESSION_STATE_LOGIN_SECONDARY)
On 2014/07/31 18:28:31, oshima wrote:
> Am I correct that many checks are basically "if a user is primary, unlocked"?
If
> so, can you add a predicate method in Shell and use it instead of repeating
this
> everywhere?

This method does exist (IsUserSessionBlocked) but if we use it we completely
ignore LoginStatus param from CreateDefaultView. This is a case of changing
LoginStatus
https://code.google.com/p/chromium/codesearch#chromium/src/ash/system/tray/sy....
And there is a test for it.

[oshima, 2014-07-31 18:55:14]

On 2014/07/31 18:51:32, Roman Sorokin wrote:
>
https://codereview.chromium.org/357323002/diff/100001/ash/system/bluetooth/tr...
> File ash/system/bluetooth/tray_bluetooth.cc (right):
> 
>
https://codereview.chromium.org/357323002/diff/100001/ash/system/bluetooth/tr...
> ash/system/bluetooth/tray_bluetooth.cc:309:
> ash::SessionStateDelegate::SESSION_STATE_LOGIN_SECONDARY)
> On 2014/07/31 18:28:31, oshima wrote:
> > Am I correct that many checks are basically "if a user is primary,
unlocked"?
> If
> > so, can you add a predicate method in Shell and use it instead of repeating
> this
> > everywhere?
> 
> This method does exist (IsUserSessionBlocked) but if we use it we completely
> ignore LoginStatus param from CreateDefaultView. This is a case of changing
> LoginStatus
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/system/tray/sy....
> And there is a test for it.

Can you elaborate? (I'm busy today and don't have time to go though code and
digest it correctly)

[Roman Sorokin (ftl), 2014-07-31 19:07:04]

It means that check in Shell could be different cause LoginStatus in Shell is
different from LoginStatus that was passed in method to create view.
On 2014/07/31 18:55:14, oshima wrote:
> On 2014/07/31 18:51:32, Roman Sorokin wrote:
> >
>
https://codereview.chromium.org/357323002/diff/100001/ash/system/bluetooth/tr...
> > File ash/system/bluetooth/tray_bluetooth.cc (right):
> > 
> >
>
https://codereview.chromium.org/357323002/diff/100001/ash/system/bluetooth/tr...
> > ash/system/bluetooth/tray_bluetooth.cc:309:
> > ash::SessionStateDelegate::SESSION_STATE_LOGIN_SECONDARY)
> > On 2014/07/31 18:28:31, oshima wrote:
> > > Am I correct that many checks are basically "if a user is primary,
> unlocked"?
> > If
> > > so, can you add a predicate method in Shell and use it instead of
repeating
> > this
> > > everywhere?
> > 
> > This method does exist (IsUserSessionBlocked) but if we use it we completely
> > ignore LoginStatus param from CreateDefaultView. This is a case of changing
> > LoginStatus
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/system/tray/sy....
> > And there is a test for it.
> 
> Can you elaborate? (I'm busy today and don't have time to go though code and
> digest it correctly)

[oshima, 2014-08-01 20:31:23]

>Shell could be different cause LoginStatus in Shell is
>different from LoginStatus that was passed in method to create view.

How they can be different? Aren't we using
system_tray_delegate()->GetUserLoginStatus() ?

https://codereview.chromium.org/357323002/diff/120001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/357323002/diff/120001/ash/shell.h#newcode405
ash/shell.h:405: SessionStateDelegate* session_state_delegate);
move this to ash/test/shell_test_api

https://codereview.chromium.org/357323002/diff/120001/ash/system/bluetooth/tr...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/bluetooth/tr...
ash/system/bluetooth/tray_bluetooth.cc:309:
ash::SessionStateDelegate::SESSION_STATE_LOGIN_SECONDARY);
could you mind adding IsInSecondaryLoginScreen() utility method ?
(to be consistent with enum)

The better approach would be to have a object that represents the session state
(not a delegate), instead of enum but that's probably outside of the scope of
this CL.

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/net...
File ash/system/chromeos/network/network_state_list_detailed_view.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/net...
ash/system/chromeos/network/network_state_list_detailed_view.cc:746:
ash::SessionStateDelegate::SESSION_STATE_LOGIN_SECONDARY);
shouldn't this be in ShouldShowSettings() ?

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/net...
File ash/system/chromeos/network/tray_vpn.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/net...
ash/system/chromeos/network/tray_vpn.cc:133: this, status !=
user::LOGGED_IN_LOCKED && !userAddingRunning);
i can can see the vpn status on lock screen. It's just not active.

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/ses...
File ash/system/chromeos/session/logout_button_tray.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/ses...
ash/system/chromeos/session/logout_button_tray.cc:167: login_status_ !=
user::LOGGED_IN_LOCKED && !userAddingRunning);
does secondary login screen have a sign out out button? (I think we don't show
these on lock screen because lock screen has its own shutdown/signout button)

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/set...
File ash/system/chromeos/settings/tray_settings.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/set...
ash/system/chromeos/settings/tray_settings.cc:155: 
same here. this can be handled by ShouldShowSettings()

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/tra...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/tra...
ash/system/chromeos/tray_display.cc:116: return;
Do you need this? If you do, can you use ShouldShowSettings instead?

[Roman Sorokin (ftl), 2014-08-05 13:43:31]

On 2014/08/01 20:31:23, oshima wrote:
> How they can be different? Aren't we using
> system_tray_delegate()->GetUserLoginStatus() ?

There is at least one case when we pass LOGGED_IN_LOCKED  even though user is
logged and not locked. It is when modal window is opened. And there is a test on
this thing.

[Roman Sorokin (ftl), 2014-08-05 13:44:09]

https://codereview.chromium.org/357323002/diff/120001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/357323002/diff/120001/ash/shell.h#newcode405
ash/shell.h:405: SessionStateDelegate* session_state_delegate);
On 2014/08/01 20:31:22, oshima wrote:
> move this to ash/test/shell_test_api

Done.

https://codereview.chromium.org/357323002/diff/120001/ash/system/bluetooth/tr...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/bluetooth/tr...
ash/system/bluetooth/tray_bluetooth.cc:309:
ash::SessionStateDelegate::SESSION_STATE_LOGIN_SECONDARY);
On 2014/08/01 20:31:22, oshima wrote:
> could you mind adding IsInSecondaryLoginScreen() utility method ?
> (to be consistent with enum)
> 
> The better approach would be to have a object that represents the session
state
> (not a delegate), instead of enum but that's probably outside of the scope of
> this CL.

Done.

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/net...
File ash/system/chromeos/network/network_state_list_detailed_view.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/net...
ash/system/chromeos/network/network_state_list_detailed_view.cc:746:
ash::SessionStateDelegate::SESSION_STATE_LOGIN_SECONDARY);
On 2014/08/01 20:31:22, oshima wrote:
> shouldn't this be in ShouldShowSettings() ?

Done.

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/net...
File ash/system/chromeos/network/tray_vpn.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/net...
ash/system/chromeos/network/tray_vpn.cc:133: this, status !=
user::LOGGED_IN_LOCKED && !userAddingRunning);
Yes, and it will be the same on the multiple signin screen.
On 2014/08/01 20:31:22, oshima wrote:
> i can can see the vpn status on lock screen. It's just not active.

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/ses...
File ash/system/chromeos/session/logout_button_tray.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/ses...
ash/system/chromeos/session/logout_button_tray.cc:167: login_status_ !=
user::LOGGED_IN_LOCKED && !userAddingRunning);
Turned out this is button for public sessions so does not needed to do anything
On 2014/08/01 20:31:22, oshima wrote:
> does secondary login screen have a sign out out button? (I think we don't show
> these on lock screen because lock screen has its own shutdown/signout button)

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/set...
File ash/system/chromeos/settings/tray_settings.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/set...
ash/system/chromeos/settings/tray_settings.cc:155: 
On 2014/08/01 20:31:22, oshima wrote:
> same here. this can be handled by ShouldShowSettings()

Done.

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/tra...
File ash/system/chromeos/tray_display.cc (right):

https://codereview.chromium.org/357323002/diff/120001/ash/system/chromeos/tra...
ash/system/chromeos/tray_display.cc:116: return;
On 2014/08/01 20:31:22, oshima wrote:
> Do you need this? If you do, can you use ShouldShowSettings instead?

Done.

[oshima, 2014-08-06 19:47:41]

lgtm if you addressed below.

https://codereview.chromium.org/357323002/diff/140001/ash/session/session_sta...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/357323002/diff/140001/ash/session/session_sta...
ash/session/session_state_delegate.h:141: bool IsInSecondaryLoginScreen() const
{
move this to (new) .cc (preferred). I'm fine to move it to Shell if you want.

[Roman Sorokin (ftl), 2014-08-07 12:45:55]

https://codereview.chromium.org/357323002/diff/140001/ash/session/session_sta...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/357323002/diff/140001/ash/session/session_sta...
ash/session/session_state_delegate.h:141: bool IsInSecondaryLoginScreen() const
{
On 2014/08/06 19:47:41, oshima wrote:
> move this to (new) .cc (preferred). I'm fine to move it to Shell if you want. 

Done.

[Roman Sorokin (ftl), 2014-08-07 17:29:58]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2014-08-07 17:35:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsorokin@chromium.org/357323002/160001

[commit-bot: I haz the power, 2014-08-07 19:05:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-08 00:45:46]

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

[commit-bot: I haz the power, 2014-08-08 00:45:53]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

Transient error: <urlopen error [Errno 104] Connection reset by peer>

[Roman Sorokin (ftl), 2014-08-08 07:28:35]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2014-08-08 07:31:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsorokin@chromium.org/357323002/160001

[commit-bot: I haz the power, 2014-08-08 08:04:50]

Change committed as 288245