Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(904)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: cc/surfaces/surface_manager_ref_unittest.cc

Issue 2715973003: Refactor how temporary references are stored. (Closed)
Patch Set: Rebase. Created 3 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « cc/surfaces/surface_manager.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: cc/surfaces/surface_manager_ref_unittest.cc
diff --git a/cc/surfaces/surface_manager_ref_unittest.cc b/cc/surfaces/surface_manager_ref_unittest.cc
index 5f68c165f78294eafbd8aab5575e3a81ec3855e2..dabebaa3ecd7b2791019529a981af0cd43d72933 100644
--- a/cc/surfaces/surface_manager_ref_unittest.cc
+++ b/cc/surfaces/surface_manager_ref_unittest.cc
@@ -71,6 +71,11 @@ class SurfaceManagerRefTest : public testing::Test {
manager_->RemoveSurfaceReferences({SurfaceReference(parent_id, child_id)});
}
+ void AddSurfaceReference(const SurfaceId& parent_id,
+ const SurfaceId& child_id) {
+ manager_->AddSurfaceReferences({SurfaceReference(parent_id, child_id)});
+ }
+
// Returns all the references where |surface_id| is the parent.
const SurfaceManager::SurfaceIdSet& GetReferencesFrom(
const SurfaceId& surface_id) {
@@ -83,24 +88,12 @@ class SurfaceManagerRefTest : public testing::Test {
return manager().child_to_parent_refs_[surface_id];
}
- const SurfaceManager::SurfaceIdSet& GetReferencesFromRoot() {
- return GetReferencesFrom(manager().GetRootSurfaceId());
- }
-
- // Returns all the temporary references for the given frame sink id.
- std::vector<LocalSurfaceId> GetTempReferencesFor(
- const FrameSinkId& frame_sink_id) {
- return manager().temp_references_[frame_sink_id];
- }
-
// Temporary references are stored as a map in SurfaceManager. This method
// converts the map to a vector.
std::vector<SurfaceId> GetAllTempReferences() {
std::vector<SurfaceId> temp_references;
- for (auto& map_entry : manager().temp_references_) {
- for (auto local_surface_id : map_entry.second)
- temp_references.push_back(SurfaceId(map_entry.first, local_surface_id));
- }
+ for (auto& map_entry : manager().temporary_references_)
+ temp_references.push_back(map_entry.first);
return temp_references;
}
@@ -128,7 +121,7 @@ class SurfaceManagerRefTest : public testing::Test {
TEST_F(SurfaceManagerRefTest, AddReference) {
SurfaceId id1 = CreateSurface(kFrameSink1, 1);
- manager().AddSurfaceReference(manager().GetRootSurfaceId(), id1);
+ AddSurfaceReference(manager().GetRootSurfaceId(), id1);
EXPECT_THAT(GetReferencesFor(id1),
UnorderedElementsAre(manager().GetRootSurfaceId()));
@@ -138,8 +131,8 @@ TEST_F(SurfaceManagerRefTest, AddReference) {
TEST_F(SurfaceManagerRefTest, AddRemoveReference) {
SurfaceId id1 = CreateSurface(kFrameSink1, 1);
SurfaceId id2 = CreateSurface(kFrameSink2, 1);
- manager().AddSurfaceReference(manager().GetRootSurfaceId(), id1);
- manager().AddSurfaceReference(id1, id2);
+ AddSurfaceReference(manager().GetRootSurfaceId(), id1);
+ AddSurfaceReference(id1, id2);
EXPECT_THAT(GetReferencesFor(id1),
UnorderedElementsAre(manager().GetRootSurfaceId()));
@@ -159,9 +152,9 @@ TEST_F(SurfaceManagerRefTest, NewSurfaceFromFrameSink) {
SurfaceId id2 = CreateSurface(kFrameSink2, 1);
SurfaceId id3 = CreateSurface(kFrameSink3, 1);
- manager().AddSurfaceReference(manager().GetRootSurfaceId(), id1);
- manager().AddSurfaceReference(id1, id2);
- manager().AddSurfaceReference(id2, id3);
+ AddSurfaceReference(manager().GetRootSurfaceId(), id1);
+ AddSurfaceReference(id1, id2);
+ AddSurfaceReference(id2, id3);
// |kFramesink2| received a CompositorFrame with a new size, so it destroys
// |id2| and creates |id2_next|. No reference have been removed yet.
@@ -170,8 +163,8 @@ TEST_F(SurfaceManagerRefTest, NewSurfaceFromFrameSink) {
EXPECT_NE(nullptr, manager().GetSurfaceForId(id2_next));
// Add references to and from |id2_next|.
- manager().AddSurfaceReference(id1, id2_next);
- manager().AddSurfaceReference(id2_next, id3);
+ AddSurfaceReference(id1, id2_next);
+ AddSurfaceReference(id2_next, id3);
EXPECT_THAT(GetReferencesFor(id2), UnorderedElementsAre(id1));
EXPECT_THAT(GetReferencesFor(id2_next), UnorderedElementsAre(id1));
EXPECT_THAT(GetReferencesFor(id3), UnorderedElementsAre(id2, id2_next));
@@ -192,12 +185,12 @@ TEST_F(SurfaceManagerRefTest, ReferenceCycleGetsDeleted) {
SurfaceId id2 = CreateSurface(kFrameSink2, 1);
SurfaceId id3 = CreateSurface(kFrameSink3, 1);
- manager().AddSurfaceReference(manager().GetRootSurfaceId(), id1);
- manager().AddSurfaceReference(id1, id2);
- manager().AddSurfaceReference(id2, id3);
+ AddSurfaceReference(manager().GetRootSurfaceId(), id1);
+ AddSurfaceReference(id1, id2);
+ AddSurfaceReference(id2, id3);
// This reference forms a cycle between id2 and id3.
- manager().AddSurfaceReference(id3, id2);
+ AddSurfaceReference(id3, id2);
DestroySurface(id3);
DestroySurface(id2);
@@ -212,12 +205,12 @@ TEST_F(SurfaceManagerRefTest, ReferenceCycleGetsDeleted) {
EXPECT_EQ(nullptr, manager().GetSurfaceForId(id3));
}
-TEST_F(SurfaceManagerRefTest, CheckGC) {
+TEST_F(SurfaceManagerRefTest, SurfacesAreDeletedDuringGarbageCollection) {
SurfaceId id1 = CreateSurface(kFrameSink1, 1);
SurfaceId id2 = CreateSurface(kFrameSink2, 1);
- manager().AddSurfaceReference(manager().GetRootSurfaceId(), id1);
- manager().AddSurfaceReference(id1, id2);
+ AddSurfaceReference(manager().GetRootSurfaceId(), id1);
+ AddSurfaceReference(id1, id2);
EXPECT_NE(nullptr, manager().GetSurfaceForId(id1));
EXPECT_NE(nullptr, manager().GetSurfaceForId(id2));
@@ -238,14 +231,14 @@ TEST_F(SurfaceManagerRefTest, CheckGC) {
EXPECT_EQ(nullptr, manager().GetSurfaceForId(id1));
}
-TEST_F(SurfaceManagerRefTest, CheckGCRecusiveFull) {
+TEST_F(SurfaceManagerRefTest, GarbageCollectionWorksRecusively) {
SurfaceId id1 = CreateSurface(kFrameSink1, 1);
SurfaceId id2 = CreateSurface(kFrameSink2, 1);
SurfaceId id3 = CreateSurface(kFrameSink3, 1);
- manager().AddSurfaceReference(manager().GetRootSurfaceId(), id1);
- manager().AddSurfaceReference(id1, id2);
- manager().AddSurfaceReference(id2, id3);
+ AddSurfaceReference(manager().GetRootSurfaceId(), id1);
+ AddSurfaceReference(id1, id2);
+ AddSurfaceReference(id2, id3);
DestroySurface(id3);
DestroySurface(id2);
@@ -266,43 +259,45 @@ TEST_F(SurfaceManagerRefTest, CheckGCRecusiveFull) {
EXPECT_EQ(nullptr, manager().GetSurfaceForId(id3));
}
-TEST_F(SurfaceManagerRefTest, TryDoubleAddReference) {
+TEST_F(SurfaceManagerRefTest, TryAddReferenceSameReferenceTwice) {
SurfaceId id1 = CreateSurface(kFrameSink1, 1);
SurfaceId id2 = CreateSurface(kFrameSink2, 1);
- manager().AddSurfaceReference(manager().GetRootSurfaceId(), id1);
- manager().AddSurfaceReference(id1, id2);
+ AddSurfaceReference(manager().GetRootSurfaceId(), id1);
+ AddSurfaceReference(id1, id2);
EXPECT_THAT(GetReferencesFor(id2), SizeIs(1));
EXPECT_THAT(GetReferencesFrom(id1), SizeIs(1));
// The second request should be ignored without crashing.
- manager().AddSurfaceReference(id1, id2);
+ AddSurfaceReference(id1, id2);
EXPECT_THAT(GetReferencesFor(id2), SizeIs(1));
EXPECT_THAT(GetReferencesFrom(id1), SizeIs(1));
}
-TEST_F(SurfaceManagerRefTest, TryAddSelfReference) {
- SurfaceId id1 = CreateSurface(kFrameSink1, 1);
+TEST_F(SurfaceManagerRefTest, AddingSelfReferenceFails) {
+ SurfaceId id1 = CreateSurface(kFrameSink2, 1);
// A temporary reference must exist to |id1|.
- EXPECT_THAT(GetReferencesFor(id1), SizeIs(1));
+ EXPECT_THAT(GetAllTempReferences(), ElementsAre(id1));
+ EXPECT_THAT(GetReferencesFrom(id1), IsEmpty());
+ EXPECT_THAT(GetReferencesFor(id1), IsEmpty());
// Try to add a self reference. This should fail.
- manager().AddSurfaceReference(id1, id1);
+ AddSurfaceReference(id1, id1);
- // Adding a self reference should be ignored without crashing.
+ // Adding a self reference should be ignored without crashing. The temporary
+ // reference to |id1| must still exist.
+ EXPECT_THAT(GetAllTempReferences(), ElementsAre(id1));
EXPECT_THAT(GetReferencesFrom(id1), IsEmpty());
-
- // The temporary reference to |id1| must still exist.
- EXPECT_THAT(GetReferencesFor(id1), SizeIs(1));
+ EXPECT_THAT(GetReferencesFor(id1), IsEmpty());
}
-TEST_F(SurfaceManagerRefTest, TryRemoveBadReference) {
+TEST_F(SurfaceManagerRefTest, RemovingNonexistantReferenceFails) {
SurfaceId id1 = CreateSurface(kFrameSink1, 1);
SurfaceId id2 = CreateSurface(kFrameSink2, 1);
// Removing non-existent reference should be ignored.
- manager().AddSurfaceReference(id1, id2);
+ AddSurfaceReference(id1, id2);
RemoveSurfaceReference(id2, id1);
EXPECT_THAT(GetReferencesFrom(id1), SizeIs(1));
EXPECT_THAT(GetReferencesFor(id2), SizeIs(1));
@@ -314,17 +309,15 @@ TEST_F(SurfaceManagerRefTest, AddSurfaceThenReference) {
// A temporary reference must be added to |surface_id|.
EXPECT_THAT(GetAllTempReferences(), ElementsAre(surface_id));
- EXPECT_THAT(GetReferencesFromRoot(), ElementsAre(surface_id));
// Create |parent_id| and add a real reference from it to |surface_id|.
const SurfaceId parent_id = CreateSurface(kFrameSink1, 1);
- manager().AddSurfaceReference(parent_id, surface_id);
+ AddSurfaceReference(parent_id, surface_id);
// The temporary reference to |surface_id| should be gone.
// The only temporary reference should be to |parent_id|.
// There must be a real reference from |parent_id| to |child_id|.
EXPECT_THAT(GetAllTempReferences(), ElementsAre(parent_id));
- EXPECT_THAT(GetReferencesFromRoot(), ElementsAre(parent_id));
EXPECT_THAT(GetReferencesFrom(parent_id), ElementsAre(surface_id));
}
@@ -334,15 +327,15 @@ TEST_F(SurfaceManagerRefTest, AddSurfaceThenRootReference) {
// Temporary reference should be added to |surface_id|.
EXPECT_THAT(GetAllTempReferences(), ElementsAre(surface_id));
- EXPECT_THAT(GetReferencesFromRoot(), ElementsAre(surface_id));
// Add a real reference from root to |surface_id|.
- manager().AddSurfaceReference(manager().GetRootSurfaceId(), surface_id);
+ AddSurfaceReference(manager().GetRootSurfaceId(), surface_id);
- // The temporary reference should be gone.
- // There should now be a real reference from root to |surface_id|.
+ // The temporary reference should be gone and there should now be a surface
+ // reference from root to |surface_id|.
EXPECT_TRUE(GetAllTempReferences().empty());
- EXPECT_THAT(GetReferencesFromRoot(), ElementsAre(surface_id));
+ EXPECT_THAT(GetReferencesFrom(manager().GetRootSurfaceId()),
+ ElementsAre(surface_id));
}
TEST_F(SurfaceManagerRefTest, AddTwoSurfacesThenOneReference) {
@@ -353,12 +346,10 @@ TEST_F(SurfaceManagerRefTest, AddTwoSurfacesThenOneReference) {
// Temporary reference should be added for both surfaces.
EXPECT_THAT(GetAllTempReferences(),
UnorderedElementsAre(surface_id1, surface_id2));
- EXPECT_THAT(GetReferencesFromRoot(),
- UnorderedElementsAre(surface_id1, surface_id2));
// Create |parent_id| and add a real reference from it to |surface_id1|.
const SurfaceId parent_id = CreateSurface(kFrameSink1, 1);
- manager().AddSurfaceReference(parent_id, surface_id1);
+ AddSurfaceReference(parent_id, surface_id1);
// Real reference must be added to |surface_id1| and the temporary reference
// to it must be gone.
@@ -366,8 +357,6 @@ TEST_F(SurfaceManagerRefTest, AddTwoSurfacesThenOneReference) {
// A temporary reference to |parent_id| must be created.
EXPECT_THAT(GetAllTempReferences(),
UnorderedElementsAre(parent_id, surface_id2));
- EXPECT_THAT(GetReferencesFromRoot(),
- UnorderedElementsAre(parent_id, surface_id2));
EXPECT_THAT(GetReferencesFrom(parent_id), ElementsAre(surface_id1));
}
@@ -380,26 +369,22 @@ TEST_F(SurfaceManagerRefTest, AddSurfacesSkipReference) {
// Temporary references should be added for both surfaces and they should be
// stored in the order of creation.
- EXPECT_THAT(GetTempReferencesFor(surface_id1.frame_sink_id()),
- ElementsAre(surface_id1.local_surface_id(),
- surface_id2.local_surface_id()));
- EXPECT_THAT(GetReferencesFromRoot(),
+ EXPECT_THAT(GetAllTempReferences(),
UnorderedElementsAre(surface_id1, surface_id2));
// Create |parent_id| and add a reference from it to |surface_id2| which was
// created later.
const SurfaceId parent_id = CreateSurface(kFrameSink1, 1);
- manager().AddSurfaceReference(parent_id, surface_id2);
+ AddSurfaceReference(parent_id, surface_id2);
// The real reference should be added for |surface_id2| and the temporary
// references to both |surface_id1| and |surface_id2| should be gone.
// There should be a temporary reference to |parent_id|.
EXPECT_THAT(GetAllTempReferences(), ElementsAre(parent_id));
- EXPECT_THAT(GetReferencesFromRoot(), ElementsAre(parent_id));
EXPECT_THAT(GetReferencesFrom(parent_id), ElementsAre(surface_id2));
}
-TEST_F(SurfaceManagerRefTest, RemoveFirstTempRefOnly) {
+TEST_F(SurfaceManagerRefTest, RemoveFirstTempReferenceOnly) {
// 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(kFrameSink2, 1);
@@ -407,25 +392,43 @@ TEST_F(SurfaceManagerRefTest, RemoveFirstTempRefOnly) {
// Temporary references should be added for both surfaces and they should be
// stored in the order of creation.
- EXPECT_THAT(GetTempReferencesFor(surface_id1.frame_sink_id()),
- ElementsAre(surface_id1.local_surface_id(),
- surface_id2.local_surface_id()));
- EXPECT_THAT(GetReferencesFromRoot(),
+ EXPECT_THAT(GetAllTempReferences(),
UnorderedElementsAre(surface_id1, surface_id2));
// Create |parent_id| and add a reference from it to |surface_id1| which was
// created earlier.
const SurfaceId parent_id = CreateSurface(kFrameSink1, 1);
- manager().AddSurfaceReference(parent_id, surface_id1);
+ AddSurfaceReference(parent_id, surface_id1);
// The real reference should be added for |surface_id1| and its temporary
// reference should be removed. The temporary reference for |surface_id2|
// should remain. A temporary reference must be added for |parent_id|.
EXPECT_THAT(GetAllTempReferences(),
UnorderedElementsAre(parent_id, surface_id2));
- EXPECT_THAT(GetReferencesFromRoot(),
- UnorderedElementsAre(parent_id, surface_id2));
EXPECT_THAT(GetReferencesFrom(parent_id), ElementsAre(surface_id1));
}
+TEST_F(SurfaceManagerRefTest, SurfaceWithTemporaryReferenceIsNotDeleted) {
+ const SurfaceId id1 = CreateSurface(kFrameSink1, 1);
+ AddSurfaceReference(manager().GetRootSurfaceId(), id1);
+
+ // We create |id2| and never add a real reference to it. This leaves the
+ // temporary reference.
+ const SurfaceId id2 = CreateSurface(kFrameSink2, 1);
+ ASSERT_THAT(GetAllTempReferences(), UnorderedElementsAre(id2));
+ EXPECT_NE(nullptr, manager().GetSurfaceForId(id2));
+
+ // Destroy both surfaces so they can be garbage collected. We remove the
+ // surface reference to |id1| which will run GarbageCollectSurfaces().
+ DestroySurface(id1);
+ DestroySurface(id2);
+ RemoveSurfaceReference(manager().GetRootSurfaceId(), id1);
+
+ // |id1| is destroyed and has no references, so it's deleted.
+ EXPECT_EQ(nullptr, manager().GetSurfaceForId(id1));
+
+ // |id2| is destroyed but has a temporary reference, it's not deleted.
+ EXPECT_NE(nullptr, manager().GetSurfaceForId(id2));
+}
+
} // namespace cc
« no previous file with comments | « cc/surfaces/surface_manager.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698