Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
241983003:
Stops animations when removing a window from its parent (Closed)
|
Created:
6 years, 8 months ago by varkha Modified:
6 years, 8 months ago CC:
chromium-reviews, sadrul, ben+aura_chromium.org, kalyank Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionStops animations when removing a window from its parent.
This prevents overwriting target bounds when reparenting layers during animation. In snapped scenario we snap a window (with a call to WindowState::OnWMEvent in WorkspaceWindowResizer::CompleteDrag()) which starts animation and then immediately reparent it (back to workspace in DockedWindowResizer::MaybeReparentWindowOnDragCompletion). Stopping (completing) BOUNDS animation allows the child window to honor previously set target bounds and keep them when it gets reparented.
BUG=364517, 366993
TEST=aura_unittests --gtest_filter=WindowTest.RootWindowSetWhenReparenting
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266115
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266451
Patch Set 1 : Uses GetTargetBounds() in place of bounds() when reparenting layers #
Total comments: 3
Patch Set 2 : Uses GetTargetBounds() in place of bounds() when reparenting layers (retargeting animations) #
Total comments: 2
Patch Set 3 : Uses GetTargetBounds() in place of bounds() when reparenting layers (retargeting animations) #Patch Set 4 : Stops animations when removing a window from its parent #
Total comments: 2
Patch Set 5 : Stops animations when removing a window from its parent (comment) #
Total comments: 3
Patch Set 6 : Stops animations when removing a layer from its parent and fixes broken test #
Messages
Total messages: 44 (0 generated)
pkotwicz@, what do you think about this approach? Do you think the old code was just wrong or was it using bounds() for some good reason (that I fail to see). I also struggle to think of a good tests for something like this. I remember you had to override default tests animation behavior to force animations in some test. Would you do something like this here? Thanks!
ping - let me know if you think this is a right approach.
Sorry for the delay. Your code sounds reasonable. However, I think I have gotten pushback before in doing something similar to this wrt to views::View::ConvertPointToTarget() I am deferring the review to oshima. "// SetBoundsDirectAnimated does not work when the window gets reparented. // TODO(oshima): Consider fixing it and reenable the animation." in DefaultState::UpdateBoundsFromState() seems relevant. For the sake of testing you should be ok if you force animations to have non zero length. ScopedAnimationDurationScaleMode is probably what you are looking for
Thanks, I have added a test as well. For now this CL includes the changes from https://codereview.chromium.org/241243002/ and so the test has only one slight addition from that CL (which I plan to land first as it fixes the crash in the same scenario). oshima@, could you please take a look? Thanks!
https://codereview.chromium.org/241983003/diff/30001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/241983003/diff/30001/ui/aura/window.cc#newcod... ui/aura/window.cc:523: offset += child->GetTargetBounds().OffsetFromOrigin(); Sorry if I'm misunderstanding something, but I wonder if this correct for all cases. GetTargetBounds seem to depend on the ancestor layer, while the next line can change the relative location of the layer to ancestor layer. maybe what you want to do here is something like this? layer() ? layer()->GetTargetBounds() : bounds()
https://codereview.chromium.org/241983003/diff/30001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/241983003/diff/30001/ui/aura/window.cc#newcod... ui/aura/window.cc:523: offset += child->GetTargetBounds().OffsetFromOrigin(); On 2014/04/22 20:47:11, oshima wrote: > Sorry if I'm misunderstanding something, but I wonder if this correct for all > cases. GetTargetBounds seem to depend on the ancestor layer, while the next line > can change the relative location of the layer to ancestor layer. > > maybe what you want to do here is something like this? > > layer() ? layer()->GetTargetBounds() : bounds() Isn't that how Window::GetTargetBounds is implemented for !layer() case?
https://codereview.chromium.org/241983003/diff/30001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/241983003/diff/30001/ui/aura/window.cc#newcod... ui/aura/window.cc:523: offset += child->GetTargetBounds().OffsetFromOrigin(); On 2014/04/22 20:59:05, varkha wrote: > On 2014/04/22 20:47:11, oshima wrote: > > Sorry if I'm misunderstanding something, but I wonder if this correct for all > > cases. GetTargetBounds seem to depend on the ancestor layer, while the next > line > > can change the relative location of the layer to ancestor layer. > > > > maybe what you want to do here is something like this? > > > > layer() ? layer()->GetTargetBounds() : bounds() > > Isn't that how Window::GetTargetBounds is implemented for !layer() case? It looks to me that this is wrong if a) the child has a layer and b) the parent (this) does not have a layer and c) the parent has ancestor, because Window::GetTargetBounds subtracts the offset of the ancestor from the layer()->GetTargetBounds(), even though the layer hasn't adjusted to the ancestor's bounds yet. I may be missing something though. I think either sky@ or ben@ should review this.
oshima@, can you please take another look (I will be sending this to sky@ as well)? Do you think this is more accurate? Thanks!
Hold on, still failing some tests.
oshima@, can you please take a look again? I am effectively recalculating the animation target, but allowing the animation to continue rather than stop it immediately with the new target. This helps with scenarios where one part of code sets bounds (starting an animation) and then some other part changes the parent (which should not change the eventual window position). In this patch I am not changing bounds(), other than indirectly through layer bounds change. Thanks!
https://codereview.chromium.org/241983003/diff/70001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/241983003/diff/70001/ui/aura/window.cc#newcod... ui/aura/window.cc:510: ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET); I feels like animation should be canceled upon parent change, and the client code should take care of applying animation correctly (after reparent in this case)? Is there any reason why drag code can't be fixed instead?
https://codereview.chromium.org/241983003/diff/70001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/241983003/diff/70001/ui/aura/window.cc#newcod... ui/aura/window.cc:510: ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET); On 2014/04/23 16:34:44, oshima wrote: > I feels like animation should be canceled upon parent change, and the client > code should take care of applying animation correctly (after reparent in this > case)? > > Is there any reason why drag code can't be fixed instead? I was thinking that this would be an option. It is just that the paths that may cause animation to be active during reparenting are many and not easily controlled or visible in code inspection so fixing it here made sense. Stopping animation also has some issues - I have seen the window bounds getting out of sync with layers bounds because we revert window bounds_ in ReparentLayers and UnparentLayers which only makes sense if the animation is allowed to continue. If you look at the crashing bug https://code.google.com/p/chromium/issues/detail?id=364498 - it has a few call stacks mentioned that are all slightly different but all similar in that they all involve animation during reparenting. It feels that we would have to hunt each of those separately with the same recipe.
On 2014/04/23 16:44:04, varkha wrote: > https://codereview.chromium.org/241983003/diff/70001/ui/aura/window.cc > File ui/aura/window.cc (right): > > https://codereview.chromium.org/241983003/diff/70001/ui/aura/window.cc#newcod... > ui/aura/window.cc:510: ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET); > On 2014/04/23 16:34:44, oshima wrote: > > I feels like animation should be canceled upon parent change, and the client > > code should take care of applying animation correctly (after reparent in this > > case)? > > > > Is there any reason why drag code can't be fixed instead? > > I was thinking that this would be an option. It is just that the paths that may > cause animation to be active during reparenting are many and not easily > controlled or visible in code inspection so fixing it here made sense. Stopping > animation also has some issues - I have seen the window bounds getting out of > sync with layers bounds because we revert window bounds_ in ReparentLayers and > UnparentLayers which only makes sense if the animation is allowed to continue. > If you look at the crashing bug > https://code.google.com/p/chromium/issues/detail?id=364498 - it has a few call > stacks mentioned that are all slightly different but all similar in that they > all involve animation during reparenting. It feels that we would have to hunt > each of those separately with the same recipe. Sorry, I thought I sent my comment earlier but apparently I didn't. I still feel having animation code inside is weird, but I think you should get sky/ben/vollic's opinion instead now.
sky@, can you please look at this CL? I also have a feeling that having animations related code here is wrong but I am not sure which alternative would be better. There are multiple code paths that call AddParent and figuring out which of those may be in context of an unfinished animation may be tricky. Certainly scenarios involving starting and ending drags, moving windows from one screen to another when a screen get disconnected may or may not have an animation pending and so affect the coordinates that the window should end at. You have just looked at my CL for http://crbug.com/364498 which dealt with crashing in those scenarios. As I can see in the call stacks mentioned in that bug there are many such scenarios - should we deal with them one by one or is the approach here OK (or is there some other way to get window reparented mid-animation)? Thanks!
It doesn't make sense to me that removing a view would make animating the bounds continue on. I think we should stop animations when a window is removed (removal implicitly happens from add).
sky@, please take another look. This is indeed much simpler and accomplishes the same goal. Thanks!
LGTM https://codereview.chromium.org/241983003/diff/110001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/241983003/diff/110001/ui/aura/window.cc#newco... ui/aura/window.cc:1018: if (child->layer() && child->layer()->GetAnimator()) Add a comment as to why we do this.
https://codereview.chromium.org/241983003/diff/110001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/241983003/diff/110001/ui/aura/window.cc#newco... ui/aura/window.cc:1018: if (child->layer() && child->layer()->GetAnimator()) On 2014/04/24 16:06:19, sky wrote: > Add a comment as to why we do this. Done.
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/241983003/130001
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 varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/241983003/130001
Message was sent while issue was closed.
Change committed as 266115
Message was sent while issue was closed.
https://codereview.chromium.org/241983003/diff/130001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/241983003/diff/130001/ui/aura/window.cc#newco... ui/aura/window.cc:1021: child->layer()->GetAnimator()->StopAnimating(); Is it possible to do this in the layer's RemoveChild instead of Window's?
Message was sent while issue was closed.
https://codereview.chromium.org/241983003/diff/130001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/241983003/diff/130001/ui/aura/window.cc#newco... ui/aura/window.cc:1021: child->layer()->GetAnimator()->StopAnimating(); On 2014/04/25 03:28:01, Ben Goodger (Google) wrote: > Is it possible to do this in the layer's RemoveChild instead of Window's? Yes, it would make more sense. I'll explore it. Thanks for the idea.
https://codereview.chromium.org/241983003/diff/130001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/241983003/diff/130001/ui/aura/window.cc#newco... ui/aura/window.cc:1021: child->layer()->GetAnimator()->StopAnimating(); On 2014/04/25 03:28:01, Ben Goodger (Google) wrote: > Is it possible to do this in the layer's RemoveChild instead of Window's? Done. https://codereview.chromium.org/241983003/diff/170001/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/241983003/diff/170001/ui/compositor/layer.cc#... ui/compositor/layer.cc:171: ui::LayerAnimationElement::BOUNDS); Only BOUNDS animations truly need to be completed, so this no longer stops ALL animations (which fixes bug 366993).
lgtm!
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/241983003/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/241983003/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/241983003/190001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/241983003/190001
Message was sent while issue was closed.
Change committed as 266451 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
