[cc] Track observer status in ExternalBeginFrameSource.

cc/scheduler/begin_frame_source_unittest.cc

Index: cc/scheduler/begin_frame_source_unittest.cc
diff --git a/cc/scheduler/begin_frame_source_unittest.cc b/cc/scheduler/begin_frame_source_unittest.cc
index eac6507e25ae96c238bf80260eca4da37317835f..2f5077ba43e7054a45e4e2315a7e66367e743857 100644
--- a/cc/scheduler/begin_frame_source_unittest.cc
+++ b/cc/scheduler/begin_frame_source_unittest.cc
@@ -582,5 +582,363 @@ TEST_F(DelayBasedBeginFrameSourceTest, DoubleTickMissedFrame) {
   source_->RemoveObserver(&obs);
 }
 
+// BeginFrameObserverAckTracker testing ----------------------------------------
+class TestBeginFrameConsumer : public BeginFrameObserverBase {
+ public:
+  void SetDiscardNextBeginFrame() { discard_next_begin_frame_ = true; }
+
+ private:
+  bool OnBeginFrameDerivedImpl(const BeginFrameArgs& args) override {
+    // Consume the args and indicate whether we used them.
+    bool discard_this_begin_frame = discard_next_begin_frame_;
+    discard_next_begin_frame_ = false;
+    return !discard_this_begin_frame;
+  }
+  void OnBeginFrameSourcePausedChanged(bool paused) override {}
+
+  bool discard_next_begin_frame_ = false;
+};
+
+// Use EXPECT_TRUE instead of EXPECT_EQ for |finished| and |damage| as gcc 4.7
+// issues the following warning on EXPECT_EQ(false, x), which is turned into an
+// error with -Werror=conversion-null:
+//
+//   converting 'false' to pointer type for argument 1 of
+//   'char testing::internal::IsNullLiteralHelper(testing::internal::Secret*)'
+#define EXPECT_ACK_TRACKER_STATE(finished, damage, latest_confirmed)       \
+  EXPECT_TRUE(finished == tracker_->AllObserversFinishedFrame())           \
+      << "expected: " << finished;                                         \
+  EXPECT_TRUE(damage == tracker_->AnyObserversHadDamage()) << "expected: " \
+                                                           << damage;      \
+  EXPECT_EQ(latest_confirmed, tracker_->LatestConfirmedSequenceNumber())
+
+class BeginFrameObserverAckTrackerTest : public ::testing::Test {
+ public:
+  BeginFrameArgs current_args_;
+  std::unique_ptr<BeginFrameObserverAckTracker> tracker_;
+  std::unique_ptr<TestBeginFrameConsumer> obs1_;
+  std::unique_ptr<TestBeginFrameConsumer> obs2_;

[brianderson, 2017/02/17 21:26:09]

Do these need to be unique_ptrs?

[Eric Seckler, 2017/02/21 12:10:57]

Since I now no longer need to recreate the TestBeginFrameConsumers (don't have
any state anymore), I got rid of their unique_ptrs.

For the tracker, I'm sticking with unique_ptr to avoid copy-assign, which might
incur a copy, and to match the style of the other test harnesses in this file.
As long as you don't have an objection :)

+
+  void SetUp() override {
+    current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 1);
+    tracker_.reset(new BeginFrameObserverAckTracker());
+    obs1_.reset(new TestBeginFrameConsumer);
+    obs2_.reset(new TestBeginFrameConsumer);
+  }
+};
+
+TEST_F(BeginFrameObserverAckTrackerTest, CorrectnessWithoutObservers) {
+  // Check initial state.
+  EXPECT_ACK_TRACKER_STATE(true, false, 1u);
+
+  // A new BeginFrame is immediately finished and confirmed.
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 2);
+  tracker_->OnBeginFrame(current_args_);
+  EXPECT_ACK_TRACKER_STATE(true, false, 2u);
+}
+
+TEST_F(BeginFrameObserverAckTrackerTest, CorrectnessWith1Observer) {
+  // Check initial state.
+  EXPECT_ACK_TRACKER_STATE(true, false, 1u);
+
+  // After adding an observer, the BeginFrame is not finished or confirmed.
+  tracker_->OnObserverAdded(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(false, false, 0u);  // up to date to previous frame.
+
+  // On removing it, the BeginFrame is back to original state.
+  tracker_->OnObserverRemoved(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(true, false, 1u);
+
+  // After adding it back, the BeginFrame is again not finished or confirmed.
+  tracker_->OnObserverAdded(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(false, false, 0u);  // up to date to previous frame.
+
+  // When the observer finishes and confirms, the BeginFrame is finished
+  // and confirmed.
+  obs1_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs1_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs1_.get(),
+                                    BeginFrameAck(0, 1, 1, 0, false));
+  EXPECT_ACK_TRACKER_STATE(true, false, 1u);
+
+  // A new BeginFrame is initially not finished or confirmed.
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 2);
+  tracker_->OnBeginFrame(current_args_);
+  EXPECT_ACK_TRACKER_STATE(false, false, 1u);
+
+  // Stray ACK for an old BeginFrame is ignored.
+  tracker_->OnObserverFinishedFrame(obs1_.get(),
+                                    BeginFrameAck(0, 1, 1, 0, false));
+  EXPECT_ACK_TRACKER_STATE(false, false, 1u);
+
+  // When the observer finishes but doesn't confirm, the BeginFrame is finished
+  // but not confirmed.
+  obs1_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs1_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs1_.get(),
+                                    BeginFrameAck(0, 2, 1, 0, false));
+  EXPECT_ACK_TRACKER_STATE(true, false, 1u);
+
+  // Damage from ACK propagates.
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 3);
+  tracker_->OnBeginFrame(current_args_);
+  EXPECT_ACK_TRACKER_STATE(false, false, 1u);
+  obs1_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs1_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs1_.get(),
+                                    BeginFrameAck(0, 3, 3, 0, true));
+  EXPECT_ACK_TRACKER_STATE(true, true, 3u);
+
+  // Observer discarding frame during OnBeginFrame also finishes BeginFrame, but
+  // doesn't confirm it.
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 4);
+  tracker_->OnBeginFrame(current_args_);
+  EXPECT_ACK_TRACKER_STATE(false, false, 3u);
+  obs1_->SetDiscardNextBeginFrame();
+  obs1_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs1_.get(), current_args_);
+  EXPECT_ACK_TRACKER_STATE(true, false, 3u);
+
+  // Removing the observer confirms the latest BeginFrame.
+  tracker_->OnObserverRemoved(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(true, false, 4u);
+}
+
+TEST_F(BeginFrameObserverAckTrackerTest, CorrectnessWith2Observers) {
+  // Check initial state.
+  EXPECT_ACK_TRACKER_STATE(true, false, 1u);
+
+  // After adding observers, the BeginFrame is not finished or confirmed.
+  tracker_->OnObserverAdded(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(false, false, 0u);  // up to date to previous frame.
+  tracker_->OnObserverAdded(obs2_.get());
+  EXPECT_ACK_TRACKER_STATE(false, false, 0u);  // up to date to previous frame.
+
+  // Removing one of them changes nothing. Same for adding back.
+  tracker_->OnObserverRemoved(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(false, false, 0u);
+  tracker_->OnObserverAdded(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(false, false, 0u);
+
+  // When one observer finishes and confirms, nothing changes.
+  obs1_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs1_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs1_.get(),
+                                    BeginFrameAck(0, 1, 1, 0, false));
+  EXPECT_ACK_TRACKER_STATE(false, false, 0u);
+  // When both finish and confirm, the BeginFrame is finished and confirmed.
+  obs2_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs2_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs2_.get(),
+                                    BeginFrameAck(0, 1, 1, 0, false));
+  EXPECT_ACK_TRACKER_STATE(true, false, 1u);
+
+  // A new BeginFrame is not finished or confirmed.
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 2);
+  tracker_->OnBeginFrame(current_args_);
+  EXPECT_ACK_TRACKER_STATE(false, false, 1u);
+
+  // When both observers finish but only one confirms, the BeginFrame is
+  // finished but not confirmed.
+  obs1_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs1_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs1_.get(),
+                                    BeginFrameAck(0, 2, 2, 0, false));
+  EXPECT_ACK_TRACKER_STATE(false, false, 1u);
+  obs2_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs2_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs2_.get(),
+                                    BeginFrameAck(0, 2, 1, 0, false));
+  EXPECT_ACK_TRACKER_STATE(true, false, 1u);
+
+  // With reversed confirmations in the next ACKs, the latest confirmed frame
+  // increases but the latest BeginFrame remains unconfirmed.
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 3);
+  tracker_->OnBeginFrame(current_args_);
+  EXPECT_ACK_TRACKER_STATE(false, false, 1u);
+  obs1_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs1_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs1_.get(),
+                                    BeginFrameAck(0, 3, 2, 0, false));
+  EXPECT_ACK_TRACKER_STATE(false, false, 1u);
+  obs2_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs2_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs2_.get(),
+                                    BeginFrameAck(0, 3, 3, 0, false));
+  EXPECT_ACK_TRACKER_STATE(true, false, 2u);
+
+  // Only a single ACK with damage suffices.
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 4);
+  tracker_->OnBeginFrame(current_args_);
+  EXPECT_ACK_TRACKER_STATE(false, false, 2u);
+  obs1_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs1_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs1_.get(),
+                                    BeginFrameAck(0, 4, 4, 0, true));
+  EXPECT_ACK_TRACKER_STATE(false, true, 3u);
+  obs2_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs2_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs2_.get(),
+                                    BeginFrameAck(0, 4, 4, 0, false));
+  EXPECT_ACK_TRACKER_STATE(true, true, 4u);
+
+  // Removing the damaging observer makes no difference in this case.
+  tracker_->OnObserverRemoved(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(true, true, 4u);
+
+  // Adding the observer back considers it up to date up to the current
+  // BeginFrame, because it is the last used one. Thus, the current BeginFrame
+  // is still finished, too.
+  tracker_->OnObserverAdded(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(true, true, 4u);
+
+  // Adding the observer back after the next BeginFrame considers it up to date
+  // up to last BeginFrame only.
+  tracker_->OnObserverRemoved(obs1_.get());
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 5);
+  tracker_->OnBeginFrame(current_args_);
+  tracker_->OnObserverAdded(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(false, false, 4u);
+  // Both observers need to finish for the BeginFrame to be finished.
+  obs1_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs1_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs1_.get(),
+                                    BeginFrameAck(0, 5, 5, 0, false));
+  EXPECT_ACK_TRACKER_STATE(false, false, 4u);
+  obs2_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs2_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs2_.get(),
+                                    BeginFrameAck(0, 5, 5, 0, false));
+  EXPECT_ACK_TRACKER_STATE(true, false, 5u);
+}
+
+TEST_F(BeginFrameObserverAckTrackerTest, ChangingSourceIdOnBeginFrame) {
+  // Check initial state.
+  EXPECT_ACK_TRACKER_STATE(true, false, 1u);
+
+  // Changing source id without observer updates confirmed BeginFrame.
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 1, 10);
+  tracker_->OnBeginFrame(current_args_);
+  EXPECT_ACK_TRACKER_STATE(true, false, 10u);
+
+  // Setup an observer for current BeginFrame.
+  tracker_->OnObserverAdded(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(false, false, 9u);  // up to date to previous frame.
+  obs1_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs1_.get(), current_args_);
+  tracker_->OnObserverFinishedFrame(obs1_.get(),
+                                    BeginFrameAck(1, 10, 10, 0, true));
+  EXPECT_ACK_TRACKER_STATE(true, true, 10u);
+
+  // Changing source id with an observer sets confirmed BeginFrame to invalid.
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 2, 20);
+  tracker_->OnBeginFrame(current_args_);
+  EXPECT_ACK_TRACKER_STATE(false, false, BeginFrameArgs::kInvalidFrameNumber);
+}
+
+TEST_F(BeginFrameObserverAckTrackerTest, ChangingSourceIdOnObserverBeginFrame) {
+  // Check initial state.
+  EXPECT_ACK_TRACKER_STATE(true, false, 1u);
+
+  // Advance to a sequence number that isn't kInvalidFrameNumber+1.
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 5);
+  tracker_->OnBeginFrame(current_args_);
+  EXPECT_ACK_TRACKER_STATE(true, false, 5u);
+
+  // Assume that source has changed. When an observer is added before the next
+  // OnBeginFrame() from the new source, the tracker may see a call to
+  // OnObserverBeginFrame() with a new source_id. In this case, the confirmed
+  // BeginFrame should be set to invalid.
+  tracker_->OnObserverAdded(obs1_.get());
+  EXPECT_ACK_TRACKER_STATE(false, false, 4u);  // up to date to previous frame.
+  current_args_ = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 1, 10);
+  obs1_->OnBeginFrame(current_args_);
+  tracker_->OnObserverBeginFrame(obs1_.get(), current_args_);
+  EXPECT_ACK_TRACKER_STATE(false, false, BeginFrameArgs::kInvalidFrameNumber);
+}
+
+// ExternalBeginFrameSource testing --------------------------------------------
+class MockExternalBeginFrameSourceClient
+    : public ExternalBeginFrameSourceClient {
+ public:
+  MOCK_METHOD1(OnNeedsBeginFrames, void(bool));
+  MOCK_METHOD1(OnDidFinishFrame, void(const BeginFrameAck&));
+};
+
+class ExternalBeginFrameSourceTest : public ::testing::Test {
+ public:
+  std::unique_ptr<MockExternalBeginFrameSourceClient> client_;
+  std::unique_ptr<ExternalBeginFrameSource> source_;
+  std::unique_ptr<MockBeginFrameObserver> obs_;

[brianderson, 2017/02/17 21:26:09]

Do these need to be unique_ptrs?

[Eric Seckler, 2017/02/21 12:10:57]

At the very least, obs_ and client_ should stay one. They should be reset during
TearDown() (which I also failed to do :)).

