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..30e0eda4f2b5850cbd68a3271153555474a0a7d8 100644
--- a/services/ui/ws/frame_generator_unittest.cc
+++ b/services/ui/ws/frame_generator_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "services/ui/ws/frame_generator.h"
 
+#include "base/debug/stack_trace.h"

[Fady Samuel, 2017/05/18 11:55:28]

Delete this.

[Alex Z., 2017/05/18 14:32:22]

Done.

 #include "base/macros.h"
 #include "cc/output/compositor_frame_sink.h"
 #include "cc/scheduler/begin_frame_source.h"
@@ -38,38 +39,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,102 +85,61 @@ 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 OnDidFinishFrame(const cc::BeginFrameAck& ack) override {
-    begin_frame_source_->DidFinishFrame(this, ack);
+  void SetBeginFrameSource(cc::BeginFrameSource* begin_frame_source) {
+    begin_frame_source_ = begin_frame_source;
   }
 
-  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() {}
-  ~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));
   };
 
   void InitWithSurfaceInfo() {
     // FrameGenerator requires a valid SurfaceInfo before generating
     // CompositorFrames.
+    std::unique_ptr<TestClientBinding> client_binding =
+        base::MakeUnique<TestClientBinding>(frame_generator_.get());
+    binding_ = client_binding.get();
+    client_binding->SetBeginFrameSource(begin_frame_source_.get());
+    frame_generator_->Bind(std::move(client_binding));
     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);
+    frame_generator_->OnSurfaceCreated(kArbitrarySurfaceInfo);
     IssueBeginFrame();
-    EXPECT_EQ(1, NumberOfFramesReceived());
-  }
-
-  int NumberOfFramesReceived() {
-    return compositor_frame_sink_->number_frames_received();
+    EXPECT_EQ(1, binding_->frames_submitted());
   }
 
   void IssueBeginFrame() {
@@ -183,46 +148,46 @@ class FrameGeneratorTest : public testing::Test {
     ++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);
 };
 
-TEST_F(FrameGeneratorTest, InvalidSurfaceInfo) {
-  // After SetUP(), frame_generator() has its |is_window_visible_| set to true

[Fady Samuel, 2017/05/18 11:55:28]

Why delete this?

[Alex Z., 2017/05/18 14:32:22]

It's restored.
It was deleted because there wasn't much to test when the test bindging
was using gmock.

-  // and |bounds_| to an arbitrary non-empty gfx::Rect but not a valid
-  // SurfaceInfo. frame_generator() should not request BeginFrames in this
-  // state.
-  IssueBeginFrame();
-  EXPECT_EQ(0, NumberOfFramesReceived());
-}
-
 TEST_F(FrameGeneratorTest, OnSurfaceCreated) {
-  EXPECT_EQ(0, NumberOfFramesReceived());

[Fady Samuel, 2017/05/18 11:55:28]

Why delete this?

[Alex Z., 2017/05/18 14:32:23]

It's moved to InitWithSurfaceInfo in the next patch.

-
+  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 11:55:28]

Why can't this be part of the setup?

[Alex Z., 2017/05/18 14:32:23]

It's using InitWithSurfaceInfo() now. 
I didn't use it because I wanted it to be more explicit.

   // FrameGenerator does not request BeginFrames upon creation.
   IssueBeginFrame();
-  EXPECT_EQ(0, NumberOfFramesReceived());
-  EXPECT_EQ(cc::BeginFrameAck(), LastBeginFrameAck());
+  EXPECT_EQ(cc::BeginFrameAck(), LastBeginFrameAck(binding));
 
   const cc::SurfaceId kArbitrarySurfaceId(
       cc::FrameSinkId(1, 1),
@@ -230,54 +195,38 @@ TEST_F(FrameGeneratorTest, OnSurfaceCreated) {
   const cc::SurfaceInfo kArbitrarySurfaceInfo(kArbitrarySurfaceId, 1.0f,
                                               gfx::Size(100, 100));
   frame_generator()->OnSurfaceCreated(kArbitrarySurfaceInfo);
-  EXPECT_EQ(0, NumberOfFramesReceived());
+  frame_generator()->OnWindowSizeChanged(gfx::Size(1, 2));

[Fady Samuel, 2017/05/18 11:55:27]

Why introduce this?

[Alex Z., 2017/05/18 14:32:23]

Thanks for pointing this out. It was part of the remainder of some of my
experiments with the tests. It has been removed.

 
   IssueBeginFrame();
-  EXPECT_EQ(1, NumberOfFramesReceived());
+  EXPECT_EQ(1, binding->frames_submitted());
 
   // Verify that the CompositorFrame refers to the window manager's surface via
   // referenced_surfaces.
-  const cc::CompositorFrameMetadata& last_metadata = LastMetadata();
+  const cc::CompositorFrameMetadata& last_metadata = binding->last_metadata();
   const std::vector<cc::SurfaceId>& referenced_surfaces =
       last_metadata.referenced_surfaces;
   EXPECT_EQ(1lu, referenced_surfaces.size());
   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 11:55:28]

Maybe store binding in FrameGeneratorTest instead?

[Alex Z., 2017/05/18 14:32:22]

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));
+  InitWithSurfaceInfo();
   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();

[Fady Samuel, 2017/05/18 11:55:28]

Are we losing part of the test?

[Alex Z., 2017/05/18 14:32:23]

This is now in InitWithSurfaceInfo. It's fine since the SurfaceInfo
part is also tested in OnSurfaceCreated test.

-  EXPECT_EQ(1, NumberOfFramesReceived());
-
-  // FrameGenerator stops requesting BeginFrames after receiving one.
-  IssueBeginFrame();
-  EXPECT_EQ(1, NumberOfFramesReceived());

[Fady Samuel, 2017/05/18 11:55:28]

Why delete this?

[Alex Z., 2017/05/18 14:32:22]

The "FrameGenerator stopping requesting BeginFrames after submitting
a CompositorFrame" is tested in OnSurfaceCreated test.

-
   // FrameGenerator does not request BeginFrames if its device scale factor
   // remains unchanged.
   frame_generator()->SetDeviceScaleFactor(kDefaultScaleFactor);
   IssueBeginFrame();
-  EXPECT_EQ(1, NumberOfFramesReceived());

[Fady Samuel, 2017/05/18 11:55:28]

Why delete this?

[Alex Z., 2017/05/18 14:32:22]

Thanks for pointing it out. It was removed when I used gmock. It's 
restored now.

   const cc::CompositorFrameMetadata& last_metadata = LastMetadata();
   EXPECT_EQ(kDefaultScaleFactor, last_metadata.device_scale_factor);
 
@@ -336,7 +285,6 @@ TEST_F(FrameGeneratorTest, WindowBoundsChangedTwice) {
 
   // frame_generator() stops requesting BeginFrames after getting one.
   IssueBeginFrame();
-  EXPECT_EQ(2, NumberOfFramesReceived());

[Fady Samuel, 2017/05/18 11:55:28]

Why delete this?

[Alex Z., 2017/05/18 14:32:23]

It was deleted while converting to use gmock. It's restored.

 }
 
 TEST_F(FrameGeneratorTest, WindowDamaged) {