Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
289743002:
Overview Button Tray Animation accounting for startup (Closed)
|
Created:
6 years, 7 months ago by jonross Modified:
6 years, 7 months ago CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionOverview Button Tray Animation accounting for startup
Reapply the animation of TrayBackgroundView. Update to account for visibility changes that occur during animations.
Added a test for to cover state changes during animations.
Original review: https://codereview.chromium.org/251193004/
Revert review: https://codereview.chromium.org/281793002/
This reverts commit 399aa22e04f289f496f60ad23379a098e389d6a1
TEST=SystemTrayTest
BUG=372285, 363714
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272829
Patch Set 1 #Patch Set 2 : Implement Fix #
Total comments: 6
Patch Set 3 : #
Total comments: 3
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 4
Patch Set 6 : . #
Total comments: 6
Patch Set 7 : #Patch Set 8 : Rebase #
Messages
Total messages: 28 (0 generated)
https://codereview.chromium.org/289743002/diff/20001/ash/system/tray/system_t... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/289743002/diff/20001/ash/system/tray/system_t... ash/system/tray/system_tray_unittest.cc:500: EXPECT_TRUE(tray->visible()); This test should end the animation before verifying that it has the right visibility state (i.e. let ScopedAnimationDurationScaleMode go out of scope). Does it actually catch the current failure? https://codereview.chromium.org/289743002/diff/20001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/20001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:340: !(visible && layer()->GetTargetOpacity() == 0.0f)) This is probably easier to read as: if (visible == layer()->GetTargetVisibility()) return; https://codereview.chromium.org/289743002/diff/20001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:343: layer()->GetAnimator()->StopAnimating(); Is this necessary? If we're mid animation can't we animate from the current position?
https://codereview.chromium.org/289743002/diff/20001/ash/system/tray/system_t... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/289743002/diff/20001/ash/system/tray/system_t... ash/system/tray/system_tray_unittest.cc:500: EXPECT_TRUE(tray->visible()); On 2014/05/14 17:32:43, flackr wrote: > This test should end the animation before verifying that it has the right > visibility state (i.e. let ScopedAnimationDurationScaleMode go out of scope). > Does it actually catch the current failure? Updated to set clear the scoped duration, and set a zero duration. To confirm the final states. The test was written before the fix and identifies the error. https://codereview.chromium.org/289743002/diff/20001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/20001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:340: !(visible && layer()->GetTargetOpacity() == 0.0f)) On 2014/05/14 17:32:43, flackr wrote: > This is probably easier to read as: > if (visible == layer()->GetTargetVisibility()) > return; Done. https://codereview.chromium.org/289743002/diff/20001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:343: layer()->GetAnimator()->StopAnimating(); On 2014/05/14 17:32:43, flackr wrote: > Is this necessary? If we're mid animation can't we animate from the current > position? Done.
https://codereview.chromium.org/289743002/diff/40001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:375: // must be turned on again here. This doesn't seem like it should be the case. If a call SetVisible(true) has run isn't the TargetVisibility now true which would prevent calling SetVisible(false)? https://codereview.chromium.org/289743002/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:492: // the visibility so that the next animation may render. How about just checking GetTargetVisibility? If the reverse animation was added this should still return true.
https://codereview.chromium.org/289743002/diff/40001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:492: // the visibility so that the next animation may render. On 2014/05/15 21:39:43, flackr wrote: > How about just checking GetTargetVisibility? If the reverse animation was added > this should still return true. Unfortunately no. When a new animation is being added, the original conflicting animation is aborted. However at this time the new animation is not in the queue. So GetTargetVisibility() does not report the next target. crbug.com/374236
https://codereview.chromium.org/289743002/diff/60001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/60001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:376: layer()->SetVisible(true); This seems like a fail-safe for when we call views::View::SetVisible when visible == visible(). We want the layer to be visible during the animation, not just after it, so perhaps this should be done on 350 in case views::View::SetVisible(true) would be a no-op.
https://codereview.chromium.org/289743002/diff/60001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/60001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:376: layer()->SetVisible(true); On 2014/05/16 18:07:17, flackr wrote: > This seems like a fail-safe for when we call views::View::SetVisible when > visible == visible(). We want the layer to be visible during the animation, not > just after it, so perhaps this should be done on 350 in case > views::View::SetVisible(true) would be a no-op. Done.
https://codereview.chromium.org/289743002/diff/80001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/80001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:355: // the layer to not visibible, so it must be turned on again here. Are you sure this is what's happening? Isn't it just that views::View::SetVisible is a noop because it checks the view's visibility property rather than the layer and we delay updating the view's visibility so it assumes that it's still visible even though it's animating to not visible? I would expect that in the completion of the initial animation it would not call SetVisible(false) because the layer is still animating. https://codereview.chromium.org/289743002/diff/80001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:381: // must be turned on again here. I'm confused, this is the same comment as on 351. Are both of these necessary, if so, why?
Reduced the usage of layer-SetVisible(true) to the more direct solution. Updated the comment to be more accurate based on usage and findings. https://codereview.chromium.org/289743002/diff/80001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/80001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:355: // the layer to not visibible, so it must be turned on again here. On 2014/05/21 14:47:39, flackr wrote: > Are you sure this is what's happening? Isn't it just that > views::View::SetVisible is a noop because it checks the view's visibility > property rather than the layer and we delay updating the view's visibility so it > assumes that it's still visible even though it's animating to not visible? > > I would expect that in the completion of the initial animation it would not call > SetVisible(false) because the layer is still animating. Done. https://codereview.chromium.org/289743002/diff/80001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:381: // must be turned on again here. On 2014/05/21 14:47:39, flackr wrote: > I'm confused, this is the same comment as on 351. Are both of these necessary, > if so, why? Done.
Please update test in CL description to only name the test which verifies the animation. Otherwise LGTM.
lgtm https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:350: views::View::SetVisible(visible); SetVisible(true) to be consistent with the code below. (you may use visible below, but true is probably better. https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:352: // views::View::SetVisible(true) is a no-op. When the previous animation Isn't this a bug? aura::Window seems to be handling this correctly.
The CQ bit was checked by jonross@chromium.org
The CQ bit was unchecked by jonross@chromium.org
https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:350: views::View::SetVisible(visible); On 2014/05/22 18:39:31, oshima wrote: > SetVisible(true) > > to be consistent with the code below. (you may use visible below, but true is > probably better. Done. https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:352: // views::View::SetVisible(true) is a no-op. When the previous animation On 2014/05/22 18:39:31, oshima wrote: > Isn't this a bug? aura::Window seems to be handling this correctly. I hadn't seen that implementation. If there is precedent, then I would consider a bug. I filed a separate bug for Layer->GetTargetValue() not reporting correctly while called in the middle of adding new animations. crbug.com/374236
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jonross@chromium.org/289743002/40002
https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:352: // views::View::SetVisible(true) is a no-op. When the previous animation On 2014/05/22 20:55:37, jonross wrote: > On 2014/05/22 18:39:31, oshima wrote: > > Isn't this a bug? aura::Window seems to be handling this correctly. > > I hadn't seen that implementation. If there is precedent, then I would consider > a bug. > > I filed a separate bug for Layer->GetTargetValue() not reporting correctly while > called in the middle of adding new animations. crbug.com/374236 View::SetVisible only considers the View::visible_ state, not the layer target visibility, so when the view is animating out the call to set the View::visible_ property to false is delayed until the animation completion but the call to become visible again can come before that animation completes. https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s...
https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:352: // views::View::SetVisible(true) is a no-op. When the previous animation On 2014/05/22 21:02:50, flackr wrote: > On 2014/05/22 20:55:37, jonross wrote: > > On 2014/05/22 18:39:31, oshima wrote: > > > Isn't this a bug? aura::Window seems to be handling this correctly. > > > > I hadn't seen that implementation. If there is precedent, then I would > consider > > a bug. > > > > I filed a separate bug for Layer->GetTargetValue() not reporting correctly > while > > called in the middle of adding new animations. crbug.com/374236 > > View::SetVisible only considers the View::visible_ state, not the layer target > visibility, so when the view is animating out the call to set the View::visible_ > property to false is delayed until the animation completion but the call to > become visible again can come before that animation completes. > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s... True, however aura::Window currently also considers the target: https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&...
The CQ bit was unchecked by jonross@chromium.org
On 2014/05/22 21:02:49, flackr wrote: > https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... > File ash/system/tray/tray_background_view.cc (right): > > https://codereview.chromium.org/289743002/diff/100001/ash/system/tray/tray_ba... > ash/system/tray/tray_background_view.cc:352: // views::View::SetVisible(true) is > a no-op. When the previous animation > On 2014/05/22 20:55:37, jonross wrote: > > On 2014/05/22 18:39:31, oshima wrote: > > > Isn't this a bug? aura::Window seems to be handling this correctly. > > > > I hadn't seen that implementation. If there is precedent, then I would > consider > > a bug. > > > > I filed a separate bug for Layer->GetTargetValue() not reporting correctly > while > > called in the middle of adding new animations. crbug.com/374236 > > View::SetVisible only considers the View::visible_ state, not the layer target > visibility, so when the view is animating out the call to set the View::visible_ > property to false is delayed until the animation completion but the call to > become visible again can come before that animation completes. > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s... Yes I understand, and I think that's a bug. (i think it's just overlooked)
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jonross@chromium.org/289743002/130001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jonross@chromium.org/289743002/130001
Message was sent while issue was closed.
Change committed as 272829 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
