Reland "Fix ShelfNavigationWidget metrics reporting"

This is a reland of 387397c7

The test that started failing after original cl landed was fixed in
CL:2333742

TBR=andrewxu@chromium.org
(The change is identical to the original)

Original change's description:
> 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/+/2333141
> Reviewed-by: Andrew Xu <andrewxu@chromium.org>
> Commit-Queue: Toni Baržić <tbarzic@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#793793}

Bug: 1108641
Change-Id: I0c01f1d229937c02f7d511ba833b144d09bed2c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2336781Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794632}

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());