Moving temporary reference logic to SurfaceManager

cc/surfaces/surface_manager_ref_unittest.cc

 // Copyright 2016 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 <stddef.h>
 
 #include <unordered_map>
 #include <vector>
 
 #include "base/memory/ptr_util.h"
 #include "cc/surfaces/surface.h"
 #include "cc/surfaces/surface_factory.h"
 #include "cc/surfaces/surface_factory_client.h"
 #include "cc/surfaces/surface_id.h"
+#include "cc/surfaces/surface_info.h"

[kylechar, 2017/01/17 16:15:35]

Is this used?

[Saman Sami, 2017/01/17 18:29:05]

Done.

 #include "cc/surfaces/surface_manager.h"
 #include "cc/surfaces/surface_sequence_generator.h"
 #include "testing/gtest/include/gtest/gtest.h"
 namespace cc {
-namespace {
 
 constexpr FrameSinkId kFrameSink1(1, 0);

[kylechar, 2017/01/17 16:15:35]

You should leave constants/helper classes/helper functions in the anonymous
namespace. Only SurfaceManagerRefTest can't be in it, since you make it a friend
of SurfaceManager.

[Saman Sami, 2017/01/17 18:29:05]

Done.

 constexpr FrameSinkId kFrameSink2(2, 0);
 constexpr FrameSinkId kFrameSink3(3, 0);
 const LocalFrameId kLocalFrame1(1, base::UnguessableToken::Create());
 const LocalFrameId kLocalFrame2(2, base::UnguessableToken::Create());
 
 class StubSurfaceFactoryClient : public SurfaceFactoryClient {
  public:
   void ReturnResources(const ReturnedResourceArray& resources) override {}
   void SetBeginFrameSource(BeginFrameSource* begin_frame_source) override {}
 };
 
 // Tests for reference tracking in SurfaceManager.
 class SurfaceManagerRefTest : public testing::Test {
  public:
   SurfaceManager& manager() { return *manager_; }
 
   // Creates a new Surface with the provided SurfaceId. Will first create the
   // SurfaceFactory for |frame_sink_id| if necessary.
   SurfaceId CreateSurface(const FrameSinkId& frame_sink_id,
                           const LocalFrameId& local_frame_id) {
     GetFactory(frame_sink_id)
         .SubmitCompositorFrame(local_frame_id, CompositorFrame(),
                                SurfaceFactory::DrawCallback());
     return SurfaceId(frame_sink_id, local_frame_id);
   }
 
+  SurfaceId CreateSurface(uint32_t client_id,
+                          uint32_t sink_id,
+                          uint32_t local_id) {
+    return CreateSurface(
+        FrameSinkId(client_id, sink_id),
+        LocalFrameId(local_id, base::UnguessableToken::Deserialize(0, 1u)));
+  }
+
   // Destroy Surface with |surface_id|.
   void DestroySurface(const SurfaceId& surface_id) {
     GetFactory(surface_id.frame_sink_id()).EvictSurface();
   }
 
  protected:
   SurfaceFactory& GetFactory(const FrameSinkId& frame_sink_id) {
     auto& factory_ptr = factories_[frame_sink_id];
     if (!factory_ptr)
       factory_ptr = base::MakeUnique<SurfaceFactory>(frame_sink_id,
                                                      manager_.get(), &client_);
     return *factory_ptr;
   }
 
   // testing::Test:
   void SetUp() override {
     // Start each test with a fresh SurfaceManager instance.
-    manager_ = base::MakeUnique<SurfaceManager>();
+    manager_ = base::MakeUnique<SurfaceManager>(
+        SurfaceManager::LifetimeType::REFERENCES);
   }
   void TearDown() override {
     for (auto& factory : factories_)
       factory.second->EvictSurface();
     factories_.clear();
     manager_.reset();
   }
 
+  SurfaceManager::SurfaceIdSet refs(SurfaceId surface_id) {
+    return manager().parent_to_child_refs_[surface_id];
+  }
+
+  std::unordered_map<FrameSinkId, std::vector<LocalFrameId>, FrameSinkIdHash>
+  tmp_refs() {
+    return manager().temp_references_;
+  }
+
   std::unordered_map<FrameSinkId,
                      std::unique_ptr<SurfaceFactory>,
                      FrameSinkIdHash>
       factories_;
   std::unique_ptr<SurfaceManager> manager_;
   StubSurfaceFactoryClient client_;
 };
 
-}  // namespace
-
 TEST_F(SurfaceManagerRefTest, AddReference) {
   SurfaceId id1 = CreateSurface(kFrameSink1, kLocalFrame1);
   manager().AddSurfaceReference(manager().GetRootSurfaceId(), id1);
 
   EXPECT_EQ(manager().GetSurfaceReferenceCount(id1), 1u);
   EXPECT_EQ(manager().GetReferencedSurfaceCount(id1), 0u);
 }
 
 TEST_F(SurfaceManagerRefTest, AddRemoveReference) {
   SurfaceId id1 = CreateSurface(kFrameSink1, kLocalFrame1);
@@ skipping 120 matching lines @@
 
   manager().RemoveSurfaceReference(manager().GetRootSurfaceId(), id1);
 
   // Removing the reference from the root to id1 should allow all three surfaces
   // to be deleted during GC.
   EXPECT_EQ(manager().GetSurfaceForId(id1), nullptr);
   EXPECT_EQ(manager().GetSurfaceForId(id2), nullptr);
   EXPECT_EQ(manager().GetSurfaceForId(id3), nullptr);
 }
 
-TEST_F(SurfaceManagerRefTest, CheckGCWithSequences) {
-  SurfaceId id1 = CreateSurface(kFrameSink1, kLocalFrame1);
-  SurfaceId id2 = CreateSurface(kFrameSink2, kLocalFrame1);
-
-  manager().AddSurfaceReference(manager().GetRootSurfaceId(), id1);
-  manager().AddSurfaceReference(id1, id2);
-
-  SurfaceId id3 = CreateSurface(kFrameSink3, kLocalFrame1);
-  Surface* surface3 = manager().GetSurfaceForId(id3);
-
-  // Add destruction dependency from |id2| to |id3|.
-  manager().RegisterFrameSinkId(kFrameSink2);
-  SurfaceSequence sequence(kFrameSink2, 1u);
-  surface3->AddDestructionDependency(sequence);
-  EXPECT_EQ(surface3->GetDestructionDependencyCount(), 1u);
-
-  // Surface for |id3| isn't delete yet because it has a valid destruction
-  // dependency from |kFrameSink2|.
-  DestroySurface(id3);
-  EXPECT_NE(manager().GetSurfaceForId(id3), nullptr);
-
-  // Surface for |id2| isn't deleted because it has a reference.
-  DestroySurface(id2);
-  EXPECT_NE(manager().GetSurfaceForId(id2), nullptr);
-
-  // Satisfy destruction dependency on |id3| and delete during GC.
-  manager().SatisfySequence(sequence);
-  EXPECT_EQ(manager().GetSurfaceForId(id3), nullptr);
-
-  // Remove ref on |id2| and delete during GC.
-  manager().RemoveSurfaceReference(id1, id2);
-  EXPECT_EQ(manager().GetSurfaceForId(id2), nullptr);
-}
-
 TEST_F(SurfaceManagerRefTest, TryAddReferenceToBadSurface) {
   // Not creating an accompanying Surface and SurfaceFactory.
   SurfaceId id(FrameSinkId(100u, 200u),
                LocalFrameId(1u, base::UnguessableToken::Create()));
 
   // Adding reference from root to the Surface should do nothing because
   // SurfaceManager doesn't know Surface for |id| exists.
   manager().AddSurfaceReference(manager().GetRootSurfaceId(), id);
   EXPECT_EQ(manager().GetSurfaceReferenceCount(id), 0u);
 }
@@ skipping 11 matching lines @@
   manager().AddSurfaceReference(id1, id2);
   EXPECT_EQ(manager().GetSurfaceReferenceCount(id2), 1u);
   EXPECT_EQ(manager().GetReferencedSurfaceCount(id1), 1u);
 }
 
 TEST_F(SurfaceManagerRefTest, TryAddSelfReference) {
   SurfaceId id1 = CreateSurface(kFrameSink1, kLocalFrame1);
 
   // Adding a self reference should be ignored without crashing.
   manager().AddSurfaceReference(id1, id1);
-  EXPECT_EQ(manager().GetSurfaceReferenceCount(id1), 0u);
   EXPECT_EQ(manager().GetReferencedSurfaceCount(id1), 0u);
+  // There should still be one temporary reference to id1
+  EXPECT_EQ(manager().GetSurfaceReferenceCount(id1), 1u);

[kylechar, 2017/01/17 16:15:35]

Can you also check this is true before AddSurfaceReference is called with bad
arguments?

[Saman Sami, 2017/01/17 18:29:05]

Done.

 }
 
 TEST_F(SurfaceManagerRefTest, TryRemoveBadReference) {
   SurfaceId id1 = CreateSurface(kFrameSink1, kLocalFrame1);
   SurfaceId id2 = CreateSurface(kFrameSink2, kLocalFrame1);
 
   // Removing non-existent reference should be ignored.
   manager().AddSurfaceReference(id1, id2);
   manager().RemoveSurfaceReference(id2, id1);
   EXPECT_EQ(manager().GetReferencedSurfaceCount(id1), 1u);
   EXPECT_EQ(manager().GetSurfaceReferenceCount(id2), 1u);
 }
 
