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

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: cc/surfaces/surface_manager_ref_unittest.cc

Issue 2716553004: Add temporary reference ownership to SurfaceManager. (Closed)
Patch Set: Cleanup. 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
« cc/surfaces/surface_manager.cc ('K') | « 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..52d26cc1401b129cb001895373375d425f89bcae 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);
@@ -216,8 +209,8 @@ TEST_F(SurfaceManagerRefTest, CheckGC) {
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));
@@ -243,9 +236,9 @@ TEST_F(SurfaceManagerRefTest, CheckGCRecusiveFull) {
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);
@@ -270,31 +263,33 @@ TEST_F(SurfaceManagerRefTest, TryDoubleAddReference) {
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);
+ 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) {
@@ -302,7 +297,7 @@ TEST_F(SurfaceManagerRefTest, TryRemoveBadReference) {
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|.
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,22 +369,18 @@ 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));
}
@@ -407,25 +392,107 @@ 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, TemporaryReferenceNotGarbageCollected) {
Fady Samuel 2017/02/24 19:08:11 Top level comment about what the unit test is tryi
kylechar 2017/02/24 19:28:01 I changed the name to be more descriptive. PTAL in
+ const SurfaceId id1 = CreateSurface(kFrameSink1, 1);
+ AddSurfaceReference(manager().GetRootSurfaceId(), id1);
+
+ const SurfaceId id2 = CreateSurface(kFrameSink2, 1);
+ ASSERT_THAT(GetAllTempReferences(), UnorderedElementsAre(id2));
+ EXPECT_NE(nullptr, manager().GetSurfaceForId(id2));
+
+ // Destroy both surfaces and remove the reference to |id1|.
Fady Samuel 2017/02/24 19:08:11 Add comment about when garbage collection happens.
kylechar 2017/02/24 19:28:01 Done in other CL.
+ DestroySurface(id1);
+ DestroySurface(id2);
+ RemoveSurfaceReference(manager().GetRootSurfaceId(), id1);
+
+ // Without any references |id1| is deleted. There is still a temporary
Fady Samuel 2017/02/24 19:08:11 Move the "There is still a temporary reference to
kylechar 2017/02/24 19:28:01 Done in other CL.
+ // reference to |id2| so it is not deleted.
+ EXPECT_EQ(nullptr, manager().GetSurfaceForId(id1));
+ EXPECT_NE(nullptr, manager().GetSurfaceForId(id2));
+}
+
+TEST_F(SurfaceManagerRefTest, AssignTempReference) {
Fady Samuel 2017/02/24 19:08:11 Top level comment about what the unit test is tryi
kylechar 2017/02/24 19:57:29 Done.
+ // The newly created surface should have a temporary reference.
+ const SurfaceId id1 = CreateSurface(kFrameSink2, 1);
+ ASSERT_THAT(GetAllTempReferences(), UnorderedElementsAre(id1));
+
+ // It should still have a temporary reference after an owner is assigned.
+ manager().AssignTemporaryReference(id1, kFrameSink1);
+ ASSERT_THAT(GetAllTempReferences(), UnorderedElementsAre(id1));
+
+ // When |kFrameSink1| is invalidated the temporary reference will be removed.
+ manager().InvalidateFrameSinkId(kFrameSink1);
+ ASSERT_THAT(GetAllTempReferences(), IsEmpty());
+}
+
+TEST_F(SurfaceManagerRefTest, InvalidateHasNoEffectOnSurfaceReferences) {
Fady Samuel 2017/02/24 19:08:11 Top level comment about what the unit test is tryi
kylechar 2017/02/24 19:57:29 Done.
+ const SurfaceId parent_id = CreateSurface(kFrameSink1, 1);
+ AddSurfaceReference(manager().GetRootSurfaceId(), parent_id);
+
+ const SurfaceId id1 = CreateSurface(kFrameSink2, 1);
+ manager().AssignTemporaryReference(id1, kFrameSink1);
+ ASSERT_THAT(GetAllTempReferences(), UnorderedElementsAre(id1));
+
+ // Adding a real surface reference will remove the temporary reference.
+ AddSurfaceReference(parent_id, id1);
+ ASSERT_THAT(GetAllTempReferences(), IsEmpty());
+ ASSERT_THAT(GetReferencesFor(id1), UnorderedElementsAre(parent_id));
+
+ // When |kFrameSink1| is invalidated it shouldn't change the surface
+ // references.
+ manager().InvalidateFrameSinkId(kFrameSink1);
+ ASSERT_THAT(GetReferencesFor(id1), UnorderedElementsAre(parent_id));
+}
+
+TEST_F(SurfaceManagerRefTest, DropTempReference) {
Fady Samuel 2017/02/24 19:08:11 Top level comment about what the unit test is tryi
kylechar 2017/02/24 19:57:29 Done-ish.
+ const SurfaceId id1 = CreateSurface(kFrameSink1, 1);
+ ASSERT_THAT(GetAllTempReferences(), UnorderedElementsAre(id1));
+
+ // An example of why this could happen is the window server doesn't know the
+ // owner, maybe it has crashed and been cleanup already, and asks to drop the
+ // temporary reference.
+ manager().DropTemporaryReference(id1);
+ ASSERT_THAT(GetAllTempReferences(), IsEmpty());
+}
+
+TEST_F(SurfaceManagerRefTest, DropOnlyOneTempReference) {
Fady Samuel 2017/02/24 19:08:11 Top level comment about what the unit test is tryi
kylechar 2017/02/24 19:57:29 Done.
+ const SurfaceId parent_id = CreateSurface(kFrameSink1, 1);
+ AddSurfaceReference(manager().GetRootSurfaceId(), parent_id);
+
+ const SurfaceId id1a = CreateSurface(kFrameSink2, 1);
+ const SurfaceId id1b = CreateSurface(kFrameSink2, 2);
+
+ ASSERT_THAT(GetAllTempReferences(), UnorderedElementsAre(id1a, id1b));
+
+ // Assign |id1a| to |kFrameSink1|. This doesn't change the temporary
+ // reference, it just assigns as owner to it.
+ manager().AssignTemporaryReference(id1a, kFrameSink1);
+ ASSERT_THAT(GetAllTempReferences(), UnorderedElementsAre(id1a, id1b));
+
+ // When the owner is invalidated the temporary reference to |id2a| will
+ // removed.
+ manager().InvalidateFrameSinkId(kFrameSink1);
+ ASSERT_THAT(GetAllTempReferences(), UnorderedElementsAre(id1b));
+
+ // If the owner is invalidated then the Window Server might not
Fady Samuel 2017/02/24 19:08:11 Incomplete sentence?
kylechar 2017/02/24 19:57:29 Done.
+ manager().DropTemporaryReference(id1b);
+ ASSERT_THAT(GetAllTempReferences(), IsEmpty());
+}
+
} // namespace cc
« cc/surfaces/surface_manager.cc ('K') | « cc/surfaces/surface_manager.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698