Adds UMA for screen rotation animation smoothness.

ash/rotator/screen_rotation_animator.cc

Index: ash/rotator/screen_rotation_animator.cc
diff --git a/ash/rotator/screen_rotation_animator.cc b/ash/rotator/screen_rotation_animator.cc
index c0dbe2058d6f7a4c33314ed1f07650cde2446195..4b793635ee8f57b0eacca5eed01f11fe11ef21e5 100644
--- a/ash/rotator/screen_rotation_animator.cc
+++ b/ash/rotator/screen_rotation_animator.cc
@@ -13,7 +13,9 @@
 #include "ash/rotator/screen_rotation_animator_observer.h"
 #include "ash/shell.h"
 #include "base/command_line.h"
+#include "base/lazy_instance.h"

[varkha, 2017/03/24 02:50:25]

Not necessary?

[wutao, 2017/03/24 06:12:38]

Removed.

 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
 #include "ui/aura/window.h"
 #include "ui/compositor/layer.h"
@@ -47,6 +49,10 @@ const int kRotationDurationInMs = 250;
 const int kCounterClockWiseRotationFactor = 1;
 const int kClockWiseRotationFactor = -1;
 
+// The percentage histogram boundary.
+const int kPercentage_MIN = 1;
+const int kPercentage_MAX = 101;
+
 // Aborts the active animations of the layer, and recurses upon its child
 // layers.
 void AbortAnimations(ui::Layer* layer) {
@@ -177,11 +183,25 @@ struct ScreenRotationAnimator::ScreenRotationRequest {
   display::Display::RotationSource source;
 };
 
+class ScreenRotationAnimator::ScreenRotationAnimationMetricsReporter
+    : public ui::AnimationMetricsReporter {
+  void Report(int value) override {
+    base::LinearHistogram::FactoryGet(
+        "Ash.Rotator.ScreenRotationAnimationSmoothness", kPercentage_MIN,

[varkha, 2017/03/24 02:50:25]

Suggestion: how about "Ash.Rotation.AnimationSmoothness" to be somewhat similar
to the overview UMA names?

[wutao, 2017/03/24 06:12:38]

Done.

+        kPercentage_MAX, kPercentage_MAX + 1,
+        base::HistogramBase::kUmaTargetedHistogramFlag)
+        ->Add(value);

[varkha, 2017/03/24 02:50:25]

nit: should the line above be indented?

[wutao, 2017/03/24 06:12:38]

git cl format made this.

+  }
+};
+
 ScreenRotationAnimator::ScreenRotationAnimator(int64_t display_id)
     : display_id_(display_id),
       is_rotating_(false),
       disable_animation_timers_for_test_(false),
-      weak_factory_(this) {}
+      weak_factory_(this) {
+  metrics_reporter_ =
+      base::MakeUnique<ScreenRotationAnimationMetricsReporter>();

[varkha, 2017/03/24 02:50:25]

This could be handled in the initializer list.
Also does it have the right lifetime, i.e. could the animation sequence outlive
the ScreenRotationAnimator?

[varkha, 2017/03/24 03:01:33]

One maybe possible scenario to consider is when you shut down the OS shell that
owns |this| while animation is ongoing. You can try making animation duration
long and closing the Chrome OS simulation window.

[wutao, 2017/03/24 06:12:38]

Tried to increase the animation duration and closed the Chrome OS simulation
window. No exception.

[wutao, 2017/03/24 06:12:38]

Moved to initializer list.

Good observation.
|animator| should outlive the animation sequence of |old_layer_tree|.
The animated |old_layer_tree| is a unique_ptr member of |animator|. When
destroying |animator|, |old_layer_tree| is destroyed and the animation sequence
is aborted.
That is possible during destroying |animator|, the |metrics_reporter| is
destroyed first and then animation sequence finished. I will reorder the member
|metrics_reporter_| before |old_layer_tree_owner_|.

[varkha, 2017/03/24 15:08:47]

The lifetime part is subtle. Can you please add a comment about this in the
header or tie the lifetimes tighter in some way. Would be also good to have some
test coverage for this scenario.

+}
 
 ScreenRotationAnimator::~ScreenRotationAnimator() {}
 
@@ -278,6 +298,7 @@ void ScreenRotationAnimator::AnimateRotation(
   // control the animation.
   if (disable_animation_timers_for_test_)
     animator->set_disable_timer_for_test(true);
+  animation_sequence->SetAnimationMetricsReporter(metrics_reporter_.get());
   animator->StartAnimation(animation_sequence.release());
 
   rotation_request.reset();