Remove HomeLauncherGestureHandlerObserver - try 2

Original attempt CL:1888652 got reverted because a new usage popped up.

HomeLauncherGestureHandler currently has three observers:
1.  AppListControllerImpl, which is also a HomeScreenDelegate
    implementation (on which HomeScreenGestureHandler already
    depends, so additional observer interface just
    obscures/complicates this dependency)
2.  HomeLauncherStateWaiter which is used by interactive UI/perf tests
    to wait for launcher animations to complete
3.  BackdropController which pauses backdrop updates while home launcher
    drag is in progress
    (this landed in parallel with first attempt of landing this)

For case 1., moving the observer methods to HomeScreenDelegate
interface will work fine.

For case 2., the test waiter can register a callback with
AppListControllerImpl to be run when the home launcher animations are
complete - the same way it's done by LauncherStateWaiter, an equivalent
test waiter used in clamshell mode (or for waiting for non-visibility
app list state transitions)

For case 3. the dependency is switched - instead of BackdropController
observering HomeLauncherGestureHandler for drag events,
HomeLauncherGestureHandler requests pause in backdrop updates while home
launcher state transitions are in progress. Given that
HomeLauncherGestureHandler already depends in BackdropController (to
animate backdrop window), this removes newly added circular dependency.

BUG=1005366

Change-Id: I73e249eaf41d8ccf1e349dfa6ba023eccb97159a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1894464Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712224}

ash/BUILD.gn

@@ -348,7 +348,6 @@ component("ash") {
     "home_screen/drag_window_from_shelf_controller.h",
     "home_screen/home_launcher_gesture_handler.cc",
     "home_screen/home_launcher_gesture_handler.h",
-    "home_screen/home_launcher_gesture_handler_observer.h",
     "home_screen/home_screen_controller.cc",
     "home_screen/home_screen_controller.h",
     "home_screen/home_screen_delegate.h",

ash/app_list/app_list_controller_impl.cc

@@ -167,8 +167,6 @@ AppListControllerImpl::AppListControllerImpl()
     shell->assistant_controller()->AddObserver(this);
     shell->assistant_controller()->ui_controller()->AddModelObserver(this);
   }
