Fix OverviewWindowDragTest.DragToClose

* The test should observe the active window (that will be dragged)
  instead of all.
* PresentationTimeRecorder used to discard the presentation feedback
  that can happen after it is deleted. This CL introduces the internal
  state that can outlive until the feedback is recorded.
* I also removed the max latency as it's very noisy and normal latency
  should be enough. (SplitView test does the same)

Bug: 953355
Change-Id: I3b3e329ca4e866004763f18c55f71816d5a5d535
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1572268
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652282}

ash/app_list/views/app_list_view.cc

@@ -1540,7 +1540,7 @@ void AppListView::SetIsInDrag(bool is_in_drag) {
     DCHECK(!presentation_time_recorder_);
     if (!is_tablet_mode_) {
       presentation_time_recorder_ =
-          std::make_unique<ash::PresentationTimeHistogramRecorder>(
+          ash::CreatePresentationTimeHistogramRecorder(
               GetWidget()->GetCompositor(), kAppListDragInClamshellHistogram,
               kAppListDragInClamshellMaxLatencyHistogram);
     }

ash/app_list/views/apps_grid_view.cc

@@ -2579,15 +2579,13 @@ void AppsGridView::ScrollStarted() {
   DCHECK(!presentation_time_recorder_);
 
   if (IsTabletMode()) {
-    presentation_time_recorder_ =
-        std::make_unique<ash::PresentationTimeHistogramRecorder>(
-            GetWidget()->GetCompositor(), kPageDragScrollInTabletHistogram,
-            kPageDragScrollInTabletMaxLatencyHistogram);
+    presentation_time_recorder_ = ash::CreatePresentationTimeHistogramRecorder(
+        GetWidget()->GetCompositor(), kPageDragScrollInTabletHistogram,
+        kPageDragScrollInTabletMaxLatencyHistogram);
   } else {
-    presentation_time_recorder_ =
-        std::make_unique<ash::PresentationTimeHistogramRecorder>(
-            GetWidget()->GetCompositor(), kPageDragScrollInClamshellHistogram,
-            kPageDragScrollInClamshellMaxLatencyHistogram);
+    presentation_time_recorder_ = ash::CreatePresentationTimeHistogramRecorder(
+        GetWidget()->GetCompositor(), kPageDragScrollInClamshellHistogram,
+        kPageDragScrollInClamshellMaxLatencyHistogram);
   }
 }
 

ash/metrics/presentation_time_recorder_unittest.cc

@@ -25,7 +25,7 @@ TEST_F(PresentationTimeRecorderTest, Histogram) {
   base::HistogramTester histogram_tester;
 
   auto* compositor = CurrentContext()->layer()->GetCompositor();
-  auto test_recorder = std::make_unique<PresentationTimeHistogramRecorder>(
+  auto test_recorder = CreatePresentationTimeHistogramRecorder(
       compositor, kName, kMaxLatencyName);
 
   // Flush pending draw requests.
@@ -70,27 +70,65 @@ TEST_F(PresentationTimeRecorderTest, Histogram) {
   histogram_tester.ExpectTotalCount(kMaxLatencyName, 1);
 }
 
-TEST_F(PresentationTimeRecorderTest, Failure) {
+TEST_F(PresentationTimeRecorderTest, NoSuccessNoHistogram) {
+  base::HistogramTester histogram_tester;
   auto* compositor = CurrentContext()->layer()->GetCompositor();
-  auto test_recorder = std::make_unique<PresentationTimeHistogramRecorder>(
+  auto test_recorder = CreatePresentationTimeHistogramRecorder(
       compositor, kName, kMaxLatencyName);
+  PresentationTimeRecorder::TestApi test_api(test_recorder.get());
+  base::TimeDelta interval_not_used = base::TimeDelta::FromMilliseconds(0);
+  gfx::PresentationFeedback failure(base::TimeTicks::FromUptimeMillis(2000),
+                                    interval_not_used,
+                                    gfx::PresentationFeedback::kFailure);
+  base::TimeTicks start = base::TimeTicks::FromUptimeMillis(1000);
+  test_recorder->RequestNext();
+  test_api.OnPresented(0, start, failure);
 
+  test_recorder.reset();
+  histogram_tester.ExpectTotalCount(kName, 0);
+  histogram_tester.ExpectTotalCount(kMaxLatencyName, 0);
+}
+
+TEST_F(PresentationTimeRecorderTest, DelayedHistogram) {
+  base::HistogramTester histogram_tester;
+  auto* compositor = CurrentContext()->layer()->GetCompositor();
+  auto test_recorder = CreatePresentationTimeHistogramRecorder(
+      compositor, kName, kMaxLatencyName);
+  test_recorder->RequestNext();
+
+  // Delete the recorder while waiting for the presentation callback.
+  test_recorder.reset();
+  histogram_tester.ExpectTotalCount(kName, 0);
+  histogram_tester.ExpectTotalCount(kMaxLatencyName, 0);
+
+  // Draw next frame and make sure the histgoram is recorded.
+  compositor->ScheduleFullRedraw();
+  WaitForNextFrameToBePresented(compositor);
+  histogram_tester.ExpectTotalCount(kName, 1);
+  histogram_tester.ExpectTotalCount(kMaxLatencyName, 1);
+}
+
+TEST_F(PresentationTimeRecorderTest, Failure) {
+  auto* compositor = CurrentContext()->layer()->GetCompositor();
+  auto test_recorder = CreatePresentationTimeHistogramRecorder(
+      compositor, kName, kMaxLatencyName);
+  PresentationTimeRecorder::TestApi test_api(test_recorder.get());
   test_recorder->RequestNext();
-  test_recorder->OnCompositingDidCommit(compositor);
+  test_api.OnCompositingDidCommit(compositor);
   base::TimeDelta interval_not_used = base::TimeDelta::FromMilliseconds(0);
   base::TimeTicks start = base::TimeTicks::FromUptimeMillis(1000);
   gfx::PresentationFeedback success(base::TimeTicks::FromUptimeMillis(1100),
                                     interval_not_used, /*flags=*/0);
-  test_recorder->OnPresented(0, start, success);
-  EXPECT_EQ(100, test_recorder->max_latency_ms());
-  EXPECT_EQ(1, test_recorder->success_count());
+  test_api.OnPresented(0, start, success);
+  EXPECT_EQ(100, test_api.GetMaxLatencyMs());
+  EXPECT_EQ(1, test_api.GetSuccessCount());
   gfx::PresentationFeedback failure(base::TimeTicks::FromUptimeMillis(2000),
                                     interval_not_used,
                                     gfx::PresentationFeedback::kFailure);
-  test_recorder->OnPresented(0, start, failure);
+  test_api.OnPresented(0, start, failure);
   // Failure should not be included in max latency.
-  EXPECT_EQ(100, test_recorder->max_latency_ms());
-  EXPECT_EQ(50, test_recorder->failure_ratio());
+  EXPECT_EQ(100, test_api.GetMaxLatencyMs());
+  EXPECT_EQ(50, test_api.GetFailureRatio());
 }
 
 }  // namespace ash

ash/public/cpp/presentation_time_recorder.h

@@ -24,97 +24,53 @@ namespace ash {
 // doesn't involve animations (such as window dragging). Call |RequestNext()|
 // when you made modification to UI that should expect it will be presented.
 // TODO(oshima): Move this to ash/metrics after crbug.com/942564 is resolved.
-class ASH_PUBLIC_EXPORT PresentationTimeRecorder
-    : public ui::CompositorObserver {
+class ASH_PUBLIC_EXPORT PresentationTimeRecorder {
  public:
-  explicit PresentationTimeRecorder(ui::Compositor* compositor);
-  ~PresentationTimeRecorder() override;
+  class PresentationTimeRecorderInternal;
+  class TestApi {
+   public:
+    explicit TestApi(PresentationTimeRecorder* recorder);
+
+    void OnCompositingDidCommit(ui::Compositor* compositor);
+    void OnPresented(int count,
+                     base::TimeTicks requested_time,
+                     const gfx::PresentationFeedback& feedback);
+
+    int GetMaxLatencyMs() const;
+    int GetSuccessCount() const;
+    int GetFailureRatio() const;
+
+   private:
+    PresentationTimeRecorder* recorder_;
+    DISALLOW_COPY_AND_ASSIGN(TestApi);
+  };
+
+  explicit PresentationTimeRecorder(
+      std::unique_ptr<PresentationTimeRecorderInternal> internal);
+  ~PresentationTimeRecorder();
 
   // Start recording next frame. It skips requesting next frame and returns
   // false if the previous frame has not been committed yet.
   bool RequestNext();
 
-  // ui::CompositorObserver:
-  void OnCompositingDidCommit(ui::Compositor* compositor) override;
-  void OnCompositingShuttingDown(ui::Compositor* compositor) override;
-
   // Enable this to report the presentation time immediately with
   // fake value when RequestNext is called.
   static void SetReportPresentationTimeImmediatelyForTest(bool enable);
 
- protected:
-  FRIEND_TEST_ALL_PREFIXES(PresentationTimeRecorderTest, Failure);
-
-  int max_latency_ms() const { return max_latency_ms_; }
-  int success_count() const { return success_count_; }
-
-  // |delta| is the duration between the successful request time and
-  // presentation time.
-  virtual void ReportTime(base::TimeDelta delta) = 0;
-
-  static bool GetReportPresentationTimeImmediatelyForTest();
-
  private:
-  enum State {
-    // The frame has been presented to the screen. This is the initial state.
-    PRESENTED,
-    // The presentation feedback has been requested.
-    REQUESTED,
-    // The changes to layers have been submitted, and waiting to be presented.
-    COMMITTED,
-  };
-  void OnPresented(int count,
-                   base::TimeTicks requested_time,
-                   const gfx::PresentationFeedback& feedback);
-
-  int average_latency_ms() const {
-    return success_count_ ? total_latency_ms_ / success_count_ : 0;
-  }
-  int failure_ratio() const {
-    return failure_count_
-               ? (100 * failure_count_) / (success_count_ + failure_count_)
-               : 0;
-  }
-
-  State state_ = PRESENTED;
-
-  int success_count_ = 0;
-  int failure_count_ = 0;
-  int request_count_ = 0;
-  int total_latency_ms_ = 0;
-  int max_latency_ms_ = 0;
-
-  ui::Compositor* compositor_ = nullptr;
-
-  base::WeakPtrFactory<PresentationTimeRecorder> weak_ptr_factory_;
+  std::unique_ptr<PresentationTimeRecorderInternal> recorder_internal_;
 
   DISALLOW_COPY_AND_ASSIGN(PresentationTimeRecorder);
 };
 
-// A utility class to record timing histograms of presentation time and max
-// latency. The time range is 1 to 200 ms, with 50 buckets.
-class ASH_PUBLIC_EXPORT PresentationTimeHistogramRecorder
-    : public PresentationTimeRecorder {
- public:
-  // |presentation_time_histogram_name| records latency reported on
-  // |ReportTime()| and |max_latency_histogram_name| records the maximum latency
-  // reported during the lifetime of this object.  Histogram names must be the
-  // name of the UMA histogram defined in histograms.xml.
-  PresentationTimeHistogramRecorder(
-      ui::Compositor* compositor,
-      const char* presentation_time_histogram_name,
-      const char* max_latency_histogram_name);
-  ~PresentationTimeHistogramRecorder() override;
-
-  // PresentationTimeRecorder:
-  void ReportTime(base::TimeDelta delta) override;
-
- private:
-  base::HistogramBase* presentation_time_histogram_;
-  std::string max_latency_histogram_name_;
-
-  DISALLOW_COPY_AND_ASSIGN(PresentationTimeHistogramRecorder);
-};
+// Creates a PresentationTimeRecorder that records timing histograms of
+// presentation time and max latency. The time range is 1 to 200 ms, with 50
+// buckets.
+ASH_PUBLIC_EXPORT std::unique_ptr<PresentationTimeRecorder>
+CreatePresentationTimeHistogramRecorder(
+    ui::Compositor* compositor,
+    const char* presentation_time_histogram_name,
+    const char* max_latency_histogram_name);
 
 }  // namespace ash
 

ash/wm/overview/overview_window_drag_controller.cc

@@ -83,10 +83,9 @@ void OverviewWindowDragController::InitiateDrag(
   Shell::Get()->overview_controller()->PauseOcclusionTracker();
   DCHECK(!presentation_time_recorder_);
 
-  presentation_time_recorder_ =
-      std::make_unique<ash::PresentationTimeHistogramRecorder>(
-          item_->root_window()->layer()->GetCompositor(),
-          kOverviewWindowDragHistogram, kOverviewWindowDragMaxLatencyHistogram);
+  presentation_time_recorder_ = CreatePresentationTimeHistogramRecorder(
+      item_->root_window()->layer()->GetCompositor(),
+      kOverviewWindowDragHistogram, kOverviewWindowDragMaxLatencyHistogram);
 }
 
 void OverviewWindowDragController::Drag(const gfx::PointF& location_in_screen) {

ash/wm/splitview/split_view_controller.cc

@@ -554,23 +554,20 @@ void SplitViewController::StartResize(const gfx::Point& location_in_screen) {
 
   base::RecordAction(base::UserMetricsAction("SplitView_ResizeWindows"));
   if (state_ == BOTH_SNAPPED) {
-    presentation_time_recorder_ =
-        std::make_unique<PresentationTimeHistogramRecorder>(
-            split_view_divider_->divider_widget()->GetCompositor(),
-            kSplitViewResizeMultiHistogram,
-            kSplitViewResizeMultiMaxLatencyHistogram);
+    presentation_time_recorder_ = CreatePresentationTimeHistogramRecorder(
+        split_view_divider_->divider_widget()->GetCompositor(),
+        kSplitViewResizeMultiHistogram,
+        kSplitViewResizeMultiMaxLatencyHistogram);
   } else if (GetOverviewSession() && !GetOverviewSession()->IsEmpty()) {
-    presentation_time_recorder_ =
-        std::make_unique<PresentationTimeHistogramRecorder>(
-            split_view_divider_->divider_widget()->GetCompositor(),
-            kSplitViewResizeWithOverviewHistogram,
-            kSplitViewResizeWithOverviewMaxLatencyHistogram);
+    presentation_time_recorder_ = CreatePresentationTimeHistogramRecorder(
+        split_view_divider_->divider_widget()->GetCompositor(),
+        kSplitViewResizeWithOverviewHistogram,
+        kSplitViewResizeWithOverviewMaxLatencyHistogram);
   } else {
-    presentation_time_recorder_ =
-        std::make_unique<PresentationTimeHistogramRecorder>(
-            split_view_divider_->divider_widget()->GetCompositor(),
-            kSplitViewResizeSingleHistogram,
-            kSplitViewResizeSingleMaxLatencyHistogram);
+    presentation_time_recorder_ = CreatePresentationTimeHistogramRecorder(
+        split_view_divider_->divider_widget()->GetCompositor(),
+        kSplitViewResizeSingleHistogram,
+        kSplitViewResizeSingleMaxLatencyHistogram);
   }
 }
 

ash/wm/tablet_mode/tablet_mode_window_drag_delegate.cc

@@ -115,16 +115,13 @@ void TabletModeWindowDragDelegate::StartWindowDrag(
   DCHECK(!presentation_time_recorder_);
   presentation_time_recorder_.reset();
   if (wm::IsDraggingTabs(dragged_window)) {
-    presentation_time_recorder_ =
-        std::make_unique<PresentationTimeHistogramRecorder>(
-            dragged_window->layer()->GetCompositor(),
-            kSwipeDownDragTabHistogram, kSwipeDownDragTabMaxLatencyHistogram);
+    presentation_time_recorder_ = CreatePresentationTimeHistogramRecorder(
+        dragged_window->layer()->GetCompositor(), kSwipeDownDragTabHistogram,
+        kSwipeDownDragTabMaxLatencyHistogram);
   } else {
-    presentation_time_recorder_ =
-        std::make_unique<PresentationTimeHistogramRecorder>(
-            dragged_window->layer()->GetCompositor(),
-            kSwipeDownDragWindowHistogram,
-            kSwipeDownDragWindowMaxLatencyHistogram);
+    presentation_time_recorder_ = CreatePresentationTimeHistogramRecorder(
+        dragged_window->layer()->GetCompositor(), kSwipeDownDragWindowHistogram,
+        kSwipeDownDragWindowMaxLatencyHistogram);
   }
 
   dragged_window_ = dragged_window;

chrome/browser/ui/ash/overview_window_drag_interactive_uitest.cc

@@ -247,7 +247,6 @@ class OverviewWindowDragTest
   std::vector<std::string> GetUMAHistogramNames() const override {
     return {
         "Ash.Overview.WindowDrag.PresentationTime.TabletMode",
-        "Ash.Overview.WindowDrag.PresentationTime.MaxLatency.TabletMode",
     };
   }
 
@@ -301,7 +300,7 @@ IN_PROC_BROWSER_TEST_P(OverviewWindowDragTest, DISABLED_DragToClose) {
 
   content::WindowedNotificationObserver waiter(
       chrome::NOTIFICATION_BROWSER_CLOSED,
-      content::NotificationService::AllSources());
+      content::Source<Browser>(chrome::FindLastActive()));
 
   gfx::Point start_point = GetStartLocation(GetDisplaySize(browser_window));
   gfx::Point end_point(start_point);