overview: Cleanup some TODOs.

This CL fixes:
Consolidates TestShellObserver in window selector controller unittest and
TestOverviewObserver in window_selector_unittest. Removed two overlapping
tests.

Remove a TODO about bouncing a window, since a toast shows anyways.

Remove a enum for the header layout which is not needed anymore
because the header always fades in at the final location now. Previously
there was this enum to differentiate when the header animated with
the window, or when there was color on header which had to be slided
in.

Test: none
Bug: none
Change-Id: I7578b2809fda56b8f783875eef12f12ae6e57dd1
Reviewed-on: https://chromium-review.googlesource.com/c/1345536Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610917}

ash/wm/overview/window_selector_controller.cc

@@ -512,7 +512,6 @@ void WindowSelectorController::OnOverviewButtonTrayLongPressed(
 
   // Do nothing if no item was retrieved, or if the retrieved item is
   // unsnappable.
-  // TODO(sammiequon): Bounce the window if it is not snappable.
   if (!item_to_snap ||
       !split_view_controller->CanSnap(item_to_snap->GetWindow())) {
     return;

ash/wm/overview/window_selector_controller_unittest.cc

@@ -7,6 +7,7 @@
 #include "ash/shell.h"
 #include "ash/shell_observer.h"
 #include "ash/test/ash_test_base.h"
+#include "ash/wm/overview/window_selector.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller_test_api.h"
 #include "ash/wm/window_resizer.h"
 #include "ash/wm/window_util.h"
@@ -31,8 +32,6 @@ gfx::Point CalculateDragPoint(const WindowResizer& resizer,
   return location;
 }
 
-// TODO(sammiequan): consolidate this and TestOverviewObserver
-// in window_selector_unittest.
 class TestShellObserver : public ShellObserver {
  public:
   enum AnimationState {
@@ -41,15 +40,26 @@ class TestShellObserver : public ShellObserver {
     CANCELED,
   };
 
-  TestShellObserver() { Shell::Get()->AddShellObserver(this); }
+  explicit TestShellObserver(bool should_monitor_animation_state)
+      : should_monitor_animation_state_(should_monitor_animation_state) {
+    Shell::Get()->AddShellObserver(this);
+  }
   ~TestShellObserver() override { Shell::Get()->RemoveShellObserver(this); }
 
   // ShellObserver:
+  void OnOverviewModeStarting() override { UpdateLastAnimationWasSlide(); }
+  void OnOverviewModeEnding() override { UpdateLastAnimationWasSlide(); }
   void OnOverviewModeStartingAnimationComplete(bool canceled) override {
+    if (!should_monitor_animation_state_)
+      return;
+
     EXPECT_EQ(UNKNOWN, starting_animation_state_);
     starting_animation_state_ = canceled ? CANCELED : COMPLETED;
   }
   void OnOverviewModeEndingAnimationComplete(bool canceled) override {
+    if (!should_monitor_animation_state_)
+      return;
+
     EXPECT_EQ(UNKNOWN, ending_animation_state_);
     ending_animation_state_ = canceled ? CANCELED : COMPLETED;
   }
@@ -67,10 +77,25 @@ class TestShellObserver : public ShellObserver {
   AnimationState ending_animation_state() const {
     return ending_animation_state_;
   }
+  bool last_animation_was_slide() const { return last_animation_was_slide_; }
 
  private:
+  void UpdateLastAnimationWasSlide() {
+    WindowSelector* selector =
+        Shell::Get()->window_selector_controller()->window_selector();
+    DCHECK(selector);
+    last_animation_was_slide_ =
+        selector->enter_exit_overview_type() ==
+        WindowSelector::EnterExitOverviewType::kWindowsMinimized;
+  }
+
   AnimationState starting_animation_state_ = UNKNOWN;
   AnimationState ending_animation_state_ = UNKNOWN;
+  bool last_animation_was_slide_ = false;
+  // If false, skips the checks in OnOverviewMode Starting/Ending
+  // AnimationComplete.
+  bool should_monitor_animation_state_;
+
   DISALLOW_COPY_AND_ASSIGN(TestShellObserver);
 };
 
@@ -97,7 +122,7 @@ TEST_F(WindowSelectorControllerTest,
 TEST_F(WindowSelectorControllerTest, AnimationCallbacks) {
   ui::ScopedAnimationDurationScaleMode non_zero(
       ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
-  TestShellObserver observer;
+  TestShellObserver observer(/*should_monitor_animation_state = */ true);
   // Enter without windows.
   Shell::Get()->window_selector_controller()->ToggleOverview();
   EXPECT_TRUE(Shell::Get()->window_selector_controller()->IsSelecting());
@@ -147,6 +172,58 @@ TEST_F(WindowSelectorControllerTest, AnimationCallbacks) {
   EXPECT_EQ(TestShellObserver::CANCELED, observer.starting_animation_state());
 }
 
+// Tests the slide animation for overview is never used in clamshell.
+TEST_F(WindowSelectorControllerTest, OverviewEnterExitAnimationClamshell) {
+  TestShellObserver observer(/*should_monitor_animation_state = */ false);
+
+  const gfx::Rect bounds(200, 200);
+  std::unique_ptr<aura::Window> window(
+      CreateTestWindowInShellWithBounds(bounds));
+
+  Shell::Get()->window_selector_controller()->ToggleOverview();
+  EXPECT_FALSE(observer.last_animation_was_slide());
+
+  Shell::Get()->window_selector_controller()->ToggleOverview();
+  EXPECT_FALSE(observer.last_animation_was_slide());
+
+  // Even with all window minimized, there should not be a slide animation.
+  ASSERT_FALSE(Shell::Get()->window_selector_controller()->IsSelecting());
+  wm::GetWindowState(window.get())->Minimize();
+  Shell::Get()->window_selector_controller()->ToggleOverview();
+  EXPECT_FALSE(observer.last_animation_was_slide());
+}
+
+// Tests the slide animation for overview is used in tablet if all windows
+// are minimized, and that if overview is exited from the home launcher all
+// windows are minimized.
+TEST_F(WindowSelectorControllerTest, OverviewEnterExitAnimationTablet) {
+  TestShellObserver observer(/*should_monitor_animation_state = */ false);
+
+  // Ensure calls to EnableTabletModeWindowManager complete.
+  base::RunLoop().RunUntilIdle();
+  Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(true);
+  base::RunLoop().RunUntilIdle();
+
+  const gfx::Rect bounds(200, 200);
+  std::unique_ptr<aura::Window> window(
+      CreateTestWindowInShellWithBounds(bounds));
+
+  Shell::Get()->window_selector_controller()->ToggleOverview();
+  EXPECT_FALSE(observer.last_animation_was_slide());
+
+  // Exit to home launcher. Slide animation should be used, and all windows
+  // should be minimized.
+  Shell::Get()->window_selector_controller()->ToggleOverview(
+      WindowSelector::EnterExitOverviewType::kWindowsMinimized);
+  EXPECT_TRUE(observer.last_animation_was_slide());
+  ASSERT_FALSE(Shell::Get()->window_selector_controller()->IsSelecting());
+  EXPECT_TRUE(wm::GetWindowState(window.get())->IsMinimized());
+
+  // All windows are minimized, so we should use the slide animation.
+  Shell::Get()->window_selector_controller()->ToggleOverview();
+  EXPECT_TRUE(observer.last_animation_was_slide());
+}
+
 class OverviewVirtualKeyboardTest : public WindowSelectorControllerTest {
  protected:
   void SetUp() override {

ash/wm/overview/window_selector_item.cc

@@ -571,7 +571,6 @@ void WindowSelectorItem::Shutdown() {
 void WindowSelectorItem::PrepareForOverview() {
   transform_window_.PrepareForOverview();
   RestackItemWidget();
-  UpdateHeaderLayout(HeaderFadeInMode::kEnter, OVERVIEW_ANIMATION_NONE);
 }
 
 void WindowSelectorItem::SlideWindowIn() {
@@ -660,9 +659,7 @@ void WindowSelectorItem::SetBounds(const gfx::Rect& target_bounds,
   // If |target_bounds_| is empty, this is the first update. Let
   // UpdateHeaderLayout know, as we do not want |item_widget_| to be animated
   // with the window.
-  HeaderFadeInMode mode = target_bounds_.IsEmpty()
-                              ? HeaderFadeInMode::kFirstUpdate
-                              : HeaderFadeInMode::kUpdate;
+  const bool is_first_update = target_bounds_.IsEmpty();
   target_bounds_ = target_bounds;
 
   gfx::Rect inset_bounds(target_bounds);
@@ -671,16 +668,14 @@ void WindowSelectorItem::SetBounds(const gfx::Rect& target_bounds,
   // Do not animate if entering when the window is minimized, as it will be
   // faded in. We still want to animate if the position is changed after
   // entering.
-  if (wm::GetWindowState(GetWindow())->IsMinimized() &&
-      mode == HeaderFadeInMode::kFirstUpdate) {
+  if (wm::GetWindowState(GetWindow())->IsMinimized() && is_first_update)
     new_animation_type = OVERVIEW_ANIMATION_NONE;
-  }
-
-  SetItemBounds(inset_bounds, new_animation_type);
 
   // SetItemBounds is called before UpdateHeaderLayout so the header can
   // properly use the updated windows bounds.
-  UpdateHeaderLayout(mode, new_animation_type);
+  SetItemBounds(inset_bounds, new_animation_type);
+  UpdateHeaderLayout(is_first_update ? OVERVIEW_ANIMATION_NONE
+                                     : new_animation_type);
 
   // Shadow is normally set after an animation is finished. In the case of no
   // animations, manually set the shadow. Shadow relies on both the window
@@ -1184,7 +1179,6 @@ void WindowSelectorItem::CreateWindowLabel(const base::string16& title) {
 }
 
 void WindowSelectorItem::UpdateHeaderLayout(
-    HeaderFadeInMode mode,
     OverviewAnimationType animation_type) {
   gfx::Rect transformed_window_bounds =
       transform_window_.window_selector_bounds().value_or(
@@ -1195,17 +1189,11 @@ void WindowSelectorItem::UpdateHeaderLayout(
   label_rect.set_width(transformed_window_bounds.width());
   // For tabbed windows the initial bounds of the caption are set such that it
   // appears to be "growing" up from the window content area.
-  label_rect.set_y(
-      (mode != HeaderFadeInMode::kEnter || transform_window_.GetTopInset())
-          ? -label_rect.height()
-          : 0);
+  label_rect.set_y(-label_rect.height());
 
   aura::Window* widget_window = item_widget_->GetNativeWindow();
-  // For the first update, place the widget at its destination.
-  ScopedOverviewAnimationSettings animation_settings(
-      mode == HeaderFadeInMode::kFirstUpdate ? OVERVIEW_ANIMATION_NONE
-                                             : animation_type,
-      widget_window);
+  ScopedOverviewAnimationSettings animation_settings(animation_type,
+                                                     widget_window);
 
   // Create a start animation observer if this is an enter overview layout
   // animation.
@@ -1219,10 +1207,8 @@ void WindowSelectorItem::UpdateHeaderLayout(
   // |widget_window| covers both the transformed window and the header
   // as well as the gap between the windows to prevent events from reaching
   // the window including its sizing borders.
-  if (mode != HeaderFadeInMode::kEnter) {
     label_rect.set_height(close_button_->GetPreferredSize().height() +
                           transformed_window_bounds.height());
-  }
   label_rect.Inset(-kWindowSelectorMargin, -kWindowSelectorMargin);
   widget_window->SetBounds(label_rect);
   gfx::Transform label_transform;

ash/wm/overview/window_selector_item.h

@@ -281,20 +281,6 @@ class ASH_EXPORT WindowSelectorItem : public views::ButtonListener,
   FRIEND_TEST_ALL_PREFIXES(SplitViewWindowSelectorTest,
                            OverviewUnsnappableIndicatorVisibility);
 
-  // The different ways the overview header can fade in and be laid out.
-  // TODO(sammiequon): See if we can combine this with
-  // WindowSelector::OverviewTransition.
-  enum class HeaderFadeInMode {
-    // Used when entering overview mode, to fade in the header background color.
-    kEnter,
-    // Used when the overview header bounds change for the first time to
-    // skip animating.
-    kFirstUpdate,
-    // Used when the overview header bounds change, to animate or move the
-    // header to the desired bounds.
-    kUpdate,
-  };
-
   // Sets the bounds of this selector's items to |target_bounds| in
   // |root_window_|. The bounds change will be animated as specified
   // by |animation_type|.
@@ -304,13 +290,9 @@ class ASH_EXPORT WindowSelectorItem : public views::ButtonListener,
   // Creates the window label.
   void CreateWindowLabel(const base::string16& title);
 
-  // Updates the close button's and title label's bounds. Any change in bounds
-  // will be animated from the current bounds to the new bounds as per the
-  // |animation_type|. |mode| allows distinguishing the first time update which
-  // allows setting the initial bounds properly or exiting overview to fade out
-  // gradually.
-  void UpdateHeaderLayout(HeaderFadeInMode mode,
-                          OverviewAnimationType animation_type);
+  // Updates the |item_widget|'s bounds. Any change in bounds will be animated
+  // from the current bounds to the new bounds as per the |animation_type|.
+  void UpdateHeaderLayout(OverviewAnimationType animation_type);
 
   // Animates opacity of the |transform_window_| and its caption to |opacity|
   // using |animation_type|.

ash/wm/overview/window_selector_unittest.cc

@@ -3076,162 +3076,6 @@ TEST_F(WindowSelectorTest, PositionWindows) {
   EXPECT_NE(bounds3, item3->target_bounds());
 }
 
-namespace {
-
-// Test class that allows us to check what whether the last overview enter or
-// exit was using a slide animation. This is needed because the cached slide
-// animation variable may be reset or the WindowSelector object may not be
-// available after a toggle has completed. Also stores whether the animation
-// complete observers fired because an animation completed or was canceled.
-class TestOverviewObserver : public ShellObserver {
- public:
-  TestOverviewObserver() { Shell::Get()->AddShellObserver(this); }
-  ~TestOverviewObserver() override { Shell::Get()->RemoveShellObserver(this); }
-
-  // ShellObserver:
-  void OnOverviewModeStarting() override { UpdateLastAnimationWasSlide(); }
-  void OnOverviewModeEnding() override { UpdateLastAnimationWasSlide(); }
-  void OnOverviewModeStartingAnimationComplete(bool canceled) override {
-    animation_canceled_ = canceled;
-  }
-  void OnOverviewModeEndingAnimationComplete(bool canceled) override {
-    animation_canceled_ = canceled;
-  }
-
-  bool last_animation_was_slide() const { return last_animation_was_slide_; }
-  bool animation_canceled() const { return animation_canceled_; }
-
- private:
-  void UpdateLastAnimationWasSlide() {
-    WindowSelector* selector =
-        Shell::Get()->window_selector_controller()->window_selector();
-    DCHECK(selector);
-    last_animation_was_slide_ =
-        selector->enter_exit_overview_type() ==
-        WindowSelector::EnterExitOverviewType::kWindowsMinimized;
-  }
-
-  bool last_animation_was_slide_ = false;
-  bool animation_canceled_ = false;
-
-  DISALLOW_COPY_AND_ASSIGN(TestOverviewObserver);
-};
-
-}  // namespace
-
-// Tests the slide animation for overview is never used in clamshell.
-TEST_F(WindowSelectorTest, OverviewEnterExitAnimation) {
-  TestOverviewObserver observer;
-
-  const gfx::Rect bounds(200, 200);
-  std::unique_ptr<aura::Window> window(CreateWindow(bounds));
-
-  ToggleOverview();
-  EXPECT_FALSE(observer.last_animation_was_slide());
-
-  ToggleOverview();
-  EXPECT_FALSE(observer.last_animation_was_slide());
-
-  // Even with all window minimized, there should not be a slide animation.
-  ASSERT_FALSE(IsSelecting());
-  wm::GetWindowState(window.get())->Minimize();
-  ToggleOverview();
-  EXPECT_FALSE(observer.last_animation_was_slide());
-}
-
-// Tests the slide animation for overview is used in tablet if all windows
-// are minimized, and that if overview is exited from the home launcher all
-// windows are minimized.
-TEST_F(WindowSelectorTest, OverviewEnterExitAnimationTablet) {
-  TestOverviewObserver observer;
-
-  // Ensure calls to EnableTabletModeWindowManager complete.
-  base::RunLoop().RunUntilIdle();
-  Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(true);
-  base::RunLoop().RunUntilIdle();
-
-  const gfx::Rect bounds(200, 200);
-  std::unique_ptr<aura::Window> window(CreateWindow(bounds));
-
-  ToggleOverview();
-  EXPECT_FALSE(observer.last_animation_was_slide());
-
-  // Exit to home launcher. Slide animation should be used, and all windows
-  // should be minimized.
-  ToggleOverview(WindowSelector::EnterExitOverviewType::kWindowsMinimized);
-  EXPECT_TRUE(observer.last_animation_was_slide());
-  ASSERT_FALSE(IsSelecting());
-  EXPECT_TRUE(wm::GetWindowState(window.get())->IsMinimized());
-
-  // All windows are minimized, so we should use the slide animation.
-  ToggleOverview();
-  EXPECT_TRUE(observer.last_animation_was_slide());
-}
-
-// Tests that the overview enter animation observer works as expected.
-TEST_F(WindowSelectorTest, OverviewEnterAnimationObserver) {
-  TestOverviewObserver observer;
-
-  ui::ScopedAnimationDurationScaleMode animation_scale_mode(
-      ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
-
-  const gfx::Rect bounds(200, 200);
-  std::unique_ptr<aura::Window> window(CreateWindow(bounds));
-
-  // Test that if the animations are allowed to run out on enter the observer
-  // will be notified of complete animations.
-  ToggleOverview();
-  WindowSelectorItem* item = GetWindowItemForWindow(0, window.get());
-  window->layer()->GetAnimator()->StopAnimating();
-  item_widget(item)->GetNativeWindow()->layer()->GetAnimator()->StopAnimating();
-  EXPECT_FALSE(observer.animation_canceled());
-
-  ToggleOverview();
-
-  // Test that if the animations are canceled after entering by exiting overview
-  // right away, the observer will be notified of incomplete animations.
-  ToggleOverview();
-  ToggleOverview();
-  EXPECT_TRUE(observer.animation_canceled());
-}
-
-// Tests that the overview exit animation observer works as expected.
-TEST_F(WindowSelectorTest, OverviewExitAnimationObserver) {
-  TestOverviewObserver observer;
-
-  ui::ScopedAnimationDurationScaleMode animation_scale_mode(
-      ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
-
-  const gfx::Rect bounds(200, 200);
-  std::unique_ptr<aura::Window> window(CreateWindow(bounds));
-
-  ToggleOverview();
-
-  // Test that if the animations are allowed to run out on exit the observer
-  // will be notified of complete animations.
-  ToggleOverview();
-  window->layer()->GetAnimator()->StopAnimating();
-  std::vector<std::unique_ptr<DelayedAnimationObserver>>& delayed_animations =
-      window_selector_controller()->delayed_animations_;
-  // On animation complete |delayed_animations| will erase its own members, so
-  // use a while loop to avoid indexing errors.
-  ASSERT_FALSE(delayed_animations.empty());
-  while (!delayed_animations.empty()) {
-    views::Widget* item_widget =
-        static_cast<CleanupAnimationObserver*>(delayed_animations.back().get())
-            ->widget_.get();
-    item_widget->GetNativeWindow()->layer()->GetAnimator()->StopAnimating();
-  }
-  EXPECT_FALSE(observer.animation_canceled());
-
-  // Test that if the animations are canceled after exiting by reentering
-  // overview right away, the observer will be notified of incomplete
-  // animations.
-  ToggleOverview();
-  ToggleOverview();
-  EXPECT_TRUE(observer.animation_canceled());
-}
-
 // Tests that overview mode is entered with kWindowDragged mode when an app is
 // dragged from the top of the screen.
 TEST_F(WindowSelectorTest, DraggingFromTopAnimation) {