Revert "[cros] Record Animation Smoothness for Clear All"
This reverts commit bc0d6f90d87d51276a6d67e1e38185ad67ec1c08.
Reason for revert: The newly added test is flaky on chromeos
But this is not a pure revert, as the CL has to keep the added
histograms.
Original change's description:
> [cros] Record Animation Smoothness for Clear All
>
> For the cros message center, record animation smoothness
> for the different animations. This CL covers "Clear All" which is
> triggered by clicking the clear all button, and "Move Down" which
> is done as a newer notification is deleted and the older ones animate
> downwards.
>
> These will be tested in a tast test in a follow up!
>
> Bug: 1225818
> Change-Id: Ie5acc0de70f15a3e0b3778c1dc5a7f0e0ecc56c9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3000861
> Commit-Queue: Alex Newcomer <newcomer@chromium.org>
> Reviewed-by: Ahmed Mehfooz <amehfooz@chromium.org>
> Reviewed-by: Brian White <bcwhite@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#899753}
Bug: 1225818
Change-Id: I5146559fff00c8301b198fbf0e0f6894cded0220
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3016849
Auto-Submit: Lingqi Chi <lingqi@chromium.org>
Reviewed-by: Asami Doi <asamidoi@chromium.org>
Commit-Queue: Lingqi Chi <lingqi@chromium.org>
Owners-Override: Lingqi Chi <lingqi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#899873}
diff --git a/ash/system/message_center/unified_message_center_bubble_unittest.cc b/ash/system/message_center/unified_message_center_bubble_unittest.cc
index 5adac41..5089df6 100644
--- a/ash/system/message_center/unified_message_center_bubble_unittest.cc
+++ b/ash/system/message_center/unified_message_center_bubble_unittest.cc
@@ -8,7 +8,6 @@
#include "ash/shell.h"
#include "ash/system/message_center/unified_message_center_view.h"
-#include "ash/system/message_center/unified_message_list_view.h"
#include "ash/system/tray/tray_constants.h"
#include "ash/system/unified/unified_system_tray.h"
#include "ash/system/unified/unified_system_tray_bubble.h"
@@ -16,7 +15,6 @@
#include "ash/system/unified/unified_system_tray_view.h"
#include "ash/test/ash_test_base.h"
#include "base/strings/stringprintf.h"
-#include "base/test/metrics/histogram_tester.h"
#include "ui/message_center/message_center.h"
using message_center::MessageCenter;
@@ -93,18 +91,8 @@
void WaitForAnimation() {
while (GetSystemTrayBubble()
->controller_for_test()
- ->animation_->is_animating()) {
+ ->animation_->is_animating())
base::RunLoop().RunUntilIdle();
- }
- }
-
- void WaitForNotificationAnimation() {
- while (GetMessageCenterBubble()
- ->message_center_view()
- ->message_list_view_for_test()
- ->IsAnimating()) {
- base::RunLoop().RunUntilIdle();
- }
}
views::View* GetFirstMessageCenterFocusable() {
@@ -335,26 +323,4 @@
GetFirstQuickSettingsFocusable());
}
-// Verifies that the Clear All animation in the child UnifiedMessageListView,
-// which occurs on notifications being removed via the Clear All button, records
-// metrics.
-TEST_F(UnifiedMessageCenterBubbleTest,
- AnimationSmoothnessMetricsClearAllVisible) {
- // Add a notification, then expand the message center.
- AddNotification();
- GetPrimaryUnifiedSystemTray()->ShowBubble();
- WaitForAnimation();
- ToggleExpanded();
- WaitForAnimation();
-
- // Clear all notifications, the Clear All (visible) notification should
- // trigger, and a histogram for animation performance should be logged.
- base::HistogramTester histogram_tester;
- GetMessageCenterBubble()->message_center_view()->ClearAllNotifications();
- WaitForNotificationAnimation();
-
- histogram_tester.ExpectTotalCount(
- "Ash.Notification.ClearAllVisible.AnimationSmoothness", 1);
-}
-
} // namespace ash
diff --git a/ash/system/message_center/unified_message_center_view.h b/ash/system/message_center/unified_message_center_view.h
index 288b920c..8111822 100644
--- a/ash/system/message_center/unified_message_center_view.h
+++ b/ash/system/message_center/unified_message_center_view.h
@@ -135,10 +135,6 @@
bool collapsed() { return collapsed_; }
- UnifiedMessageListView* message_list_view_for_test() {
- return message_list_view_;
- }
-
private:
friend class UnifiedMessageCenterViewTest;
friend class UnifiedMessageCenterBubbleTest;
diff --git a/ash/system/message_center/unified_message_list_view.cc b/ash/system/message_center/unified_message_list_view.cc
index 037d946..2eb63e3 100644
--- a/ash/system/message_center/unified_message_list_view.cc
+++ b/ash/system/message_center/unified_message_list_view.cc
@@ -3,9 +3,7 @@
// found in the LICENSE file.
#include "ash/system/message_center/unified_message_list_view.h"
-#include <string>
-#include "ash/public/cpp/metrics_util.h"
#include "ash/system/message_center/message_center_style.h"
#include "ash/system/message_center/message_center_utils.h"
#include "ash/system/message_center/metrics_utils.h"
@@ -14,10 +12,7 @@
#include "ash/system/tray/tray_constants.h"
#include "ash/system/unified/unified_system_tray_model.h"
#include "base/auto_reset.h"
-#include "base/callback_forward.h"
-#include "base/metrics/histogram_functions.h"
#include "base/metrics/histogram_macros.h"
-#include "ui/compositor/compositor.h"
#include "ui/gfx/animation/linear_animation.h"
#include "ui/gfx/canvas.h"
#include "ui/message_center/message_center.h"
@@ -26,7 +21,6 @@
#include "ui/views/border.h"
#include "ui/views/layout/box_layout.h"
#include "ui/views/layout/fill_layout.h"
-#include "ui/views/widget/widget.h"
using message_center::MessageCenter;
using message_center::MessageView;
@@ -43,31 +37,6 @@
constexpr base::TimeDelta kClearAllVisibleAnimationDuration =
base::TimeDelta::FromMilliseconds(160);
-constexpr char kMoveDownAnimationSmoothnessHistogramName[] =
- "Ash.Notification.MoveDown.AnimationSmoothness";
-constexpr char kClearAllStackedAnimationSmoothnessHistogramName[] =
- "Ash.Notification.ClearAllStacked.AnimationSmoothness";
-constexpr char kClearAllVisibleAnimationSmoothnessHistogramName[] =
- "Ash.Notification.ClearAllVisible.AnimationSmoothness";
-
-void RecordAnimationSmoothness(const std::string& histogram_name,
- int smoothness) {
- base::UmaHistogramPercentage(histogram_name, smoothness);
-}
-
-void SetupThroughputTrackerForAnimationSmoothness(
- views::Widget* widget,
- absl::optional<ui::ThroughputTracker>& tracker,
- const char* histogram_name) {
- // `widget` may not exist in tests.
- if (!widget)
- return;
-
- tracker.emplace(widget->GetCompositor()->RequestNewThroughputTracker());
- tracker->Start(ash::metrics_util::ForSmoothness(
- base::BindRepeating(&RecordAnimationSmoothness, histogram_name)));
-}
-
} // namespace
// Container view of notification and swipe control.
@@ -506,12 +475,6 @@
}
void UnifiedMessageListView::AnimationEnded(const gfx::Animation* animation) {
- if (throughput_tracker_) {
- // Reset `throughput_tracker_` to reset animation metrics recording.
- throughput_tracker_->Stop();
- throughput_tracker_.reset();
- }
-
// This is also called from AnimationCanceled().
animation_->SetCurrentValue(1.0);
PreferredSizeChanged();
@@ -660,33 +623,22 @@
void UnifiedMessageListView::StartAnimation() {
DCHECK_NE(state_, State::IDLE);
- base::TimeDelta animation_duration;
-
switch (state_) {
case State::IDLE:
break;
case State::MOVE_DOWN:
- SetupThroughputTrackerForAnimationSmoothness(
- GetWidget(), throughput_tracker_,
- kMoveDownAnimationSmoothnessHistogramName);
- animation_duration = kClosingAnimationDuration;
+ animation_->SetDuration(kClosingAnimationDuration);
+ animation_->Start();
break;
case State::CLEAR_ALL_STACKED:
- SetupThroughputTrackerForAnimationSmoothness(
- GetWidget(), throughput_tracker_,
- kClearAllStackedAnimationSmoothnessHistogramName);
- animation_duration = kClearAllStackedAnimationDuration;
+ animation_->SetDuration(kClearAllStackedAnimationDuration);
+ animation_->Start();
break;
case State::CLEAR_ALL_VISIBLE:
- SetupThroughputTrackerForAnimationSmoothness(
- GetWidget(), throughput_tracker_,
- kClearAllVisibleAnimationSmoothnessHistogramName);
- animation_duration = kClearAllVisibleAnimationDuration;
+ animation_->SetDuration(kClearAllVisibleAnimationDuration);
+ animation_->Start();
break;
}
-
- animation_->SetDuration(animation_duration);
- animation_->Start();
}
void UnifiedMessageListView::UpdateClearAllAnimation() {
@@ -708,6 +660,8 @@
}
PreferredSizeChanged();
+
+ state_ = State::CLEAR_ALL_STACKED;
} else {
state_ = State::CLEAR_ALL_VISIBLE;
}
diff --git a/ash/system/message_center/unified_message_list_view.h b/ash/system/message_center/unified_message_list_view.h
index bfbb3d83..538ce32 100644
--- a/ash/system/message_center/unified_message_list_view.h
+++ b/ash/system/message_center/unified_message_list_view.h
@@ -6,7 +6,6 @@
#define ASH_SYSTEM_MESSAGE_CENTER_UNIFIED_MESSAGE_LIST_VIEW_H_
#include "ash/ash_export.h"
-#include "ui/compositor/throughput_tracker.h"
#include "ui/message_center/message_center_observer.h"
#include "ui/message_center/views/message_view.h"
#include "ui/views/animation/animation_delegate_views.h"
@@ -70,7 +69,7 @@
// Returns the total number of pinned notifications in the list.
int GetTotalPinnedNotificationCount() const;
- // Returns true if `animation_` is currently in progress.
+ // Returns true if an animation is currently in progress.
bool IsAnimating() const;
// Called when a notification is slid out so we can run the MOVE_DOWN
@@ -119,8 +118,8 @@
class Background;
class MessageViewContainer;
- // UnifiedMessageListView always runs a single animation at one time. When
- // `state_` is IDLE, `animation_->is_animating()` is always false and vice
+ // UnifiedMessageListView always runs single animation at one time. When
+ // |state_| is IDLE, animation_->is_animating() is always false and vice
// versa.
enum class State {
// No animation is running.
@@ -201,9 +200,6 @@
// implicit animation.
const std::unique_ptr<gfx::LinearAnimation> animation_;
- // Measure animation smoothness metrics for `animation_`.
- absl::optional<ui::ThroughputTracker> throughput_tracker_;
-
State state_ = State::IDLE;
// The height the UnifiedMessageListView starts animating from. If not
