desks: Fix metrics collection for touchpad swipes.

It was double counting because it recorded one visible desk change per
root window. This CL keeps the same logic, but moves it to
DeskActivationAnimation, so that the metric is only counted once.

Also, extend the time fast forward in a flaky test.

Bug: 1151856
Test: manual
Change-Id: Ica101a25df867ce5b37bfce08699c2d4715a9230
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587974Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839035}

ash/wm/desks/desk_animation_base.cc

@@ -138,10 +138,6 @@ void DeskAnimationBase::OnDeskSwitchAnimationFinished() {
   // `this` is now deleted.
 }
 
-void DeskAnimationBase::OnVisibleDeskChanged() {
-  ++visible_desk_changes_;
-}
-
 RootWindowDeskSwitchAnimator*
 DeskAnimationBase::GetDeskSwitchAnimatorAtIndexForTesting(size_t index) const {
   DCHECK_LT(index, desk_switch_animators_.size());

ash/wm/desks/desk_animation_base.h

@@ -56,7 +56,6 @@ class ASH_EXPORT DeskAnimationBase
   void OnStartingDeskScreenshotTaken(int ending_desk_index) override;
   void OnEndingDeskScreenshotTaken() override;
   void OnDeskSwitchAnimationFinished() override;
-  void OnVisibleDeskChanged() override;
 
   void set_skip_notify_controller_on_animation_finished_for_testing(bool val) {
     skip_notify_controller_on_animation_finished_for_testing_ = val;

ash/wm/desks/desk_animation_impl.cc

@@ -59,6 +59,7 @@ DeskActivationAnimation::DeskActivationAnimation(DesksController* controller,
                         IsForContinuousGestures(source)),
       switch_source_(source),
       update_window_activation_(update_window_activation),
+      visible_desk_index_(starting_desk_index),
       presentation_time_recorder_(CreatePresentationTimeHistogramRecorder(
           desks_util::GetSelectedCompositorForPerformanceMetrics(),
           kDeskUpdateGestureHistogramName,
@@ -154,6 +155,18 @@ bool DeskActivationAnimation::UpdateSwipeAnimation(float scroll_delta_x) {
       animator->UpdateSwipeAnimation(scroll_delta_x);
   }
 
+  // See if the animator of the first display has visibly changed desks. If so,
+  // update |visible_desk_changes_| for metrics collection purposes.
+  auto* first_animator = desk_switch_animators_.front().get();
+  DCHECK(first_animator);
+  if (first_animator->starting_desk_screenshot_taken() &&
+      first_animator->ending_desk_screenshot_taken()) {
+    const int old_visible_desk_index = visible_desk_index_;
+    visible_desk_index_ = first_animator->GetIndexOfMostVisibleDeskScreenshot();
+    if (visible_desk_index_ != old_visible_desk_index)
+      ++visible_desk_changes_;
+  }
+
   // No screenshot needed.
   if (!ending_desk_index)
     return true;

ash/wm/desks/desk_animation_impl.h

@@ -47,6 +47,10 @@ class ASH_EXPORT DeskActivationAnimation : public DeskAnimationBase {
   // when the desk is switched.
   const bool update_window_activation_;
 
+  // The index of the desk that is most visible to the user based on the
+  // transform of the animation layer.
+  int visible_desk_index_;
+
   // Used to measure the presentation time of a continuous gesture swipe.
   std::unique_ptr<PresentationTimeRecorder> presentation_time_recorder_;
 };

ash/wm/desks/desk_animation_impl_unittest.cc

@@ -76,4 +76,61 @@ TEST_F(DeskActivationAnimationTest, UpdateSwipeNewScreenshotCrash) {
   animation.EndSwipeAnimation();
 }
 
+// Tests that visible desk change count updates as expected. It is used higher
+// up for metrics collection, but the logic is in this class.
+TEST_F(DeskActivationAnimationTest, VisibleDeskChangeCount) {
+  // Add three desks for a total of four.
+  auto* desks_controller = DesksController::Get();
+  desks_controller->NewDesk(DesksCreationRemovalSource::kButton);
+  desks_controller->NewDesk(DesksCreationRemovalSource::kButton);
+  desks_controller->NewDesk(DesksCreationRemovalSource::kButton);
+
+  DeskActivationAnimation animation(desks_controller, 0, 1,
+                                    DesksSwitchSource::kDeskSwitchTouchpad,
+                                    /*update_window_activation=*/false);
+  animation.set_skip_notify_controller_on_animation_finished_for_testing(true);
+  animation.Launch();
+
+  auto wait_ending_screenshot_taken = [](DeskActivationAnimation* animation) {
+    base::RunLoop run_loop;
+    auto* desk_switch_animator =
+        animation->GetDeskSwitchAnimatorAtIndexForTesting(0);
+    RootWindowDeskSwitchAnimatorTestApi(desk_switch_animator)
+        .SetOnEndingScreenshotTakenCallback(run_loop.QuitClosure());
+    run_loop.Run();
+  };
+
+  wait_ending_screenshot_taken(&animation);
+  EXPECT_EQ(0, animation.visible_desk_changes());
+
+  const int touchpad_swipe_length_for_desk_change =
+      RootWindowDeskSwitchAnimator::kTouchpadSwipeLengthForDeskChange;
+
+  // Swipe enough so that our third and fourth desk screenshots are taken, and
+  // then swipe so that the fourth desk is fully shown. There should be 3
+  // visible desk changes in total.
+  animation.UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
+  wait_ending_screenshot_taken(&animation);
+
+  animation.UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
+  wait_ending_screenshot_taken(&animation);
+
+  animation.UpdateSwipeAnimation(-3 * touchpad_swipe_length_for_desk_change);
+  EXPECT_EQ(3, animation.visible_desk_changes());
+
+  // Do some minor swipes to the right. We should still be focused on the last
+  // desk so the visible desk change count remains the same.
+  animation.UpdateSwipeAnimation(touchpad_swipe_length_for_desk_change / 10);
+  animation.UpdateSwipeAnimation(touchpad_swipe_length_for_desk_change / 10);
+  EXPECT_EQ(3, animation.visible_desk_changes());
+
+  // Do two full swipes to the right, and then two full swipes to the left. Test
+  // that the desk change count has increased by four.
+  animation.UpdateSwipeAnimation(touchpad_swipe_length_for_desk_change);
+  animation.UpdateSwipeAnimation(touchpad_swipe_length_for_desk_change);
+  animation.UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
+  animation.UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
+  EXPECT_EQ(7, animation.visible_desk_changes());
+}
+
 }  // namespace ash

ash/wm/desks/desks_unittests.cc

@@ -3770,8 +3770,7 @@ class DesksMockTimeTest : public AshTestBase {
   ~DesksMockTimeTest() override = default;
 };
 
-// TODO(crbug.com/1151856) flaky test
-TEST_F(DesksMockTimeTest, DISABLED_DeskTraversalNonTouchpadMetrics) {
+TEST_F(DesksMockTimeTest, DeskTraversalNonTouchpadMetrics) {
   NewDesk();
   NewDesk();
   NewDesk();
@@ -3795,7 +3794,7 @@ TEST_F(DesksMockTimeTest, DISABLED_DeskTraversalNonTouchpadMetrics) {
   histogram_tester.ExpectBucketCount(kDeskTraversalsHistogramName, 5, 0);
 
   // Advance the time to end the timer. There should be 5 desks recorded.
-  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(5));
+  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(8));
   histogram_tester.ExpectBucketCount(kDeskTraversalsHistogramName, 5, 1);
 }
 

ash/wm/desks/root_window_desk_switch_animator.cc

@@ -132,7 +132,6 @@ RootWindowDeskSwitchAnimator::RootWindowDeskSwitchAnimator(
     : root_window_(root),
       starting_desk_index_(starting_desk_index),
       ending_desk_index_(ending_desk_index),
-      visible_desk_index_(starting_desk_index),
       delegate_(delegate),
       animation_layer_owner_(CreateAnimationLayerOwner(root)),
       root_window_size_(
@@ -305,20 +304,13 @@ base::Optional<int> RootWindowDeskSwitchAnimator::UpdateSwipeAnimation(
           : transformed_animation_layer_bounds.x() >
                 -kMinDistanceBeforeScreenshotDp;
 
-  // TODO(sammiequon): Make GetIndexOfMostVisibleDeskScreenshot() public and
-  // have DeskActivationAnimation keep track of |visible_desk_index_|. Right now
-  // OnVisibleDeskChanged will get called once for each display.
-  const int old_visible_desk_index = visible_desk_index_;
-  visible_desk_index_ = GetIndexOfMostVisibleDeskScreenshot();
-  if (old_visible_desk_index != visible_desk_index_)
-    delegate_->OnVisibleDeskChanged();
-
   if (!going_out_of_bounds)
     return base::nullopt;
 
   // The upcoming desk we need to show will be an adjacent desk to the desk at
-  // |visible_desk_index_| based on |moving_left|.
-  const int new_desk_index = visible_desk_index_ + (moving_left ? 1 : -1);
+  // the visible desk index based on |moving_left|.
+  const int new_desk_index =
+      GetIndexOfMostVisibleDeskScreenshot() + (moving_left ? 1 : -1);
 
   if (new_desk_index < 0 ||
       new_desk_index >= int{DesksController::Get()->desks().size()}) {
@@ -348,7 +340,8 @@ int RootWindowDeskSwitchAnimator::EndSwipeAnimation() {
     return ending_desk_index;
   }
 
-  // If the ending desk screenshot has not finished, |visible_desk_index_| will
+  // If the ending desk screenshot has not finished,
+  // GetIndexOfMostVisibleDeskScreenshot() will
   // still return a valid desk index that we can animate to, but we need to make
   // sure the ending desk screenshot callback does not get called.
   if (!ending_desk_screenshot_taken_)
@@ -357,12 +350,40 @@ int RootWindowDeskSwitchAnimator::EndSwipeAnimation() {
   // In tests, StartAnimation() may trigger OnDeskSwitchAnimationFinished()
   // right away which may delete |this|. Store the target index in a
   // local so we do not try to access a member of a deleted object.
-  const int ending_desk_index = visible_desk_index_;
+  const int ending_desk_index = GetIndexOfMostVisibleDeskScreenshot();
   ending_desk_index_ = ending_desk_index;
   StartAnimation();
   return ending_desk_index;
 }
 
+int RootWindowDeskSwitchAnimator::GetIndexOfMostVisibleDeskScreenshot() const {
+  int index = -1;
+
+  // The most visible desk is the one whose screenshot layer bounds, including
+  // the transform of its parent that has its origin closest to the root window
+  // origin (0, 0).
+  const gfx::Transform transform = animation_layer_owner_->root()->transform();
+  int min_distance = INT_MAX;
+  for (int i = 0; i < int{screenshot_layers_.size()}; ++i) {
+    ui::Layer* layer = screenshot_layers_[i];
+    if (!layer)
+      continue;
+
+    gfx::RectF bounds(layer->bounds());
+    transform.TransformRect(&bounds);
+    const int distance = std::abs(bounds.x());
+    if (distance < min_distance) {
+      min_distance = distance;
+      index = i;
+    }
+  }
+
+  // TODO(crbug.com/1134390): Convert back to DCHECK when the issue is fixed.
+  CHECK_GE(index, 0);
+  CHECK_LT(index, int{DesksController::Get()->desks().size()});
+  return index;
+}
+
 void RootWindowDeskSwitchAnimator::OnImplicitAnimationsCompleted() {
   // |setting_new_transform_| is true we call SetTransform while an animation is
   // under progress. Do not notify our delegate in that case.
@@ -601,32 +622,4 @@ int RootWindowDeskSwitchAnimator::GetXPositionOfScreenshot(int index) {
   return layer->bounds().x();
 }
 
-int RootWindowDeskSwitchAnimator::GetIndexOfMostVisibleDeskScreenshot() const {
-  int index = -1;
-
-  // The most visible desk is the one whose screenshot layer bounds, including
-  // the transform of its parent that has its origin closest to the root window
-  // origin (0, 0).
-  const gfx::Transform transform = animation_layer_owner_->root()->transform();
-  int min_distance = INT_MAX;
-  for (int i = 0; i < int{screenshot_layers_.size()}; ++i) {
-    ui::Layer* layer = screenshot_layers_[i];
-    if (!layer)
-      continue;
-
-    gfx::RectF bounds(layer->bounds());
-    transform.TransformRect(&bounds);
-    const int distance = std::abs(bounds.x());
-    if (distance < min_distance) {
-      min_distance = distance;
-      index = i;
-    }
-  }
-
-  // TODO(crbug.com/1134390): Convert back to DCHECK when the issue is fixed.
-  CHECK_GE(index, 0);
-  CHECK_LT(index, int{DesksController::Get()->desks().size()});
-  return index;
-}
-
 }  // namespace ash

ash/wm/desks/root_window_desk_switch_animator.h

@@ -197,10 +197,6 @@ class ASH_EXPORT RootWindowDeskSwitchAnimator
     // desk screenshot is now showing on the screen.
     virtual void OnDeskSwitchAnimationFinished() = 0;
 
-    // Called while doing a continuous gesture to notify when the desk that is
-    // visible to the user has changed. Used for metrics collection.
-    virtual void OnVisibleDeskChanged() = 0;
-
    protected:
     virtual ~Delegate() = default;
   };
@@ -279,6 +275,11 @@ class ASH_EXPORT RootWindowDeskSwitchAnimator
   // visible desk, whose index is also returned.
   int EndSwipeAnimation();
 
+  // Gets the index of the desk whose screenshot of the animation layer is most
+  // visible to the user. That desk screenshot is the one which aligns the most
+  // with the root window bounds.
+  int GetIndexOfMostVisibleDeskScreenshot() const;
+
   // ui::ImplicitAnimationObserver:
   void OnImplicitAnimationsCompleted() override;
 
@@ -311,11 +312,6 @@ class ASH_EXPORT RootWindowDeskSwitchAnimator
   // its parent layer's coordinates (|animation_layer_owner_->root()|).
   int GetXPositionOfScreenshot(int index);
 
-  // Gets the index of the desk whose screenshot of the animation layer is most
-  // visible to the user. That desk screenshot is the one which aligns the most
-  // with the root window bounds.
-  int GetIndexOfMostVisibleDeskScreenshot() const;
-
   // The root window that this animator is associated with.
   aura::Window* const root_window_;
 
@@ -325,10 +321,6 @@ class ASH_EXPORT RootWindowDeskSwitchAnimator
   // The index of the desk to activate and animate to with this animator.
   int ending_desk_index_;
 
-  // The index of the desk that is most visible to the user based on the
-  // transform of the animation layer.
-  int visible_desk_index_;
-
   Delegate* const delegate_;
 
   // The owner of the layer tree of the old detached layers of the removed

ash/wm/desks/root_window_desk_switch_animator_unittest.cc

@@ -141,7 +141,6 @@ class RootWindowDeskSwitchAnimatorTest
   }
 
   void OnDeskSwitchAnimationFinished() override {}
-  void OnVisibleDeskChanged() override { ++visible_desk_changed_count_; }
 
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
@@ -467,53 +466,4 @@ TEST_F(RootWindowDeskSwitchAnimatorTest,
   animator()->EndSwipeAnimation();
 }
 
-// Tests that visible desk change count updates as expected. It is used higher
-// up for metrics collection, but the logic is in this class.
-TEST_F(RootWindowDeskSwitchAnimatorTest, VisibleDeskChangeCount) {
-  // Add three desks for a total of four.
-  DesksController::Get()->NewDesk(DesksCreationRemovalSource::kButton);
-  DesksController::Get()->NewDesk(DesksCreationRemovalSource::kButton);
-  DesksController::Get()->NewDesk(DesksCreationRemovalSource::kButton);
-
-  InitAnimator(0, 1);
-  TakeStartingDeskScreenshotAndWait();
-  TakeEndingDeskScreenshotAndWait();
-  EXPECT_EQ(0, visible_desk_changed_count());
-
-  const int touchpad_swipe_length_for_desk_change =
-      RootWindowDeskSwitchAnimator::kTouchpadSwipeLengthForDeskChange;
-
-  // Swipe enough so that our third and fourth desk screenshots are taken, and
-  // then swipe so that the fourth desk is fully shown. There should be 3
-  // visible desk changes in total.
-  base::Optional<int> new_index =
-      animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
-  ASSERT_TRUE(new_index.has_value());
-  animator()->PrepareForEndingDeskScreenshot(*new_index);
-  TakeEndingDeskScreenshotAndWait();
-
-  new_index =
-      animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
-  ASSERT_TRUE(new_index.has_value());
-  animator()->PrepareForEndingDeskScreenshot(*new_index);
-  TakeEndingDeskScreenshotAndWait();
-
-  animator()->UpdateSwipeAnimation(-3 * touchpad_swipe_length_for_desk_change);
-  EXPECT_EQ(3, visible_desk_changed_count());
-
-  // Do some minor swipes to the right. We should still be focused on the last
-  // desk so the visible desk change count remains the same.
-  animator()->UpdateSwipeAnimation(touchpad_swipe_length_for_desk_change / 10);
-  animator()->UpdateSwipeAnimation(touchpad_swipe_length_for_desk_change / 10);
-  EXPECT_EQ(3, visible_desk_changed_count());
-
-  // Do two full swipes to the right, and then two full swipes to the left. Test
-  // that the desk change count has increased by four.
-  animator()->UpdateSwipeAnimation(touchpad_swipe_length_for_desk_change);
-  animator()->UpdateSwipeAnimation(touchpad_swipe_length_for_desk_change);
-  animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
-  animator()->UpdateSwipeAnimation(-touchpad_swipe_length_for_desk_change);
-  EXPECT_EQ(7, visible_desk_changed_count());
-}
-
 }  // namespace ash