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..4c9f29c39aa05f58f7f518ba0ff88a8b51b38b2a 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"
 #include "base/macros.h"
 #include "cc/output/compositor_frame_sink.h"
 #include "cc/scheduler/begin_frame_source.h"
@@ -38,38 +39,38 @@ 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;
-  }
-
-  void DetachFromClient() override {
-    cc::CompositorFrameSink::DetachFromClient();
-  }
+  explicit TestClientBinding(
+      cc::mojom::MojoCompositorFrameSinkClient* sink_client)
+      : sink_client_(sink_client) {}
+  ~TestClientBinding() override = default;
 
+  // CompositorFrameSinkClientBinding implementation:
   void SubmitCompositorFrame(cc::CompositorFrame frame) override {
-    ++number_frames_received_;
+    ++frames_submitted_;
     last_frame_ = std::move(frame);
+    begin_frame_source_->DidFinishFrame(this,
+                                        last_frame_.metadata.begin_frame_ack);
+  }
+
+  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);
   }
 
   // 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 +80,59 @@ 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() {
+  void InitWithSurfaceInfo(std::unique_ptr<TestClientBinding> client_binding) {
     // FrameGenerator requires a valid SurfaceInfo before generating
     // CompositorFrames.
+    TestClientBinding* 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 +141,33 @@ class FrameGeneratorTest : public testing::Test {
     ++next_sequence_number_;
   }
 
-  FrameGenerator* frame_generator() { return frame_generator_.get(); }
-
-  const cc::CompositorFrameMetadata& LastMetadata() const {
-    return compositor_frame_sink_->last_metadata();
+  const cc::BeginFrameAck& LastBeginFrameAck(
+      TestClientBinding* test_client_binding) {
+    return begin_frame_source_->LastAckForObserver(test_client_binding);
   }
 
-  const cc::RenderPassList& LastRenderPassList() const {
-    return compositor_frame_sink_->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();
   }
 
  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_;
   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
-  // 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());
-
+  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));
   // 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,89 +175,84 @@ 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));
 
   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));
   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));
+  std::unique_ptr<TestClientBinding> test_client_binding =
+      base::MakeUnique<TestClientBinding>(frame_generator());
+  TestClientBinding* binding = test_client_binding.get();
+  InitWithSurfaceInfo(std::move(test_client_binding));
   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());
-
   // FrameGenerator does not request BeginFrames if its device scale factor
   // remains unchanged.
   frame_generator()->SetDeviceScaleFactor(kDefaultScaleFactor);
   IssueBeginFrame();
-  EXPECT_EQ(1, NumberOfFramesReceived());
-  const cc::CompositorFrameMetadata& last_metadata = LastMetadata();
+  const cc::CompositorFrameMetadata& last_metadata = binding->last_metadata();
   EXPECT_EQ(kDefaultScaleFactor, last_metadata.device_scale_factor);
 
   frame_generator()->SetDeviceScaleFactor(kArbitraryScaleFactor);
   IssueBeginFrame();
