diff options
author | jonross@chromium.org <jonross@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-06-19 22:50:43 +0000 |
---|---|---|
committer | jonross@chromium.org <jonross@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-06-19 22:50:43 +0000 |
commit | 50964633964641ef6ecc46e11803c01b88326c15 (patch) | |
tree | f4d3ee11c8fb5fa1ed30e6ebf4e247d3b5d55cfb /ash | |
parent | d01f31157323886cac62425781056ae7bf6c09ba (diff) | |
download | chromium_src-50964633964641ef6ecc46e11803c01b88326c15.zip chromium_src-50964633964641ef6ecc46e11803c01b88326c15.tar.gz chromium_src-50964633964641ef6ecc46e11803c01b88326c15.tar.bz2 |
Revert of Reland Window Control Animations for TouchView. (https://codereview.chromium.org/316693002/)
Reason for revert:
In Immersice Fullscreen, when a mouse hovers near the top of the screen the header animates in. This is done by animating TopContainerView. However this view does not contain all of the title bar, and forces a paint to its canvas. With FrameCaptionButtonContainerView painting to a layer it no longer paints to the canvas of TopContainerView. Due to z-order of the views FrameCaptionButtonContainer exists below the TopContainerView as well as below the ClientView of the browser.
Reverting the use of layers in M37.
For M38 the use of layers can be rexamined, however changes to the header views, or how immersive mode renders, will be needed before relanding this.
Original issue's description:
> Reland Window Control Animations for TouchView
>
> The original change contained a use-after-free bug. This was caused by window crossfade animation during
> maximizing, combined with input event processing. The crossfade would take the current layer and delete it.
> However the next input event to the window tree would attempt to access the now deleted layer.
> Original Code Review: https://codereview.chromium.org/271913002/
> Revert Review: https://codereview.chromium.org/309973002/
> Revert "Revert of Animate window control changes in TouchView (https://codereview.chromium.org/271913002/)"
> This reverts commit 8863218a5e97c4914a5a03df59e9d5d17f53a2cb.
>
> TBR=jamescook@chromium.org
> TEST=FrameCaptionButtonContainerViewTest.AnimationUpdatesLayerTree
> BUG=363717
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276782
TBR=jamescook@chromium.org
BUG=384612, 363717
Review URL: https://codereview.chromium.org/343433009
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278520 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'ash')
7 files changed, 12 insertions, 266 deletions
diff --git a/ash/frame/caption_buttons/frame_caption_button.cc b/ash/frame/caption_buttons/frame_caption_button.cc index df41fb4..c8d71c3 100644 --- a/ash/frame/caption_buttons/frame_caption_button.cc +++ b/ash/frame/caption_buttons/frame_caption_button.cc @@ -5,11 +5,9 @@ #include "ash/frame/caption_buttons/frame_caption_button.h" #include "ui/base/resource/resource_bundle.h" -#include "ui/compositor/layer.h" #include "ui/gfx/animation/slide_animation.h" #include "ui/gfx/animation/throb_animation.h" #include "ui/gfx/canvas.h" -#include "ui/views/widget/widget.h" namespace ash { @@ -39,10 +37,6 @@ FrameCaptionButton::FrameCaptionButton(views::ButtonListener* listener, swap_images_animation_(new gfx::SlideAnimation(this)) { swap_images_animation_->Reset(1); - SetPaintToLayer(true); - SetFillsBoundsOpaquely(false); - set_layer_owner_delegate(this); - // Do not flip the gfx::Canvas passed to the OnPaint() method. The snap left // and snap right button icons should not be flipped. The other icons are // horizontally symmetrical. @@ -180,9 +174,4 @@ void FrameCaptionButton::PaintCentered(gfx::Canvas* canvas, paint); } -void FrameCaptionButton::OnLayerRecreated(ui::Layer* old_layer, - ui::Layer* new_layer) { - GetWidget()->UpdateRootLayers(); -} - } // namespace ash diff --git a/ash/frame/caption_buttons/frame_caption_button.h b/ash/frame/caption_buttons/frame_caption_button.h index d42d791..6fd9831 100644 --- a/ash/frame/caption_buttons/frame_caption_button.h +++ b/ash/frame/caption_buttons/frame_caption_button.h @@ -8,7 +8,6 @@ #include "ash/ash_export.h" #include "ash/frame/caption_buttons/caption_button_types.h" #include "base/memory/scoped_ptr.h" -#include "ui/compositor/layer_owner_delegate.h" #include "ui/gfx/image/image_skia.h" #include "ui/views/controls/button/custom_button.h" @@ -20,8 +19,7 @@ namespace ash { // Base class for the window caption buttons (minimize, maximize, restore, // close). -class ASH_EXPORT FrameCaptionButton : public views::CustomButton, - public ui::LayerOwnerDelegate { +class ASH_EXPORT FrameCaptionButton : public views::CustomButton { public: enum Animate { ANIMATE_YES, @@ -74,10 +72,6 @@ class ASH_EXPORT FrameCaptionButton : public views::CustomButton, const gfx::ImageSkia& to_center, int alpha); - // ui::LayerOwnerDelegate: - virtual void OnLayerRecreated(ui::Layer* old_layer, - ui::Layer* new_layer) OVERRIDE; - // The button's current icon. CaptionButtonIcon icon_; diff --git a/ash/frame/caption_buttons/frame_caption_button_container_view.cc b/ash/frame/caption_buttons/frame_caption_button_container_view.cc index 643993e..5cb0529 100644 --- a/ash/frame/caption_buttons/frame_caption_button_container_view.cc +++ b/ash/frame/caption_buttons/frame_caption_button_container_view.cc @@ -16,10 +16,7 @@ #include "grit/ui_strings.h" // Accessibility names #include "ui/base/hit_test.h" #include "ui/base/l10n/l10n_util.h" -#include "ui/compositor/layer.h" #include "ui/compositor/scoped_animation_duration_scale_mode.h" -#include "ui/compositor/scoped_layer_animation_settings.h" -#include "ui/gfx/animation/tween.h" #include "ui/gfx/canvas.h" #include "ui/gfx/insets.h" #include "ui/gfx/point.h" @@ -30,14 +27,6 @@ namespace ash { namespace { -// Visual design parameters for animating the transition to maximize mode. -// When the size button hides we delay sliding the minimize button into its -// location. Also used to delay showing the size button so that the minimize -// button slides out of that position. -const int kAnimationDelayMs = 100; -const int kMinimizeSlideDurationMs = 500; -const int kSizeFadeDurationMs = 250; - // Converts |point| from |src| to |dst| and hittests against |dst|. bool ConvertPointToViewAndHitTest(const views::View* src, const views::View* dst, @@ -60,10 +49,6 @@ FrameCaptionButtonContainerView::FrameCaptionButtonContainerView( minimize_button_(NULL), size_button_(NULL), close_button_(NULL) { - SetPaintToLayer(true); - SetFillsBoundsOpaquely(false); - set_layer_owner_delegate(this); - // Insert the buttons left to right. minimize_button_ = new FrameCaptionButton(this, CAPTION_BUTTON_ICON_MINIMIZE); minimize_button_->SetAccessibleName( @@ -137,47 +122,10 @@ int FrameCaptionButtonContainerView::NonClientHitTest( } void FrameCaptionButtonContainerView::UpdateSizeButtonVisibility() { - bool visible = !Shell::GetInstance()->maximize_mode_controller()-> + size_button_->SetVisible( + !Shell::GetInstance()->maximize_mode_controller()-> IsMaximizeModeWindowManagerEnabled() && - frame_->widget_delegate()->CanMaximize(); - - // Turning visibility off prevents animations from rendering. Setting the - // size button visibility to false will occur after the animation. - if (visible) { - size_button_->SetVisible(true); - // 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. - size_button_->layer()->SetVisible(true); - } - - ui::ScopedLayerAnimationSettings settings( - size_button_->layer()->GetAnimator()); - settings.SetTransitionDuration( - base::TimeDelta::FromMilliseconds(kSizeFadeDurationMs)); - settings.SetPreemptionStrategy( - ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET); - - if (visible) { - settings.SetTweenType(gfx::Tween::EASE_OUT); - // Delay fade in so that the minimize button has begun its sliding - // animation. - size_button_->layer()->GetAnimator()->SchedulePauseForProperties( - base::TimeDelta::FromMilliseconds(kAnimationDelayMs), - ui::LayerAnimationElement::OPACITY); - size_button_->layer()->SetOpacity(1.0f); - } else { - settings.SetTweenType(gfx::Tween::EASE_IN); - // Observer will call size_button_->SetVisible(false) upon completion of - // the animation. - // TODO(jonross): avoid the delayed SetVisible(false) call by acquring - // the size_button's layer before making it invisible. That layer can then - // be animated and deleted upon completion of the animation. See - // LayerOwner::RecreateLayer - settings.AddObserver(this); - size_button_->layer()->SetOpacity(0.0f); - size_button_->layer()->SetVisible(false); - } + frame_->widget_delegate()->CanMaximize()); } gfx::Size FrameCaptionButtonContainerView::GetPreferredSize() const { @@ -191,56 +139,15 @@ gfx::Size FrameCaptionButtonContainerView::GetPreferredSize() const { } void FrameCaptionButtonContainerView::Layout() { - int x = width(); - // Offsets the initial position of a child, so that buttons slide into the - // place as other buttons are added/removed. - int offset_x = 0; - for (int i = child_count() - 1; i >= 0; --i) { + int x = 0; + for (int i = 0; i < child_count(); ++i) { views::View* child = child_at(i); - ui::LayerAnimator* child_animator = child->layer()->GetAnimator(); - bool child_animating = child_animator->is_animating(); - // The actual property visibility is not being animated, otherwise the - // view does not render. - bool child_animating_opacity = child_animator-> - IsAnimatingProperty(ui::LayerAnimationElement::OPACITY); - bool child_target_visibility = child->layer()->GetTargetVisibility(); - - if (child_animating_opacity) { - if (child_target_visibility) - offset_x += child->width(); - else - offset_x -= child->width(); - } - - if (!child->visible() || !child_target_visibility) + if (!child->visible()) continue; - scoped_ptr<ui::ScopedLayerAnimationSettings> animation; gfx::Size size = child->GetPreferredSize(); - x -= size.width(); - - // Animate the button if a previous button is currently animating - // its visibility. - if (offset_x != 0) { - if (!child_animating) - child->SetBounds(x + offset_x, 0, size.width(), size.height()); - if (offset_x < 0) { - // Delay sliding to where the previous button was located. - child_animator->SchedulePauseForProperties( - base::TimeDelta::FromMilliseconds(kAnimationDelayMs), - ui::LayerAnimationElement::BOUNDS); - } - - ui::ScopedLayerAnimationSettings* settings = - new ui::ScopedLayerAnimationSettings(child_animator); - settings->SetTransitionDuration( - base::TimeDelta::FromMilliseconds(kMinimizeSlideDurationMs)); - settings->SetPreemptionStrategy( - ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET); - settings->SetTweenType(gfx::Tween::EASE_OUT); - animation.reset(settings); - } child->SetBounds(x, 0, size.width(), size.height()); + x += size.width(); } } @@ -379,29 +286,6 @@ void FrameCaptionButtonContainerView::SetHoveredAndPressedButtons( } } -void FrameCaptionButtonContainerView::OnImplicitAnimationsCompleted() { - // If there is another animation in the queue, the reverse animation was - // triggered before the completion of animating to invisible. Do not turn off - // the visibility so that the next animation may render. - if (!size_button_->layer()->GetAnimator()->is_animating() && - !size_button_->layer()->GetTargetVisibility()) { - size_button_->SetVisible(false); - } - // TODO(jonross): currently we need to delay telling the parent about the - // size change from visibility. When the size changes this forces a relayout - // and we want to animate both the bounds of FrameCaptionButtonContainerView - // along with that of its children. However when the parent is currently - // having its bounds changed this leads to strange animations where this view - // renders outside of its parents. Create a more specific animation where - // height and y are immediately fixed, and where we only animate width and x. - PreferredSizeChanged(); -} - -void FrameCaptionButtonContainerView::OnLayerRecreated(ui::Layer* old_layer, - ui::Layer* new_layer) { - GetWidget()->UpdateRootLayers(); -} - FrameCaptionButtonContainerView::ButtonIconIds::ButtonIconIds() : icon_image_id(-1), inactive_icon_image_id(-1), diff --git a/ash/frame/caption_buttons/frame_caption_button_container_view.h b/ash/frame/caption_buttons/frame_caption_button_container_view.h index 39191b2..579d5cd 100644 --- a/ash/frame/caption_buttons/frame_caption_button_container_view.h +++ b/ash/frame/caption_buttons/frame_caption_button_container_view.h @@ -9,8 +9,6 @@ #include "ash/ash_export.h" #include "ash/frame/caption_buttons/frame_size_button_delegate.h" -#include "ui/compositor/layer_animation_observer.h" -#include "ui/compositor/layer_owner_delegate.h" #include "ui/views/controls/button/button.h" #include "ui/views/view.h" @@ -25,9 +23,7 @@ namespace ash { class ASH_EXPORT FrameCaptionButtonContainerView : public views::View, public views::ButtonListener, - public FrameSizeButtonDelegate, - public ui::ImplicitAnimationObserver, - public ui::LayerOwnerDelegate { + public FrameSizeButtonDelegate { public: static const char kViewClassName[]; @@ -142,13 +138,6 @@ class ASH_EXPORT FrameCaptionButtonContainerView const FrameCaptionButton* to_hover, const FrameCaptionButton* to_press) OVERRIDE; - // ui::ImplicitAnimationObserver: - virtual void OnImplicitAnimationsCompleted() OVERRIDE; - - // ui::LayerOwnerDelegate: - virtual void OnLayerRecreated(ui::Layer* old_layer, - ui::Layer* new_layer) OVERRIDE; - // The widget that the buttons act on. views::Widget* frame_; diff --git a/ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc b/ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc index 30e3317..acd72c8 100644 --- a/ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc +++ b/ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc @@ -5,12 +5,8 @@ #include "ash/frame/caption_buttons/frame_caption_button_container_view.h" #include "ash/frame/caption_buttons/frame_caption_button.h" -#include "ash/shell.h" #include "ash/test/ash_test_base.h" -#include "ash/wm/maximize_mode/maximize_mode_controller.h" #include "grit/ash_resources.h" -#include "ui/compositor/layer.h" -#include "ui/gfx/geometry/rect.h" #include "ui/views/widget/widget.h" #include "ui/views/widget/widget_delegate.h" @@ -112,7 +108,6 @@ TEST_F(FrameCaptionButtonContainerViewTest, ButtonVisibility) { FrameCaptionButtonContainerView container1(widget_can_maximize.get(), FrameCaptionButtonContainerView::MINIMIZE_ALLOWED); SetMockImages(&container1); - container1.SetBoundsRect(gfx::Rect(container1.GetPreferredSize())); container1.Layout(); FrameCaptionButtonContainerView::TestApi t1(&container1); EXPECT_TRUE(t1.minimize_button()->visible()); @@ -128,7 +123,6 @@ TEST_F(FrameCaptionButtonContainerViewTest, ButtonVisibility) { FrameCaptionButtonContainerView container2(widget_cannot_maximize.get(), FrameCaptionButtonContainerView::MINIMIZE_ALLOWED); SetMockImages(&container2); - container2.SetBoundsRect(gfx::Rect(container2.GetPreferredSize())); container2.Layout(); FrameCaptionButtonContainerView::TestApi t2(&container2); EXPECT_TRUE(t2.minimize_button()->visible()); @@ -142,7 +136,6 @@ TEST_F(FrameCaptionButtonContainerViewTest, ButtonVisibility) { FrameCaptionButtonContainerView container3(widget_cannot_maximize.get(), FrameCaptionButtonContainerView::MINIMIZE_DISALLOWED); SetMockImages(&container3); - container3.SetBoundsRect(gfx::Rect(container3.GetPreferredSize())); container3.Layout(); FrameCaptionButtonContainerView::TestApi t3(&container3); EXPECT_FALSE(t3.minimize_button()->visible()); @@ -152,97 +145,4 @@ TEST_F(FrameCaptionButtonContainerViewTest, ButtonVisibility) { &container3, *t3.close_button(), *t3.close_button())); } -// Tests that the layout animations trigered by button visibilty result in the -// correct placement of the buttons. -TEST_F(FrameCaptionButtonContainerViewTest, - TestUpdateSizeButtonVisibilityAnimation) { - scoped_ptr<views::Widget> widget_can_maximize( - CreateTestWidget(MAXIMIZE_ALLOWED)); - FrameCaptionButtonContainerView container(widget_can_maximize.get(), - FrameCaptionButtonContainerView::MINIMIZE_ALLOWED); - SetMockImages(&container); - container.SetBoundsRect(gfx::Rect(container.GetPreferredSize())); - container.Layout(); - - FrameCaptionButtonContainerView::TestApi test(&container); - gfx::Rect initial_minimize_button_bounds = test.minimize_button()->bounds(); - gfx::Rect initial_size_button_bounds = test.size_button()->bounds(); - gfx::Rect initial_close_button_bounds = test.close_button()->bounds(); - gfx::Rect initial_container_bounds = container.bounds(); - - ASSERT_EQ(initial_size_button_bounds.x(), - initial_minimize_button_bounds.right()); - ASSERT_EQ(initial_close_button_bounds.x(), - initial_size_button_bounds.right()); - - // Hidden size button should result in minimize button animating to the - // right. The size button should not be visible, but should not have moved. - Shell::GetInstance()->maximize_mode_controller()-> - EnableMaximizeModeWindowManager(true); - container.UpdateSizeButtonVisibility(); - container.Layout(); - - EXPECT_TRUE(test.minimize_button()->visible()); - EXPECT_FALSE(test.size_button()->visible()); - EXPECT_TRUE(test.close_button()->visible()); - gfx::Rect minimize_button_bounds = test.minimize_button()->bounds(); - gfx::Rect close_button_bounds = test.close_button()->bounds(); - EXPECT_EQ(close_button_bounds.x(), minimize_button_bounds.right()); - EXPECT_EQ(initial_size_button_bounds, test.size_button()->bounds()); - EXPECT_EQ(initial_close_button_bounds, close_button_bounds); - EXPECT_LT(container.GetPreferredSize().width(), - initial_container_bounds.width()); - - // Revealing the size button should cause the minimze button to return to its - // original position. - Shell::GetInstance()->maximize_mode_controller()-> - EnableMaximizeModeWindowManager(false); - container.UpdateSizeButtonVisibility(); - container.Layout(); - EXPECT_TRUE(test.minimize_button()->visible()); - EXPECT_TRUE(test.size_button()->visible()); - EXPECT_TRUE(test.close_button()->visible()); - EXPECT_EQ(initial_minimize_button_bounds, test.minimize_button()->bounds()); - EXPECT_EQ(initial_size_button_bounds, test.size_button()->bounds()); - EXPECT_EQ(initial_close_button_bounds, test.close_button()->bounds()); - EXPECT_EQ(container.GetPreferredSize().width(), - initial_container_bounds.width()); -} - -// Tests that after an animation to maximized state, that the layer tree has -// been updated to reflect the swapped layers. -TEST_F(FrameCaptionButtonContainerViewTest, AnimationUpdatesLayerTree) { - scoped_ptr<views::Widget> widget_can_maximize( - CreateTestWidget(MAXIMIZE_ALLOWED)); - FrameCaptionButtonContainerView container(widget_can_maximize.get(), - FrameCaptionButtonContainerView::MINIMIZE_ALLOWED); - SetMockImages(&container); - container.SetBoundsRect(gfx::Rect(container.GetPreferredSize())); - container.Layout(); - ui::Layer* original_layer = container.layer(); - - // The container must be a child of the window to be affected by the - // animation. - widget_can_maximize->non_client_view()->AddChildView(&container); - widget_can_maximize->Show(); - // The original cache of the layer tree must be created before the animation - widget_can_maximize->UpdateRootLayers(); - widget_can_maximize->GetRootLayers(); - - // After the maximizing animation has completed, |original_layer| will have - // been deleted. Dereferencing it could cause a segfault. - widget_can_maximize->Maximize(); - RunAllPendingInMessageLoop(); - EXPECT_TRUE(widget_can_maximize->IsMaximized()); - - std::vector<ui::Layer*> layers = widget_can_maximize->GetRootLayers(); - - EXPECT_GT(layers.size(), 0u); - EXPECT_NE(layers.end(), - std::find(layers.begin(), layers.end(), container.layer())); - EXPECT_EQ(layers.end(), - std::find(layers.begin(), layers.end(), original_layer)); - EXPECT_NE(original_layer, container.layer()); -} - } // namespace ash diff --git a/ash/frame/custom_frame_view_ash.cc b/ash/frame/custom_frame_view_ash.cc index 508f830..3aa503c 100644 --- a/ash/frame/custom_frame_view_ash.cc +++ b/ash/frame/custom_frame_view_ash.cc @@ -153,7 +153,6 @@ class CustomFrameViewAsh::HeaderView // views::View: virtual void Layout() OVERRIDE; virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE; - virtual void ChildPreferredSizeChanged(views::View* child) OVERRIDE; // ShellObserver: virtual void OnMaximizeModeStarted() OVERRIDE; @@ -288,17 +287,6 @@ void CustomFrameViewAsh::HeaderView::OnPaint(gfx::Canvas* canvas) { header_painter_->PaintHeader(canvas, header_mode); } -void CustomFrameViewAsh::HeaderView:: - ChildPreferredSizeChanged(views::View* child) { - // FrameCaptionButtonContainerView animates the visibility changes in - // UpdateSizeButtonVisibility(false). Due to this a new size is not available - // until the completion of the animation. Layout it response to the preferred - // size changes. - if (child != caption_button_container_) - return; - parent()->Layout(); -} - /////////////////////////////////////////////////////////////////////////////// // CustomFrameViewAsh::HeaderView, ShellObserver overrides: diff --git a/ash/frame/default_header_painter.cc b/ash/frame/default_header_painter.cc index e8a46ef..fd31d77 100644 --- a/ash/frame/default_header_painter.cc +++ b/ash/frame/default_header_painter.cc @@ -200,6 +200,8 @@ void DefaultHeaderPainter::PaintHeader(gfx::Canvas* canvas, Mode mode) { } void DefaultHeaderPainter::LayoutHeader() { + caption_button_container_->Layout(); + gfx::Size caption_button_container_size = caption_button_container_->GetPreferredSize(); caption_button_container_->SetBounds( @@ -207,7 +209,7 @@ void DefaultHeaderPainter::LayoutHeader() { 0, caption_button_container_size.width(), caption_button_container_size.height()); - caption_button_container_->Layout(); + if (window_icon_) { // Vertically center the window icon with respect to the caption button // container. |