I'm going to leave source_ as one as well, since all other test harnesses also
use it as a unique_ptr - and I'd rather like that to be consistent within the
file.

+
+  void SetUp() override {
+    client_.reset(new MockExternalBeginFrameSourceClient);
+    source_.reset(new ExternalBeginFrameSource(client_.get()));
+    obs_.reset(new MockBeginFrameObserver);
+  }
+};
+
+TEST_F(ExternalBeginFrameSourceTest, CallsOnDidFinishFrameWithoutObservers) {
+  EXPECT_CALL((*client_), OnDidFinishFrame(BeginFrameAck(0, 2, 2, 0, false)))
+      .Times(1);
+  source_->OnBeginFrame(CreateBeginFrameArgsForTesting(
+      BEGINFRAME_FROM_HERE, 0, 2, base::TimeTicks::FromInternalValue(10000)));
+}
+
+TEST_F(ExternalBeginFrameSourceTest,
+       CallsOnDidFinishFrameWhenObserverFinishes) {
+  EXPECT_BEGIN_FRAME_SOURCE_PAUSED(*obs_, false);
+  EXPECT_CALL((*client_), OnNeedsBeginFrames(true)).Times(1);
+  source_->AddObserver(obs_.get());
+
+  BeginFrameArgs args = CreateBeginFrameArgsForTesting(
+      BEGINFRAME_FROM_HERE, 0, 2, base::TimeTicks::FromInternalValue(10000));
+  EXPECT_BEGIN_FRAME_ARGS_USED(*obs_, args);
+  source_->OnBeginFrame(args);
+
+  EXPECT_CALL((*client_), OnDidFinishFrame(BeginFrameAck(0, 2, 2, 0, true)))
+      .Times(1);
+  source_->DidFinishFrame(obs_.get(), BeginFrameAck(0, 2, 2, 0, true));
+
+  args = CreateBeginFrameArgsForTesting(
+      BEGINFRAME_FROM_HERE, 0, 3, base::TimeTicks::FromInternalValue(20000));
+  EXPECT_BEGIN_FRAME_ARGS_USED(*obs_, args);
+  source_->OnBeginFrame(args);
+
+  EXPECT_CALL((*client_), OnDidFinishFrame(BeginFrameAck(0, 3, 2, 0, false)))
+      .Times(1);
+  source_->DidFinishFrame(obs_.get(), BeginFrameAck(0, 3, 2, 0, false));
+}
+
+TEST_F(ExternalBeginFrameSourceTest,
+       CallsOnDidFinishFrameWhenObserverDropsBeginFrame) {
+  EXPECT_BEGIN_FRAME_SOURCE_PAUSED(*obs_, false);
+  EXPECT_CALL((*client_), OnNeedsBeginFrames(true)).Times(1);
+  source_->AddObserver(obs_.get());
+
+  BeginFrameArgs args = CreateBeginFrameArgsForTesting(
+      BEGINFRAME_FROM_HERE, 0, 2, base::TimeTicks::FromInternalValue(10000));
+  EXPECT_BEGIN_FRAME_ARGS_DROP(*obs_, args);
+  EXPECT_CALL((*client_), OnDidFinishFrame(BeginFrameAck(0, 2, 0, 0, false)))
+      .Times(1);
+  source_->OnBeginFrame(args);
+}
+
+TEST_F(ExternalBeginFrameSourceTest, CallsOnDidFinishFrameWhenObserverRemoved) {
+  EXPECT_BEGIN_FRAME_SOURCE_PAUSED(*obs_, false);
+  EXPECT_CALL((*client_), OnNeedsBeginFrames(true)).Times(1);
+  source_->AddObserver(obs_.get());
+
+  BeginFrameArgs args = CreateBeginFrameArgsForTesting(
+      BEGINFRAME_FROM_HERE, 0, 2, base::TimeTicks::FromInternalValue(10000));
+  EXPECT_BEGIN_FRAME_ARGS_USED(*obs_, args);
+  source_->OnBeginFrame(args);
+
+  EXPECT_CALL((*client_), OnDidFinishFrame(BeginFrameAck(0, 2, 2, 0, false)))
+      .Times(1);
+  EXPECT_CALL((*client_), OnNeedsBeginFrames(false)).Times(1);
+  source_->RemoveObserver(obs_.get());
+}
+
 }  // namespace
 }  // namespace cc