Creating multi profile animations for switching users and teleporting of windows.

Creating multi profile animations for switching users and teleporting of windows.

This CL is adding window animations for the following multi profile related actions:
- switching the user
- teleporting of windows
- window ownership changes

The user switch animation is as follows:

Time:    ----->
Screen:  A         X         B

- The desktop cross dissolves between A -> B
- User A's windows fade out between A -> X
- User B's windows get faded in between X -> B
- User A's shelf gets hidden between A -> X
- The user icon in the system tray as well as the shelf configuration changes at 'X'.
- User B's shelf gets faded on between X -> B

So at time X the user would see a half way cross dissolved desktop and shared windows (if there are any).

Since there is no guarantee that there is an animation going from A -> X an additional timer was used to kick off the second animation portion.

Further gotchas:
- Animations of the individual shelf items were in the past incorrectly performed with another animator which produced a lag for some components (e.g. the activation bar lagged and / or there was a gap between icons in the tray and the screen border when playing the animation fast.
- Wallpaper loading was so delayed that the animations started after all other animations were done.

BUG=336639, 307279
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247517

[Mr4D (OOO till 08-26), 2014-01-25 01:12:44]

Please have a look!

[Harry McCleave, 2014-01-25 01:28:01]

https://codereview.chromium.org/130983007/diff/1/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/130983007/diff/1/ash/shelf/shelf_view.cc#newc...
ash/shelf/shelf_view.cc:130: bounds_animator_->SetAnimationDuration(1);
Is the 1 ms needed here, would it be better to have 0, and have the condition
for the ShelfLayoutManager::duration_override_in_ms_ to be considered be
non-negative?

[Alexander Alekseev, 2014-01-27 12:36:41]

+bshe@ as author of WallpaperManager
+nkostylev@ as login/ OWNER.

lgtm

[sky, 2014-01-27 15:28:54]

What parts of this did you need me to review?

On Mon, Jan 27, 2014 at 4:36 AM,  <alemate@chromium.org> wrote:
> +bshe@ as author of WallpaperManager
> +nkostylev@ as login/ OWNER.
>
> lgtm
>
> https://codereview.chromium.org/130983007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mr4D (OOO till 08-26), 2014-01-27 15:37:51]

Fixed unit tests and clang issue

https://codereview.chromium.org/130983007/diff/1/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/130983007/diff/1/ash/shelf/shelf_view.cc#newc...
ash/shelf/shelf_view.cc:130: bounds_animator_->SetAnimationDuration(1);
Unfortunately a 1 is required, otherwise there will be no update happening
whatsoever.

[Mr4D (OOO till 08-26), 2014-01-27 15:39:33]

Sky - I need your owners review for the files below, but would value your input
of the other files as well if you would like to have a look!

  ui/views/animation/bounds_animator.h
  ui/views/corewm/window_animations.cc
  ui/views/corewm/window_animations.h

[bshe, 2014-01-27 16:00:14]

https://codereview.chromium.org/130983007/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/130983007/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wallpaper_manager.cc:1202:
wallpaper_cache_.insert(std::make_pair(email, wallpaper.image()));
If a user changes wallpaper during multi profile session, we probably want to
update the cache as well. Cache is a std::map, so "insert" probably cant be used
to update the cache. Maybe switch to
"wallpaper_cache_[email] = wallpaper.image();" or something?

[sky, 2014-01-27 17:17:08]

Said files LGTM with the following changes.

https://codereview.chromium.org/130983007/diff/580001/ui/views/animation/boun...
File ui/views/animation/bounds_animator.h (right):

https://codereview.chromium.org/130983007/diff/580001/ui/views/animation/boun...
ui/views/animation/bounds_animator.h:99: int GetAnimationDuration() { return
animation_duration_ms_; }
animation_duration_ms() const.

https://codereview.chromium.org/130983007/diff/580001/ui/views/corewm/window_...
File ui/views/corewm/window_animations.h (right):

https://codereview.chromium.org/130983007/diff/580001/ui/views/corewm/window_...
ui/views/corewm/window_animations.h:78: aura::Window* window);
Can this take a const aura::Window*?

[Mr4D (OOO till 08-26), 2014-01-27 18:44:17]

Addressed and fixed unit test

