cros: Remove supervised user methods from SystemTrayDelegate

cros: Remove supervised user methods from ash::SystemTrayDelegate

For mustash we need to replace SystemTrayDelegate. Use SessionController
for supervised user data instead.

Send a UserSession update when the Profile becomes available, because
supervised user data is kept in a profile keyed service.

Don't try to update the system tray item text while the menu is open.
The supervised user custodian almost never changes, so we'll see the new
value when the menu is opened again.

For testing, allow FakeChromeUserManager to skip the fake-addition of a
profile to a user on login. It shouldn't be doing this -- in production
the profile is loaded later.

BUG=712799
TEST=added to ash_unittests and unit_tests

Review-Url: https://codereview.chromium.org/2832903002
Cr-Commit-Position: refs/heads/master@{#466752}
Committed: https://chromium.googlesource.com/chromium/src/+/b7360f79e32c16690c062f0b44fecd7d5d452c1e

[James Cook, 2017-04-20 18:33:50]

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

[commit-bot: I haz the power, 2017-04-20 18:34:27]

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-04-20 19:27:30]

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

[commit-bot: I haz the power, 2017-04-20 19:27:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2017-04-20 19:43:04]

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

[commit-bot: I haz the power, 2017-04-20 19:44:05]

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-04-20 20:39:25]

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

[commit-bot: I haz the power, 2017-04-20 20:39:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2017-04-21 20:08:21]

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

[commit-bot: I haz the power, 2017-04-21 20:08:42]

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

[James Cook, 2017-04-21 20:51:52]

Description was changed from

==========
WIP: cros: Remove supervised user methods from SystemTrayDelegate


move strings to ash, seems to spawn 3 notifications


seems to work


it doesn't work, looking up active user and profile is wrong


Remove unused GetSupervisedUserManagerName

BUG=
==========

to

==========
cros: Remove supervised user methods from ash::SystemTrayDelegate

For mustash we need to replace SystemTrayDelegate. Use SessionController
for supervised user data instead.

Send a UserSession update when the Profile becomes available, because
supervised user data is kept in a profile keyed service.

Don't try to update the system tray item text while the menu is open.
The supervised user custodian almost never changes, so we'll see the new
value when the menu is opened again.

For testing, allow FakeChromeUserManager to skip the fake-addition of a
profile to a user on login. It shouldn't be doing this -- in production
the profile is loaded later.

BUG=712799
TEST=added to ash_unittests and unit_tests
==========

[James Cook, 2017-04-21 20:53:43]

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

[commit-bot: I haz the power, 2017-04-21 20:54:02]

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

[James Cook, 2017-04-21 20:54:18]

Patchset #3 (id:40001) has been deleted

[James Cook, 2017-04-21 20:56:14]

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

[James Cook, 2017-04-21 20:56:14]

xiyuan, please take a look at everything.

tsepez, please take a look at ash session_controller.mojom.

[Tom Sepez, 2017-04-21 21:12:27]

lgtm

[commit-bot: I haz the power, 2017-04-21 22:30:31]

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

[commit-bot: I haz the power, 2017-04-21 22:30:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2017-04-21 22:54:58]

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

[commit-bot: I haz the power, 2017-04-21 22:55:38]

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-21 23:14:35]

https://codereview.chromium.org/2832903002/diff/80001/ash/system/supervised/t...
File ash/system/supervised/tray_supervised_user.cc (right):

https://codereview.chromium.org/2832903002/diff/80001/ash/system/supervised/t...
ash/system/supervised/tray_supervised_user.cc:77: 
nit: OnUserSessionUpdated could be called even when a user is not active (e.g.
from sync).

It might be safer to check
  user_session->account_id == account_id

to only proceed when the update is for the active user.

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right):

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:145:
user->set_profile_is_created();
break; ?

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:297: SendActiveUserSession();
Is this safe because supervised user is not allowed in multi-profile? Maybe add
a comment to document?

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:370: // method for
gfx::ImageSkia to allow mojom::UserSession comparison.
We can use ImageSkia::BackedBySameObjectAs to impl Equals. At least, for user
avater case, if they are the same, they would be backed by the same storage
object.

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client.h (right):

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.h:124: Profile* login_profile_ =
nullptr;
login_profile_ -> user_profile_ to match other places in code.

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.h:124: Profile* login_profile_ =
nullptr;
How would this work with multi-profile? We might need a map<User*, Profile*>.

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client_unittest.cc (right):

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client_unittest.cc:390: EXPECT_EQ(1u,
session_controller.last_user_session()->session_id);
Let's just verify this is >0. We might change how session id is generated in the
future.

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client_unittest.cc:408: EXPECT_EQ(1u,
session_controller.last_user_session()->session_id);
ditto, or just remove

https://codereview.chromium.org/2832903002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2832903002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_controller_client.cc:61: Profile* login_profile) {
nit: We could use ProfileHelper to look up user profile from user?

