Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9348089:
Make power button controller restore to original transformation on the layer. (Closed)
|
Created:
8 years, 10 months ago by alicet1 Modified:
8 years, 10 months ago Reviewers:
Daniel Erat CC:
chromium-reviews, dhollowa+watch_chromium.org, sadrul, ben+watch_chromium.org Visibility:
Public. |
DescriptionIf there are existing transformation on the layer before screen locking, we should restore it after screen lock is dismissed.
BUG=113756
TEST=verified on alex.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122307
Patch Set 1 #Patch Set 2 : udpate. #
Total comments: 42
Patch Set 3 : update #
Total comments: 4
Patch Set 4 : update #
Messages
Total messages: 8 (0 generated)
hi dan, I'm not sure if this needs fixing if ian's change http://codereview.chromium.org/9388018/ goes in (no resize of default container layer). As it is now on ToT though, compact mode will need this fix to make sure all transformation on the layer is restored after screen lock. thanx, alice
What happens if you press Alt-Tab just after you start holding the power button? http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... File ash/wm/power_button_controller.cc (right): http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:99: // base transformation containers for slow-close animation. this comment doesn't make sense to me. something like "Returns the transform, based on |base_transform|, that should be applied to containers for the slow-close animation" would be better. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:110: // Returns the transform that should be applied to containers in addition to the ditto here http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:135: const ui::Transform& base_transform) { |base_transform| doesn't seem right here. |orig_transform|? http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:181: void RestoreWindow(aura::Window* window, const ui::Transform& transform) { s/transform/orig_transform/ http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:224: ash::WindowTransformsMap* window_transforms) { you don't need "ash::" here; this is already in the ash namespace http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:226: type != PowerButtonController::ANIMATION_RESTORE && hoist these checks out to StartAnimation(), so you just call StoreOriginalTransform() when appropriate. i'd remove the NULL check, or just use DCHECK(window_transforms) instead http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:234: ash::WindowTransformsMap* window_transforms) { make this method take a const ash::WindowTransformsMap& instead http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:249: ui::Transform transform; this doesn't look like it gets used anywhere... ? http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:311: if (layer->GetTargetTransform() != GetFastCloseTransform( improve the indenting here: if (layer->Get... != GetFastCloseTransform(...) || or something similar http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:326: layer->transform() != RetrieveOriginalTransform( ditto http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:386: ANIMATION_RESTORE, &window_transforms_); nit: one param per line unless they all fit on a single line (ditto for later function calls) http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:404: StartAnimation(SCREEN_LOCKER_CONTAINERS, ANIMATION_HIDE, NULL); it'd be simpler to always pass &window_transforms_ instead of passing NULL if the animation doesn't require it http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... File ash/wm/power_button_controller.h (right): http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:17: #include "ui/aura/window.h" forward-declare aura::Window instead of including the header http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:30: typedef std::map<aura::Window*, ui::Transform> WindowTransformsMap; move this inside of the class, in the private block http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:31: typedef WindowTransformsMap::const_iterator WindowTransformsConstIter; don't put this in the header; you don't use it there (or better, just delete it, since you only use it once) http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:129: void GetContainerWindows(ContainerGroup group, this name (and especially the comment) are misleading. GetContainersInGroup() instead? http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:248: // Map of window and their corresponding layer transform before animation. // Map from containers to their original layer transforms before we started animating them. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:249: WindowTransformsMap window_transforms_; rename to container_transforms_ http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... File ash/wm/power_button_controller_unittest.cc (right): http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller_unittest.cc:276: // Test that we restore transformation that are non empty. s/transformation/transformations/ http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller_unittest.cc:315: EXPECT_FALSE(expected_transform == (*it)->layer()->GetTargetTransform()); EXPECT_NE?
http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... File ash/wm/power_button_controller.cc (right): http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:244: void StartAnimation(PowerButtonController::ContainerGroup group, actually, a better approach would probably be to make StartAnimation(), StoreOriginalTransform(), and RetrieveOriginalTransform() be private methods of PowerButtonController. i originally made StartAnimation just be a function since it didn't need any state from the class, but that's no longer the case now that it needs to access the cached original transforms.
address the comments in code. I haven't checked on alt+tab while screenlocking yet, my guess is something will be screwed up, I'll try it on device. thanx, alice http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... File ash/wm/power_button_controller.cc (right): http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:99: // base transformation containers for slow-close animation. On 2012/02/15 05:50:03, Daniel Erat wrote: > this comment doesn't make sense to me. something like "Returns the transform, > based on |base_transform|, that should be applied to containers for the > slow-close animation" would be better. much better. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:110: // Returns the transform that should be applied to containers in addition to the On 2012/02/15 05:50:03, Daniel Erat wrote: > ditto here Done. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:135: const ui::Transform& base_transform) { On 2012/02/15 05:50:03, Daniel Erat wrote: > |base_transform| doesn't seem right here. |orig_transform|? Done. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:181: void RestoreWindow(aura::Window* window, const ui::Transform& transform) { On 2012/02/15 05:50:03, Daniel Erat wrote: > s/transform/orig_transform/ Done. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:224: ash::WindowTransformsMap* window_transforms) { On 2012/02/15 05:50:03, Daniel Erat wrote: > you don't need "ash::" here; this is already in the ash namespace removed. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:226: type != PowerButtonController::ANIMATION_RESTORE && On 2012/02/15 05:50:03, Daniel Erat wrote: > hoist these checks out to StartAnimation(), so you just call > StoreOriginalTransform() when appropriate. i'd remove the NULL check, or just > use DCHECK(window_transforms) instead removed. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:234: ash::WindowTransformsMap* window_transforms) { On 2012/02/15 05:50:03, Daniel Erat wrote: > make this method take a const ash::WindowTransformsMap& instead ah yes. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:244: void StartAnimation(PowerButtonController::ContainerGroup group, On 2012/02/15 06:07:51, Daniel Erat wrote: > actually, a better approach would probably be to make StartAnimation(), > StoreOriginalTransform(), and RetrieveOriginalTransform() be private methods of > PowerButtonController. i originally made StartAnimation just be a function > since it didn't need any state from the class, but that's no longer the case now > that it needs to access the cached original transforms. done. left RetrieveOriginalTransform in default namespace as it is only a helper. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:249: ui::Transform transform; On 2012/02/15 05:50:03, Daniel Erat wrote: > this doesn't look like it gets used anywhere... ? removed. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:311: if (layer->GetTargetTransform() != GetFastCloseTransform( On 2012/02/15 05:50:03, Daniel Erat wrote: > improve the indenting here: > > if (layer->Get... != > GetFastCloseTransform(...) || > > or something similar Done. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:326: layer->transform() != RetrieveOriginalTransform( On 2012/02/15 05:50:03, Daniel Erat wrote: > ditto Done. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:386: ANIMATION_RESTORE, &window_transforms_); On 2012/02/15 05:50:03, Daniel Erat wrote: > nit: one param per line unless they all fit on a single line (ditto for later > function calls) removed. altho, style guide said when calling a function, collapsing args maybe ok... http://dev.chromium.org/developers/coding-style#TOC-Code-formatting http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.cc:404: StartAnimation(SCREEN_LOCKER_CONTAINERS, ANIMATION_HIDE, NULL); On 2012/02/15 05:50:03, Daniel Erat wrote: > it'd be simpler to always pass &window_transforms_ instead of passing NULL if > the animation doesn't require it removed. since startanimation is now part of the class. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... File ash/wm/power_button_controller.h (right): http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:17: #include "ui/aura/window.h" On 2012/02/15 05:50:03, Daniel Erat wrote: > forward-declare aura::Window instead of including the header I lifted GetContainers up to class, and so it needs aura::Window::Windows from the header also. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:30: typedef std::map<aura::Window*, ui::Transform> WindowTransformsMap; On 2012/02/15 05:50:03, Daniel Erat wrote: > move this inside of the class, in the private block Done. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:31: typedef WindowTransformsMap::const_iterator WindowTransformsConstIter; On 2012/02/15 05:50:03, Daniel Erat wrote: > don't put this in the header; you don't use it there (or better, just delete it, > since you only use it once) Done. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:129: void GetContainerWindows(ContainerGroup group, On 2012/02/15 05:50:03, Daniel Erat wrote: > this name (and especially the comment) are misleading. GetContainersInGroup() > instead? removed. I moved GetContainers to public, either that or make unittest class be a friend class of power button controller if we ant to keep it private. I am fine either ways, this added method doesn't do more than recall the real GetContainers(), so I'd rather not have it. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:248: // Map of window and their corresponding layer transform before animation. On 2012/02/15 05:50:03, Daniel Erat wrote: > // Map from containers to their original layer transforms before we started > animating them. Done. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller.h:249: WindowTransformsMap window_transforms_; On 2012/02/15 05:50:03, Daniel Erat wrote: > rename to container_transforms_ Done. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... File ash/wm/power_button_controller_unittest.cc (right): http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller_unittest.cc:276: // Test that we restore transformation that are non empty. On 2012/02/15 05:50:03, Daniel Erat wrote: > s/transformation/transformations/ Done. http://codereview.chromium.org/9348089/diff/2003/ash/wm/power_button_controll... ash/wm/power_button_controller_unittest.cc:315: EXPECT_FALSE(expected_transform == (*it)->layer()->GetTargetTransform()); On 2012/02/15 05:50:03, Daniel Erat wrote: > EXPECT_NE? Done.
LGTM with some tiny nits. Thanks! http://codereview.chromium.org/9348089/diff/13001/ash/wm/power_button_control... File ash/wm/power_button_controller.cc (left): http://codereview.chromium.org/9348089/diff/13001/ash/wm/power_button_control... ash/wm/power_button_controller.cc:463: LOG(ERROR) << "Screen lock request timed out"; nit: mind keeping this in there? we sadly seem to still hit this case frequently the first time i try to lock the screen (maybe it should be a warning instead) http://codereview.chromium.org/9348089/diff/13001/ash/wm/power_button_control... File ash/wm/power_button_controller.cc (right): http://codereview.chromium.org/9348089/diff/13001/ash/wm/power_button_control... ash/wm/power_button_controller.cc:513: if (type != ANIMATION_RESTORE && type != ANIMATION_UNDO_SLOW_CLOSE) { nit: delete space before '&&' don't need curly braces for single-line statement
tested on alex for "What happens if you press Alt-Tab just after you start holding the power button?" seems to be acting like we missed the alt-tab all together, window lock animation comes in, and unlocks to the same tab. I also tried holding down on alt-tab, see the windows flying, and then press power button to screen lock. unlocking reveals a window, I couldn't recognize which was the last visible one before locking. seems to be working. thanx, alice http://codereview.chromium.org/9348089/diff/13001/ash/wm/power_button_control... File ash/wm/power_button_controller.cc (left): http://codereview.chromium.org/9348089/diff/13001/ash/wm/power_button_control... ash/wm/power_button_controller.cc:463: LOG(ERROR) << "Screen lock request timed out"; On 2012/02/15 20:57:53, Daniel Erat wrote: > nit: mind keeping this in there? we sadly seem to still hit this case > frequently the first time i try to lock the screen (maybe it should be a warning > instead) oops, reverted. I thought it was one of my debugging. http://codereview.chromium.org/9348089/diff/13001/ash/wm/power_button_control... File ash/wm/power_button_controller.cc (right): http://codereview.chromium.org/9348089/diff/13001/ash/wm/power_button_control... ash/wm/power_button_controller.cc:513: if (type != ANIMATION_RESTORE && type != ANIMATION_UNDO_SLOW_CLOSE) { On 2012/02/15 20:57:53, Daniel Erat wrote: > nit: delete space before '&&' > > don't need curly braces for single-line statement Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alicet@chromium.org/9348089/10004
Change committed as 122307 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
