ash: SessionControllerClient observes user image change

ash: SessionControllerClient observes user image change

BUG=670422
Review-Url: https://codereview.chromium.org/2619653002
Cr-Commit-Position: refs/heads/master@{#443431}
Committed: https://chromium.googlesource.com/chromium/src/+/64fa7e77502cc896a5ba7d8eab1dd19447fda467

[xiyuan, 2017-01-06 22:51:02]

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

[James Cook, 2017-01-06 23:08:32]

LGTM

[xiyuan, 2017-01-06 23:09:01]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2017-01-06 23:09:32]

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-01-06 23:35:28]

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

[commit-bot: I haz the power, 2017-01-06 23:35:29]

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_...)

[xiyuan, 2017-01-07 00:17:44]

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

[commit-bot: I haz the power, 2017-01-07 00:18:09]

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-01-07 00:55:23]

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

[commit-bot: I haz the power, 2017-01-07 00:55:23]

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_...)

[xiyuan, 2017-01-09 18:27:23]

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

[commit-bot: I haz the power, 2017-01-09 18:27:51]

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-01-09 19:05:18]

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

[commit-bot: I haz the power, 2017-01-09 19:05:19]

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_...)

[xiyuan, 2017-01-09 21:08:41]

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

[commit-bot: I haz the power, 2017-01-09 21:09:23]

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-01-09 21:12:15]

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

[commit-bot: I haz the power, 2017-01-09 21:12:16]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[xiyuan, 2017-01-09 21:42:52]

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

[commit-bot: I haz the power, 2017-01-09 21:43:55]

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

[xiyuan, 2017-01-09 22:37:29]

Mind taking another look?

The original patch (#1) does not cover the case that user image could be updated
before login (i.e. on the login screen). #2/#3 fixes the problem.

#2 changes to use session id from SessionManager instead of using the index of
logged_in_users. This is needed because logged_in_users could be populated after
UserManager atempts to update the user image (e.g. for a new user). And we want
to do that anyway (removed a TODO).

#3 changes to skip SendUserSession if the user has no session. This is needed
because some tests create a user without signing in.

#4 fixes WallpaperManagerBrowserTest that still uses UserManager::UserLoggedIn
to similar login so we fail with missing user session DCHECKs, migrated to use
SessionManager::CreateSession. I will also get a wallpaper owner to review that.

Thanks.

[commit-bot: I haz the power, 2017-01-09 23:00:59]

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

[commit-bot: I haz the power, 2017-01-09 23:01:00]

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_...)

[xiyuan, 2017-01-09 23:13:57]

Please hold off reviewing. More test failures. Let me fix them and ping back. :(

[xiyuan, 2017-01-09 23:40:42]

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

[commit-bot: I haz the power, 2017-01-09 23:41:22]

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-01-10 01:00:51]

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

[commit-bot: I haz the power, 2017-01-10 01:00:51]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-01-11 17:03:58]

Finally got the bots green. Could you take another look?

Thanks.

[James Cook, 2017-01-11 19:27:20]

LGTM unless my question about the wallpaper test is actually a problem.

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_manager_browsertest.cc (right):

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:640: if
(log_in)
nit: {}, or "using" to shorten the line

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
(right):

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc:457:
// No need to explicitly login for crash-n-restore.
Q: Why not? Isn't this testing that the user's wallpaper is restored? Or is the
--login-user flag above sufficient to test this case?

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

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:51:
ash::mojom::UserSessionPtr UserToUserSession(const User& user) {
nit: Document this function (especially why/what it means to return null)

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:281: // Bail if user has no
session.
Is this for tests or does it happen in production? This might need more comment.

[xiyuan, 2017-01-11 20:48:38]

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_manager_browsertest.cc (right):

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:640: if
(log_in)
On 2017/01/11 19:27:20, James Cook wrote:
> nit: {}, or "using" to shorten the line

Done.

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
(right):

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc:457:
// No need to explicitly login for crash-n-restore.
On 2017/01/11 19:27:20, James Cook wrote:
> Q: Why not? Isn't this testing that the user's wallpaper is restored? Or is
the
> --login-user flag above sufficient to test this case?

"--login-user" would trigger an immediate login in PreProfileInit [1]. Thus no
need to explicitly login again here. We were fine before because UserManager
does not care UserLoggedIn being called multiple times. SessionManager cares
about this and has a DCHECK to catch create a user session for the same user
more than once.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m...

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

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:51:
ash::mojom::UserSessionPtr UserToUserSession(const User& user) {
On 2017/01/11 19:27:20, James Cook wrote:
> nit: Document this function (especially why/what it means to return null)

Done.

https://codereview.chromium.org/2619653002/diff/90001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_controller_client.cc:281: // Bail if user has no
session.
On 2017/01/11 19:27:20, James Cook wrote:
> Is this for tests or does it happen in production? This might need more
comment.

OnUserImageChanged can be called on the login screen when UserManager loads the
user meta data (e.g. policy change caused a public session user being added, or
tests that add new users on the login screen.)

This is the only code path that runs into this situation. I could add the
session check in OnUserImageChanged. But thought it might be better to do it
here just in case we run into similar situations in the future.

Added more comments.

[xiyuan, 2017-01-11 21:00:18]

xiyuan@chromium.org changed reviewers:
+ asanka@chromium.org, oshima@chromium.org

[xiyuan, 2017-01-11 21:00:19]

Please help with owners review:

asanka@ for
  c/b/download/notification/download_notification_browsertest.cc

oshima@ for
  c/b/chromeos/file_manager/file_manager_browsertest.cc
  c/b/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc

The changes in the tests are mainly switching from UserManager::UserLoggedIn to
SessionManager::CreateSession. The later is newly introduced to move user
session related logic out of UserManager.

This CL itself hooks up the user image change with SessionControllerClient (for
mash and eventually for classic ash as well) and the tests need to be updated
now to make SessionManager aware of the user sessions (aka logged-in users) so
that it can prepare a UserSession struct to call
SessionController::UpdateUserSession.

[asanka, 2017-01-11 21:02:43]

lgtm stamp for /download/

[xiyuan, 2017-01-12 22:50:25]

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

[commit-bot: I haz the power, 2017-01-12 22:51: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-01-12 23:39:57]

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

[commit-bot: I haz the power, 2017-01-12 23:39:58]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-01-12 23:40:35]

lgtm

[xiyuan, 2017-01-12 23:44:18]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-01-12 23:44:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/2619653002/#ps130001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-12 23:45:46]

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-01-13 01:23:35]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1484264658918010,
"parent_rev": "7de09d362b972881698c15fc80d66fbb03c55adb", "commit_rev":
"64fa7e77502cc896a5ba7d8eab1dd19447fda467"}

[commit-bot: I haz the power, 2017-01-13 01:24:23]

Description was changed from

==========
ash: SessionControllerClient observes user image change

BUG=670422
==========

to

==========
ash: SessionControllerClient observes user image change

BUG=670422

Review-Url: https://codereview.chromium.org/2619653002
Cr-Commit-Position: refs/heads/master@{#443431}
Committed:
https://chromium.googlesource.com/chromium/src/+/64fa7e77502cc896a5ba7d8eab1d...
==========

[commit-bot: I haz the power, 2017-01-13 01:24:24]

Committed patchset #8 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/64fa7e77502cc896a5ba7d8eab1d...