Fix ShelfNavigationWidget metrics reporting

Update shelf navigation widget metrics reporter to use the hotseat
state from the time of reporter callback creation to select which
hotseat state dependant metric to report. Note that shelf component
layout happens after the hotseat state is set, so the hotseat state
should have the desired value by the time navigation widget transitions
start.

The report callback is run asynchronously from the time the animation
completes, so the hotseat state can change between the hotseat animation
changing and the reporter callback running (e.g. if an app window is
closed just after the animation to hidden hotseat ends, which may happen
in perf tests). With the existing logic, where the reporter uses the
current hotseat state to select the histogram to report, the animation
may get registered for an incorrect transition.

Also, when animating home button bounds, do not request animation, and
create metrics reporter if the view is already animating to the same
target bounds.

BUG=1108641
TEST=ran ui.HotseatAnimation tast tests a couple of times

Change-Id: Idde82ce98545d7ee7e54170abea588c9793e6d3d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2333141Reviewed-by: Andrew Xu <andrewxu@chromium.org>
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793793}

ash/shelf/shelf.cc

@@ -140,25 +140,19 @@ class HotseatWidgetAnimationMetricsReporter {
 };
 
 // An animation metrics reporter for the shelf navigation widget.
-class ASH_EXPORT NavigationWidgetAnimationMetricsReporter
-    : public ShelfLayoutManagerObserver {
+class ASH_EXPORT NavigationWidgetAnimationMetricsReporter {
  public:
-  explicit NavigationWidgetAnimationMetricsReporter(Shelf* shelf)
-      : shelf_(shelf) {
-    shelf_->shelf_layout_manager()->AddObserver(this);
-  }
+  NavigationWidgetAnimationMetricsReporter() = default;
 
-  ~NavigationWidgetAnimationMetricsReporter() override {
-    shelf_->shelf_layout_manager()->RemoveObserver(this);
-  }
+  ~NavigationWidgetAnimationMetricsReporter() = default;
 
   NavigationWidgetAnimationMetricsReporter(
       const NavigationWidgetAnimationMetricsReporter&) = delete;
   NavigationWidgetAnimationMetricsReporter& operator=(
       const NavigationWidgetAnimationMetricsReporter&) = delete;
 
-  void ReportSmoothness(int smoothness) {
-    switch (target_state_) {
+  void ReportSmoothness(HotseatState target_hotseat_state, int smoothness) {
+    switch (target_hotseat_state) {
       case HotseatState::kShownClamshell:
       case HotseatState::kShownHomeLauncher:
         UMA_HISTOGRAM_PERCENTAGE(
@@ -184,24 +178,15 @@ class ASH_EXPORT NavigationWidgetAnimationMetricsReporter
     }
   }
 
-  metrics_util::ReportCallback GetReportCallback() {
+  metrics_util::ReportCallback GetReportCallback(
+      HotseatState target_hotseat_state) {
+    DCHECK_NE(target_hotseat_state, HotseatState::kNone);
     return metrics_util::ForSmoothness(base::BindRepeating(
         &NavigationWidgetAnimationMetricsReporter::ReportSmoothness,
-        weak_ptr_factory_.GetWeakPtr()));
-  }
-
-  // ShelfLayoutManagerObserver:
-  void OnHotseatStateChanged(HotseatState old_state,
-                             HotseatState new_state) override {
-    target_state_ = new_state;
+        weak_ptr_factory_.GetWeakPtr(), target_hotseat_state));
   }
 
  private:
-  Shelf* const shelf_;
-
-  // The state to which the animation is transitioning.
-  HotseatState target_state_ = HotseatState::kShownHomeLauncher;
-
   base::WeakPtrFactory<NavigationWidgetAnimationMetricsReporter>
       weak_ptr_factory_{this};
 };
@@ -389,7 +374,7 @@ void Shelf::CreateNavigationWidget(aura::Window* container) {
       this, hotseat_widget()->GetShelfView());
   navigation_widget_->Initialize(container);
   navigation_widget_metrics_reporter_ =
-      std::make_unique<NavigationWidgetAnimationMetricsReporter>(this);
+      std::make_unique<NavigationWidgetAnimationMetricsReporter>();
 }
 
 void Shelf::CreateHotseatWidget(aura::Window* container) {
@@ -685,9 +670,10 @@ metrics_util::ReportCallback Shelf::GetTranslucentBackgroundReportCallback(
       target_state);
 }
 
-metrics_util::ReportCallback
-Shelf::GetNavigationWidgetAnimationReportCallback() {
-  return navigation_widget_metrics_reporter_->GetReportCallback();
+metrics_util::ReportCallback Shelf::GetNavigationWidgetAnimationReportCallback(
+    HotseatState target_hotseat_state) {
+  return navigation_widget_metrics_reporter_->GetReportCallback(
+      target_hotseat_state);
 }
 
 void Shelf::WillDeleteShelfLayoutManager() {

ash/shelf/shelf.h

@@ -237,7 +237,8 @@ class ASH_EXPORT Shelf : public ShelfLayoutManagerObserver {
   metrics_util::ReportCallback GetTranslucentBackgroundReportCallback(
       HotseatState target_state);
 
-  metrics_util::ReportCallback GetNavigationWidgetAnimationReportCallback();
+  metrics_util::ReportCallback GetNavigationWidgetAnimationReportCallback(
+      HotseatState target_hotseat_state);
 
  protected:
   // ShelfLayoutManagerObserver:

ash/shelf/shelf_layout_manager.cc

@@ -1318,7 +1318,7 @@ void ShelfLayoutManager::SetState(ShelfVisibilityState visibility_state) {
 
 HotseatState ShelfLayoutManager::CalculateHotseatState(
     ShelfVisibilityState visibility_state,
-    ShelfAutoHideState auto_hide_state) {
+    ShelfAutoHideState auto_hide_state) const {
   if (!IsHotseatEnabled() || !shelf_->IsHorizontalAlignment())
     return HotseatState::kShownClamshell;
 
@@ -1507,6 +1507,7 @@ bool ShelfLayoutManager::SetDimmed(bool dimmed) {
     return false;
 
   dimmed_for_inactivity_ = dimmed;
+
   CalculateTargetBoundsAndUpdateWorkArea();
 
   const base::TimeDelta dim_animation_duration =
@@ -1519,7 +1520,7 @@ bool ShelfLayoutManager::SetDimmed(bool dimmed) {
   if (animate) {
     navigation_widget_reporter.emplace(
         GetLayer(shelf_->navigation_widget())->GetAnimator(),
-        shelf_->GetNavigationWidgetAnimationReportCallback());
+        shelf_->GetNavigationWidgetAnimationReportCallback(hotseat_state()));
   }
 
   AnimateOpacity(GetLayer(shelf_->navigation_widget()), target_opacity_,

ash/shelf/shelf_layout_manager.h

@@ -295,7 +295,7 @@ class ASH_EXPORT ShelfLayoutManager
   // Overview, Shelf, and any active gestures.
   // TODO(manucornet): Move this to the hotseat class.
   HotseatState CalculateHotseatState(ShelfVisibilityState visibility_state,
-                                     ShelfAutoHideState auto_hide_state);
+                                     ShelfAutoHideState auto_hide_state) const;
 
  private:
   class UpdateShelfObserver;

ash/shelf/shelf_navigation_widget.cc

@@ -145,8 +145,7 @@ class BoundsAnimationReporter : public gfx::AnimationDelegate {
 }  // namespace
 
 // An animation metrics reporter for the shelf navigation buttons.
-class ASH_EXPORT NavigationButtonAnimationMetricsReporter
-    : public ShelfLayoutManagerObserver {
+class ASH_EXPORT NavigationButtonAnimationMetricsReporter {
  public:
   // The different kinds of navigation buttons.
   enum class NavigationButtonType {
@@ -155,24 +154,19 @@ class ASH_EXPORT NavigationButtonAnimationMetricsReporter
     // The Navigation Widget's home button.
     kHomeButton
   };
-  NavigationButtonAnimationMetricsReporter(
-      Shelf* shelf,
+  explicit NavigationButtonAnimationMetricsReporter(
       NavigationButtonType navigation_button_type)
-      : shelf_(shelf), navigation_button_type_(navigation_button_type) {
-    shelf_->shelf_layout_manager()->AddObserver(this);
-  }
+      : navigation_button_type_(navigation_button_type) {}
 
-  ~NavigationButtonAnimationMetricsReporter() override {
-    shelf_->shelf_layout_manager()->RemoveObserver(this);
-  }
+  ~NavigationButtonAnimationMetricsReporter() = default;
 
   NavigationButtonAnimationMetricsReporter(
       const NavigationButtonAnimationMetricsReporter&) = delete;
   NavigationButtonAnimationMetricsReporter& operator=(
       const NavigationButtonAnimationMetricsReporter&) = delete;
 
-  void ReportSmoothness(int smoothness) {
-    switch (target_state_) {
+  void ReportSmoothness(HotseatState target_hotseat_state, int smoothness) {
+    switch (target_hotseat_state) {
       case HotseatState::kShownClamshell:
       case HotseatState::kShownHomeLauncher:
         switch (navigation_button_type_) {
@@ -237,28 +231,18 @@ class ASH_EXPORT NavigationButtonAnimationMetricsReporter
     }
   }
 
-  metrics_util::ReportCallback GetReportCallback() {
+  metrics_util::ReportCallback GetReportCallback(
+      HotseatState target_hotseat_state) {
+    DCHECK_NE(target_hotseat_state, HotseatState::kNone);
     return metrics_util::ForSmoothness(base::BindRepeating(
         &NavigationButtonAnimationMetricsReporter::ReportSmoothness,
-        weak_ptr_factory_.GetWeakPtr()));
-  }
-
-  // ShelfLayoutManagerObserver:
-  void OnHotseatStateChanged(HotseatState old_state,
-                             HotseatState new_state) override {
-    target_state_ = new_state;
+        weak_ptr_factory_.GetWeakPtr(), target_hotseat_state));
   }
 
  private:
-  // Owned by RootWindowController
-  Shelf* const shelf_;
-
   // The type of navigation button that is animated.
   const NavigationButtonType navigation_button_type_;
 
-  // The state to which the animation is transitioning.
-  HotseatState target_state_ = HotseatState::kShownHomeLauncher;
-
   base::WeakPtrFactory<NavigationButtonAnimationMetricsReporter>
       weak_ptr_factory_{this};
 };
@@ -455,12 +439,10 @@ ShelfNavigationWidget::ShelfNavigationWidget(Shelf* shelf,
                                                   /*use_transforms=*/true)),
       back_button_metrics_reporter_(
           std::make_unique<NavigationButtonAnimationMetricsReporter>(
-              shelf,
               NavigationButtonAnimationMetricsReporter::NavigationButtonType::
                   kBackButton)),
       home_button_metrics_reporter_(
           std::make_unique<NavigationButtonAnimationMetricsReporter>(
-              shelf,
               NavigationButtonAnimationMetricsReporter::NavigationButtonType::
                   kHomeButton)) {
   DCHECK(shelf_);
@@ -595,6 +577,10 @@ void ShelfNavigationWidget::UpdateLayout(bool animate) {
       animate ? ShelfConfig::Get()->shelf_animation_duration()
               : base::TimeDelta::FromMilliseconds(0);
 
+  const HotseatState target_hotseat_state =
+      layout_manager->CalculateHotseatState(layout_manager->visibility_state(),
+                                            layout_manager->auto_hide_state());
+
   const bool update_opacity = !animate || GetLayer()->GetTargetOpacity() !=
                                               layout_manager->GetOpacity();
   const bool update_bounds =
@@ -611,7 +597,8 @@ void ShelfNavigationWidget::UpdateLayout(bool animate) {
     base::Optional<ui::AnimationThroughputReporter> reporter;
     if (animate) {
       reporter.emplace(nav_animation_setter.GetAnimator(),
-                       shelf_->GetNavigationWidgetAnimationReportCallback());
+                       shelf_->GetNavigationWidgetAnimationReportCallback(
+                           target_hotseat_state));
     }
     if (update_opacity)
       GetLayer()->SetOpacity(layout_manager->GetOpacity());
@@ -624,11 +611,13 @@ void ShelfNavigationWidget::UpdateLayout(bool animate) {
 
   views::View* const back_button = delegate_->back_button();
   UpdateButtonVisibility(back_button, back_button_shown, animate,
-                         back_button_metrics_reporter_.get());
+                         back_button_metrics_reporter_.get(),
+                         target_hotseat_state);
 
   views::View* const home_button = delegate_->home_button();
   UpdateButtonVisibility(home_button, home_button_shown, animate,
-                         home_button_metrics_reporter_.get());
+                         home_button_metrics_reporter_.get(),
+                         target_hotseat_state);
 
   if (back_button_shown) {
     // TODO(https://crbug.com/1058205): Test this behavior.
@@ -647,13 +636,16 @@ void ShelfNavigationWidget::UpdateLayout(bool animate) {
                         : GetFirstButtonBounds(shelf_->IsHorizontalAlignment());
 
   if (animate) {
-    bounds_animator_->SetAnimationDuration(animation_duration);
-    bounds_animator_->AnimateViewTo(
-        home_button, home_button_bounds,
-        std::make_unique<BoundsAnimationReporter>(
-            home_button, home_button_metrics_reporter_->GetReportCallback()));
+    if (bounds_animator_->GetTargetBounds(home_button) != home_button_bounds) {
+      bounds_animator_->SetAnimationDuration(animation_duration);
+      bounds_animator_->AnimateViewTo(
+          home_button, home_button_bounds,
+          std::make_unique<BoundsAnimationReporter>(
+              home_button, home_button_metrics_reporter_->GetReportCallback(
+                               target_hotseat_state)));
+    }
   } else {
-    bounds_animator_->StopAnimatingView(home_button);
+    bounds_animator_->Cancel();
     home_button->SetBoundsRect(home_button_bounds);
   }
 
@@ -683,7 +675,8 @@ void ShelfNavigationWidget::UpdateButtonVisibility(
     views::View* button,
     bool visible,
     bool animate,
-    NavigationButtonAnimationMetricsReporter* metrics_reporter) {
+    NavigationButtonAnimationMetricsReporter* metrics_reporter,
+    HotseatState target_hotseat_state) {
   if (animate && button->layer()->GetTargetOpacity() == visible)
     return;
 
@@ -705,7 +698,7 @@ void ShelfNavigationWidget::UpdateButtonVisibility(
   base::Optional<ui::AnimationThroughputReporter> reporter;
   if (animate) {
     reporter.emplace(opacity_settings.GetAnimator(),
-                     metrics_reporter->GetReportCallback());
+                     metrics_reporter->GetReportCallback(target_hotseat_state));
   }
 
   if (!visible)

ash/shelf/shelf_navigation_widget.h

@@ -24,6 +24,7 @@ class BoundsAnimator;
 namespace ash {
 class BackButton;
 class HomeButton;
+enum class HotseatState;
 class NavigationButtonAnimationMetricsReporter;
 class Shelf;
 class ShelfView;
@@ -98,7 +99,8 @@ class ASH_EXPORT ShelfNavigationWidget : public ShelfComponent,
       views::View* button,
       bool visible,
       bool animate,
-      NavigationButtonAnimationMetricsReporter* metrics_reporter);
+      NavigationButtonAnimationMetricsReporter* metrics_reporter,
+      HotseatState target_hotseat_state);
 
   // Returns the clip rectangle.
   gfx::Rect CalculateClipRect() const;

ash/shelf/shelf_view_test_api.cc

@@ -81,6 +81,9 @@ void ShelfViewTestAPI::SetAnimationDuration(base::TimeDelta duration) {
 
 void ShelfViewTestAPI::RunMessageLoopUntilAnimationsDone(
     views::BoundsAnimator* bounds_animator) {
+  if (!bounds_animator->IsAnimating())
+    return;
+
   std::unique_ptr<TestAPIAnimationObserver> observer(
       new TestAPIAnimationObserver());