cc: Plumbing for BeginFrameSource based on Surfaces

cc/surfaces/surface_unittest.cc

Index: cc/surfaces/surface_unittest.cc
diff --git a/cc/surfaces/surface_unittest.cc b/cc/surfaces/surface_unittest.cc
index 42fd55f4a03a9b3ebb0ca67aab8d3c49cb05bce6..b9be5160bcbfcdd8e6d6f5a2b872b35b9ab281a3 100644
--- a/cc/surfaces/surface_unittest.cc
+++ b/cc/surfaces/surface_unittest.cc
@@ -4,16 +4,35 @@
 
 #include "cc/surfaces/surface.h"
 #include "cc/surfaces/surface_factory.h"
+#include "cc/surfaces/surface_factory_client.h"
 #include "cc/surfaces/surface_manager.h"
+#include "cc/test/scheduler_test_common.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace cc {
 namespace {
 
+class FakeSurfaceFactoryClient : public SurfaceFactoryClient {
+ public:
+  FakeSurfaceFactoryClient() : begin_frame_source_(nullptr) {}
+
+  void ReturnResources(const ReturnedResourceArray& resources) override {}
+
+  void SetBeginFrameSource(BeginFrameSource* begin_frame_source) override {
+    begin_frame_source_ = begin_frame_source;
+  }
+
+  BeginFrameSource* begin_frame_source() { return begin_frame_source_; }
+
+ private:
+  BeginFrameSource* begin_frame_source_;
+};
+
 TEST(SurfaceTest, SurfaceLifetime) {
   SurfaceManager manager;
-  SurfaceFactory factory(&manager, NULL);
+  FakeSurfaceFactoryClient surface_factory_client;
+  SurfaceFactory factory(&manager, &surface_factory_client);
 
   SurfaceId surface_id(6);
   {
@@ -25,5 +44,101 @@ TEST(SurfaceTest, SurfaceLifetime) {
   EXPECT_EQ(NULL, manager.GetSurfaceForId(surface_id));
 }
 
+TEST(SurfaceTest, StableBeginFrameSourceIndependentOfOrderAdded) {
+  SurfaceManager manager;
+  FakeSurfaceFactoryClient surface_factory_client;
+  SurfaceFactory factory(&manager, &surface_factory_client);
+
+  SurfaceId surface_id(6);
+  factory.Create(surface_id);
+  Surface* surface = manager.GetSurfaceForId(surface_id);
+
+  FakeBeginFrameSource bfs1;
+  FakeBeginFrameSource bfs2;
+  FakeBeginFrameSource bfs3;
+

[mithro-old, 2015/10/01 03:00:24]

This test is a bit weird, but I'm unsure how to make it easier to understand.

I wonder if you are better off having 6 separate surfaces (you could wrap the
setup in a class or macro) rather than doing the RemoveBeginFrameSource bit...

I feel like these EXPECT_EQ should be under each the AddBeginFrameSource bits
too.

[brianderson, 2015/10/07 20:54:48]

Given each order would also need a SurfaceFactoryClient and SurfaceFactory,
there is a lot of boilerplate to do that. Callin REmoveBeginFrameSource a bunch
of times seems like the easiest way forward. I added some checks after the
sources are removed to make sure they were actually removed.
Done.

+  // Order 1.
+  surface->AddBeginFrameSource(&bfs1);
+  surface->AddBeginFrameSource(&bfs2);
+  surface->AddBeginFrameSource(&bfs3);
+  BeginFrameSource* bfs_order1 = surface_factory_client.begin_frame_source();
+  surface->RemoveBeginFrameSource(&bfs1);
+  surface->RemoveBeginFrameSource(&bfs2);
+  surface->RemoveBeginFrameSource(&bfs3);
+
+  // Order 2.
+  surface->AddBeginFrameSource(&bfs1);
+  surface->AddBeginFrameSource(&bfs3);
+  surface->AddBeginFrameSource(&bfs2);
+  BeginFrameSource* bfs_order2 = surface_factory_client.begin_frame_source();
+  surface->RemoveBeginFrameSource(&bfs1);
+  surface->RemoveBeginFrameSource(&bfs2);
+  surface->RemoveBeginFrameSource(&bfs3);
+
+  // Order 3.
+  surface->AddBeginFrameSource(&bfs2);
+  surface->AddBeginFrameSource(&bfs1);
+  surface->AddBeginFrameSource(&bfs3);
+  BeginFrameSource* bfs_order3 = surface_factory_client.begin_frame_source();
+  surface->RemoveBeginFrameSource(&bfs1);
+  surface->RemoveBeginFrameSource(&bfs2);
+  surface->RemoveBeginFrameSource(&bfs3);
+
+  // Order 4.
+  surface->AddBeginFrameSource(&bfs2);
+  surface->AddBeginFrameSource(&bfs3);
+  surface->AddBeginFrameSource(&bfs1);
+  BeginFrameSource* bfs_order4 = surface_factory_client.begin_frame_source();
+  surface->RemoveBeginFrameSource(&bfs1);
+  surface->RemoveBeginFrameSource(&bfs2);
+  surface->RemoveBeginFrameSource(&bfs3);
+
+  // Order 5.
+  surface->AddBeginFrameSource(&bfs3);
+  surface->AddBeginFrameSource(&bfs1);
+  surface->AddBeginFrameSource(&bfs2);
+  BeginFrameSource* bfs_order5 = surface_factory_client.begin_frame_source();
+  surface->RemoveBeginFrameSource(&bfs1);
+  surface->RemoveBeginFrameSource(&bfs2);
+  surface->RemoveBeginFrameSource(&bfs3);
+
+  // Order 6.
+  surface->AddBeginFrameSource(&bfs3);
+  surface->AddBeginFrameSource(&bfs2);
+  surface->AddBeginFrameSource(&bfs1);
+  BeginFrameSource* bfs_order6 = surface_factory_client.begin_frame_source();
+  surface->RemoveBeginFrameSource(&bfs1);
+  surface->RemoveBeginFrameSource(&bfs2);
+  surface->RemoveBeginFrameSource(&bfs3);
+
+  EXPECT_NE(nullptr, bfs_order1);

[mithro-old, 2015/10/01 03:00:24]

Should this be an ASSERT_NE?

[brianderson, 2015/10/07 20:54:48]

nullptr could be a valid result if the surface doesn't belong to any display.

+  EXPECT_TRUE(&bfs1 == bfs_order1 || &bfs2 == bfs_order1 ||
+              &bfs3 == bfs_order1);

[mithro-old, 2015/10/01 03:00:24]

I'm not sure what the above EXPECT_TRUE is checking?

[brianderson, 2015/10/07 20:54:48]

Added a comment. It checks that one of the sources provided was actually chosen.

+  EXPECT_EQ(bfs_order1, bfs_order2);
+  EXPECT_EQ(bfs_order2, bfs_order3);
+  EXPECT_EQ(bfs_order3, bfs_order4);
+  EXPECT_EQ(bfs_order4, bfs_order5);
+  EXPECT_EQ(bfs_order5, bfs_order6);
+}
+
+TEST(SurfaceTest, BeginFrameSourceRemovedOnSurfaceDestruction) {
+  SurfaceManager manager;
+  FakeSurfaceFactoryClient surface_factory_client;
+  SurfaceFactory factory(&manager, &surface_factory_client);
+  FakeBeginFrameSource bfs;
+
+  SurfaceId surface_id(6);
+  factory.Create(surface_id);
+  Surface* surface = manager.GetSurfaceForId(surface_id);
+  surface->AddBeginFrameSource(&bfs);
+
+  BeginFrameSource* bfs_before = surface_factory_client.begin_frame_source();
+  factory.Destroy(surface_id);
+  BeginFrameSource* bfs_after = surface_factory_client.begin_frame_source();
+
+  EXPECT_EQ(&bfs, bfs_before);
+  EXPECT_EQ(nullptr, bfs_after);
+}
+
 }  // namespace
 }  // namespace cc