Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
180273025:
Keep dedicated layers for hiding animation (Closed)
|
Created:
6 years, 9 months ago by oshima Modified:
6 years, 9 months ago CC:
chromium-reviews, tfarina, ben+corewm_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionKeep dedicated layers for hiding animation
Introduced ScopedHidingAnimationSettings to set hiding animations
Remove the restacking code from FocusController, and other obsolete tests.
BUG=315843
TEST=WindowAnimationTest.HideAnimationDetachLayers, WindowAnimationTest.HideAnimationDetachLayersWithTransientChild
R=sky@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258220
Patch Set 1 : #Patch Set 2 : fix OnWnidowDestroying #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Total comments: 17
Patch Set 5 : #
Total comments: 17
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 11
Patch Set 10 : use new RecreateLayers + LayerTreeOwner #Patch Set 11 : #
Total comments: 5
Patch Set 12 : #Patch Set 13 : ScopedHidingAnimationSettings #
Total comments: 9
Patch Set 14 : updated comments #Patch Set 15 : #
Total comments: 1
Patch Set 16 : #Patch Set 17 : #
Total comments: 14
Patch Set 18 : #Patch Set 19 : #Messages
Total messages: 58 (0 generated)
I'll add test if you're ok with this approach. please let me know.
On 2014/03/04 23:00:17, oshima wrote: > I'll add test if you're ok with this approach. please let me know. Hold on this patch has a bug.
uploaded new patch. PTAL.
I'm still not clear on why we need this. I get that the order is out of sync, but I'm wondering if we can keep doing what we're doing and reorder when the animation is done, or make the code that does the stacking treat hiding windows similar to windows with null delegates. Can you write a test for the case that is causing problems now? Also, I was planning on moving views::corewm::RecreateWindowLayers to ash. Primarily as ui/views/corewm shouldn't depend upon the rest of views. Your change would mean I can't do that. Ben is also looking at similar changes to HidingWindowObserver, so I'm adding him too.
Ben's change is here: https://codereview.chromium.org/184983005/ . It looks like it won't overlap with what you're doing. https://codereview.chromium.org/180273025/diff/140001/ui/views/corewm/focus_c... File ui/views/corewm/focus_controller.cc (right): https://codereview.chromium.org/180273025/diff/140001/ui/views/corewm/focus_c... ui/views/corewm/focus_controller.cc:222: StackWindowLayerAbove(window, next_window); Is it possible to remove this code entirely and instead change the hiding code to acquire the layers immediately for the animation?
Another note... sky you were going to move RecreateWindowLayers back to ash... but won't calling it here prevent you from doing that? On Wed, Mar 5, 2014 at 7:49 AM, <sky@chromium.org> wrote: > Ben's change is here: https://codereview.chromium.org/184983005/ . It > looks like > it won't overlap with what you're doing. > > > https://codereview.chromium.org/180273025/diff/140001/ui/ > views/corewm/focus_controller.cc > File ui/views/corewm/focus_controller.cc (right): > > https://codereview.chromium.org/180273025/diff/140001/ui/ > views/corewm/focus_controller.cc#newcode222 > ui/views/corewm/focus_controller.cc:222: StackWindowLayerAbove(window, > next_window); > Is it possible to remove this code entirely and instead change the > hiding code to acquire the layers immediately for the animation? > > https://codereview.chromium.org/180273025/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Indeed. If we really need RecreateWindowLayers here then I'll need to add a new method to LayerDelegate. -Scott On Wed, Mar 5, 2014 at 8:12 AM, Ben Goodger (Google) <ben@chromium.org> wrote: > Another note... sky you were going to move RecreateWindowLayers back to > ash... but won't calling it here prevent you from doing that? > > > On Wed, Mar 5, 2014 at 7:49 AM, <sky@chromium.org> wrote: >> >> Ben's change is here: https://codereview.chromium.org/184983005/ . It >> looks like >> it won't overlap with what you're doing. >> >> >> >> https://codereview.chromium.org/180273025/diff/140001/ui/views/corewm/focus_c... >> File ui/views/corewm/focus_controller.cc (right): >> >> >> https://codereview.chromium.org/180273025/diff/140001/ui/views/corewm/focus_c... >> ui/views/corewm/focus_controller.cc:222: StackWindowLayerAbove(window, >> next_window); >> Is it possible to remove this code entirely and instead change the >> hiding code to acquire the layers immediately for the animation? >> >> https://codereview.chromium.org/180273025/ > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/180273025/diff/140001/ui/views/corewm/focus_c... File ui/views/corewm/focus_controller.cc (right): https://codereview.chromium.org/180273025/diff/140001/ui/views/corewm/focus_c... ui/views/corewm/focus_controller.cc:222: StackWindowLayerAbove(window, next_window); On 2014/03/05 15:49:05, sky wrote: > Is it possible to remove this code entirely and instead change the hiding code > to acquire the layers immediately for the animation? Only down side is that it will always bring the window's layer to front even if the window wasn't active (in which case, we may not care). Scott, we talked about if this is really necessary for desktop aura. If desktop_aura do not need this behavior, I can move this logic to ash. (I can do this if the window has hide animation observer when the window is deactivated). WDYT?
On Wed, Mar 5, 2014 at 9:14 AM, <oshima@chromium.org> wrote: > > https://codereview.chromium.org/180273025/diff/140001/ui/views/corewm/focus_c... > File ui/views/corewm/focus_controller.cc (right): > > https://codereview.chromium.org/180273025/diff/140001/ui/views/corewm/focus_c... > ui/views/corewm/focus_controller.cc:222: StackWindowLayerAbove(window, > next_window); > On 2014/03/05 15:49:05, sky wrote: >> >> Is it possible to remove this code entirely and instead change the > > hiding code >> >> to acquire the layers immediately for the animation? > > > Only down side is that it will always bring the window's layer to front > even if the window wasn't active (in which case, we may not care). I'm not sure of the exact sequence here. I believe it invokes: 1. hiding/minimizing window 2. animating window hidden 3. moving newly activated window to top of stack As long as 3 is last, then by virtue of the acquired windows having NULL delegates then the newly activated window won't get stacked on the top. In other words, if we can do it in this order then there should then there should be no reason to StackWindowLayerAbove in FocusController. > > Scott, we talked about if this is really necessary for desktop aura. If > desktop_aura do not need this behavior, I can move this logic to ash. (I > can do this if the window has hide animation observer when the window is > deactivated). As I mentioned, this code may be hit in some situations, perhaps classic. I think it should stay here. -Scott > > WDYT? > > https://codereview.chromium.org/180273025/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/05 17:34:21, sky wrote: > On Wed, Mar 5, 2014 at 9:14 AM, <mailto:oshima@chromium.org> wrote: > > > > > https://codereview.chromium.org/180273025/diff/140001/ui/views/corewm/focus_c... > > File ui/views/corewm/focus_controller.cc (right): > > > > > https://codereview.chromium.org/180273025/diff/140001/ui/views/corewm/focus_c... > > ui/views/corewm/focus_controller.cc:222: StackWindowLayerAbove(window, > > next_window); > > On 2014/03/05 15:49:05, sky wrote: > >> > >> Is it possible to remove this code entirely and instead change the > > > > hiding code > >> > >> to acquire the layers immediately for the animation? > > > > > > Only down side is that it will always bring the window's layer to front > > even if the window wasn't active (in which case, we may not care). > > I'm not sure of the exact sequence here. I believe it invokes: > 1. hiding/minimizing window > 2. animating window hidden > 3. moving newly activated window to top of stack > > As long as 3 is last, then by virtue of the acquired windows having > NULL delegates then the newly activated window won't get stacked on > the top. In other words, if we can do it in this order then there > should then there should be no reason to StackWindowLayerAbove in > FocusController. What I was worried was the scenario without 3 (just hiding inactive window), in which case, the layer should stay where it is. But I think we can avoid that scenario by doing this only if the window being hidden is active or has focus. Do you think this will work? > > > > > Scott, we talked about if this is really necessary for desktop aura. If > > desktop_aura do not need this behavior, I can move this logic to ash. (I > > can do this if the window has hide animation observer when the window is > > deactivated). > > As I mentioned, this code may be hit in some situations, perhaps > classic. I think it should stay here. > > -Scott > > > > > WDYT? > > > > https://codereview.chromium.org/180273025/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
uploaded the new patch which Recreate layer in window_animations.cc Please let me know what you think.
Is there a reason to look at focus? That is, can we do this all the time?
changed as discussed offline. Added tests. PTAL.
https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... File ui/views/corewm/window_animations.cc (right): https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:128: window_->AddObserver(this); This code previously cared about the window so that if the window was desctroyed it would grab the layers. Since you now grab the layers right away it seems like it shouldn't need to be a WindowObserver nor have the window_ member. The only thing I'm not sure about is 175-178 (new). I honestly don't understand that code. Is it needed? https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:135: layer_.reset(views::corewm::RecreateWindowLayers(window_, true)); nit: don't need views::corem here as you're in that namespace already. https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:135: layer_.reset(views::corewm::RecreateWindowLayers(window_, true)); Layer does not own its children. So, the way you have it now you leak all the descendants. https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:138: const aura::Window* const_window = window_; If you use the function in window_util it'll do the right thing and you don't need the cast. https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:141: // If the window has transient children, move above the top most Document why you need to do this. https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:197: class ScopedLayerDetacher { Description?
https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... File ui/views/corewm/window_animations.cc (right): https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:135: layer_.reset(views::corewm::RecreateWindowLayers(window_, true)); On 2014/03/07 15:14:54, sky wrote: > Layer does not own its children. So, the way you have it now you leak all the > descendants. Yeah RecreateWindowLayers seems like a weird API. How can it not leak all the descendents (and view layers too). Also this would reintroduce the bug that my last CL here fixed. Seems like RecreateWindowLayers should become RecreateLayers() and do a proper deep copy of the layer tree, returning ownership of the original tree to the caller. https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:165: // Make the new layer unvisible immediately. invisible https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:569: return new HidingWindowAnimationObserver(window); do you run into the same problem when this method is called?
https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... File ui/views/corewm/window_animations.cc (right): https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:135: layer_.reset(views::corewm::RecreateWindowLayers(window_, true)); On 2014/03/07 15:51:54, Ben Goodger (Google) wrote: > On 2014/03/07 15:14:54, sky wrote: > > Layer does not own its children. So, the way you have it now you leak all the > > descendants. > > Yeah RecreateWindowLayers seems like a weird API. How can it not leak all the > descendents (and view layers too). > > Also this would reintroduce the bug that my last CL here fixed. Seems like > RecreateWindowLayers should become RecreateLayers() and do a proper deep copy of > the layer tree, returning ownership of the original tree to the caller. I'm going to change the return value of RecreateWindowLayers now as part of another patch I'm working on.
It does look like everywhere else that uses RecreateWindowLayers remembers to call DeepDeleteLayers so there are no leaks currently. -Ben On Fri, Mar 7, 2014 at 8:34 AM, <sky@chromium.org> wrote: > > https://codereview.chromium.org/180273025/diff/280001/ui/ > views/corewm/window_animations.cc > File ui/views/corewm/window_animations.cc (right): > > https://codereview.chromium.org/180273025/diff/280001/ui/ > views/corewm/window_animations.cc#newcode135 > ui/views/corewm/window_animations.cc:135: > layer_.reset(views::corewm::RecreateWindowLayers(window_, true)); > On 2014/03/07 15:51:54, Ben Goodger (Google) wrote: > >> On 2014/03/07 15:14:54, sky wrote: >> > Layer does not own its children. So, the way you have it now you >> > leak all the > >> > descendants. >> > > Yeah RecreateWindowLayers seems like a weird API. How can it not leak >> > all the > >> descendents (and view layers too). >> > > Also this would reintroduce the bug that my last CL here fixed. Seems >> > like > >> RecreateWindowLayers should become RecreateLayers() and do a proper >> > deep copy of > >> the layer tree, returning ownership of the original tree to the >> > caller. > > I'm going to change the return value of RecreateWindowLayers now as part > of another patch I'm working on. > > https://codereview.chromium.org/180273025/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'll update the RecreateLayer once sky's change is landed but the rest is ready. PTAL. https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... File ui/views/corewm/window_animations.cc (right): https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:128: window_->AddObserver(this); On 2014/03/07 15:14:54, sky wrote: > This code previously cared about the window so that if the window was desctroyed > it would grab the layers. Since you now grab the layers right away it seems like > it shouldn't need to be a WindowObserver nor have the window_ member. The only > thing I'm not sure about is 175-178 (new). I honestly don't understand that > code. Is it needed? I tried that but it broke the test due to the AnimationHost not being called at the end, and this seems to be used by desktop aura. (It's added here https://chromiumcodereview.appspot.com/12342028) https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:135: layer_.reset(views::corewm::RecreateWindowLayers(window_, true)); On 2014/03/07 15:51:54, Ben Goodger (Google) wrote: > On 2014/03/07 15:14:54, sky wrote: > > Layer does not own its children. So, the way you have it now you leak all the > > descendants. > > Yeah RecreateWindowLayers seems like a weird API. How can it not leak all the > descendents (and view layers too). > > Also this would reintroduce the bug that my last CL here fixed. Seems like > RecreateWindowLayers should become RecreateLayers() and do a proper deep copy of > the layer tree, returning ownership of the original tree to the caller. Fixed to use DeepDeleteLayers for now. I'll update when sky's change is in. https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:138: const aura::Window* const_window = window_; On 2014/03/07 15:14:54, sky wrote: > If you use the function in window_util it'll do the right thing and you don't > need the cast. Ah, I didn't see that. Fixed https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:141: // If the window has transient children, move above the top most On 2014/03/07 15:14:54, sky wrote: > Document why you need to do this. Done. https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:165: // Make the new layer unvisible immediately. On 2014/03/07 15:51:54, Ben Goodger (Google) wrote: > invisible Done. https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:197: class ScopedLayerDetacher { On 2014/03/07 15:14:54, sky wrote: > Description? Moved to header and added comment. https://codereview.chromium.org/180273025/diff/280001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:569: return new HidingWindowAnimationObserver(window); On 2014/03/07 15:51:54, Ben Goodger (Google) wrote: > do you run into the same problem when this method is called? Thanks. I looked at usages and looks like I should support both scenarios (detach upon deletion, and detach & recreate when animation starts). I replaced this with two utility functions DetachAndRecreateLayersForHidingAnimation and DetachLayersForHidingAnimationWhenDestroyed. https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... File ui/views/corewm/window_animations.cc (left): https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:362: new HidingWindowAnimationObserver(window); This leaks the observer because it's not observing the animation. I'll address this in separate CL.
https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... File ui/views/corewm/window_animations.cc (right): https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:201: // hiding animation, creates new layers for the window and stack them stacks https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:203: // new active widnow will no hide the hiding animation. widnow->window no->not https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:210: : HidingWindowAnimationObserver(window, settings) { nit: spacing if off here. https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:230: DCHECK(iter != window_->parent()->children().end()); nit: move DCHECK up one line (since 229 doesn't effect it and and best to keep DCHECK as close as possible to what you're asserting). https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:566: class LayerDetacherForHidingAnimationImpl Add description. In fact all these classes here are rather confusing and it isn't clear why one variant is needed over the other. Please add better descriptions of what they are all for. https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... File ui/views/corewm/window_animations.h (right): https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:100: // This has to be called before applying animations to |settings| because Is there anyway to this has been called before applying animations? https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:101: // this will install animation observer. an animation observer https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:108: // associated with a |winddow| so that the hiding animation can continue 'a |winddow|' -> "window"
https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... File ui/views/corewm/window_animations.cc (right): https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:201: // hiding animation, creates new layers for the window and stack them On 2014/03/10 13:30:40, sky wrote: > stacks Done. https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:203: // new active widnow will no hide the hiding animation. On 2014/03/10 13:30:40, sky wrote: > widnow->window > no->not Done. https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:210: : HidingWindowAnimationObserver(window, settings) { On 2014/03/10 13:30:40, sky wrote: > nit: spacing if off here. Done. https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:230: DCHECK(iter != window_->parent()->children().end()); On 2014/03/10 13:30:40, sky wrote: > nit: move DCHECK up one line (since 229 doesn't effect it and and best to keep > DCHECK as close as possible to what you're asserting). Done. https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:566: class LayerDetacherForHidingAnimationImpl On 2014/03/10 13:30:40, sky wrote: > Add description. In fact all these classes here are rather confusing and it > isn't clear why one variant is needed over the other. Please add better > descriptions of what they are all for. The description is in header file. I added more description there. I added this to safely detach and recreate layers when applying animation in less error prone way. I wish I could simplify a bit more, and I'm happy to change if there is a better way. Alternatively, if we detach the layers when the focus changes (like before), the code would be simpler because we don't have to worry about the order of the applying animation and the adding observer. https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... File ui/views/corewm/window_animations.h (right): https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:100: // This has to be called before applying animations to |settings| because On 2014/03/10 13:30:40, sky wrote: > Is there anyway to this has been called before applying animations? Not sure if I understood your question correctly. If you change the properties on layers, it's too late to call this because the animation is already scheduled by the |settings| and you can't observer them. https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:101: // this will install animation observer. On 2014/03/10 13:30:40, sky wrote: > an animation observer Done. https://codereview.chromium.org/180273025/diff/350001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:108: // associated with a |winddow| so that the hiding animation can continue On 2014/03/10 13:30:40, sky wrote: > 'a |winddow|' -> "window" Done.
https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... File ui/views/corewm/window_animations.h (right): https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:89: // deteached layer's animator, and will be deleted when the animation deteached->detached https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:91: class LayerDetacherForHidingAnimation { Why bother with this effectively pure virtual class? Why not move LayerDetacherForHidingAnimationImpl here? If you say to avoid forward declaring DetachAndRecreateLayersAnimationObserver that seems like a weak argument. https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:100: // This is used to make sure the hiding animation is visible even if These two functions are nearly the same and it isn't quite clear (from the descriptions) as to when you would use one or the other. I think you want: The following two functions are used to animate a window hiding (this includes minimize and close). DetachAndRecreateLayersForHidingAnimation() is intended for use when the window is going to persist (hide or minimize) and DetachLayersForHidingAnimationWhenDestroyed() when the window is being destroyed. Both ensure the layers for the window are visible even if the window is destroyed. https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:102: // This should be called before applying animatinos to |settings| animatinos->animations https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:111: // associated with a winddow. This is used to make sure that the winddow->window
https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... File ui/views/corewm/window_animations.h (right): https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:89: // deteached layer's animator, and will be deleted when the animation On 2014/03/11 16:10:53, sky wrote: > deteached->detached Done. https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:91: class LayerDetacherForHidingAnimation { On 2014/03/11 16:10:53, sky wrote: > Why bother with this effectively pure virtual class? Why not move > LayerDetacherForHidingAnimationImpl here? If you say to avoid forward declaring > DetachAndRecreateLayersAnimationObserver that seems like a weak argument. It also requires the constructor which a user don't care, and I thought it's better to hide it rather than expose it. I don't feel strongly about it, so changed it. https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:100: // This is used to make sure the hiding animation is visible even if On 2014/03/11 16:10:53, sky wrote: > These two functions are nearly the same and it isn't quite clear (from the > descriptions) as to when you would use one or the other. I think you want: > > The following two functions are used to animate a window hiding (this includes > minimize and close). DetachAndRecreateLayersForHidingAnimation() is intended for > use when the window is going to persist (hide or minimize) and > DetachLayersForHidingAnimationWhenDestroyed() when the window is being > destroyed. Both ensure the layers for the window are visible even if the window > is destroyed. That's not correct. The former is to make sure the animation is visible after activation change, regardless of whether the window will be destroyed. The latter is the case where we simply want to make sure the animation will continue after deletion, and can become behind the active window. We discussed this over the chat. If you think we should use the former for the latter and will not cause a regression, I will do so. Please let me know. https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:102: // This should be called before applying animatinos to |settings| On 2014/03/11 16:10:53, sky wrote: > animatinos->animations Done. https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:111: // associated with a winddow. This is used to make sure that the On 2014/03/11 16:10:53, sky wrote: > winddow->window Done.
https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... File ui/views/corewm/window_animations.h (right): https://codereview.chromium.org/180273025/diff/440001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:100: // This is used to make sure the hiding animation is visible even if On 2014/03/11 17:45:20, oshima wrote: > On 2014/03/11 16:10:53, sky wrote: > > These two functions are nearly the same and it isn't quite clear (from the > > descriptions) as to when you would use one or the other. I think you want: > > > > The following two functions are used to animate a window hiding (this includes > > minimize and close). DetachAndRecreateLayersForHidingAnimation() is intended > for > > use when the window is going to persist (hide or minimize) and > > DetachLayersForHidingAnimationWhenDestroyed() when the window is being > > destroyed. Both ensure the layers for the window are visible even if the > window > > is destroyed. > > That's not correct. The former is to make sure the animation is visible after > activation change, regardless of whether the window will be destroyed. The > latter is the case where we simply want to make sure the animation will continue > after deletion, and can become behind the active window. > > We discussed this over the chat. If you think we should use the former for the > latter and will not cause a regression, I will do so. Please let me know. My main concern with what you have here (and in the latest patch) is that it is not clear which one I want to use in a particular situation. That needs to be made clearer. https://codereview.chromium.org/180273025/diff/520001/ui/views/corewm/window_... File ui/views/corewm/window_animations.cc (right): https://codereview.chromium.org/180273025/diff/520001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:196: std::vector<ui::Layer*> layers_; Used a ScopedVector here? https://codereview.chromium.org/180273025/diff/520001/ui/views/corewm/window_... ui/views/corewm/window_animations.cc:560: window_->layer()->SetOpacity(0); These two calls make me nervous. I think they should be done in Window::RecreateLayer, otherwise we risk the two being out of sync. Perhaps what should happen is the new layer (in RecreateLayer()) should setVisibile based on Window::visible_ and target opacity. https://codereview.chromium.org/180273025/diff/520001/ui/views/corewm/window_... File ui/views/corewm/window_animations.h (right): https://codereview.chromium.org/180273025/diff/520001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:109: // This should be called before applying animatinos to |settings| animatinos->animations https://codereview.chromium.org/180273025/diff/520001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:118: // associated with a winddow. This is used to make sure that the winddow->window https://codereview.chromium.org/180273025/diff/520001/ui/views/corewm/window_... ui/views/corewm/window_animations.h:120: // this will install an animation observer. this->THis
Changed the approach as discussed offline. This is not final review and tests needs to be updated as well. sky@, please take a look at the approach and let me know if this is ok with you.
With this approach it is totally non-obvious that the focus controller from shell is needed for the animations. I prefer the last patchset since the code is all consolidated and relatively straightforward to find.
On 2014/03/12 02:30:34, sky wrote: > With this approach it is totally non-obvious that the focus controller from > shell is needed for the animations. I prefer the last patchset since the code is > all consolidated and relatively straightforward to find. I think one can argue either way. For example, previous one is more error prone because there are two choices and it's up to the user whether or not it will work correctly. Another point is that previous one is not obvious how the animation is made visible after activation change. You need to know how the activation restacking work in order to understand how the animation is visible after activation change, while this patch is more clear and direct as we're restacking when the focus/activation changes. Let's discuss offline.
updated the patch as discussed. Please let me know what you think.
https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... File ui/wm/core/window_animations.h (right): https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... ui/wm/core/window_animations.h:99: // When set to true, this recreates layers for the window and and puts Document when you would want to do this. Can you pass this to the constructor instead of a setter? https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... ui/wm/core/window_animations.h:100: // the original (and animating) layers on top of the |window_| and 'the |window_|' -> |window_| https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... ui/wm/core/window_animations.h:103: // Note that this is effective only if the |window| has a focus when 'a focus' -> focus https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... ui/wm/core/window_animations.h:106: ensure_visibility_after_focus_lost_ = value; Make field and method name match here.
https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... File ui/wm/core/window_animations.h (right): https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... ui/wm/core/window_animations.h:99: // When set to true, this recreates layers for the window and and puts On 2014/03/13 21:09:37, sky wrote: > Document when you would want to do this. I added an example. > Can you pass this to the constructor instead of a setter? It thought about it but I preferred this way because it's better than the following IMHO. ScopedHidingAnimationSettings( window, ture /* ensure visible after focus lost */); https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... ui/wm/core/window_animations.h:100: // the original (and animating) layers on top of the |window_| and On 2014/03/13 21:09:37, sky wrote: > 'the |window_|' -> |window_| Done. https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... ui/wm/core/window_animations.h:103: // Note that this is effective only if the |window| has a focus when On 2014/03/13 21:09:37, sky wrote: > 'a focus' -> focus Done. https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... ui/wm/core/window_animations.h:106: ensure_visibility_after_focus_lost_ = value; On 2014/03/13 21:09:37, sky wrote: > Make field and method name match here. Done.
https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... File ui/wm/core/window_animations.h (right): https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... ui/wm/core/window_animations.h:99: // When set to true, this recreates layers for the window and and puts On 2014/03/13 22:03:16, oshima wrote: > On 2014/03/13 21:09:37, sky wrote: > > Document when you would want to do this. > > I added an example. > > > Can you pass this to the constructor instead of a setter? > > It thought about it but I preferred this way because it's better than the > following IMHO. > > ScopedHidingAnimationSettings( > window, > ture /* ensure visible after focus lost */); > Then add an enum.
On 2014/03/13 22:04:04, sky wrote: > https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... > File ui/wm/core/window_animations.h (right): > > https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... > ui/wm/core/window_animations.h:99: // When set to true, this recreates layers > for the window and and puts > On 2014/03/13 22:03:16, oshima wrote: > > On 2014/03/13 21:09:37, sky wrote: > > > Document when you would want to do this. > > > > I added an example. > > > > > Can you pass this to the constructor instead of a setter? > > > > It thought about it but I preferred this way because it's better than the > > following IMHO. > > > > ScopedHidingAnimationSettings( > > window, > > ture /* ensure visible after focus lost */); > > > > Then add an enum.
On 2014/03/13 22:04:04, sky wrote: > https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... > File ui/wm/core/window_animations.h (right): > > https://codereview.chromium.org/180273025/diff/700001/ui/wm/core/window_anima... > ui/wm/core/window_animations.h:99: // When set to true, this recreates layers > for the window and and puts > On 2014/03/13 22:03:16, oshima wrote: > > On 2014/03/13 21:09:37, sky wrote: > > > Document when you would want to do this. > > > > I added an example. > > > > > Can you pass this to the constructor instead of a setter? > > > > It thought about it but I preferred this way because it's better than the > > following IMHO. > > > > ScopedHidingAnimationSettings( > > window, > > ture /* ensure visible after focus lost */); > > > > Then add an enum. I can use ENSURE_VISIBLE_AFTER_FOCUS_LOST for true. What do you suggest for false case?
I think I like enum LayerStrategy {
LAYER_STRATEGY_RECREATE_LAYERS,
LAYER_STRATEGY_AQCUIRE_LAYERS
}
inside ScopedHidingAnimationSettings
https://codereview.chromium.org/180273025/diff/740001/ui/wm/core/window_anima...
File ui/wm/core/window_animations.cc (right):
https://codereview.chromium.org/180273025/diff/740001/ui/wm/core/window_anima...
ui/wm/core/window_animations.cc:551: HidingWindowAnimationObserver* observer =
Why isn't this a member?
removed acquire layer path as discussed. PTAL.
Nice, LGTM https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... File ui/wm/core/window_animations.cc (right): https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:64: // 1) Notifies AnimatinoHost the end of hiding animation. 'AnimatinoHost the end of'->'AnimationHost at the end of a' https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:65: // 2) Detaches the window's layers for hiding animation and delete delete->deletes https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:67: // ensure that the animation continues in the even of the window being even->event https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:69: // window gets restacked below other wndows when focus or activation wndows->windows https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:85: // Stack the origina layers above |window_| and its transient children. origina->original https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:85: // Stack the origina layers above |window_| and its transient children. Since you describe restacking here I think the comment you have around 485 should be here. https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:488: if(window_->parent()) { 'if(window'->'if (window'
https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... File ui/wm/core/window_animations.cc (right): https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:64: // 1) Notifies AnimatinoHost the end of hiding animation. On 2014/03/14 14:32:13, sky wrote: > 'AnimatinoHost the end of'->'AnimationHost at the end of a' Done. https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:65: // 2) Detaches the window's layers for hiding animation and delete On 2014/03/14 14:32:13, sky wrote: > delete->deletes Done. https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:67: // ensure that the animation continues in the even of the window being On 2014/03/14 14:32:13, sky wrote: > even->event Done. https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:69: // window gets restacked below other wndows when focus or activation On 2014/03/14 14:32:13, sky wrote: > wndows->windows Done. https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:85: // Stack the origina layers above |window_| and its transient children. On 2014/03/14 14:32:13, sky wrote: > origina->original Done. https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:85: // Stack the origina layers above |window_| and its transient children. On 2014/03/14 14:32:13, sky wrote: > Since you describe restacking here I think the comment you have around 485 > should be here. Done. https://codereview.chromium.org/180273025/diff/750011/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:488: if(window_->parent()) { On 2014/03/14 14:32:13, sky wrote: > 'if(window'->'if (window' Done.
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/180273025/820001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) ash_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
On 2014/03/14 20:06:51, I haz the power (commit-bot) wrote: > Retried try job too often on win_rel for step(s) ash_unittests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... win failure looks real, sigh.. I'll look into it next week.
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/180273025/950001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for ash/wm/maximize_mode/workspace_backdrop_delegate.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file ash/wm/maximize_mode/workspace_backdrop_delegate.cc
Hunk #2 FAILED at 49.
1 out of 2 hunks FAILED -- saving rejects to file
ash/wm/maximize_mode/workspace_backdrop_delegate.cc.rej
Patch: ash/wm/maximize_mode/workspace_backdrop_delegate.cc
Index: ash/wm/maximize_mode/workspace_backdrop_delegate.cc
diff --git a/ash/wm/maximize_mode/workspace_backdrop_delegate.cc
b/ash/wm/maximize_mode/workspace_backdrop_delegate.cc
index
093e403b4ffb0ec324c32268cfa512f4127a4cdb..f5ef1fefd7d6ecfcba7b874622b0416e9c13c7d6
100644
--- a/ash/wm/maximize_mode/workspace_backdrop_delegate.cc
+++ b/ash/wm/maximize_mode/workspace_backdrop_delegate.cc
@@ -12,6 +12,7 @@
#include "ui/compositor/scoped_layer_animation_settings.h"
#include "ui/views/background.h"
#include "ui/views/widget/widget.h"
+#include "ui/wm/core/window_animations.h"
#include "ui/wm/core/window_util.h"
namespace ash {
@@ -48,11 +49,9 @@
WorkspaceBackdropDelegate::WorkspaceBackdropDelegate(aura::Window* container)
WorkspaceBackdropDelegate::~WorkspaceBackdropDelegate() {
container_->RemoveObserver(this);
- ui::ScopedLayerAnimationSettings settings(
- background_->GetNativeView()->layer()->GetAnimator());
+ views::corewm::ScopedHidingAnimationSettings hiding_settings(
+ background_->GetNativeView());
background_->Close();
- settings.AddObserver(views::corewm::CreateHidingWindowAnimationObserver(
- background_->GetNativeView()));
background_->GetNativeView()->layer()->SetOpacity(0.0f);
}
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/180273025/960001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
rebased & uploaded new patch. Failing thest is: SystemTrayTest.SystemTrayNoViewItems Here is the summary of the current issue: aura::Window::RecreateLayer() moves entire sub-tree to new layer (which is owned by aura window) and wm::RecreateLayers assume this behavior to create new tree. On the other hand, views::View::RecreateLayer() only moves layers that are owned by Views. If one of sub layers has been acquired, that that layer remains on the old layer. During the recursive call in wm::RecreateLayers, it may end up with view's RecreateLayer, and when this happens, the layer that has already acquired may stay in the old tree, which becomes the part of the tree owned by LayerTreeHost. Eventually, this layer will be deleted twice, by LayerTreeHost, and by the view's code that recreated the layer. (see SystemTrayBubble::UpdateView) sky@ will address this issue first then I'll land this patch.
On 2014/03/19 17:46:18, oshima wrote: > rebased & uploaded new patch. > > Failing thest is: SystemTrayTest.SystemTrayNoViewItems > Here is the summary of the current issue: > > aura::Window::RecreateLayer() moves entire sub-tree to new layer (which is owned > by aura window) and > wm::RecreateLayers assume this behavior to create new tree. > > On the other hand, views::View::RecreateLayer() only moves layers that are owned > by Views. > If one of sub layers has been acquired, that that layer remains on the old > layer. > > During the recursive call in wm::RecreateLayers, it may end up with view's > RecreateLayer, and > when this happens, the layer that has already acquired may stay in the old tree, > which becomes > the part of the tree owned by LayerTreeHost. > > Eventually, this layer will be deleted twice, by LayerTreeHost, and by the > view's code that recreated > the layer. (see SystemTrayBubble::UpdateView) > > sky@ will address this issue first then I'll land this patch. sky's fix has landed (https://codereview.chromium.org/204793004/)
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/180273025/1020001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/180273025/1020001
Message was sent while issue was closed.
Change committed as 258220 |