+// Temporary Reference Tests
+
+TEST_F(SurfaceManagerRefTest, AddSurfaceThenReference) {
+  const SurfaceId surface_id = CreateSurface(2, 1, 1);
+
+  // Temporary reference must be added.
+  EXPECT_EQ(1u, tmp_refs().size());
+  EXPECT_EQ(std::vector<LocalFrameId>{surface_id.local_frame_id()},

[kylechar, 2017/01/17 16:15:35]

The tests become pretty difficult to read. Changing the helper method names and
using matchers might be helpful. Also improving the comments. Here is a rough
suggestion for something that seems clearer to me but feel free to modify it as
you feel see fit.

const SurfaceId surface_id = CreateSurface(kFrameSink2, kLocalFrame1);

EXPECT_THAT(GetTempReferencesFor(kFrameSink2), ElementsAre(kLocalFrame1));
EXPECT_THAT(GetReferencesFor(manager().GetRootSurfaceId()),
ElementsAre(surface_id));

const SurfaceId parent_id = CreateSurface(kFrameSink1, kLocalFrame1);
manager().AddSurfaceReference(parent_id, surface_id);

...

[Saman Sami, 2017/01/17 18:29:05]

Neat.

+            tmp_refs()[surface_id.frame_sink_id()]);
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).size());
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(surface_id));
+
+  // Add a real reference to surface_id.
+  const SurfaceId parent_id = CreateSurface(1, 1, 1);
+  manager().AddSurfaceReference(parent_id, surface_id);
+
+  // Real reference is added and temporary reference to surface_id is removed.
+  EXPECT_EQ(1u, tmp_refs().size());
+  EXPECT_EQ(std::vector<LocalFrameId>{parent_id.local_frame_id()},
+            tmp_refs()[parent_id.frame_sink_id()]);
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).size());
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(parent_id));
+  EXPECT_EQ(1u, refs(parent_id).size());
+  EXPECT_EQ(1u, refs(parent_id).count(surface_id));
+}
+
+TEST_F(SurfaceManagerRefTest, AddSurfaceThenRootReference) {
+  const SurfaceId surface_id = CreateSurface(1, 1, 1);
+
+  // Temporary reference should be added.
+  EXPECT_EQ(1u, tmp_refs().size());
+  EXPECT_EQ(std::vector<LocalFrameId>{surface_id.local_frame_id()},
+            tmp_refs()[surface_id.frame_sink_id()]);
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).size());
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(surface_id));
+
+  // Add a real reference from root to surface_id.
+  manager().AddSurfaceReference(manager().GetRootSurfaceId(), surface_id);
+
+  // Adding real reference doesn't need to change anything in
+  // SurfaceReferenceManager does remove the temporary reference marker.
+  EXPECT_EQ(0u, tmp_refs().size());
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).size());
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(surface_id));
+}
+
+TEST_F(SurfaceManagerRefTest, AddTwoSurfacesThenOneReference) {
+  // Add two surfaces with different FrameSinkIds.
+  const SurfaceId surface_id1 = CreateSurface(2, 1, 1);
+  const SurfaceId surface_id2 = CreateSurface(3, 1, 1);
+
+  // Temporary reference should be added for both surfaces.
+  EXPECT_EQ(2u, tmp_refs().size());
+  EXPECT_EQ(std::vector<LocalFrameId>{surface_id1.local_frame_id()},
+            tmp_refs()[surface_id1.frame_sink_id()]);
+  EXPECT_EQ(std::vector<LocalFrameId>{surface_id2.local_frame_id()},
+            tmp_refs()[surface_id2.frame_sink_id()]);
+  EXPECT_EQ(2u, refs(manager().GetRootSurfaceId()).size());
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(surface_id1));
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(surface_id2));
+
+  // Add a real reference to surface_id1.
+  const SurfaceId parent_id = CreateSurface(1, 1, 1);
+  manager().AddSurfaceReference(parent_id, surface_id1);
+
+  // Real reference is added then temporary reference removed for 2:1:1. There
+  // should still be a temporary reference left to 3:1:1
+  EXPECT_EQ(2u, tmp_refs().size());
+  EXPECT_EQ(std::vector<LocalFrameId>{surface_id2.local_frame_id()},
+            tmp_refs()[surface_id2.frame_sink_id()]);
+  EXPECT_EQ(std::vector<LocalFrameId>{parent_id.local_frame_id()},
+            tmp_refs()[parent_id.frame_sink_id()]);
+  EXPECT_EQ(2u, refs(manager().GetRootSurfaceId()).size());
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(parent_id));
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(surface_id2));
+  EXPECT_EQ(1u, refs(parent_id).size());
+  EXPECT_EQ(1u, refs(parent_id).count(surface_id1));
+}
+
+TEST_F(SurfaceManagerRefTest, AddSurfacesSkipReference) {
+  // Add two surfaces that have the same FrameSinkId. This would happen when a
+  // client submits two CFs before parent submits a new CF.
+  const SurfaceId surface_id1 = CreateSurface(2, 1, 1);
+  const SurfaceId surface_id2 = CreateSurface(2, 1, 2);
+
+  // Temporary references should be added for both surfaces.
+  std::vector<LocalFrameId> expected_ids = {surface_id1.local_frame_id(),
+                                            surface_id2.local_frame_id()};
+  EXPECT_EQ(1u, tmp_refs().size());
+  EXPECT_EQ(expected_ids, tmp_refs()[surface_id1.frame_sink_id()]);
+  EXPECT_EQ(2u, refs(manager().GetRootSurfaceId()).size());
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(surface_id1));
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(surface_id2));
+
+  // Add a reference to the surface with the later LocalFrameId.
+  const SurfaceId parent_id = CreateSurface(1, 1, 1);
+  manager().AddSurfaceReference(parent_id, surface_id2);
+
+  // The real reference should be added for 2:1:2 and both temporary references
+  // should be removed.
+  EXPECT_EQ(1u, tmp_refs().size());
+  EXPECT_EQ(std::vector<LocalFrameId>{parent_id.local_frame_id()},
+            tmp_refs()[parent_id.frame_sink_id()]);
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).size());
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(parent_id));
+  EXPECT_EQ(1u, refs(parent_id).size());
+  EXPECT_EQ(1u, refs(parent_id).count(surface_id2));
+}
+
+TEST_F(SurfaceManagerRefTest, RemoveFirstTempRefOnly) {
+  // Add two surfaces that have the same FrameSinkId. This would happen when a
+  // client submits two CFs before parent submits a new CF.
+  const SurfaceId surface_id1 = CreateSurface(2, 1, 1);
+  const SurfaceId surface_id2 = CreateSurface(2, 1, 2);
+
+  // Temporary references should be added for both surfaces.
+  std::vector<LocalFrameId> expected_ids = {surface_id1.local_frame_id(),
+                                            surface_id2.local_frame_id()};
+  EXPECT_EQ(1u, tmp_refs().size());
+  EXPECT_EQ(expected_ids, tmp_refs()[surface_id1.frame_sink_id()]);
+  EXPECT_EQ(2u, refs(manager().GetRootSurfaceId()).size());
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(surface_id1));
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(surface_id2));
+
+  // Add a reference to the surface with the earlier LocalFrameId.
+  const SurfaceId parent_id = CreateSurface(1, 1, 1);
+  manager().AddSurfaceReference(parent_id, surface_id1);
+
+  // The real reference should be added for 2:1:1 and temporary reference
+  // should be removed. The temporary reference for 2:1:2 should remain.
+  EXPECT_EQ(2u, tmp_refs().size());
+  EXPECT_EQ(std::vector<LocalFrameId>{surface_id2.local_frame_id()},
+            tmp_refs()[surface_id2.frame_sink_id()]);
+  EXPECT_EQ(std::vector<LocalFrameId>{parent_id.local_frame_id()},
+            tmp_refs()[parent_id.frame_sink_id()]);
+  EXPECT_EQ(2u, refs(manager().GetRootSurfaceId()).size());
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(surface_id2));
+  EXPECT_EQ(1u, refs(manager().GetRootSurfaceId()).count(parent_id));
+  EXPECT_EQ(1u, refs(parent_id).size());
+  EXPECT_EQ(1u, refs(parent_id).count(surface_id1));
+}
+
 }  // namespace cc