Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
271913002:
Animate window control changes in TouchView (Closed)
|
Created:
6 years, 7 months ago by jonross Modified:
6 years, 6 months ago CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAnimate window control changes in TouchView
Add animation for the hiding and showing of the resize button. Add an animation
for sliding the minimize button into the position of the resize button. Delay
changing the size of FrameCaptionButtonContainerView to account for conflicting
layout changes caused by the transition to TouchView.
TEST=FrameCaptionButtonContainerViewTest
TEST=CustomFrameViewAshTest
TES=BrowserNonClientFrameViewAshTest
BUG=363717
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274371
Patch Set 1 #
Total comments: 33
Patch Set 2 : Make button layout animations generic #
Total comments: 13
Patch Set 3 : #
Total comments: 4
Patch Set 4 : Change layout to be agnostic of which button is animating #
Total comments: 7
Patch Set 5 : Rebase #Patch Set 6 : #
Total comments: 13
Patch Set 7 : Add test for the animation #
Total comments: 8
Patch Set 8 : #
Total comments: 10
Patch Set 9 : #
Total comments: 4
Patch Set 10 : Rebase #Patch Set 11 : #
Total comments: 4
Patch Set 12 : #Messages
Total messages: 28 (0 generated)
Another animation change. Similar style to the OverviewButton. This time it impacts the windowing controls.
https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:8: #include <map> unused? https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:37: const int kAnimationDelay = 100; Units? https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:192: settings.AddObserver(this); nit: comment this will SetVisible(false) when animation is complete. https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:193: size_button_->layer()->SetOpacity(0.0f); Also ->SetVisible(false); https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:213: (child == size_button_ && child->layer()->GetTargetOpacity() == 0.0f)) GetTargetVisibility. Try to avoid making this specific to size_button_, assume any button could animate. https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:220: size_button_->layer()->GetAnimator()->is_animating()) { This is awfully specific to the button that's going away and the button order. Instead, if you're traversing from right to left, you could track if you've seen a button which is animating and delay the movement of all buttons after it. https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:293: if (frame_->IsFullscreen()) { // Can be clicked in immersive fullscreen. Why two spaces before comment? https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:387: // renders outside of its parents. Create a more specific animation where Can we avoid this by animating Translate transforms to the correct location but having the bounds set immediately? https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... File ash/frame/caption_buttons/frame_caption_button_container_view.h (right): https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.h:8: #include <map> Unused? https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.h:30: ui::ImplicitAnimationObserver { public ui::ImplicitAnimationObserver https://codereview.chromium.org/271913002/diff/1/ash/frame/custom_frame_view_... File ash/frame/custom_frame_view_ash.cc (right): https://codereview.chromium.org/271913002/diff/1/ash/frame/custom_frame_view_... ash/frame/custom_frame_view_ash.cc:7: #include <algorithm> not used? https://codereview.chromium.org/271913002/diff/1/ash/frame/custom_frame_view_... ash/frame/custom_frame_view_ash.cc:8: #include <vector> not used? https://codereview.chromium.org/271913002/diff/1/ash/frame/custom_frame_view_... ash/frame/custom_frame_view_ash.cc:317: if (child != caption_button_container_) Comment why. https://codereview.chromium.org/271913002/diff/1/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/271913002/diff/1/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:7: #include <algorithm> not used? https://codereview.chromium.org/271913002/diff/1/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:386: if (child != caption_button_container_) Also comment why. https://codereview.chromium.org/271913002/diff/1/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:390: frame()->GetRootView()->Layout(); I'm not quite sure what invalidating the layouts and doing an immediate layout is necessary for. https://codereview.chromium.org/271913002/diff/1/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right): https://codereview.chromium.org/271913002/diff/1/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:72: // views::View: nit: For consistency, update other comments.
https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:8: #include <map> On 2014/05/08 19:02:21, flackr wrote: > unused? Cpplint, used outside my changed. https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:37: const int kAnimationDelay = 100; On 2014/05/08 19:02:21, flackr wrote: > Units? Done. https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:192: settings.AddObserver(this); On 2014/05/08 19:02:21, flackr wrote: > nit: comment this will SetVisible(false) when animation is complete. Done. https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:193: size_button_->layer()->SetOpacity(0.0f); On 2014/05/08 19:02:21, flackr wrote: > Also ->SetVisible(false); Done. https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:213: (child == size_button_ && child->layer()->GetTargetOpacity() == 0.0f)) On 2014/05/08 19:02:21, flackr wrote: > GetTargetVisibility. Try to avoid making this specific to size_button_, assume > any button could animate. Done. https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:220: size_button_->layer()->GetAnimator()->is_animating()) { On 2014/05/08 19:02:21, flackr wrote: > This is awfully specific to the button that's going away and the button order. > Instead, if you're traversing from right to left, you could track if you've seen > a button which is animating and delay the movement of all buttons after it. Done. https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.cc:293: if (frame_->IsFullscreen()) { // Can be clicked in immersive fullscreen. On 2014/05/08 19:02:21, flackr wrote: > Why two spaces before comment? cpplint marked this. https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... File ash/frame/caption_buttons/frame_caption_button_container_view.h (right): https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.h:8: #include <map> On 2014/05/08 19:02:21, flackr wrote: > Unused? Cpplint, used outside my changed. https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.h:30: ui::ImplicitAnimationObserver { On 2014/05/08 19:02:21, flackr wrote: > public ui::ImplicitAnimationObserver Done. https://codereview.chromium.org/271913002/diff/1/ash/frame/custom_frame_view_... File ash/frame/custom_frame_view_ash.cc (right): https://codereview.chromium.org/271913002/diff/1/ash/frame/custom_frame_view_... ash/frame/custom_frame_view_ash.cc:7: #include <algorithm> On 2014/05/08 19:02:21, flackr wrote: > not used? Cpplint, used outside my changed. https://codereview.chromium.org/271913002/diff/1/ash/frame/custom_frame_view_... ash/frame/custom_frame_view_ash.cc:8: #include <vector> On 2014/05/08 19:02:21, flackr wrote: > not used? Cpplint, used outside my changed. https://codereview.chromium.org/271913002/diff/1/ash/frame/custom_frame_view_... ash/frame/custom_frame_view_ash.cc:317: if (child != caption_button_container_) On 2014/05/08 19:02:21, flackr wrote: > Comment why. Done. https://codereview.chromium.org/271913002/diff/1/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/271913002/diff/1/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:7: #include <algorithm> On 2014/05/08 19:02:21, flackr wrote: > not used? Cpplint, used outside my changed. https://codereview.chromium.org/271913002/diff/1/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:390: frame()->GetRootView()->Layout(); On 2014/05/08 19:02:21, flackr wrote: > I'm not quite sure what invalidating the layouts and doing an immediate layout > is necessary for. That was recommended when I implemented the Maximize Mode observing in this class. However it does not affect this functionality. https://codereview.chromium.org/271913002/diff/1/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right): https://codereview.chromium.org/271913002/diff/1/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:72: // views::View: On 2014/05/08 19:02:21, flackr wrote: > nit: For consistency, update other comments. Done.
https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... File ash/frame/caption_buttons/frame_caption_button_container_view.h (right): https://codereview.chromium.org/271913002/diff/1/ash/frame/caption_buttons/fr... ash/frame/caption_buttons/frame_caption_button_container_view.h:8: #include <map> On 2014/05/08 21:15:40, jonross wrote: > On 2014/05/08 19:02:21, flackr wrote: > > Unused? > > Cpplint, used outside my changed. Ah, sorry about that, good work. https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:185: // animation. For visual design. For visual design reads a bit strange, I'm not sure it's necessary. https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:192: // Observer will call size_button_->SetVisible(false) upon completion of We could probably avoid delaying the SetVisible(false) by acquiring the view's layer before making it invisible and then animating and deleting the layer when done. https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye... That being said, this could be a TODO. https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:393: size_button_->SetVisible(false); This seems easy to accidentally trigger in some future patch. Is there any way we can assure that this is only used from the animation to hide the size button? Maybe DCHECK(!size_button_->layer()->GetTargetVisibility())? https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:401: PreferredSizeChanged(); We only listen to animations complete on removing the button, do we need to notify the parent when making the size button visible? https://codereview.chromium.org/271913002/diff/40001/ash/frame/custom_frame_v... File ash/frame/custom_frame_view_ash.cc (right): https://codereview.chromium.org/271913002/diff/40001/ash/frame/custom_frame_v... ash/frame/custom_frame_view_ash.cc:315: void nit: wrap after one of the ::'s, probably after HeaderView::. Also, always indent wrapped lines. https://codereview.chromium.org/271913002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/271913002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:384: void Same comment about wrapping.
https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:185: // animation. For visual design. On 2014/05/09 14:15:14, flackr wrote: > For visual design reads a bit strange, I'm not sure it's necessary. Done. https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:192: // Observer will call size_button_->SetVisible(false) upon completion of On 2014/05/09 14:15:14, flackr wrote: > We could probably avoid delaying the SetVisible(false) by acquiring the view's > layer before making it invisible and then animating and deleting the layer when > done. > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye... > > That being said, this could be a TODO. Done. https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:393: size_button_->SetVisible(false); On 2014/05/09 14:15:14, flackr wrote: > This seems easy to accidentally trigger in some future patch. Is there any way > we can assure that this is only used from the animation to hide the size button? > Maybe DCHECK(!size_button_->layer()->GetTargetVisibility())? Done. https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:401: PreferredSizeChanged(); On 2014/05/09 14:15:14, flackr wrote: > We only listen to animations complete on removing the button, do we need to > notify the parent when making the size button visible? Currently views follow this pattern: - toggle visibility - set new bounds - layout - paint So the container triggering the button becoming visibility already performs an immediate layout based on the new size. When not animating this also covers hiding. So we would not gain anything from also doing this on becoming visible. https://codereview.chromium.org/271913002/diff/40001/ash/frame/custom_frame_v... File ash/frame/custom_frame_view_ash.cc (right): https://codereview.chromium.org/271913002/diff/40001/ash/frame/custom_frame_v... ash/frame/custom_frame_view_ash.cc:315: void On 2014/05/09 14:15:14, flackr wrote: > nit: wrap after one of the ::'s, probably after HeaderView::. > Also, always indent wrapped lines. Done. https://codereview.chromium.org/271913002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/271913002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:384: void On 2014/05/09 14:15:14, flackr wrote: > Same comment about wrapping. Done.
https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/40001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:401: PreferredSizeChanged(); On 2014/05/09 14:45:47, jonross wrote: > On 2014/05/09 14:15:14, flackr wrote: > > We only listen to animations complete on removing the button, do we need to > > notify the parent when making the size button visible? > > Currently views follow this pattern: > - toggle visibility > - set new bounds > - layout > - paint > > So the container triggering the button becoming visibility already performs an > immediate layout based on the new size. > > When not animating this also covers hiding. > > So we would not gain anything from also doing this on becoming visible. Sounds good. https://codereview.chromium.org/271913002/diff/60001/ash/frame/caption_button... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/60001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:235: child->SetBounds(x, 0, size.width(), size.height()); Couldn't this accidentally restart the slide animation if Layout is triggered partway through it? Also, if a previous child is becoming visible, we need to animate over every button after it by the combined widths of all of the previous buttons becoming visible. https://codereview.chromium.org/271913002/diff/60001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:244: new ui::ScopedLayerAnimationSettings(child->layer()->GetAnimator()); This goes out of scope before the call to child->SetBounds, won't this fail to animate? There should probably just be a bool for whether to animate in the later SetBounds call. https://codereview.chromium.org/271913002/diff/60001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:251: previous_child_target_visibility = child->layer()->GetTargetVisibility(); This seems redundant with line 257
https://codereview.chromium.org/271913002/diff/60001/ash/frame/caption_button... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/60001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:244: new ui::ScopedLayerAnimationSettings(child->layer()->GetAnimator()); On 2014/05/09 15:44:23, flackr wrote: > This goes out of scope before the call to child->SetBounds, won't this fail to > animate? There should probably just be a bool for whether to animate in the > later SetBounds call. It's stored in a scoped_ptr on 250, stays in scope until either layout is done, or a new animation is created. https://codereview.chromium.org/271913002/diff/80001/ash/frame/caption_button... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/80001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:214: void FrameCaptionButtonContainerView::Layout() { Redone to not care which button is changing visibility. I had one thought though. This might be cleaner if we just tracked when a visibility animation was triggered. And enable animations for each child's layout during that time.
https://codereview.chromium.org/271913002/diff/80001/ash/frame/caption_button... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/80001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:215: scoped_ptr<ui::ScopedLayerAnimationSettings> animation; nit: Scope this to it's use, i.e. in the loop body. https://codereview.chromium.org/271913002/diff/80001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:226: IsAnimatingProperty(ui::LayerAnimationElement::OPACITY); Instead of checking if it's animating opacity, checking current opacity > 0.0f might work as long as this is called immediately after UpdateSizeButtonVisibility right? This would also prevent a double animation if it was called again and won't require digging into all of the animators. https://codereview.chromium.org/271913002/diff/80001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:233: previous_child_target_visibility = child_target_visibility; This really needs to be an offset width. The buttons aren't necessarily the same size, and in theory there can be multiple hiding. I think both of these bools can be captured by a single value for the x offset to start animating all subsequent buttons from. Then the animation should pause if that offset is negative (i.e. wait for things which are hiding).
https://codereview.chromium.org/271913002/diff/80001/ash/frame/caption_button... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/80001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:226: IsAnimatingProperty(ui::LayerAnimationElement::OPACITY); On 2014/05/09 18:57:58, flackr wrote: > Instead of checking if it's animating opacity, checking current opacity > 0.0f > might work as long as this is called immediately after > UpdateSizeButtonVisibility right? This would also prevent a double animation if > it was called again and won't require digging into all of the animators. Issue with that is that layout is called 7+ times after UpdateSizeButtonVisibility, while transitioning to maximize mode. Depending on when it is called we could have non-zero opacity for both animations.
https://codereview.chromium.org/271913002/diff/80001/ash/frame/caption_button... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/80001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:215: scoped_ptr<ui::ScopedLayerAnimationSettings> animation; On 2014/05/09 18:57:58, flackr wrote: > nit: Scope this to it's use, i.e. in the loop body. Done. https://codereview.chromium.org/271913002/diff/80001/ash/frame/caption_button... ash/frame/caption_buttons/frame_caption_button_container_view.cc:233: previous_child_target_visibility = child_target_visibility; On 2014/05/09 18:57:58, flackr wrote: > This really needs to be an offset width. The buttons aren't necessarily the same > size, and in theory there can be multiple hiding. I think both of these bools > can be captured by a single value for the x offset to start animating all > subsequent buttons from. > > Then the animation should pause if that offset is negative (i.e. wait for things > which are hiding). Done. https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:209: bool child_animating_visibility = child_animator-> Not removed yet, as layout can be called multiple times throughout the stages of the animations. Going to look at a way to address this.
The animation is quite complex. Can you have a test to verify that it works how you expect it to? https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:215: offset_x += child->width(); Isn't the child's size accounted for by not breaking out of the loop early if it is becoming visible? https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:220: if (!child->visible() || !child->layer()->GetTargetVisibility()) !child_target_visibility? https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:227: // Animate the button if the previous button is currently animating s/if the/if a https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:231: child->SetBounds(x+offset_x, 0, size.width(), size.height() ); s/x+offset_x/x + offset_x s/ );/); https://codereview.chromium.org/271913002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/271913002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:388: // available nit: bad wrapping. https://codereview.chromium.org/271913002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:389: // until the completion of the animation. Layout it response to the s/it/in
Added a unit test to ensure that button visibility changes are appropriately handled via the layout animations. https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:215: offset_x += child->width(); On 2014/05/16 18:40:12, flackr wrote: > Isn't the child's size accounted for by not breaking out of the loop early if it > is becoming visible? The child's size is accounted for when becoming visible. However that affects the final layout position. If we don't track it for initial positioning we don't get the other buttons animating bounds to make room. https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:220: if (!child->visible() || !child->layer()->GetTargetVisibility()) On 2014/05/16 18:40:12, flackr wrote: > !child_target_visibility? Done. https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:227: // Animate the button if the previous button is currently animating On 2014/05/16 18:40:12, flackr wrote: > s/if the/if a Done. https://codereview.chromium.org/271913002/diff/120001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:231: child->SetBounds(x+offset_x, 0, size.width(), size.height() ); On 2014/05/16 18:40:12, flackr wrote: > s/x+offset_x/x + offset_x > s/ );/); Done. https://codereview.chromium.org/271913002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/271913002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:388: // available On 2014/05/16 18:40:12, flackr wrote: > nit: bad wrapping. Done. https://codereview.chromium.org/271913002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:389: // until the completion of the animation. Layout it response to the On 2014/05/16 18:40:12, flackr wrote: > s/it/in Done.
https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:209: bool child_animating_visibility = child_animator-> Call this child_animating_opacity to correctly reflect what it is. https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc (right): https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:172: container.Layout(); Does the container size change at some point? https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:179: EXPECT_EQ(close_button_bounds.x() - minimize_button_bounds.width(), Can this just be EXPECT_EQ(close_button_bounds.x(), minimize_button_bounds.right())? https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:181: EXPECT_GT(minimize_button_bounds.x(), initial_minimize_button_bounds.x()); This is probably expected given that it's now flush with the close button. Just have an initial expectation that it's flush with the size button which is flush with the close button.
https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:209: bool child_animating_visibility = child_animator-> On 2014/05/21 14:42:21, flackr wrote: > Call this child_animating_opacity to correctly reflect what it is. Done. https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc (right): https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:172: container.Layout(); On 2014/05/21 14:42:21, flackr wrote: > Does the container size change at some point? Done. https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:179: EXPECT_EQ(close_button_bounds.x() - minimize_button_bounds.width(), On 2014/05/21 14:42:21, flackr wrote: > Can this just be EXPECT_EQ(close_button_bounds.x(), > minimize_button_bounds.right())? Done. https://codereview.chromium.org/271913002/diff/140001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:181: EXPECT_GT(minimize_button_bounds.x(), initial_minimize_button_bounds.x()); On 2014/05/21 14:42:21, flackr wrote: > This is probably expected given that it's now flush with the close button. Just > have an initial expectation that it's flush with the size button which is flush > with the close button. Done.
https://codereview.chromium.org/271913002/diff/160001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/160001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:159: size_button_->layer()->SetVisible(true); nit: Comment why this is necessary. https://codereview.chromium.org/271913002/diff/160001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:203: int offset_x = 0; nit: The offset_x usage is sufficiently complex that it wouldn't hurt to have a bit of commenting as to what it does. It offsets the initial position so that buttons slide into place when other buttons are removed? https://codereview.chromium.org/271913002/diff/160001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc (right): https://codereview.chromium.org/271913002/diff/160001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:171: initial_minimize_button_bounds.right()); And close and size buttons should be flush? https://codereview.chromium.org/271913002/diff/160001/ash/frame/default_heade... File ash/frame/default_header_painter.cc (right): https://codereview.chromium.org/271913002/diff/160001/ash/frame/default_heade... ash/frame/default_header_painter.cc:210: caption_button_container_->Layout(); Isn't there a risk that the caption button container preferred size will be completely wrong if layout hasn't been called yet? I could see instead updating GetPreferredSize to return the larger size if a button is animating out. https://codereview.chromium.org/271913002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right): https://codereview.chromium.org/271913002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_header_painter_ash.cc:243: caption_button_container_->Layout(); Ditto, isn't there a risk of using an invalid preferred size to set the caption button container position?
https://codereview.chromium.org/271913002/diff/160001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/160001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:159: size_button_->layer()->SetVisible(true); On 2014/05/22 15:46:18, flackr wrote: > nit: Comment why this is necessary. Done. https://codereview.chromium.org/271913002/diff/160001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:203: int offset_x = 0; On 2014/05/22 15:46:18, flackr wrote: > nit: The offset_x usage is sufficiently complex that it wouldn't hurt to have a > bit of commenting as to what it does. It offsets the initial position so that > buttons slide into place when other buttons are removed? Done. https://codereview.chromium.org/271913002/diff/160001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc (right): https://codereview.chromium.org/271913002/diff/160001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:171: initial_minimize_button_bounds.right()); On 2014/05/22 15:46:18, flackr wrote: > And close and size buttons should be flush? Done. https://codereview.chromium.org/271913002/diff/160001/ash/frame/default_heade... File ash/frame/default_header_painter.cc (right): https://codereview.chromium.org/271913002/diff/160001/ash/frame/default_heade... ash/frame/default_header_painter.cc:210: caption_button_container_->Layout(); GetPreferredSize calculates based on visibility of the components, and their desired layout. Layout is restricted to the bounds being set. So this order is needed to handle a change in desired size. On 2014/05/22 15:46:18, flackr wrote: > Isn't there a risk that the caption button container preferred size will be > completely wrong if layout hasn't been called yet? > > I could see instead updating GetPreferredSize to return the larger size if a > button is animating out. https://codereview.chromium.org/271913002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right): https://codereview.chromium.org/271913002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_header_painter_ash.cc:243: caption_button_container_->Layout(); No, as preferred size reflects the current visibility. While layout is restricted to the bounds set on the view. On 2014/05/22 15:46:18, flackr wrote: > Ditto, isn't there a risk of using an invalid preferred size to set the caption > button container position?
LGTM with nits. https://codereview.chromium.org/271913002/diff/180001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/180001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:163: // property, and keeps the layer visible. I would just say, Because we delay calling View::SetVisible(false) until the end of the animation, if SetVisible(true) is called mid-animation, the View still believes it is visible and will not update the target layer visibility. https://codereview.chromium.org/271913002/diff/180001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:400: !size_button_->layer()->GetTargetVisibility()) nit: multiline if condition needs braces { }.
Hi, Could you review my change to animate window controls when entering maximize mode? https://codereview.chromium.org/271913002/diff/180001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/180001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:163: // property, and keeps the layer visible. On 2014/05/23 15:17:26, flackr wrote: > I would just say, > Because we delay calling View::SetVisible(false) until the end of the animation, > if SetVisible(true) is called mid-animation, the View still believes it is > visible and will not update the target layer visibility. Done. https://codereview.chromium.org/271913002/diff/180001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:400: !size_button_->layer()->GetTargetVisibility()) On 2014/05/23 15:17:26, flackr wrote: > nit: multiline if condition needs braces { }. Done.
lgtm https://codereview.chromium.org/271913002/diff/220001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/220001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:147: // because Shell::IsMaximizeWindowManagerEnabled is still false at the Can we fix this now (in separate CL)? https://codereview.chromium.org/271913002/diff/220001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:157: size_button_->SetVisible(visible); true (to be consistent with SetVisible below)
Review comments have been addressed. jochen would you be able to provide an owner review for the changes to chrome/browser files? https://codereview.chromium.org/271913002/diff/220001/ash/frame/caption_butto... File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right): https://codereview.chromium.org/271913002/diff/220001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:147: // because Shell::IsMaximizeWindowManagerEnabled is still false at the On 2014/05/23 23:25:57, oshima wrote: > Can we fix this now (in separate CL)? This is currently being addressed in https://codereview.chromium.org/308683002/ These animation changes will be merged into the review once they land. https://codereview.chromium.org/271913002/diff/220001/ash/frame/caption_butto... ash/frame/caption_buttons/frame_caption_button_container_view.cc:157: size_button_->SetVisible(visible); On 2014/05/23 23:25:57, oshima wrote: > true (to be consistent with SetVisible below) Done.
On 2014/05/29 15:45:42, jonross wrote: > Review comments have been addressed. > > jochen would you be able to provide an owner review for the changes to > chrome/browser files? please pick a reviewer from an OWNERS file closer to your changes
Hi James, Could you review the changes to: chrome/browser/ui/views/frame/browser_header_painter_ash.cc chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc Narrowing the scope of selected owners.
LGTM for frame
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/271913002/240001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
Message was sent while issue was closed.
Change committed as 274371
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/309973002/ by tzik@chromium.org. The reason for reverting is: This CL seems to cause Linux Chromium OS ASan LSan bots failure. The failure logs are: http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20... http://build.chromium.org/p/chromium.memory/buildstatus?builder=Linux%20Chrom... http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20... . |
