Add CompositorFrameSinkClientBinding To Be Used By FrameGenerator

services/ui/ws/frame_generator_unittest.cc

Index: services/ui/ws/frame_generator_unittest.cc
diff --git a/services/ui/ws/frame_generator_unittest.cc b/services/ui/ws/frame_generator_unittest.cc
index 03aef330bc0a16480fe56d8318314d4899301ade..d3670a66314f5e3fcc7709509c2cf66bb3dd8c63 100644
--- a/services/ui/ws/frame_generator_unittest.cc
+++ b/services/ui/ws/frame_generator_unittest.cc
@@ -18,6 +18,22 @@ namespace ui {
 namespace ws {
 namespace test {
 
+namespace {
+
+constexpr float kRefreshRate = 0.f;
+constexpr bool kTickAutomatically = false;
+constexpr float kDefaultScaleFactor = 1.0f;
+constexpr float kArbitraryScaleFactor = 0.5f;
+constexpr gfx::Size kArbitrarySize(3, 4);
+constexpr gfx::Size kAnotherArbitrarySize(5, 6);
+const cc::SurfaceId kArbitrarySurfaceId(
+    cc::FrameSinkId(1, 1),
+    cc::LocalSurfaceId(1, base::UnguessableToken::Create()));
+const cc::SurfaceInfo kArbitrarySurfaceInfo(kArbitrarySurfaceId,
+                                            1.0f,
+                                            gfx::Size(100, 100));
+}
+
 // TestServerWindowDelegate implements ServerWindowDelegate and returns nullptrs
 // when either of the methods from the interface is called.
 class TestServerWindowDelegate : public ServerWindowDelegate {
@@ -38,38 +54,43 @@ class TestServerWindowDelegate : public ServerWindowDelegate {
   DISALLOW_COPY_AND_ASSIGN(TestServerWindowDelegate);
 };
 
-// FakeCompositorFrameSink observes a FakeExternalBeginFrameSource and receives
-// CompositorFrames from a FrameGenerator.
-class FakeCompositorFrameSink : public cc::CompositorFrameSink,
-                                public cc::BeginFrameObserver,
-                                public cc::ExternalBeginFrameSourceClient {
+// TestClientBinding Observes a BeginFrame and accepts CompositorFrame submitted
+// from FrameGenerator. It provides a way to inspect CompositorFrames.
+class TestClientBinding : public CompositorFrameSinkClientBinding,
+                          public cc::BeginFrameObserver {
  public:
-  FakeCompositorFrameSink()
-      : cc::CompositorFrameSink(nullptr, nullptr, nullptr, nullptr) {}
-
-  // cc::CompositorFrameSink implementation:
-  bool BindToClient(cc::CompositorFrameSinkClient* client) override {
-    if (!cc::CompositorFrameSink::BindToClient(client))
-      return false;
-
-    external_begin_frame_source_ =
-        base::MakeUnique<cc::ExternalBeginFrameSource>(this);
-    client_->SetBeginFrameSource(external_begin_frame_source_.get());
-    return true;
+  explicit TestClientBinding(
+      cc::mojom::MojoCompositorFrameSinkClient* sink_client)
+      : sink_client_(sink_client) {}
+  ~TestClientBinding() override = default;
+
+  // CompositorFrameSinkClientBinding implementation:
+  void SubmitCompositorFrame(cc::CompositorFrame frame) override {
+    ++frames_submitted_;
+    last_frame_ = std::move(frame);
+    begin_frame_source_->DidFinishFrame(this,
+                                        last_frame_.metadata.begin_frame_ack);
   }
 
-  void DetachFromClient() override {
-    cc::CompositorFrameSink::DetachFromClient();
+  void SetNeedsBeginFrame(bool needs_begin_frame) override {
+    if (needs_begin_frame == observing_begin_frames_)
+      return;
+
+    observing_begin_frames_ = needs_begin_frame;
+    if (needs_begin_frame) {
+      begin_frame_source_->AddObserver(this);
+    } else
+      begin_frame_source_->RemoveObserver(this);
   }
 
-  void SubmitCompositorFrame(cc::CompositorFrame frame) override {
-    ++number_frames_received_;
-    last_frame_ = std::move(frame);
+  void BeginFrameDidNotSwap(const cc::BeginFrameAck& ack) override {
+    if (observing_begin_frames_)
+      begin_frame_source_->DidFinishFrame(this, ack);
   }
 
   // cc::BeginFrameObserver implementation.
   void OnBeginFrame(const cc::BeginFrameArgs& args) override {
-    external_begin_frame_source_->OnBeginFrame(args);
+    sink_client_->OnBeginFrame(args);
     last_begin_frame_args_ = args;
   }
 
@@ -79,161 +100,118 @@ class FakeCompositorFrameSink : public cc::CompositorFrameSink,
 
   void OnBeginFrameSourcePausedChanged(bool paused) override {}
 
-  // cc::ExternalBeginFrameSourceClient implementation:
-  void OnNeedsBeginFrames(bool needs_begin_frames) override {
-    needs_begin_frames_ = needs_begin_frames;
-    UpdateNeedsBeginFramesInternal();
+  void SetBeginFrameSource(cc::BeginFrameSource* begin_frame_source) {
+    begin_frame_source_ = begin_frame_source;
   }
 
-  void OnDidFinishFrame(const cc::BeginFrameAck& ack) override {
-    begin_frame_source_->DidFinishFrame(this, ack);
-  }
-
-  void SetBeginFrameSource(cc::BeginFrameSource* source) {
-    if (begin_frame_source_ && observing_begin_frames_) {
-      begin_frame_source_->RemoveObserver(this);
-      observing_begin_frames_ = false;
-    }
-    begin_frame_source_ = source;
-    UpdateNeedsBeginFramesInternal();
+  const cc::RenderPassList& last_render_pass_list() const {
+    return last_frame_.render_pass_list;
   }
 
   const cc::CompositorFrameMetadata& last_metadata() const {
     return last_frame_.metadata;
   }
 
-  const cc::RenderPassList& last_render_pass_list() const {
-    return last_frame_.render_pass_list;
-  }
-
-  int number_frames_received() { return number_frames_received_; }
+  int frames_submitted() const { return frames_submitted_; }
 
  private:
-  void UpdateNeedsBeginFramesInternal() {
-    if (!begin_frame_source_)
-      return;
-
-    if (needs_begin_frames_ == observing_begin_frames_)
-      return;
-
-    observing_begin_frames_ = needs_begin_frames_;
-    if (needs_begin_frames_) {
-      begin_frame_source_->AddObserver(this);
-    } else {
-      begin_frame_source_->RemoveObserver(this);
-    }
-  }
-
-  int number_frames_received_ = 0;
-  std::unique_ptr<cc::ExternalBeginFrameSource> external_begin_frame_source_;
-  cc::BeginFrameSource* begin_frame_source_ = nullptr;
+  cc::mojom::MojoCompositorFrameSinkClient* sink_client_;
   cc::BeginFrameArgs last_begin_frame_args_;
-  bool observing_begin_frames_ = false;
-  bool needs_begin_frames_ = false;
   cc::CompositorFrame last_frame_;
-
-  DISALLOW_COPY_AND_ASSIGN(FakeCompositorFrameSink);
+  cc::BeginFrameSource* begin_frame_source_ = nullptr;
+  bool observing_begin_frames_ = false;
+  int frames_submitted_ = 0;
 };
 
 class FrameGeneratorTest : public testing::Test {
  public:
   FrameGeneratorTest() {}

[Fady Samuel, 2017/05/18 14:48:08]

nit: default

[Alex Z., 2017/05/18 15:09:14]

Done.

-  ~FrameGeneratorTest() override {}
+  ~FrameGeneratorTest() override = default;
 
   // testing::Test overrides:
   void SetUp() override {
     testing::Test::SetUp();
 
-    std::unique_ptr<FakeCompositorFrameSink> compositor_frame_sink =
-        base::MakeUnique<FakeCompositorFrameSink>();
-    compositor_frame_sink_ = compositor_frame_sink.get();
-
-    constexpr float kRefreshRate = 0.f;
-    constexpr bool kTickAutomatically = false;
+    frame_generator_ = base::MakeUnique<FrameGenerator>();
     begin_frame_source_ = base::MakeUnique<cc::FakeExternalBeginFrameSource>(
         kRefreshRate, kTickAutomatically);
-    compositor_frame_sink_->SetBeginFrameSource(begin_frame_source_.get());
-    server_window_delegate_ = base::MakeUnique<TestServerWindowDelegate>();
-    frame_generator_ =
-        base::MakeUnique<FrameGenerator>(std::move(compositor_frame_sink));
-    frame_generator_->OnWindowSizeChanged(gfx::Size(1, 2));
   };
 
+  // InitWithSurfaceInfo creates a TestClientBinding and binds it to
+  // |frame_generator_|. After InitWithSurfaceInfo finishes, |frame_generator_|
+  // has a valid SurfaceInfo and does not request BeginFrames.
   void InitWithSurfaceInfo() {
     // FrameGenerator requires a valid SurfaceInfo before generating
     // CompositorFrames.
-    const cc::SurfaceId kArbitrarySurfaceId(
-        cc::FrameSinkId(1, 1),
-        cc::LocalSurfaceId(1, base::UnguessableToken::Create()));
-    const cc::SurfaceInfo kArbitrarySurfaceInfo(kArbitrarySurfaceId, 1.0f,
-                                                gfx::Size(100, 100));
+    std::unique_ptr<TestClientBinding> client_binding =
+        base::MakeUnique<TestClientBinding>(frame_generator_.get());
+    binding_ = client_binding.get();
+    IssueBeginFrame();
+
+    // FrameGenerator does not request BeginFrames right after creation.
+    EXPECT_EQ(0, NumberOfFramesReceived());
+    client_binding->SetBeginFrameSource(begin_frame_source_.get());
+    frame_generator_->Bind(std::move(client_binding));
+    frame_generator_->OnSurfaceCreated(kArbitrarySurfaceInfo);
 
-    frame_generator()->OnSurfaceCreated(kArbitrarySurfaceInfo);
+    // Issue a BeginFrame so that frame_generator_ stops requesting BeginFrames
+    // after submitting a CompositorFrame.
     IssueBeginFrame();
     EXPECT_EQ(1, NumberOfFramesReceived());
   }
 
-  int NumberOfFramesReceived() {
-    return compositor_frame_sink_->number_frames_received();
-  }
-
   void IssueBeginFrame() {
     begin_frame_source_->TestOnBeginFrame(cc::CreateBeginFrameArgsForTesting(
         BEGINFRAME_FROM_HERE, 0, next_sequence_number_));
     ++next_sequence_number_;
   }
 
-  FrameGenerator* frame_generator() { return frame_generator_.get(); }
+  int NumberOfFramesReceived() const { return binding_->frames_submitted(); }
+
+  const cc::BeginFrameAck& LastBeginFrameAck(
+      TestClientBinding* test_client_binding) {
+    return begin_frame_source_->LastAckForObserver(test_client_binding);
+  }
 
   const cc::CompositorFrameMetadata& LastMetadata() const {
-    return compositor_frame_sink_->last_metadata();
+    return binding_->last_metadata();
   }
 
   const cc::RenderPassList& LastRenderPassList() const {
-    return compositor_frame_sink_->last_render_pass_list();
+    return binding_->last_render_pass_list();
   }
 
-  const cc::BeginFrameAck& LastBeginFrameAck() {
-    return begin_frame_source_->LastAckForObserver(compositor_frame_sink_);
+  FrameGenerator* frame_generator() { return frame_generator_.get(); }
+  cc::BeginFrameSource* begin_frame_source() {
+    return begin_frame_source_.get();
   }
 
+  TestClientBinding* binding() { return binding_; }
+
  private:
-  FakeCompositorFrameSink* compositor_frame_sink_ = nullptr;
   std::unique_ptr<cc::FakeExternalBeginFrameSource> begin_frame_source_;
-  std::unique_ptr<TestServerWindowDelegate> server_window_delegate_;
   std::unique_ptr<FrameGenerator> frame_generator_;
+  TestClientBinding* binding_ = nullptr;
   int next_sequence_number_ = 1;
 
   DISALLOW_COPY_AND_ASSIGN(FrameGeneratorTest);
 };
 
+// FrameGenerator has an invalid SurfaceInfo as default. FrameGenerator does not
+// submit CompositorFrames when its SurfaceInfo is invalid.
 TEST_F(FrameGeneratorTest, InvalidSurfaceInfo) {
-  // After SetUP(), frame_generator() has its |is_window_visible_| set to true
-  // and |bounds_| to an arbitrary non-empty gfx::Rect but not a valid
-  // SurfaceInfo. frame_generator() should not request BeginFrames in this
-  // state.
+  std::unique_ptr<TestClientBinding> test_client_binding =
+      base::MakeUnique<TestClientBinding>(frame_generator());
+  test_client_binding->SetBeginFrameSource(begin_frame_source());
+  TestClientBinding* binding = test_client_binding.get();
+  frame_generator()->Bind(std::move(test_client_binding));

[Fady Samuel, 2017/05/18 14:48:08]

Why are you doing this here? Is this still necessary now that the ClientBinding
is in FrameGeneratorTest?

[Alex Z., 2017/05/18 15:09:14]

I moved part of the InitWithSurfaceInfo into SetUp and now this
is not needed anymore.

   IssueBeginFrame();
-  EXPECT_EQ(0, NumberOfFramesReceived());
+  EXPECT_EQ(0, binding->frames_submitted());
+  EXPECT_EQ(cc::BeginFrameAck(), LastBeginFrameAck(binding));
 }
 
 TEST_F(FrameGeneratorTest, OnSurfaceCreated) {
-  EXPECT_EQ(0, NumberOfFramesReceived());
-
-  // FrameGenerator does not request BeginFrames upon creation.
-  IssueBeginFrame();
-  EXPECT_EQ(0, NumberOfFramesReceived());
-  EXPECT_EQ(cc::BeginFrameAck(), LastBeginFrameAck());
-
-  const cc::SurfaceId kArbitrarySurfaceId(
-      cc::FrameSinkId(1, 1),
-      cc::LocalSurfaceId(1, base::UnguessableToken::Create()));
-  const cc::SurfaceInfo kArbitrarySurfaceInfo(kArbitrarySurfaceId, 1.0f,
-                                              gfx::Size(100, 100));
-  frame_generator()->OnSurfaceCreated(kArbitrarySurfaceInfo);
-  EXPECT_EQ(0, NumberOfFramesReceived());
-
-  IssueBeginFrame();
-  EXPECT_EQ(1, NumberOfFramesReceived());
+  InitWithSurfaceInfo();
 
   // Verify that the CompositorFrame refers to the window manager's surface via
   // referenced_surfaces.
@@ -244,34 +222,17 @@ TEST_F(FrameGeneratorTest, OnSurfaceCreated) {
   EXPECT_EQ(kArbitrarySurfaceId, referenced_surfaces.front());
 
   cc::BeginFrameAck expected_ack(0, 2, 2, true);
-  EXPECT_EQ(expected_ack, LastBeginFrameAck());
+  EXPECT_EQ(expected_ack, LastBeginFrameAck(binding()));

[Fady Samuel, 2017/05/18 14:48:08]

Do we need this param if binding() is part of FrameGeneratorTest?

[Alex Z., 2017/05/18 15:11:13]

Done.

   EXPECT_EQ(expected_ack, last_metadata.begin_frame_ack);
 
   // FrameGenerator stops requesting BeginFrames after submitting a
   // CompositorFrame.
   IssueBeginFrame();
-  EXPECT_EQ(1, NumberOfFramesReceived());
-  EXPECT_EQ(expected_ack, LastBeginFrameAck());
+  EXPECT_EQ(expected_ack, LastBeginFrameAck(binding()));
 }
 
 TEST_F(FrameGeneratorTest, SetDeviceScaleFactor) {
-  EXPECT_EQ(0, NumberOfFramesReceived());
-  const cc::SurfaceId kArbitrarySurfaceId(
-      cc::FrameSinkId(1, 1),
-      cc::LocalSurfaceId(1, base::UnguessableToken::Create()));
-  const cc::SurfaceInfo kArbitrarySurfaceInfo(kArbitrarySurfaceId, 1.0f,
-                                              gfx::Size(100, 100));
-  constexpr float kDefaultScaleFactor = 1.0f;
-  constexpr float kArbitraryScaleFactor = 0.5f;
-
-  // A valid SurfaceInfo is required before setting device scale factor.
-  frame_generator()->OnSurfaceCreated(kArbitrarySurfaceInfo);
-  IssueBeginFrame();
-  EXPECT_EQ(1, NumberOfFramesReceived());
-
-  // FrameGenerator stops requesting BeginFrames after receiving one.
-  IssueBeginFrame();
-  EXPECT_EQ(1, NumberOfFramesReceived());
+  InitWithSurfaceInfo();
 
   // FrameGenerator does not request BeginFrames if its device scale factor
   // remains unchanged.
@@ -308,7 +269,6 @@ TEST_F(FrameGeneratorTest, WindowBoundsChanged) {
 
   // Window bounds change triggers a BeginFrame.
   constexpr int expected_render_pass_id = 1;
-  const gfx::Size kArbitrarySize(3, 4);
   frame_generator()->OnWindowSizeChanged(kArbitrarySize);
   IssueBeginFrame();
   EXPECT_EQ(2, NumberOfFramesReceived());
@@ -324,8 +284,6 @@ TEST_F(FrameGeneratorTest, WindowBoundsChanged) {
 TEST_F(FrameGeneratorTest, WindowBoundsChangedTwice) {
   InitWithSurfaceInfo();
 
-  const gfx::Size kArbitrarySize(3, 4);
-  const gfx::Size kAnotherArbitrarySize(5, 6);
   frame_generator()->OnWindowSizeChanged(kArbitrarySize);
   frame_generator()->OnWindowSizeChanged(kAnotherArbitrarySize);
   IssueBeginFrame();