[James Cook, 2017-04-21 23:18:26]

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

[commit-bot: I haz the power, 2017-04-21 23:18:49]

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-04-22 01:01:07]

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

[commit-bot: I haz the power, 2017-04-22 01:01:08]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-04-24 17:47:14]

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

[James Cook, 2017-04-24 17:47:26]

xiyuan, please take another look.

https://codereview.chromium.org/2832903002/diff/80001/ash/system/supervised/t...
File ash/system/supervised/tray_supervised_user.cc (right):

https://codereview.chromium.org/2832903002/diff/80001/ash/system/supervised/t...
ash/system/supervised/tray_supervised_user.cc:77: 
On 2017/04/21 23:14:35, xiyuan wrote:
> nit: OnUserSessionUpdated could be called even when a user is not active (e.g.
> from sync).
> 
> It might be safer to check
>   user_session->account_id == account_id
> 
> to only proceed when the update is for the active user.

Done.

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right):

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:145:
user->set_profile_is_created();
On 2017/04/21 23:14:35, xiyuan wrote:
> break; ?

Done.

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:297: SendActiveUserSession();
On 2017/04/21 23:14:35, xiyuan wrote:
> Is this safe because supervised user is not allowed in multi-profile? Maybe
add
> a comment to document?

How does this look?

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:370: // method for
gfx::ImageSkia to allow mojom::UserSession comparison.
On 2017/04/21 23:14:35, xiyuan wrote:
> We can use ImageSkia::BackedBySameObjectAs to impl Equals. At least, for user
> avater case, if they are the same, they would be backed by the same storage
> object.

Is it OK if I do this in a separate CL?

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client.h (right):

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.h:124: Profile* login_profile_ =
nullptr;
On 2017/04/21 23:14:35, xiyuan wrote:
> How would this work with multi-profile? We might need a map<User*, Profile*>.

How does this look?

Alternately, I could create a
SessionControllerClient::SupervisedUserServiceObserver object for each profile
as it is loaded. (I would have to do this to track which Profile* is being
observed, because OnCustodianInfoChanged() doesn't provide a Profile*.) This
would support multi-profile with multiple supervised users and would allow me to
call SendUserSession() on the correct user for custodian changes. However, we
don't actually support multiple supervised users in the product. WDYT?

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client_unittest.cc (right):

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client_unittest.cc:390: EXPECT_EQ(1u,
session_controller.last_user_session()->session_id);
On 2017/04/21 23:14:35, xiyuan wrote:
> Let's just verify this is >0. We might change how session id is generated in
the
> future.

Done.

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client_unittest.cc:408: EXPECT_EQ(1u,
session_controller.last_user_session()->session_id);
On 2017/04/21 23:14:35, xiyuan wrote:
> ditto, or just remove

Done.

https://codereview.chromium.org/2832903002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2832903002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_controller_client.cc:61: Profile* login_profile) {
On 2017/04/21 23:14:35, xiyuan wrote:
> nit: We could use ProfileHelper to look up user profile from user?

Done.

[commit-bot: I haz the power, 2017-04-24 17:48:12]

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-24 18:10:09]

Mostly good.

One question on using PostTask because ProfileHelper is not ready...

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:297: SendActiveUserSession();
On 2017/04/24 17:47:26, James Cook wrote:
> On 2017/04/21 23:14:35, xiyuan wrote:
> > Is this safe because supervised user is not allowed in multi-profile? Maybe
> add
> > a comment to document?
> 
> How does this look?

Looks good.

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_controller_client.h (right):

https://codereview.chromium.org/2832903002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.h:124: Profile* login_profile_ =
nullptr;
On 2017/04/24 17:47:26, James Cook wrote:
> On 2017/04/21 23:14:35, xiyuan wrote:
> > How would this work with multi-profile? We might need a map<User*,
Profile*>.
> 
> How does this look?
> 
> Alternately, I could create a
> SessionControllerClient::SupervisedUserServiceObserver object for each profile
> as it is loaded. (I would have to do this to track which Profile* is being
> observed, because OnCustodianInfoChanged() doesn't provide a Profile*.) This
> would support multi-profile with multiple supervised users and would allow me
to
> call SendUserSession() on the correct user for custodian changes. However, we
> don't actually support multiple supervised users in the product. WDYT?

Having a single |supervised_user_profile_| is okay since we don't support
multiple supervised users.

https://codereview.chromium.org/2832903002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2832903002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_controller_client.cc:342: // Needed because
chromeos::ProfileHelper isn't updated at this point, so we
ProfileHelper should be updated when a user profile is loaded (i.e.
NOTIFICATION_LOGIN_USER_PROFILE_PREPARED is sent). It depends on two things:
1. user hash, which we got after user login and before loading the profile
2. profile path, which we should have after profile is loaded

What problem did you see if we don't PostTask here?

[James Cook, 2017-04-24 18:34:51]

https://codereview.chromium.org/2832903002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2832903002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_controller_client.cc:342: // Needed because
chromeos::ProfileHelper isn't updated at this point, so we
On 2017/04/24 18:10:09, xiyuan wrote:
> ProfileHelper should be updated when a user profile is loaded (i.e.
> NOTIFICATION_LOGIN_USER_PROFILE_PREPARED is sent). It depends on two things:
> 1. user hash, which we got after user login and before loading the profile
> 2. profile path, which we should have after profile is loaded
> 
> What problem did you see if we don't PostTask here?

chromeos::ProfileHelper::Get()->GetProfileByUser(&user) returns nullptr in
UserToUserSession.

This happens because user->is_profile_created() returns false in
ProfileHelper::GetProfileByUser:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/profiles/profile...

set_profile_is_created() is called in response to NOTIFICATION_PROFILE_CREATED
here:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro...

That happens after LOGIN_USER_PROFILE_PREPARED.

Should I listen for NOTIFICATION_PROFILE_CREATED instead?

[commit-bot: I haz the power, 2017-04-24 18:58:14]

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

[commit-bot: I haz the power, 2017-04-24 18:58:14]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-04-24 19:03:07]

