Fix a bug in FrameSequenceTrackerCollection::NotifyFramePresented

Previously in this small refactor CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1769025,
we introduced a bug that StopSequence moves a tracker to the list of
|removal_tracker_|, and that NotifyFramePresented will never apply to
that list.

This CL fixes the bug and added unit testing.

Bug: None
Change-Id: I37a39994b26c5627e5fc1add9d9688cbf69184a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1774313
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692095}

cc/metrics/frame_sequence_tracker.cc

@@ -132,6 +132,9 @@ void FrameSequenceTrackerCollection::NotifyFramePresented(
   for (auto& tracker : frame_trackers_)
     tracker.second->ReportFramePresented(frame_token, feedback);
 
+  for (auto& tracker : removal_trackers_)
+    tracker->ReportFramePresented(frame_token, feedback);
+
   // Destroy the trackers that are ready to be terminated.
   base::EraseIf(
       removal_trackers_,

cc/metrics/frame_sequence_tracker.h

@@ -89,6 +89,8 @@ class CC_EXPORT FrameSequenceTrackerCollection {
   FrameSequenceTracker* GetTrackerForTesting(FrameSequenceTrackerType type);
 
  private:
+  friend class FrameSequenceTrackerTest;
+
   // The callsite can use the type to manipulate the tracker.
   base::flat_map<FrameSequenceTrackerType,
                  std::unique_ptr<FrameSequenceTracker>>
@@ -160,6 +162,7 @@ class CC_EXPORT FrameSequenceTracker {
 
  private:
   friend class FrameSequenceTrackerCollection;
+  friend class FrameSequenceTrackerTest;
 
   explicit FrameSequenceTracker(FrameSequenceTrackerType type);
 

cc/metrics/frame_sequence_tracker_unittest.cc

@@ -8,11 +8,9 @@
 #include "cc/metrics/compositor_frame_reporting_controller.h"
 #include "components/viz/common/frame_sinks/begin_frame_args.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/presentation_feedback.h"
 
 namespace cc {
-namespace {
-
-class FrameSequenceTrackerTest;
 
 class FrameSequenceTrackerTest : public testing::Test {
  public:
@@ -75,6 +73,30 @@ class FrameSequenceTrackerTest : public testing::Test {
     return ++frame_token;
   }
 
+  void TestNotifyFramePresented() {
+    collection_.StartSequence(FrameSequenceTrackerType::kCompositorAnimation);
+    collection_.StartSequence(FrameSequenceTrackerType::kMainThreadAnimation);
+    EXPECT_EQ(collection_.frame_trackers_.size(), 3u);
+
+    collection_.StopSequence(kCompositorAnimation);
+    EXPECT_EQ(collection_.frame_trackers_.size(), 2u);
+    EXPECT_TRUE(collection_.frame_trackers_.contains(
+        FrameSequenceTrackerType::kMainThreadAnimation));
+    EXPECT_TRUE(collection_.frame_trackers_.contains(
+        FrameSequenceTrackerType::kTouchScroll));
+    ASSERT_EQ(collection_.removal_trackers_.size(), 1u);
+    EXPECT_EQ(collection_.removal_trackers_[0]->type_,
+              FrameSequenceTrackerType::kCompositorAnimation);
+
+    gfx::PresentationFeedback feedback;
+    collection_.NotifyFramePresented(1u, feedback);
+    // NotifyFramePresented should call ReportFramePresented on all the
+    // |removal_trackers_|, which changes their termination_status_ to
+    // kReadyForTermination. So at this point, the |removal_trackers_| should be
+    // empty.
+    EXPECT_TRUE(collection_.removal_trackers_.empty());
+  }
+
  protected:
   std::unique_ptr<CompositorFrameReportingController>
       compositor_frame_reporting_controller_;
@@ -113,5 +135,8 @@ TEST_F(FrameSequenceTrackerTest, SourceIdChangeDuringSequence) {
                                 viz::BeginFrameAck(args_2, true), args_1);
 }
 
-}  // namespace
+TEST_F(FrameSequenceTrackerTest, TestNotifyFramePresented) {
+  TestNotifyFramePresented();
+}
+
 }  // namespace cc