-  EXPECT_EQ(2, NumberOfFramesReceived());
-  const cc::CompositorFrameMetadata& second_last_metadata = LastMetadata();
+  EXPECT_EQ(2, binding->frames_submitted());
+  const cc::CompositorFrameMetadata& second_last_metadata =
+      binding->last_metadata();
   EXPECT_EQ(kArbitraryScaleFactor, second_last_metadata.device_scale_factor);
 }
 
 TEST_F(FrameGeneratorTest, SetHighContrastMode) {
-  InitWithSurfaceInfo();
+  std::unique_ptr<TestClientBinding> test_client_binding =
+      base::MakeUnique<TestClientBinding>(frame_generator());
+  TestClientBinding* binding = test_client_binding.get();
+  InitWithSurfaceInfo(std::move(test_client_binding));
 
   // Changing high contrast mode should trigger a BeginFrame.
   frame_generator()->SetHighContrastMode(true);
   IssueBeginFrame();
-  EXPECT_EQ(2, NumberOfFramesReceived());
+  EXPECT_EQ(2, binding->frames_submitted());
 
   // Verify that the last frame has an invert filter.
-  const cc::RenderPassList& render_pass_list = LastRenderPassList();
+  const cc::RenderPassList& render_pass_list = binding->last_render_pass_list();
   const cc::FilterOperations expected_filters(
       {cc::FilterOperation::CreateInvertFilter(1.f)});
   EXPECT_EQ(expected_filters, render_pass_list.front()->filters);
 }
 
 TEST_F(FrameGeneratorTest, WindowBoundsChanged) {
-  InitWithSurfaceInfo();
+  std::unique_ptr<TestClientBinding> test_client_binding =
+      base::MakeUnique<TestClientBinding>(frame_generator());
+  TestClientBinding* binding = test_client_binding.get();
+  InitWithSurfaceInfo(std::move(test_client_binding));
 
   // 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());
-  cc::RenderPass* received_render_pass = LastRenderPassList().front().get();
+  EXPECT_EQ(2, binding->frames_submitted());
+  cc::RenderPass* received_render_pass =
+      binding->last_render_pass_list().front().get();
   EXPECT_EQ(expected_render_pass_id, received_render_pass->id);
   EXPECT_EQ(kArbitrarySize, received_render_pass->output_rect.size());
   EXPECT_EQ(kArbitrarySize, received_render_pass->damage_rect.size());
@@ -322,29 +262,35 @@ TEST_F(FrameGeneratorTest, WindowBoundsChanged) {
 // Change window bounds twice before issuing a BeginFrame. The CompositorFrame
 // submitted by frame_generator() should only has the second bounds.
 TEST_F(FrameGeneratorTest, WindowBoundsChangedTwice) {
-  InitWithSurfaceInfo();
+  std::unique_ptr<TestClientBinding> test_client_binding =
+      base::MakeUnique<TestClientBinding>(frame_generator());
+  TestClientBinding* binding = test_client_binding.get();
+  InitWithSurfaceInfo(std::move(test_client_binding));
 
   const gfx::Size kArbitrarySize(3, 4);
   const gfx::Size kAnotherArbitrarySize(5, 6);
   frame_generator()->OnWindowSizeChanged(kArbitrarySize);
   frame_generator()->OnWindowSizeChanged(kAnotherArbitrarySize);
   IssueBeginFrame();
-  EXPECT_EQ(2, NumberOfFramesReceived());
-  cc::RenderPass* received_render_pass = LastRenderPassList().front().get();
+  EXPECT_EQ(2, binding->frames_submitted());
+  cc::RenderPass* received_render_pass =
+      binding->last_render_pass_list().front().get();
   EXPECT_EQ(kAnotherArbitrarySize, received_render_pass->output_rect.size());
   EXPECT_EQ(kAnotherArbitrarySize, received_render_pass->damage_rect.size());
 
   // frame_generator() stops requesting BeginFrames after getting one.
   IssueBeginFrame();
-  EXPECT_EQ(2, NumberOfFramesReceived());
 }
 
 TEST_F(FrameGeneratorTest, WindowDamaged) {
-  InitWithSurfaceInfo();
+  std::unique_ptr<TestClientBinding> test_client_binding =
+      base::MakeUnique<TestClientBinding>(frame_generator());
+  TestClientBinding* binding = test_client_binding.get();

[Fady Samuel, 2017/05/17 20:33:04]

This pattern seems to be used over and over again... Can we simply move this to
InitWithSurfaceInfo?

[Alex Z., 2017/05/17 21:20:25]

Done.

+  InitWithSurfaceInfo(std::move(test_client_binding));
 
   frame_generator()->OnWindowDamaged();
   IssueBeginFrame();
-  EXPECT_EQ(2, NumberOfFramesReceived());

[Fady Samuel, 2017/05/17 20:33:04]

If you restore this method then you don't need this change?

[Alex Z., 2017/05/17 21:20:25]

Done.

+  EXPECT_EQ(2, binding->frames_submitted());
 }
 
 }  // namespace test