lgtm

https://codereview.chromium.org/2832903002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/session_controller_client.cc (right):

https://codereview.chromium.org/2832903002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_controller_client.cc:342: // Needed because
chromeos::ProfileHelper isn't updated at this point, so we
On 2017/04/24 18:34:51, James Cook wrote:
> On 2017/04/24 18:10:09, xiyuan wrote:
> > ProfileHelper should be updated when a user profile is loaded (i.e.
> > NOTIFICATION_LOGIN_USER_PROFILE_PREPARED is sent). It depends on two things:
> > 1. user hash, which we got after user login and before loading the profile
> > 2. profile path, which we should have after profile is loaded
> > 
> > What problem did you see if we don't PostTask here?
> 
> chromeos::ProfileHelper::Get()->GetProfileByUser(&user) returns nullptr in
> UserToUserSession.
> 
> This happens because user->is_profile_created() returns false in
> ProfileHelper::GetProfileByUser:
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/profiles/profile...
> 
> set_profile_is_created() is called in response to NOTIFICATION_PROFILE_CREATED
> here:
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/chro...
> 
> That happens after LOGIN_USER_PROFILE_PREPARED.
> 
> Should I listen for NOTIFICATION_PROFILE_CREATED instead?

I thought the post task is for profile -> user. And user -> profile mapping is
available later, so post task is warranted here.

Either of the two notifcations are fine. I would prefer to use
LOGIN_USER_PROFILE_PREPARED since it is meant for user profile and I have a TODO
to migrate this to an observer. Using it would be easier to find this code
later.

[James Cook, 2017-04-24 19:26:04]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2017-04-24 19:26:04]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2832903002/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-24 19:26:45]

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-24 20:44:55]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493061964099190,
"parent_rev": "15e38286df8e3aa77c5be34472ba43045cb4a866", "commit_rev":
"b7360f79e32c16690c062f0b44fecd7d5d452c1e"}

[commit-bot: I haz the power, 2017-04-24 20:45:07]

Description was changed from

==========
cros: Remove supervised user methods from ash::SystemTrayDelegate

For mustash we need to replace SystemTrayDelegate. Use SessionController
for supervised user data instead.

Send a UserSession update when the Profile becomes available, because
supervised user data is kept in a profile keyed service.

Don't try to update the system tray item text while the menu is open.
The supervised user custodian almost never changes, so we'll see the new
value when the menu is opened again.

For testing, allow FakeChromeUserManager to skip the fake-addition of a
profile to a user on login. It shouldn't be doing this -- in production
the profile is loaded later.

BUG=712799
TEST=added to ash_unittests and unit_tests
==========

to

==========
cros: Remove supervised user methods from ash::SystemTrayDelegate

For mustash we need to replace SystemTrayDelegate. Use SessionController
for supervised user data instead.

Send a UserSession update when the Profile becomes available, because
supervised user data is kept in a profile keyed service.

Don't try to update the system tray item text while the menu is open.
The supervised user custodian almost never changes, so we'll see the new
value when the menu is opened again.

For testing, allow FakeChromeUserManager to skip the fake-addition of a
profile to a user on login. It shouldn't be doing this -- in production
the profile is loaded later.

BUG=712799
TEST=added to ash_unittests and unit_tests

Review-Url: https://codereview.chromium.org/2832903002
Cr-Commit-Position: refs/heads/master@{#466752}
Committed:
https://chromium.googlesource.com/chromium/src/+/b7360f79e32c16690c062f0b44fe...
==========

[commit-bot: I haz the power, 2017-04-24 20:45:08]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/b7360f79e32c16690c062f0b44fe...