Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2141133002:
[ash-md] Animates overview shield in and out (Closed)
DescriptionInitializes overview shield with the same opacity as the shelf.
Completes shield opacity animation after overview closes.
This CL deals with the transition into and out of the overview
mode.
When the shelf is in SHELF_BACKGROUND_MAXIMIZED state the shield starts
black and animates to 70% black.
When the shelf is transparent or semi-transparent the shield starts
transparent and animates to 70% black.
When overview mode finishes an observer is installed on the shield
widget that deletes itself and the widget once the opacity animation
completes.
BUG=624443
Committed: https://crrev.com/35147c4bd5de5e684c3478ab9ce92cff74cb1b87
Cr-Commit-Position: refs/heads/master@{#406122}
Patch Set 1 #Patch Set 2 : [ash-md] Completes shield opacity animation after overview closes #
Total comments: 2
Patch Set 3 : [ash-md] Completes shield opacity animation after overview closes (fixed non-MD case) #Patch Set 4 : [ash-md] Completes shield opacity animation after overview closes (nits) #Patch Set 5 : [ash-md] Completes shield opacity animation after overview closes (avoids crashes with delayed anim… #
Total comments: 14
Patch Set 6 : [ash-md] Completes shield opacity animation after overview closes (comments) #Patch Set 7 : [ash-md] Completes shield opacity animation after overview closes (unit test) #Patch Set 8 : [ash-md] Completes shield opacity animation after overview closes (rebased) #
Total comments: 6
Patch Set 9 : [ash-md] Completes shield opacity animation after overview closes (nits) #Messages
Total messages: 63 (48 generated)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
varkha@chromium.org changed reviewers: + bruthig@chromium.org
bruthig@, can you please take a look? Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ash-md] Initializes overview shield with the same opacity as the shelf This CL deals with the transition into overview mode. Transition out of the overview mode will need to be handled separately. When the shelf is in SHELF_BACKGROUND_MAXIMIZED state the shield starts black and animates to 70% black. When the shelf is transparent or semi-transparent the shield starts transparent and animates to 70% black. BUG=624443 ========== to ========== Initializes overview shield with the same opacity as the shelf. Completes shield opacity animation after overview closes. This CL deals with the transition into and out of the overview mode. When the shelf is in SHELF_BACKGROUND_MAXIMIZED state the shield starts black and animates to 70% black. When the shelf is transparent or semi-transparent the shield starts transparent and animates to 70% black. When overview mode finishes an observer is installed on the shield widget that deletes itself and the widget once the opacity animation completes. BUG=624443 ==========
PS2 deals with both fading in and fading out - seemed simple enough thanks to CleanupWidgetAfterAnimationObserver class that was already there.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2141133002/diff/20001/ash/common/wm/overview/... File ash/common/wm/overview/window_grid.cc (right): https://codereview.chromium.org/2141133002/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/window_grid.cc:820: ->GetBackgroundType() == SHELF_BACKGROUND_MAXIMIZED) nit: This is currently based on some assumptions in the shelf implementation, e.g. the shelf is black in MAXIMIZED mode. This assumption will change when we colour the shelf based on the wallpaper. I think we can defer a solution that uses the same source colour until later but I think this warrants a TODO breadcrumb somewhere in this file.
Thanks! https://codereview.chromium.org/2141133002/diff/20001/ash/common/wm/overview/... File ash/common/wm/overview/window_grid.cc (right): https://codereview.chromium.org/2141133002/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/window_grid.cc:820: ->GetBackgroundType() == SHELF_BACKGROUND_MAXIMIZED) On 2016/07/12 20:43:37, bruthig wrote: > nit: This is currently based on some assumptions in the shelf implementation, > e.g. the shelf is black in MAXIMIZED mode. This assumption will change when we > colour the shelf based on the wallpaper. I think we can defer a solution that > uses the same source colour until later but I think this warrants a TODO > breadcrumb somewhere in this file. Done.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2141133002/#ps60001 (title: "[ash-md] Completes shield opacity animation after overview closes (nits)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bruthig@, I feel that this would benefit from another review pass over the changes in the last patch set. I had to improvise a back and forth communication with the animation observer to make it work both (1) when the animation has time to complete and then deletes the widget from completion callback and (2) when the UI shell is shut down while the animations are still in progress. One scenario like this was caught by the tests and can be reproduced by changing animation duration and exiting the Ui shell. Ironically with aura::Window using set_owned_by_parent(false) on the selector and shield widgets would have been sufficient but this violated the carefully refactored new ash common code that no longer can use aura directly.
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... File ash/common/wm/overview/window_grid.cc (right): https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_grid.cc:55: class CleanupWidgetAfterAnimationObserver Although this is a pretty small class there certainly is a lot of complexity to it. It might be worthwhile pulling it out into it's own .h and .cc files so that you can add some tests around it. The tests would definitely make it easier to understand. https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_grid.cc:83: void CleanupWidgetAfterAnimationObserver::OnImplicitAnimationsCompleted() { I was looking in to whether or not OnImplicitAnimationsCompleted() is guaranteed to be called and I think there are some rare cases where it might not be. If you look at the ANIMATED_PROPERTY macro here https://cs.chromium.org/chromium/src/ui/compositor/layer_animator.cc?rcl=1468... you will see if the duration is zero and some other conditions are true, then the property value is set directly on the delegate() and no animation is started. In which case I believe OnImplicitAnimationsCompleted() will never be called. This is clearly not something that should be addressed in this CL, but it makes me wonder how much risk there is here. What are your thoughts? https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_grid.cc:84: if (!widget_) Is it accurate that this check will only be true during/after Shutdown() has been called? If so a comment would be helpful. It might be worth adding a DCHECK(widget) in the constructor too. https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... File ash/common/wm/overview/window_selector_controller.cc (right): https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_selector_controller.cc:95: void WindowSelectorController::RemoveDelayedAnimationObserver( This is only implemented correctly if called from within the CleanupWidgetAfterAnimationObserver::OnImplicitAnimationsCompleted() function. If |animation_observer| is still active and this is called before OnImplicitAnimationsCompleted() is, then I believe some bad things may happen. e.g. 1. the |animation_observer| is destroyed 2. |animation_observer->widget_| is destroyed 3. |animation_observer|->OnImplicitAnimationsCompleted() will be called 4. the |animation_observer| will NOT be removed from the WindowSelectorDelegate because |widget_| is null. 5. When the WindowSelectorDelegate is destroyed it will try to double delete the |animation_observer| Perhaps we should support this use case, detect it somehow with a DCHECK(), or add a comment. https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... File ash/common/wm/overview/window_selector_controller.h (right): https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_selector_controller.h:49: void RemoveDelayedAnimationObserver( This might be better named as RemoveAndDestroyAnimationObserver(). https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... File ash/common/wm/overview/window_selector_delegate.h (right): https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_selector_delegate.h:23: // Can be called by the |delegate| to delete the owned widget. Is it true that if the owner of |this| calls shutdown that they are then responsible for deleting the instance? It appears to be a contract of the API and is worthy of documentation. https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_selector_delegate.h:34: virtual void AddDelayedAnimationObserver( Instead of overloading the WindowSelectorDelegate, would it make more sense to introduce a DelayedAnimationObserverOwner interface instead. IMO this would certainly make the types and variable names in CleanupWidgetAfterAnimationObserver a lot more understandable. Or maybe even just renaming CleanupWidgetAfterAnimationObserver::SetDelegate() to SetOwner() might make it a lot more understandable.
https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... File ash/common/wm/overview/window_grid.cc (right): https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_grid.cc:55: class CleanupWidgetAfterAnimationObserver On 2016/07/15 17:05:38, bruthig wrote: > Although this is a pretty small class there certainly is a lot of complexity to > it. It might be worthwhile pulling it out into it's own .h and .cc files so > that you can add some tests around it. The tests would definitely make it > easier to understand. Done. See if you like this better while I am putting a test together. https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_grid.cc:83: void CleanupWidgetAfterAnimationObserver::OnImplicitAnimationsCompleted() { On 2016/07/15 17:05:38, bruthig wrote: > I was looking in to whether or not OnImplicitAnimationsCompleted() is guaranteed > to be called and I think there are some rare cases where it might not be. If > you look at the ANIMATED_PROPERTY macro here > https://cs.chromium.org/chromium/src/ui/compositor/layer_animator.cc?rcl=1468... > you will see if the duration is zero and some other conditions are true, then > the property value is set directly on the delegate() and no animation is > started. In which case I believe OnImplicitAnimationsCompleted() will never be > called. > > This is clearly not something that should be addressed in this CL, but it makes > me wonder how much risk there is here. What are your thoughts? Animations here are non-zero duration so this is probably not a concern but worth commenting. I can see if there is an easy way to detect this and not pass the ownership but we use this pattern in a few places so I am not overly concerned. https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_grid.cc:84: if (!widget_) On 2016/07/15 17:05:38, bruthig wrote: > Is it accurate that this check will only be true during/after Shutdown() has > been called? If so a comment would be helpful. > > It might be worth adding a DCHECK(widget) in the constructor too. Done. https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... File ash/common/wm/overview/window_selector_controller.cc (right): https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_selector_controller.cc:95: void WindowSelectorController::RemoveDelayedAnimationObserver( On 2016/07/15 17:05:38, bruthig wrote: > This is only implemented correctly if called from within the > CleanupWidgetAfterAnimationObserver::OnImplicitAnimationsCompleted() function. > If |animation_observer| is still active and this is called before > OnImplicitAnimationsCompleted() is, then I believe some bad things may happen. > > e.g. > 1. the |animation_observer| is destroyed > 2. |animation_observer->widget_| is destroyed > 3. |animation_observer|->OnImplicitAnimationsCompleted() will be called > 4. the |animation_observer| will NOT be removed from the WindowSelectorDelegate > because |widget_| is null. > 5. When the WindowSelectorDelegate is destroyed it will try to double delete the > |animation_observer| > > Perhaps we should support this use case, detect it somehow with a DCHECK(), or > add a comment. Done (Added some comment in the RemoveDelayedAnimationObserver doc). https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... File ash/common/wm/overview/window_selector_controller.h (right): https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_selector_controller.h:49: void RemoveDelayedAnimationObserver( On 2016/07/15 17:05:38, bruthig wrote: > This might be better named as RemoveAndDestroyAnimationObserver(). Done. https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... File ash/common/wm/overview/window_selector_delegate.h (right): https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_selector_delegate.h:23: // Can be called by the |delegate| to delete the owned widget. On 2016/07/15 17:05:38, bruthig wrote: > Is it true that if the owner of |this| calls shutdown that they are then > responsible for deleting the instance? It appears to be a contract of the API > and is worthy of documentation. Done. https://codereview.chromium.org/2141133002/diff/140001/ash/common/wm/overview... ash/common/wm/overview/window_selector_delegate.h:34: virtual void AddDelayedAnimationObserver( On 2016/07/15 17:05:38, bruthig wrote: > Instead of overloading the WindowSelectorDelegate, would it make more sense to > introduce a DelayedAnimationObserverOwner interface instead. IMO this would > certainly make the types and variable names in > CleanupWidgetAfterAnimationObserver a lot more understandable. > > Or maybe even just renaming CleanupWidgetAfterAnimationObserver::SetDelegate() > to SetOwner() might make it a lot more understandable. I like the last suggestion. Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:220001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some nits otherwise, lgtm https://codereview.chromium.org/2141133002/diff/240001/ash/common/wm/overview... File ash/common/wm/overview/cleanup_animation_observer_unittest.cc (right): https://codereview.chromium.org/2141133002/diff/240001/ash/common/wm/overview... ash/common/wm/overview/cleanup_animation_observer_unittest.cc:45: class IsEqual { It would be nice if we didn't duplicate this logic but I'm not sure of a good way to address that... https://codereview.chromium.org/2141133002/diff/240001/ash/common/wm/overview... ash/common/wm/overview/cleanup_animation_observer_unittest.cc:101: widget_destroyed_ = true; Any reason you are not setting |widget_| = nullptr here? If so a comment might be helpful. https://codereview.chromium.org/2141133002/diff/240001/ash/common/wm/overview... ash/common/wm/overview/cleanup_animation_observer_unittest.cc:154: // have crashed of not for TestWindowSelectorDelegate calling Shutdown() on sp: of -> if
https://codereview.chromium.org/2141133002/diff/240001/ash/common/wm/overview... File ash/common/wm/overview/cleanup_animation_observer_unittest.cc (right): https://codereview.chromium.org/2141133002/diff/240001/ash/common/wm/overview... ash/common/wm/overview/cleanup_animation_observer_unittest.cc:45: class IsEqual { On 2016/07/18 18:49:09, bruthig wrote: > It would be nice if we didn't duplicate this logic but I'm not sure of a good > way to address that... Yeah, templates are frowned upon in client code as far as I can tell and I didn't want to expose something like that too wide. https://codereview.chromium.org/2141133002/diff/240001/ash/common/wm/overview... ash/common/wm/overview/cleanup_animation_observer_unittest.cc:101: widget_destroyed_ = true; On 2016/07/18 18:49:09, bruthig wrote: > Any reason you are not setting |widget_| = nullptr here? If so a comment might > be helpful. Done. https://codereview.chromium.org/2141133002/diff/240001/ash/common/wm/overview... ash/common/wm/overview/cleanup_animation_observer_unittest.cc:154: // have crashed of not for TestWindowSelectorDelegate calling Shutdown() on On 2016/07/18 18:49:09, bruthig wrote: > sp: of -> if Done.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2141133002/#ps260001 (title: "[ash-md] Completes shield opacity animation after overview closes (nits)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Initializes overview shield with the same opacity as the shelf. Completes shield opacity animation after overview closes. This CL deals with the transition into and out of the overview mode. When the shelf is in SHELF_BACKGROUND_MAXIMIZED state the shield starts black and animates to 70% black. When the shelf is transparent or semi-transparent the shield starts transparent and animates to 70% black. When overview mode finishes an observer is installed on the shield widget that deletes itself and the widget once the opacity animation completes. BUG=624443 ========== to ========== Initializes overview shield with the same opacity as the shelf. Completes shield opacity animation after overview closes. This CL deals with the transition into and out of the overview mode. When the shelf is in SHELF_BACKGROUND_MAXIMIZED state the shield starts black and animates to 70% black. When the shelf is transparent or semi-transparent the shield starts transparent and animates to 70% black. When overview mode finishes an observer is installed on the shield widget that deletes itself and the widget once the opacity animation completes. BUG=624443 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Initializes overview shield with the same opacity as the shelf. Completes shield opacity animation after overview closes. This CL deals with the transition into and out of the overview mode. When the shelf is in SHELF_BACKGROUND_MAXIMIZED state the shield starts black and animates to 70% black. When the shelf is transparent or semi-transparent the shield starts transparent and animates to 70% black. When overview mode finishes an observer is installed on the shield widget that deletes itself and the widget once the opacity animation completes. BUG=624443 ========== to ========== Initializes overview shield with the same opacity as the shelf. Completes shield opacity animation after overview closes. This CL deals with the transition into and out of the overview mode. When the shelf is in SHELF_BACKGROUND_MAXIMIZED state the shield starts black and animates to 70% black. When the shelf is transparent or semi-transparent the shield starts transparent and animates to 70% black. When overview mode finishes an observer is installed on the shield widget that deletes itself and the widget once the opacity animation completes. BUG=624443 Committed: https://crrev.com/35147c4bd5de5e684c3478ab9ce92cff74cb1b87 Cr-Commit-Position: refs/heads/master@{#406122} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/35147c4bd5de5e684c3478ab9ce92cff74cb1b87 Cr-Commit-Position: refs/heads/master@{#406122} |