https://codereview.chromium.org/130983007/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/130983007/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wallpaper_manager.cc:1202:
wallpaper_cache_.insert(std::make_pair(email, wallpaper.image()));
On 2014/01/27 16:00:15, bshe wrote:
> If a user changes wallpaper during multi profile session, we probably want to
> update the cache as well. Cache is a std::map, so "insert" probably cant be
used
> to update the cache. Maybe switch to
> "wallpaper_cache_[email] = wallpaper.image();" or something?

Done.

https://codereview.chromium.org/130983007/diff/580001/ui/views/animation/boun...
File ui/views/animation/bounds_animator.h (right):

https://codereview.chromium.org/130983007/diff/580001/ui/views/animation/boun...
ui/views/animation/bounds_animator.h:99: int GetAnimationDuration() { return
animation_duration_ms_; }
On 2014/01/27 17:17:09, sky wrote:
> animation_duration_ms() const.

Done.

https://codereview.chromium.org/130983007/diff/580001/ui/views/corewm/window_...
File ui/views/corewm/window_animations.h (right):

https://codereview.chromium.org/130983007/diff/580001/ui/views/corewm/window_...
ui/views/corewm/window_animations.h:78: aura::Window* window);
On 2014/01/27 17:17:09, sky wrote:
> Can this take a const aura::Window*?

Done.

[Harry McCleave, 2014-01-27 18:56:38]

lgtm for ash/shelf

[bshe, 2014-01-27 19:21:28]

On 2014/01/27 18:56:38, Harry McCleave wrote:
> lgtm for ash/shelf

lgtm for wallpaper related

[Nikita (slow), 2014-01-28 16:27:22]

Stefan, can you tried pressing desktop switching shortcut and keeping it pressed
so that animation proceed non-stop? What's the performance?

https://codereview.chromium.org/130983007/diff/760003/ash/system/tray/default...
File ash/system/tray/default_system_tray_delegate.h (right):

https://codereview.chromium.org/130983007/diff/760003/ash/system/tray/default...
ash/system/tray/default_system_tray_delegate.h:92: 
nit: Nuke extra empty line.

https://codereview.chromium.org/130983007/diff/760003/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h
(right):

https://codereview.chromium.org/130983007/diff/760003/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h:103: void
DisableAnimationsForTest(bool disable);
nit: Rename to SetAnimationsForTest since it is able to enable/disable it?

[Mr4D (OOO till 08-26), 2014-01-28 16:48:02]

Addressed!

For the continuous key press: Tested that before and you see it switching
several times and then it stops visible switching, doing the last iteration once
the keys are let go.

https://codereview.chromium.org/130983007/diff/760003/ash/system/tray/default...
File ash/system/tray/default_system_tray_delegate.h (right):

https://codereview.chromium.org/130983007/diff/760003/ash/system/tray/default...
ash/system/tray/default_system_tray_delegate.h:92: 
On 2014/01/28 16:27:23, Nikita Kostylev wrote:
> nit: Nuke extra empty line.

Done.

https://codereview.chromium.org/130983007/diff/760003/chrome/browser/ui/ash/m...
File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h
(right):

https://codereview.chromium.org/130983007/diff/760003/chrome/browser/ui/ash/m...
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h:103: void
DisableAnimationsForTest(bool disable);
On 2014/01/28 16:27:23, Nikita Kostylev wrote:
> nit: Rename to SetAnimationsForTest since it is able to enable/disable it?

Done.

[Nikita (slow), 2014-01-28 17:54:10]

lgt

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nikita (slow), 2014-01-28 17:54:11]

lgtm

Let's see how these animations look on different hardware once this is
landed.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-01-28 18:17:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/130983007/840001

[commit-bot: I haz the power, 2014-01-28 23:21:33]

Change committed as 247517

[Nikita (slow), 2014-01-30 14:03:40]

https://codereview.chromium.org/130983007/diff/840001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/130983007/diff/840001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wallpaper_manager.cc:1203:
(UserManager::Get()->GetLoggedInUsers().size() > 1)) {
I've found that this doesn't quite work.

1. Add couple of users to multi-profile session
2. Now change wallpaper for user A. Wallpaper is changed.
3. Switch to user B desktop and back.
4. See that user A old wallpaper is shown.
5. Switch to user B and changes wallpaper, same issue after switching to user A
and back.

[Mr4D (OOO till 08-26), 2014-01-30 19:33:37]

Created issue 339576 to track and address this.