CompositorFrameSinkSupport should return resources before sending an ack

cc/surfaces/compositor_frame_sink_support_unittest.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/surfaces/compositor_frame_sink_support.h"
 
+#include "base/debug/stack_trace.h"
 #include "base/macros.h"
 #include "cc/output/compositor_frame.h"
 #include "cc/surfaces/compositor_frame_sink_support_client.h"
 #include "cc/surfaces/frame_sink_id.h"
 #include "cc/surfaces/surface_id.h"
 #include "cc/surfaces/surface_manager.h"
 #include "cc/test/begin_frame_args_test.h"
 #include "cc/test/fake_external_begin_frame_source.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::UnorderedElementsAre;
 using testing::IsEmpty;
 using testing::SizeIs;
+using testing::Invoke;
+using testing::_;
+using testing::InSequence;
 
 namespace cc {
 namespace test {
 namespace {
 
 constexpr FrameSinkId kDisplayFrameSink(2, 0);
 constexpr FrameSinkId kParentFrameSink(3, 0);
 constexpr FrameSinkId kChildFrameSink1(65563, 0);
 constexpr FrameSinkId kChildFrameSink2(65564, 0);
 constexpr FrameSinkId kArbitraryFrameSink(1337, 7331);
 
+class MockCompositorFrameSinkSupportClient
+    : public CompositorFrameSinkSupportClient {
+ public:
+  MockCompositorFrameSinkSupportClient() {
+    ON_CALL(*this, ReclaimResources(_))
+        .WillByDefault(Invoke(
+            this,
+            &MockCompositorFrameSinkSupportClient::ReclaimResourcesInternal));
+  }
+
+  ReturnedResourceArray& last_returned_resources() {
+    return last_returned_resources_;
+  }
+
+  // CompositorFrameSinkSupportClient implementation.
+  MOCK_METHOD0(DidReceiveCompositorFrameAck, void());
+  MOCK_METHOD1(OnBeginFrame, void(const BeginFrameArgs&));
+  MOCK_METHOD1(ReclaimResources, void(const ReturnedResourceArray&));
+  MOCK_METHOD2(WillDrawSurface, void(const LocalSurfaceId&, const gfx::Rect&));
+
+ private:
+  void ReclaimResourcesInternal(const ReturnedResourceArray& resources) {
+    last_returned_resources_ = resources;
+  }
+
+  ReturnedResourceArray last_returned_resources_;
+};
+
 std::vector<SurfaceId> empty_surface_ids() {
   return std::vector<SurfaceId>();
 }
 
 SurfaceId MakeSurfaceId(const FrameSinkId& frame_sink_id, uint32_t local_id) {
   return SurfaceId(
       frame_sink_id,
       LocalSurfaceId(local_id, base::UnguessableToken::Deserialize(0, 1u)));
 }
 
@@ skipping 22 matching lines @@
   TransferableResource resource;
   resource.id = id;
   resource.format = format;
   resource.filter = filter;
   resource.size = size;
   return resource;
 }
 
 }  // namespace
 
-class CompositorFrameSinkSupportTest : public testing::Test,
-                                       public CompositorFrameSinkSupportClient {
+class CompositorFrameSinkSupportTest : public testing::Test {
  public:
   CompositorFrameSinkSupportTest()
       : surface_manager_(SurfaceManager::LifetimeType::REFERENCES) {}
   ~CompositorFrameSinkSupportTest() override {}
 
   CompositorFrameSinkSupport& display_support() { return *supports_[0]; }
 
   CompositorFrameSinkSupport& parent_support() { return *supports_[1]; }
   Surface* parent_surface() {
     return parent_support().current_surface_for_testing();
@@ skipping 31 matching lines @@
   }
 
   SurfaceDependencyTracker& dependency_tracker() {
     return *surface_manager_.dependency_tracker();
   }
 
   FakeExternalBeginFrameSource* begin_frame_source() {
     return begin_frame_source_.get();
   }
 
-  ReturnedResourceArray& last_returned_resources() {
-    return last_returned_resources_;
-  }
-
   // testing::Test:
   void SetUp() override {
     testing::Test::SetUp();
     begin_frame_source_ =
         base::MakeUnique<FakeExternalBeginFrameSource>(0.f, false);
     std::unique_ptr<SurfaceDependencyTracker> dependency_tracker(
         new SurfaceDependencyTracker(&surface_manager_,
                                      begin_frame_source_.get()));
     surface_manager_.SetDependencyTracker(std::move(dependency_tracker));
     supports_.push_back(base::MakeUnique<CompositorFrameSinkSupport>(
-        this, &surface_manager_, kDisplayFrameSink, true /* is_root */,
-        true /* handles_frame_sink_id_invalidation */,
+        &support_client_, &surface_manager_, kDisplayFrameSink,
+        true /* is_root */, true /* handles_frame_sink_id_invalidation */,
         true /* needs_sync_points */));
     supports_.push_back(base::MakeUnique<CompositorFrameSinkSupport>(
-        this, &surface_manager_, kParentFrameSink, false /* is_root */,
-        true /* handles_frame_sink_id_invalidation */,
+        &support_client_, &surface_manager_, kParentFrameSink,
+        false /* is_root */, true /* handles_frame_sink_id_invalidation */,
         true /* needs_sync_points */));
     supports_.push_back(base::MakeUnique<CompositorFrameSinkSupport>(
-        this, &surface_manager_, kChildFrameSink1, false /* is_root */,
-        true /* handles_frame_sink_id_invalidation */,
+        &support_client_, &surface_manager_, kChildFrameSink1,
+        false /* is_root */, true /* handles_frame_sink_id_invalidation */,
         true /* needs_sync_points */));
     supports_.push_back(base::MakeUnique<CompositorFrameSinkSupport>(
-        this, &surface_manager_, kChildFrameSink2, false /* is_root */,
-        true /* handles_frame_sink_id_invalidation */,
+        &support_client_, &surface_manager_, kChildFrameSink2,
+        false /* is_root */, true /* handles_frame_sink_id_invalidation */,
         true /* needs_sync_points */));
 
     // Normally, the BeginFrameSource would be registered by the Display. We
     // register it here so that BeginFrames are received by the display support,
     // for use in the PassesOnBeginFrameAcks test. Other supports do not receive
     // BeginFrames, since the frame sink hierarchy is not set up in this test.
     surface_manager_.RegisterBeginFrameSource(begin_frame_source_.get(),
                                               kDisplayFrameSink);
   }
 
   void TearDown() override {
     surface_manager_.SetDependencyTracker(nullptr);
     surface_manager_.UnregisterBeginFrameSource(begin_frame_source_.get());
 
     // SurfaceDependencyTracker depends on this BeginFrameSource and so it must
     // be destroyed AFTER the dependency tracker is destroyed.
     begin_frame_source_.reset();
 
     supports_.clear();
   }
 
-  // CompositorFrameSinkSupportClient implementation.
-  void DidReceiveCompositorFrameAck() override {}
-  void OnBeginFrame(const BeginFrameArgs& args) override {}
-  void ReclaimResources(const ReturnedResourceArray& resources) override {
-    last_returned_resources_ = resources;
-  }
-  void WillDrawSurface(const LocalSurfaceId& local_surface_id,
-                       const gfx::Rect& damage_rect) override {}
+ protected:
+  testing::NiceMock<MockCompositorFrameSinkSupportClient> support_client_;
 
  private:
   SurfaceManager surface_manager_;
   std::unique_ptr<FakeExternalBeginFrameSource> begin_frame_source_;
   std::vector<std::unique_ptr<CompositorFrameSinkSupport>> supports_;
-  ReturnedResourceArray last_returned_resources_;
 
   DISALLOW_COPY_AND_ASSIGN(CompositorFrameSinkSupportTest);
 };
 
 // The display root surface should have a surface reference from the top-level
 // root added/removed when a CompositorFrame is submitted with a new SurfaceId.
 TEST_F(CompositorFrameSinkSupportTest, RootSurfaceReceivesReferences) {
   const SurfaceId display_id_first = MakeSurfaceId(kDisplayFrameSink, 1);
   const SurfaceId display_id_second = MakeSurfaceId(kDisplayFrameSink, 2);
 
@@ skipping 375 matching lines @@
 
   // The parent submits a CompositorFrame without any dependencies. That frame
   // should activate immediately, replacing the earlier frame. The resource from
   // the earlier frame should be returned to the client.
   parent_support().SubmitCompositorFrame(
       parent_id.local_surface_id(), MakeCompositorFrame({empty_surface_ids()}));
   EXPECT_TRUE(parent_surface()->HasActiveFrame());
   EXPECT_FALSE(parent_surface()->HasPendingFrame());
   EXPECT_THAT(parent_surface()->blocking_surfaces_for_testing(), IsEmpty());
   ReturnedResource returned_resource = resource.ToReturnedResource();
-  EXPECT_THAT(last_returned_resources(),
+  EXPECT_THAT(support_client_.last_returned_resources(),
               UnorderedElementsAre(returned_resource));
 }
 
 // This test verifies that a SurfaceReference from parent to child can be added
 // prior to the child submitting a CompositorFrame. This test also verifies that
 // when the child later submits a CompositorFrame,
 TEST_F(CompositorFrameSinkSupportTest,
        DisplayCompositorLockingReferenceAddedBeforeChildExists) {
   const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
   const SurfaceId child_id = MakeSurfaceId(kChildFrameSink1, 1);
@@ skipping 344 matching lines @@
   BeginFrameArgs args =
       CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0, 1);
   begin_frame_source()->TestOnBeginFrame(args);
 
   // Check that the support forwards our ack to the BeginFrameSource.
   BeginFrameAck ack(0, 1, 1, 0, false);
   display_support().DidFinishFrame(ack);
   EXPECT_EQ(ack, begin_frame_source()->LastAckForObserver(&display_support()));
 }
 
+// Checks whether the resources are returned before we send an ack.
+TEST_F(CompositorFrameSinkSupportTest, ReturnResourcesBeforeAck) {
+  const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
+  CompositorFrame frame;
+  TransferableResource resource;
+  resource.id = 1234;
+  frame.resource_list.push_back(resource);

[Fady Samuel, 2017/03/08 23:20:21]

You could use MakeCompositorFrameWithResources?

+  parent_support().SubmitCompositorFrame(parent_id.local_surface_id(),
+                                         std::move(frame));
+  {
+    InSequence x;
+    EXPECT_CALL(support_client_, ReclaimResources(_));
+    EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck());
+  }
+  parent_support().SubmitCompositorFrame(parent_id.local_surface_id(),
+                                         CompositorFrame());
+}
+
 }  // namespace test
 }  // namespace cc