ash: Fix system tray avatar not updated regression

ash: Fix system tray avatar not updated regression

This is caused by racing between mojo and non-mojo code path.
The avater image is updated via SessionController mojo interface
but UserObserver interface is still a direct call and could reach
ash before the mojo message. The CL fixes the issue by replacing
UserObserver interface with SessionController observer.

BUG=704862
Review-Url: https://codereview.chromium.org/2780963002
Cr-Commit-Position: refs/heads/master@{#460571}
Committed: https://chromium.googlesource.com/chromium/src/+/d24a096f2db3e68da5d48df57a75cf643da1e178

[xiyuan, 2017-03-28 23:07:12]

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

[commit-bot: I haz the power, 2017-03-28 23:08:08]

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

[xiyuan, 2017-03-28 23:11:36]

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

[xiyuan, 2017-03-28 23:11:36]

Take your time. :)

[commit-bot: I haz the power, 2017-03-28 23:47:39]

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

[commit-bot: I haz the power, 2017-03-28 23:47:40]

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

[James Cook, 2017-03-29 00:51:23]

Code looks good, but can you add a test that changes the avatar image for the
user session and verifies that the tray changes? I know the test would not have
caught this race, but we need more tests in the tray code.

You could do something simple like set an empty image then set a non-empty image
-- just anything to exercise the observers.

https://codereview.chromium.org/2780963002/diff/1/ash/common/system/user/tray...
File ash/common/system/user/tray_user.cc (right):

https://codereview.chromium.org/2780963002/diff/1/ash/common/system/user/tray...
ash/common/system/user/tray_user.cc:11: #include
"ash/common/system/tray/system_tray_notifier.h"
nit: is this still needed?

https://codereview.chromium.org/2780963002/diff/1/ash/common/system/user/tray...
ash/common/system/user/tray_user.cc:233:
UpdateAvatarImage(Shell::Get()->system_tray_delegate()->GetUserLoginStatus());
You have to use SystemTrayDelegate because SessionController::GetLoginStatus
isn't always correct?

https://codereview.chromium.org/2780963002/diff/1/ash/common/system/user/tray...
File ash/common/system/user/tray_user.h (right):

https://codereview.chromium.org/2780963002/diff/1/ash/common/system/user/tray...
ash/common/system/user/tray_user.h:84: const UserIndex user_index_;
Hooray for const!

[xiyuan, 2017-03-29 20:43:11]

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

[commit-bot: I haz the power, 2017-03-29 20:44:08]

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

[xiyuan, 2017-03-29 20:45:30]

Added a simple test to verify SessonController update reaches TrayUser.

https://codereview.chromium.org/2780963002/diff/1/ash/common/system/user/tray...
File ash/common/system/user/tray_user.cc (right):

https://codereview.chromium.org/2780963002/diff/1/ash/common/system/user/tray...
ash/common/system/user/tray_user.cc:11: #include
"ash/common/system/tray/system_tray_notifier.h"
On 2017/03/29 00:51:23, James Cook wrote:
> nit: is this still needed?

Nope. Removed.

https://codereview.chromium.org/2780963002/diff/1/ash/common/system/user/tray...
ash/common/system/user/tray_user.cc:233:
UpdateAvatarImage(Shell::Get()->system_tray_delegate()->GetUserLoginStatus());
On 2017/03/29 00:51:23, James Cook wrote:
> You have to use SystemTrayDelegate because SessionController::GetLoginStatus
> isn't always correct?

Nope. Brain-dead copy. Changed to use SystemController.

https://codereview.chromium.org/2780963002/diff/20001/ash/common/system/user/...
File ash/common/system/user/rounded_image_view.h (left):

https://codereview.chromium.org/2780963002/diff/20001/ash/common/system/user/...
ash/common/system/user/rounded_image_view.h:39: gfx::ImageSkia image_;
No need to hold |image_| since the ImageSkiaSource of |resized_| holds a ref to
its storage.

[James Cook, 2017-03-29 20:55:20]

LGTM

https://codereview.chromium.org/2780963002/diff/20001/ash/common/system/user/...
File ash/common/system/user/rounded_image_view.h (right):

https://codereview.chromium.org/2780963002/diff/20001/ash/common/system/user/...
ash/common/system/user/rounded_image_view.h:42: gfx::ImageSkia resized_;
optional: This might be a little better as |resized_image_|, but it's not a big
deal.

[xiyuan, 2017-03-29 21:02:43]

https://codereview.chromium.org/2780963002/diff/20001/ash/common/system/user/...
File ash/common/system/user/rounded_image_view.h (right):

https://codereview.chromium.org/2780963002/diff/20001/ash/common/system/user/...
ash/common/system/user/rounded_image_view.h:42: gfx::ImageSkia resized_;
On 2017/03/29 20:55:20, James Cook wrote:
> optional: This might be a little better as |resized_image_|, but it's not a
big
> deal.

Done.

[xiyuan, 2017-03-29 21:03:00]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-03-29 21:03:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2780963002/#ps40001
(title: "resized_ -> resized_image_")

[commit-bot: I haz the power, 2017-03-29 21:03:47]

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-03-29 23:16:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490821380758440,
"parent_rev": "fe08f3e8ddd0da65fe91e2537fc29d0acbcc20a4", "commit_rev":
"f7adcd872d87f005043ba959a281a69f9fbaa148"}

[commit-bot: I haz the power, 2017-03-29 23:21:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490821380758440,
"parent_rev": "430e7d67e925e6cf8c4be45fa93b4f7a0e3e142a", "commit_rev":
"d24a096f2db3e68da5d48df57a75cf643da1e178"}

[commit-bot: I haz the power, 2017-03-29 23:22:11]

Description was changed from

==========
ash: Fix system tray avatar not updated regression

This is caused by racing between mojo and non-mojo code path.
The avater image is updated via SessionController mojo interface
but UserObserver interface is still a direct call and could reach
ash before the mojo message. The CL fixes the issue by replacing
UserObserver interface with SessionController observer.

BUG=704862
==========

to

==========
ash: Fix system tray avatar not updated regression

This is caused by racing between mojo and non-mojo code path.
The avater image is updated via SessionController mojo interface
but UserObserver interface is still a direct call and could reach
ash before the mojo message. The CL fixes the issue by replacing
UserObserver interface with SessionController observer.

BUG=704862

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

[commit-bot: I haz the power, 2017-03-29 23:22:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d24a096f2db3e68da5d48df57a75...