-  shell->home_screen_controller()->home_launcher_gesture_handler()->AddObserver(
-      this);
 }
 
 AppListControllerImpl::~AppListControllerImpl() {
@@ -794,6 +792,9 @@ void AppListControllerImpl::OnHomeLauncherAnimationComplete(
   // visibility is correct first.
   OnVisibilityWillChange(shown, display_id);
   OnVisibilityChanged(shown, display_id);
+
+  if (!home_launcher_animation_callback_.is_null())
+    home_launcher_animation_callback_.Run(shown);
 }
 
 void AppListControllerImpl::OnHomeLauncherTargetPositionChanged(
@@ -932,11 +933,16 @@ void AppListControllerImpl::SetAppListModelForTest(
   model_->AddObserver(this);
 }
 
-void AppListControllerImpl::SetStateTransitionAnimationCallback(
+void AppListControllerImpl::SetStateTransitionAnimationCallbackForTesting(
     StateTransitionAnimationCallback callback) {
   state_transition_animation_callback_ = std::move(callback);
 }
 
+void AppListControllerImpl::SetHomeLauncherAnimationCallbackForTesting(
+    HomeLauncherAnimationCallback callback) {
+  home_launcher_animation_callback_ = std::move(callback);
+}
+
 void AppListControllerImpl::RecordShelfAppLaunched(
     base::Optional<AppListViewState> recorded_app_list_view_state,
     base::Optional<bool> recorded_home_launcher_shown) {
@@ -1554,9 +1560,6 @@ void AppListControllerImpl::Shutdown() {
   is_shutdown_ = true;
 
   Shell* shell = Shell::Get();
-  shell->home_screen_controller()
-      ->home_launcher_gesture_handler()
-      ->RemoveObserver(this);
   if (app_list_features::IsEmbeddedAssistantUIEnabled()) {
     shell->assistant_controller()->RemoveObserver(this);
     shell->assistant_controller()->ui_controller()->RemoveModelObserver(this);

ash/app_list/app_list_controller_impl.h

@@ -20,7 +20,6 @@
 #include "ash/assistant/assistant_controller_observer.h"
 #include "ash/assistant/model/assistant_ui_model_observer.h"
 #include "ash/display/window_tree_host_manager.h"
-#include "ash/home_screen/home_launcher_gesture_handler_observer.h"
 #include "ash/home_screen/home_screen_delegate.h"
 #include "ash/public/cpp/app_list/app_list_controller.h"
 #include "ash/public/cpp/keyboard/keyboard_controller_observer.h"
@@ -64,7 +63,6 @@ class ASH_EXPORT AppListControllerImpl
       public ash::MruWindowTracker::Observer,
       public AssistantControllerObserver,
       public AssistantUiModelObserver,
-      public HomeLauncherGestureHandlerObserver,
       public HomeScreenDelegate {
  public:
   AppListControllerImpl();
@@ -270,10 +268,6 @@ class ASH_EXPORT AppListControllerImpl
       base::Optional<AssistantEntryPoint> entry_point,
       base::Optional<AssistantExitPoint> exit_point) override;
 
-  // HomeLauncherGestureHandlerObserver:
-  void OnHomeLauncherAnimationComplete(bool shown, int64_t display_id) override;
-  void OnHomeLauncherTargetPositionChanged(bool showing,
-                                           int64_t display_id) override;
 
   // HomeScreenDelegate:
   void ShowHomeScreenView() override;
@@ -290,6 +284,9 @@ class ASH_EXPORT AppListControllerImpl
   base::Optional<base::TimeDelta> GetOptionalAnimationDuration() override;
   void NotifyHomeLauncherAnimationTransition(AnimationTrigger trigger,
                                              bool launcher_will_show) override;
+  void OnHomeLauncherAnimationComplete(bool shown, int64_t display_id) override;
+  void OnHomeLauncherTargetPositionChanged(bool showing,
+                                           int64_t display_id) override;
   bool IsHomeScreenVisible() override;
   gfx::Rect GetInitialAppListItemScreenBoundsForWindow(
       aura::Window* window) override;
@@ -327,9 +324,14 @@ class ASH_EXPORT AppListControllerImpl
   using StateTransitionAnimationCallback =
       base::RepeatingCallback<void(ash::AppListViewState)>;
 
-  void SetStateTransitionAnimationCallback(
+  void SetStateTransitionAnimationCallbackForTesting(
       StateTransitionAnimationCallback callback);
 
+  using HomeLauncherAnimationCallback =
+      base::RepeatingCallback<void(bool shown)>;
+  void SetHomeLauncherAnimationCallbackForTesting(
+      HomeLauncherAnimationCallback callback);
+
   void RecordShelfAppLaunched(
       base::Optional<AppListViewState> recorded_app_list_view_state,
       base::Optional<bool> home_launcher_shown);
@@ -423,8 +425,14 @@ class ASH_EXPORT AppListControllerImpl
   // each profile has its own AppListModelUpdater to manipulate app list items.
   int profile_id_ = kAppListInvalidProfileID;
 
+  // A callback that can be registered by a test to wait for the app list state
+  // transition animation to finish.
   StateTransitionAnimationCallback state_transition_animation_callback_;
 
+  // A callback that can be registered by a test to wait for the home launcher
+  // visibility animation to finish. Should only be used in tablet mode.
+  HomeLauncherAnimationCallback home_launcher_animation_callback_;
+
   // Used to prevent ShelfLayoutManager updating visibility state when overview
   // is showing over the AppList.
   std::unique_ptr<ShelfLayoutManager::ScopedSuspendVisibilityUpdate>

ash/autotest_private_api_utils.cc

@@ -6,8 +6,6 @@
 
 #include "ash/app_list/app_list_controller_impl.h"
 #include "ash/frame/non_client_frame_view_ash.h"
-#include "ash/home_screen/home_launcher_gesture_handler.h"
-#include "ash/home_screen/home_launcher_gesture_handler_observer.h"
 #include "ash/home_screen/home_screen_controller.h"
 #include "ash/shell.h"
 #include "ash/wm/mru_window_tracker.h"
@@ -16,26 +14,26 @@
 namespace ash {
 namespace {
 
-class HomeLauncherStateWaiter : public HomeLauncherGestureHandlerObserver {
+class HomeLauncherStateWaiter {
  public:
   HomeLauncherStateWaiter(bool target_shown, base::OnceClosure closure)
       : target_shown_(target_shown), closure_(std::move(closure)) {
     Shell::Get()
-        ->home_screen_controller()
-        ->home_launcher_gesture_handler()
-        ->AddObserver(this);
+        ->app_list_controller()
+        ->SetHomeLauncherAnimationCallbackForTesting(base::BindRepeating(
+            &HomeLauncherStateWaiter::OnHomeLauncherAnimationCompleted,
+            base::Unretained(this)));
   }
-  ~HomeLauncherStateWaiter() override {
+  ~HomeLauncherStateWaiter() {
     Shell::Get()
-        ->home_screen_controller()
-        ->home_launcher_gesture_handler()
-        ->RemoveObserver(this);
+        ->app_list_controller()
+        ->SetHomeLauncherAnimationCallbackForTesting(base::NullCallback());
   }
 
  private:
-  // HomeLauncherGestureHandlerObserver:
-  void OnHomeLauncherAnimationComplete(bool shown,
-                                       int64_t display_id) override {
+  // Passed to AppListControllerImpl as a callback to run when home launcher
+  // transition animation is complete.
+  void OnHomeLauncherAnimationCompleted(bool shown) {
     if (shown == target_shown_) {
       std::move(closure_).Run();
       delete this;
@@ -54,13 +52,15 @@ class LauncherStateWaiter {
  public:
   LauncherStateWaiter(ash::AppListViewState state, base::OnceClosure closure)
       : target_state_(state), closure_(std::move(closure)) {
-    Shell::Get()->app_list_controller()->SetStateTransitionAnimationCallback(
-        base::BindRepeating(&LauncherStateWaiter::OnStateChanged,
-                            base::Unretained(this)));
+    Shell::Get()
+        ->app_list_controller()
+        ->SetStateTransitionAnimationCallbackForTesting(base::BindRepeating(
+            &LauncherStateWaiter::OnStateChanged, base::Unretained(this)));
   }
   ~LauncherStateWaiter() {
-    Shell::Get()->app_list_controller()->SetStateTransitionAnimationCallback(
-        base::NullCallback());
+    Shell::Get()
+        ->app_list_controller()
+        ->SetStateTransitionAnimationCallbackForTesting(base::NullCallback());
   }
 
   void OnStateChanged(ash::AppListViewState state) {

ash/home_screen/home_launcher_gesture_handler.cc

@@ -9,7 +9,6 @@
 #include "ash/app_list/app_list_controller_impl.h"
 #include "ash/display/screen_orientation_controller.h"
 #include "ash/home_screen/drag_window_from_shelf_controller.h"
-#include "ash/home_screen/home_launcher_gesture_handler_observer.h"
 #include "ash/home_screen/home_screen_controller.h"
 #include "ash/home_screen/swipe_home_to_overview_controller.h"
 #include "ash/root_window_controller.h"
@@ -355,6 +354,7 @@ bool HomeLauncherGestureHandler::ShowHomeLauncher(
   display_ = display;
   mode_ = Mode::kSlideUpToShow;
 
+  PauseBackdropUpdatesForActiveWindow();
   UpdateWindowsForSlideUpOrDown(0.0, /*animate=*/false);
   AnimateToFinalState(AnimationTrigger::kLauncherButton);
   return true;
@@ -377,6 +377,7 @@ bool HomeLauncherGestureHandler::HideHomeLauncherForWindow(
   display_ = display;
   mode_ = Mode::kSlideDownToHide;
 
+  PauseBackdropUpdatesForActiveWindow();
   UpdateWindowsForSlideUpOrDown(1.0, /*animate=*/false);
   AnimateToFinalState(AnimationTrigger::kHideForWindow);
   return true;
@@ -398,28 +399,17 @@ bool HomeLauncherGestureHandler::IsDragInProgress() const {
   return mode_ != Mode::kNone;
 }
 
-void HomeLauncherGestureHandler::AddObserver(
-    HomeLauncherGestureHandlerObserver* observer) {
-  observers_.AddObserver(observer);
-}
-
-void HomeLauncherGestureHandler::RemoveObserver(
-    HomeLauncherGestureHandlerObserver* observer) {
-  observers_.RemoveObserver(observer);
-}
-
 void HomeLauncherGestureHandler::NotifyHomeLauncherTargetPositionChanged(
     bool showing,
     int64_t display_id) {
-  for (auto& observer : observers_)
-    observer.OnHomeLauncherTargetPositionChanged(showing, display_id);
+  GetHomeScreenDelegate()->OnHomeLauncherTargetPositionChanged(showing,
+                                                               display_id);
 }
 
 void HomeLauncherGestureHandler::NotifyHomeLauncherAnimationComplete(
     bool shown,
     int64_t display_id) {
-  for (auto& observer : observers_)
-    observer.OnHomeLauncherAnimationComplete(shown, display_id);
+  GetHomeScreenDelegate()->OnHomeLauncherAnimationComplete(shown, display_id);
 }
 
 void HomeLauncherGestureHandler::OnWindowDestroying(aura::Window* window) {
@@ -712,6 +702,7 @@ void HomeLauncherGestureHandler::UpdateWindowsForSlideUpOrDown(double progress,
 void HomeLauncherGestureHandler::RemoveObserversAndStopTracking() {
   display_.set_id(display::kInvalidDisplayId);
   backdrop_values_ = base::nullopt;
+  scoped_backdrop_update_pause_ = base::nullopt;
   divider_values_ = base::nullopt;
   last_event_location_ = base::nullopt;
   last_scroll_y_ = 0.f;
@@ -925,6 +916,7 @@ void HomeLauncherGestureHandler::OnDragStarted(const gfx::Point& location) {
     DCHECK(home_screen_delegate);
     home_screen_delegate->OnHomeLauncherDragStart();
 
+    PauseBackdropUpdatesForActiveWindow();
     UpdateWindowsForSlideUpOrDown(/*progress=*/0.0, /*animate=*/false);
   }
 }
@@ -992,4 +984,17 @@ void HomeLauncherGestureHandler::OnDragCancelled() {
   }
 }
 
+void HomeLauncherGestureHandler::PauseBackdropUpdatesForActiveWindow() {
+  if (scoped_backdrop_update_pause_.has_value())
+    return;
+  aura::Window* active_window = GetActiveWindow();
+  if (!active_window)
+    return;
+  scoped_backdrop_update_pause_ =
+      GetWorkspaceControllerForContext(active_window)
+          ->layout_manager()
+          ->backdrop_controller()
+          ->PauseUpdates();
+}
+
 }  // namespace ash

ash/home_screen/home_launcher_gesture_handler.h

@@ -13,6 +13,7 @@
 #include "ash/home_screen/home_screen_delegate.h"
 #include "ash/public/cpp/tablet_mode_observer.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
+#include "base/callback_helpers.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/scoped_observer.h"
@@ -24,7 +25,6 @@
 
 namespace ash {
 
-class HomeLauncherGestureHandlerObserver;
 class SwipeHomeToOverviewController;
 
 // HomeLauncherGestureHandler makes modifications to a window's transform and
@@ -80,9 +80,6 @@ class ASH_EXPORT HomeLauncherGestureHandler
 
   bool IsDragInProgress() const;
 
-  void AddObserver(HomeLauncherGestureHandlerObserver* observer);
-  void RemoveObserver(HomeLauncherGestureHandlerObserver* obsever);
-
   void NotifyHomeLauncherTargetPositionChanged(bool showing,
                                                int64_t display_id);
   void NotifyHomeLauncherAnimationComplete(bool shown, int64_t display_id);
@@ -162,6 +159,8 @@ class ASH_EXPORT HomeLauncherGestureHandler
                    base::Optional<float> velocity_y);
   void OnDragCancelled();
 
+  void PauseBackdropUpdatesForActiveWindow();
+
   Mode mode_ = Mode::kNone;
 
   // The windows we are tracking. They are null if a drag is not underway, or if
@@ -213,7 +212,11 @@ class ASH_EXPORT HomeLauncherGestureHandler
   std::unique_ptr<SwipeHomeToOverviewController>
       swipe_home_to_overview_controller_;
 
-  base::ObserverList<HomeLauncherGestureHandlerObserver> observers_;
+  // The closure runner returned by BackdropController::PauseUpdates() requested
+  // when app window drag starts to prevent backdrop from showing up mid-drag.
+  // Keeping this in scope keeps backdrop changes paused (it's reset when the
+  // drag gesture sequence finishes).
+  base::Optional<base::ScopedClosureRunner> scoped_backdrop_update_pause_;
 
   DISALLOW_COPY_AND_ASSIGN(HomeLauncherGestureHandler);
 };

ash/home_screen/home_launcher_gesture_handler_observer.h

-// Copyright 2019 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef ASH_HOME_SCREEN_HOME_LAUNCHER_GESTURE_HANDLER_OBSERVER_H_
-#define ASH_HOME_SCREEN_HOME_LAUNCHER_GESTURE_HANDLER_OBSERVER_H_
-
-#include "base/observer_list_types.h"
-
-namespace ash {
-
-class HomeLauncherGestureHandlerObserver : public base::CheckedObserver {
- public:
-  // Called when the HomeLauncher has started to be dragged, or a positional
-  // animation has begun.
-  virtual void OnHomeLauncherTargetPositionChanged(bool showing,
-                                                   int64_t display_id) {}
-
-  // Called when the HomeLauncher positional animation has completed.
-  virtual void OnHomeLauncherAnimationComplete(bool shown, int64_t display_id) {
-  }
-};
-
-}  // namespace ash
-
-#endif  // ASH_HOME_SCREEN_HOME_LAUNCHER_GESTURE_HANDLER_OBSERVER_H_

ash/home_screen/home_screen_delegate.h

@@ -98,6 +98,15 @@ class HomeScreenDelegate {
   // whether the launcher will show by the end of animation.
   virtual void NotifyHomeLauncherAnimationTransition(AnimationTrigger trigger,
                                                      bool launcher_will_show) {}
+
+  // Called when the HomeLauncher has started to be dragged, or a positional
+  // animation has begun.
+  virtual void OnHomeLauncherTargetPositionChanged(bool showing,
+                                                   int64_t display_id) {}
+
+  // Called when the HomeLauncher positional animation has completed.
+  virtual void OnHomeLauncherAnimationComplete(bool shown, int64_t display_id) {
+  }
 };
 
 }  // namespace ash

ash/wm/workspace/backdrop_controller.cc

@@ -9,8 +9,6 @@
 
 #include "ash/accessibility/accessibility_controller_impl.h"
 #include "ash/accessibility/accessibility_delegate.h"
-#include "ash/home_screen/home_launcher_gesture_handler.h"
-#include "ash/home_screen/home_screen_controller.h"
 #include "ash/public/cpp/app_types.h"
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/public/cpp/window_animation_types.h"
@@ -132,17 +130,10 @@ BackdropController::BackdropController(aura::Window* container)
   shell->accessibility_controller()->AddObserver(this);
   shell->wallpaper_controller()->AddObserver(this);
   shell->tablet_mode_controller()->AddObserver(this);
-  shell->home_screen_controller()->home_launcher_gesture_handler()->AddObserver(
-      this);
 }
 
 BackdropController::~BackdropController() {
   auto* shell = Shell::Get();
-  if (shell->home_screen_controller()) {
-    shell->home_screen_controller()
-        ->home_launcher_gesture_handler()
-        ->RemoveObserver(this);
-  }
   // Shell destroys the TabletModeController before destroying all root windows.
   if (shell->tablet_mode_controller())
     shell->tablet_mode_controller()->RemoveObserver(this);
@@ -231,6 +222,14 @@ aura::Window* BackdropController::GetTopmostWindowWithBackdrop() {
   return nullptr;
 }
 
+base::ScopedClosureRunner BackdropController::PauseUpdates() {
+  DCHECK(!pause_update_);
+
+  pause_update_ = true;
+  return base::ScopedClosureRunner(base::BindOnce(
+      &BackdropController::RestoreUpdates, weak_ptr_factory_.GetWeakPtr()));
+}
+
 void BackdropController::OnOverviewModeStarting() {
   // Don't destroy backdrops, just hide them so they don't show in the overview
   // grid, but keep the widget so that it can be mirrored into the mini_desk
@@ -281,14 +280,7 @@ void BackdropController::OnTabletModeEnded() {
   UpdateBackdrop();
 }
 
-void BackdropController::OnHomeLauncherTargetPositionChanged(
-    bool showing,
-    int64_t display_id) {
-  pause_update_ = true;
-}
-
-void BackdropController::OnHomeLauncherAnimationComplete(bool shown,
-                                                         int64_t display_id) {
+void BackdropController::RestoreUpdates() {
   pause_update_ = false;
   UpdateBackdrop();
 }

ash/wm/workspace/backdrop_controller.h

@@ -9,14 +9,15 @@
 
 #include "ash/accessibility/accessibility_observer.h"
 #include "ash/ash_export.h"
-#include "ash/home_screen/home_launcher_gesture_handler_observer.h"
 #include "ash/public/cpp/tablet_mode_observer.h"
 #include "ash/public/cpp/wallpaper_controller_observer.h"
 #include "ash/public/cpp/window_properties.h"
 #include "ash/wm/overview/overview_observer.h"
 #include "ash/wm/splitview/split_view_controller.h"
 #include "ash/wm/splitview/split_view_observer.h"
+#include "base/callback_helpers.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "ui/gfx/geometry/rect.h"
 
 namespace aura {
@@ -42,13 +43,11 @@ namespace ash {
 // 3) In tablet mode:
 //        - Bottom-most snapped window in splitview,
 //        - Top-most activatable window if splitview is inactive.
-class ASH_EXPORT BackdropController
-    : public AccessibilityObserver,
-      public OverviewObserver,
-      public SplitViewObserver,
-      public WallpaperControllerObserver,
-      public TabletModeObserver,
-      public HomeLauncherGestureHandlerObserver {
+class ASH_EXPORT BackdropController : public AccessibilityObserver,
+                                      public OverviewObserver,
+                                      public SplitViewObserver,
+                                      public WallpaperControllerObserver,
+                                      public TabletModeObserver {
  public:
   explicit BackdropController(aura::Window* container);
   ~BackdropController() override;
@@ -68,6 +67,9 @@ class ASH_EXPORT BackdropController
   // the other windows in the container.
   void UpdateBackdrop();
 
+  // Pauses backdrop updates until the returned object goes out of scope.
+  base::ScopedClosureRunner PauseUpdates();
+
   // Returns the current visible top level window in the container.
   aura::Window* GetTopmostWindowWithBackdrop();
 
@@ -93,14 +95,12 @@ class ASH_EXPORT BackdropController
   void OnTabletModeStarted() override;
   void OnTabletModeEnded() override;
 
-  // HomeLauncherGestureHandlerObserver:
-  void OnHomeLauncherTargetPositionChanged(bool showing,
-                                           int64_t display_id) override;
-  void OnHomeLauncherAnimationComplete(bool shown, int64_t display_id) override;
-
  private:
   friend class WorkspaceControllerTestApi;
 
+  // Reenables updates previously pause by calling PauseUpdates().
+  void RestoreUpdates();
+
   void UpdateBackdropInternal();
 
   void EnsureBackdropWidget(BackdropWindowMode mode);
@@ -153,6 +153,8 @@ class ASH_EXPORT BackdropController
   // in overview mode.
   bool pause_update_ = false;
 
+  base::WeakPtrFactory<BackdropController> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(BackdropController);
 };