Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10909008:
Improve existing lock transition - remove black splash. (Closed)
|
Created:
8 years, 3 months ago by Nikita (slow) Modified:
8 years, 3 months ago CC:
chromium-reviews, sadrul, Ben Goodger (Google) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImprove existing lock transition - remove black splash.
During screen lock we move wallpaper from normal background
layer to screen lock background layer. That means that both
these layers should be excluded from the lock animations.
Lock background layer (with wallpaper, not black solid color layer)
is included in shutdown animation though.
BUG=144737
TEST=Manual: Lock/Shutdown from login screen/user session using power button / shortcut / UI controls.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=154936
Patch Set 1 #Patch Set 2 : +comment #
Total comments: 6
Patch Set 3 : refactor #Patch Set 4 : fix #
Total comments: 25
Patch Set 5 : review #
Total comments: 2
Patch Set 6 : nit + m #
Messages
Total messages: 15 (0 generated)
Dan, please review whole CL. I'm planning to merge this into M22 since it is a Stable blocker. Denis FYI.
http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... File ash/wm/power_button_controller.h (right): http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... ash/wm/power_button_controller.h:60: SCREEN_LOCKER_CONTAINERS_EXCEPT_WALLPAPER, I find this confusing. The containers that each enum referred to was formerly obvious: ALL_CONTAINERS: kShellWindowId_NonLockScreenContainersContainer kShellWindowId_LockScreenContainersContainer kShellWindowId_LockScreenRelatedContainersContainer SCREEN_LOCKER_CONTAINERS: kShellWindowId_LockScreenContainersContainer SCREEN_LOCKER_AND_RELATED_CONTAINERS: kShellWindowId_LockScreenContainersContainer kShellWindowId_LockScreenRelatedContainersContainer ALL_BUT_SCREEN_LOCKER_AND_RELATED_CONTAINERS: kShellWindowId_NonLockScreenContainersContainer Just from looking at the enum names now, though, I have no idea which containers are included in each. Maybe it'd be better to make a bitmap instead so it's clear from the code which containers are being affected (we'll end up with very long enum names, otherwise): const int kNonLockScreenContainersMask = 1 << 0; const int kLockScreenContainersMask = 1 << 1; // etc. StartAnimation( kLockScreenWallpaperMask | kLockScreenContainersMask | kLockScreenRelatedContainersMask, ANIMATION_HIDE);
Other than that do you have any other comments for changes in ash/root_window_controller.cc? On 2012/08/30 16:04:53, Daniel Erat wrote: > http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... > File ash/wm/power_button_controller.h (right): > > http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... > ash/wm/power_button_controller.h:60: SCREEN_LOCKER_CONTAINERS_EXCEPT_WALLPAPER, > I find this confusing. The containers that each enum referred to was formerly > obvious: > > ALL_CONTAINERS: > kShellWindowId_NonLockScreenContainersContainer > kShellWindowId_LockScreenContainersContainer > kShellWindowId_LockScreenRelatedContainersContainer > SCREEN_LOCKER_CONTAINERS: > kShellWindowId_LockScreenContainersContainer > SCREEN_LOCKER_AND_RELATED_CONTAINERS: > kShellWindowId_LockScreenContainersContainer > kShellWindowId_LockScreenRelatedContainersContainer > ALL_BUT_SCREEN_LOCKER_AND_RELATED_CONTAINERS: > kShellWindowId_NonLockScreenContainersContainer > > Just from looking at the enum names now, though, I have no idea which containers > are included in each. > > Maybe it'd be better to make a bitmap instead so it's clear from the code which > containers are being affected (we'll end up with very long enum names, > otherwise): > > const int kNonLockScreenContainersMask = 1 << 0; > const int kLockScreenContainersMask = 1 << 1; > // etc. > > StartAnimation( > kLockScreenWallpaperMask | > kLockScreenContainersMask | > kLockScreenRelatedContainersMask, > ANIMATION_HIDE);
It seems a bit dangerous to me to have some of the top-level windows be containers-of-containers and other ones just be regular containers. The whole multiple-levels-of-containers-so-we-can-animate-them approach was already a big hack. Perhaps we ought to do something else. :-/ Cc-ing Ben in case he has some ideas.
On 2012/08/30 17:02:30, Daniel Erat wrote: > It seems a bit dangerous to me to have some of the top-level windows be > containers-of-containers and other ones just be regular containers. The whole > multiple-levels-of-containers-so-we-can-animate-them approach was already a big > hack. Perhaps we ought to do something else. :-/ > > Cc-ing Ben in case he has some ideas. We need to come up with a reasonable short-term / long-term solution if needed than as this bug is a stable blocker. Current lock screen transition with black screen looks strange.
http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... File ash/wm/power_button_controller.h (right): http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... ash/wm/power_button_controller.h:60: SCREEN_LOCKER_CONTAINERS_EXCEPT_WALLPAPER, On 2012/08/30 16:04:53, Daniel Erat wrote: > Maybe it'd be better to make a bitmap instead so it's clear from the code which > containers are being affected (we'll end up with very long enum names, > otherwise): Are you proposing to abandon those predefined groups completely?
http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... File ash/wm/power_button_controller.h (right): http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... ash/wm/power_button_controller.h:60: SCREEN_LOCKER_CONTAINERS_EXCEPT_WALLPAPER, On 2012/08/30 17:04:51, Nikita Kostylev wrote: > On 2012/08/30 16:04:53, Daniel Erat wrote: > > Maybe it'd be better to make a bitmap instead so it's clear from the code > which > > containers are being affected (we'll end up with very long enum names, > > otherwise): > > Are you proposing to abandon those predefined groups completely? Yes, if the concepts of which containers each group contains become so complicated that they can't be described in fewer than 80 characters.
http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... File ash/wm/power_button_controller.h (right): http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... ash/wm/power_button_controller.h:60: SCREEN_LOCKER_CONTAINERS_EXCEPT_WALLPAPER, On 2012/08/30 16:04:53, Daniel Erat wrote: > I find this confusing. The containers that each enum referred to was formerly > obvious: I think exclusion-based name like "non-something" or "all but something" are a bit confusing - we just need some good term for containers with "user" windows - ones we are hiding when locking screen. As for bitmaps, I find them confusing. With group constants, we have have just 4 possible outcomes, with bitmaps there would be at least 2^3 ("session" group, "locked" group, "background") combinations -- harder to manage and test. I think it's better to keep with enum, but rename constants to make them clearer.
Relying on ash_unittests + manual testing of these transitions. http://codereview.chromium.org/10909008/diff/3001/ash/root_window_controller.cc File ash/root_window_controller.cc (right): http://codereview.chromium.org/10909008/diff/3001/ash/root_window_controller.... ash/root_window_controller.cc:140: aura::Window* desktop_background_containers = CreateContainer( One option is to have internal::kShellWindowId_DesktopBackgroundContainersContainer, which just holds internal::kShellWindowId_DesktopBackgroundContainer but that sounds like an overkill unless that is really needed to be on the safe side (I'll be merging this into M22 Beta/Stable branch). Ben, wdyt?
These groups seems much cleaner now. http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... File ash/wm/power_button_controller.h (right): http://codereview.chromium.org/10909008/diff/3001/ash/wm/power_button_control... ash/wm/power_button_controller.h:60: SCREEN_LOCKER_CONTAINERS_EXCEPT_WALLPAPER, On 2012/08/30 16:04:53, Daniel Erat wrote: > I find this confusing. The containers that each enum referred to was formerly > obvious: > > ALL_CONTAINERS: > kShellWindowId_NonLockScreenContainersContainer > kShellWindowId_LockScreenContainersContainer > kShellWindowId_LockScreenRelatedContainersContainer > SCREEN_LOCKER_CONTAINERS: > kShellWindowId_LockScreenContainersContainer > SCREEN_LOCKER_AND_RELATED_CONTAINERS: > kShellWindowId_LockScreenContainersContainer > kShellWindowId_LockScreenRelatedContainersContainer > ALL_BUT_SCREEN_LOCKER_AND_RELATED_CONTAINERS: > kShellWindowId_NonLockScreenContainersContainer > > Just from looking at the enum names now, though, I have no idea which containers > are included in each. > > Maybe it'd be better to make a bitmap instead so it's clear from the code which > containers are being affected (we'll end up with very long enum names, > otherwise): > > const int kNonLockScreenContainersMask = 1 << 0; > const int kLockScreenContainersMask = 1 << 1; > // etc. > > StartAnimation( > kLockScreenWallpaperMask | > kLockScreenContainersMask | > kLockScreenRelatedContainersMask, > ANIMATION_HIDE); > Done.
Thanks for changing this -- I agree that it's clearer. http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller.h File ash/wm/power_button_controller.h (left): http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:218: bool should_start_shutdown_timer_after_lock_; thanks for deleting this http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller.h File ash/wm/power_button_controller.h (right): http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:56: // Specific containers or group of containers that can be animated. nit: s/group/groups/ http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:59: DESKTOP_WALLPAPER = 1 << 0, use consistent terminology: you're using "background" for the window id but "wallpaper" here http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:65: // Desktop wallpaper is moved to this layer when screen is locked. asked over IM, but perhaps you're not there: i didn't realize earlier that the wallpaper was being moved between layers. is there any reason that it can't just be moved from NonLockScreenContainersContainer to LockScreenBackgroundContainer, so that none of the changes to PowerButtonController are necessary? where's the change that does the moving? http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:69: LOCK_SCREEN_WALLPAPER = 1 << 2, same background vs. wallpaper issue here http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:130: // was last animated with |type| (probably; the analysis is fairly ad-hoc). nit: s/was/were/ http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:131: // |container_mask| is a bitfield of a ContainerGroup. nit: s/a ContainerGroup/ContainerGroups/ http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:132: bool ContainerGroupIsAnimated(int container_mask, AnimationType type) const; nit: s/ContainerGroupIs/ContainerGroupsAre/ http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:147: // Helper method that returns bitfield mask of all containers. nit: s/returns/returns a/ http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:150: // Helper method that returns bitfield mask of all lock screen containers. nit: s/returns/returns a/ the comment here makes it sound like this method just returns LOCK_SCREEN_CONTAINERS. change it to "mask including LOCK_SCREEN_WALLPAPER, LOCK_SCREEN_CONTAINERS, and LOCK_SCREEN_RELATED_CONTAINERS", assuming that that's what it does http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:151: static int GetAllScreenLockContainersMask(); nit: s/ScreenLock/LockScreen/, so this is consistent with the mask names http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... File ash/wm/power_button_controller_unittest.cc (right): http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller_unittest.cc:120: // Previously we're checking that all containers group was animated which nit: add a blank line before this s/all containers group/the all containers group/
http://codereview.chromium.org/10909008/diff/8/ash/root_window_controller.cc File ash/root_window_controller.cc (right): http://codereview.chromium.org/10909008/diff/8/ash/root_window_controller.cc#... ash/root_window_controller.cc:151: aura::Window* lock_background_containers = CreateContainer( Nikita and I discussed this over IM. I didn't think that this container is necessary anymore; it seems like the wallpaper should just be able to always stay in DesktopBackgroundContainer. Apparently we want to move the wallpaper here when the screen is locked to make sure that there's an opaque layer occluding the non-lock-screen layers, though. It'd be good to document this in a comment somewhere.
http://codereview.chromium.org/10909008/diff/8/ash/root_window_controller.cc File ash/root_window_controller.cc (right): http://codereview.chromium.org/10909008/diff/8/ash/root_window_controller.cc#... ash/root_window_controller.cc:151: aura::Window* lock_background_containers = CreateContainer( On 2012/09/04 16:58:59, Daniel Erat wrote: > Nikita and I discussed this over IM. I didn't think that this container is > necessary anymore; it seems like the wallpaper should just be able to always > stay in DesktopBackgroundContainer. Apparently we want to move the wallpaper > here when the screen is locked to make sure that there's an opaque layer > occluding the non-lock-screen layers, though. It'd be good to document this in > a comment somewhere. Done. http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller.h File ash/wm/power_button_controller.h (right): http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:56: // Specific containers or group of containers that can be animated. On 2012/09/04 16:40:18, Daniel Erat wrote: > nit: s/group/groups/ Done. http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:59: DESKTOP_WALLPAPER = 1 << 0, On 2012/09/04 16:40:18, Daniel Erat wrote: > use consistent terminology: you're using "background" for the window id but > "wallpaper" here Done. http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:65: // Desktop wallpaper is moved to this layer when screen is locked. On 2012/09/04 16:40:18, Daniel Erat wrote: > asked over IM, but perhaps you're not there: i didn't realize earlier that the > wallpaper was being moved between layers. is there any reason that it can't > just be moved from NonLockScreenContainersContainer to > LockScreenBackgroundContainer, so that none of the changes to > PowerButtonController are necessary? where's the change that does the moving? http://codereview.chromium.org/10810039/ Discussed over IM. http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:69: LOCK_SCREEN_WALLPAPER = 1 << 2, On 2012/09/04 16:40:18, Daniel Erat wrote: > same background vs. wallpaper issue here Done. http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:130: // was last animated with |type| (probably; the analysis is fairly ad-hoc). On 2012/09/04 16:40:18, Daniel Erat wrote: > nit: s/was/were/ Done. http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:131: // |container_mask| is a bitfield of a ContainerGroup. On 2012/09/04 16:40:18, Daniel Erat wrote: > nit: s/a ContainerGroup/ContainerGroups/ As discussed renamed to Container http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:132: bool ContainerGroupIsAnimated(int container_mask, AnimationType type) const; On 2012/09/04 16:40:18, Daniel Erat wrote: > nit: s/ContainerGroupIs/ContainerGroupsAre/ Done. http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:147: // Helper method that returns bitfield mask of all containers. On 2012/09/04 16:40:18, Daniel Erat wrote: > nit: s/returns/returns a/ Done. http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:150: // Helper method that returns bitfield mask of all lock screen containers. On 2012/09/04 16:40:18, Daniel Erat wrote: > nit: s/returns/returns a/ > > the comment here makes it sound like this method just returns > LOCK_SCREEN_CONTAINERS. change it to "mask including LOCK_SCREEN_WALLPAPER, > LOCK_SCREEN_CONTAINERS, and LOCK_SCREEN_RELATED_CONTAINERS", assuming that > that's what it does Done. http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller.h:151: static int GetAllScreenLockContainersMask(); On 2012/09/04 16:40:18, Daniel Erat wrote: > nit: s/ScreenLock/LockScreen/, so this is consistent with the mask names Done. http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... File ash/wm/power_button_controller_unittest.cc (right): http://codereview.chromium.org/10909008/diff/8/ash/wm/power_button_controller... ash/wm/power_button_controller_unittest.cc:120: // Previously we're checking that all containers group was animated which On 2012/09/04 16:40:18, Daniel Erat wrote: > nit: add a blank line before this > > s/all containers group/the all containers group/ Done.
LGTM. Thanks! http://codereview.chromium.org/10909008/diff/9006/ash/wm/power_button_control... File ash/wm/power_button_controller_unittest.cc (right): http://codereview.chromium.org/10909008/diff/9006/ash/wm/power_button_control... ash/wm/power_button_controller_unittest.cc:187: // We should re-hide the black black layer after waiting long enough for nit: s/black black/black/
http://codereview.chromium.org/10909008/diff/9006/ash/wm/power_button_control... File ash/wm/power_button_controller_unittest.cc (right): http://codereview.chromium.org/10909008/diff/9006/ash/wm/power_button_control... ash/wm/power_button_controller_unittest.cc:187: // We should re-hide the black black layer after waiting long enough for On 2012/09/04 17:42:23, Daniel Erat wrote: > nit: s/black black/black/ Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
