cc: Make OutputSurface::BindToClient pure virtual and not return bool

cc/output/gl_renderer_unittest.cc

Index: cc/output/gl_renderer_unittest.cc
diff --git a/cc/output/gl_renderer_unittest.cc b/cc/output/gl_renderer_unittest.cc
index da940bfc57dc06ecff20e0eadca911f457c383a5..d9456b38a2dff99bc009d2de3a22e832ae144bb1 100644
--- a/cc/output/gl_renderer_unittest.cc
+++ b/cc/output/gl_renderer_unittest.cc
@@ -340,9 +340,8 @@ class FakeRendererGL : public GLRenderer {
 class GLRendererWithDefaultHarnessTest : public GLRendererTest {
  protected:
   GLRendererWithDefaultHarnessTest() {
-    output_surface_ =
-        FakeOutputSurface::Create3d(TestWebGraphicsContext3D::Create());
-    CHECK(output_surface_->BindToClient(&output_surface_client_));
+    output_surface_ = FakeOutputSurface::Create3d();
+    output_surface_->BindToClient(&output_surface_client_);
 
     shared_bitmap_manager_.reset(new TestSharedBitmapManager());
     resource_provider_ = FakeResourceProvider::Create(
@@ -372,7 +371,7 @@ class GLRendererShaderTest : public GLRendererTest {
  protected:
   GLRendererShaderTest() {
     output_surface_ = FakeOutputSurface::Create3d();
-    CHECK(output_surface_->BindToClient(&output_surface_client_));
+    output_surface_->BindToClient(&output_surface_client_);
 
     shared_bitmap_manager_.reset(new TestSharedBitmapManager());
     resource_provider_ = FakeResourceProvider::Create(
@@ -599,11 +598,14 @@ class ForbidSynchronousCallContext : public TestWebGraphicsContext3D {
   }
 };
 TEST_F(GLRendererTest, InitializationDoesNotMakeSynchronousCalls) {
+  auto context = base::MakeUnique<ForbidSynchronousCallContext>();
+  auto provider = TestContextProvider::Create(std::move(context));

[enne (OOO), 2016/10/25 18:22:59]

Should TestContextProvider just BindToCurrentThread in its ctor? Or if there are
use cases where it doesn't, just make a second class that doesn't? It just seems
like this gets repeated a lot and it's pretty verbose to have to create it
separately to make a call on it before you pass it somewhere else.

[danakj, 2016/10/25 18:54:18]

So.. yah. CreateWorker() will bind immediately. Layer compositor contexts don't
get bound immediately tho and that's always been the default behaviour in cc
unittests. Eventually they will too when they are created on the compositor
thread, but.. yah.

I could change all the callsites to CreateAndBind() or something instead. I'd
have to go find all the places that make a context for the layer compositor
though if I changed the default. And then we'd be missing calls to
BindToCurrentThread() that would be needed with a production ContextProvider. So
overall Idk I don't think it'd be better?

[enne (OOO), 2016/10/25 20:01:14]

How many places want the default vs auto-bind?

Is switching in a production ContextProvider vs using a test one for these tests
a use case you want to support?

[danakj, 2016/10/25 20:12:00]

Any context handed to LayerTreeHostImpl needs to not be bound on the main
thread.
Well, all our production code uses ContextProvider, and it would make it very
easy to code LTHI stop binding the context in some path for example, but tests
wouldn't notice. Esp because BindToCurrentThread is explicitly allowed to be
called twice, so we can add this default binding almost-everywhere and then
remove the production bind and tests will be fine, but we'd break chrome.

I just don't feel very safe about making such a change, and would rather get rid
of BindToCurrentThread by creating contexts on the right thread instead.

I guess an alternative I would feel safer about would be a
DeferredContextProvider interface that spits out the ContextProvider and binds
it. Then we'd get typesafe "this is bound or not" throughout the system and we
couldn't get it wrong. It's a lot like creating contexts on the right thread,
but we could just wrap the current ContextProvider instead..

+  provider->BindToCurrentThread();
+
   FakeOutputSurfaceClient output_surface_client;
   std::unique_ptr<OutputSurface> output_surface(
-      FakeOutputSurface::Create3d(std::unique_ptr<TestWebGraphicsContext3D>(
-          new ForbidSynchronousCallContext)));
-  CHECK(output_surface->BindToClient(&output_surface_client));
+      FakeOutputSurface::Create3d(std::move(provider)));
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -632,11 +634,14 @@ class LoseContextOnFirstGetContext : public TestWebGraphicsContext3D {
 };
 
 TEST_F(GLRendererTest, InitializationWithQuicklyLostContextDoesNotAssert) {
+  auto context = base::MakeUnique<LoseContextOnFirstGetContext>();
+  auto provider = TestContextProvider::Create(std::move(context));
+  provider->BindToCurrentThread();
+
   FakeOutputSurfaceClient output_surface_client;
   std::unique_ptr<OutputSurface> output_surface(
-      FakeOutputSurface::Create3d(std::unique_ptr<TestWebGraphicsContext3D>(
-          new LoseContextOnFirstGetContext)));
-  CHECK(output_surface->BindToClient(&output_surface_client));
+      FakeOutputSurface::Create3d(std::move(provider)));
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -664,10 +669,13 @@ TEST_F(GLRendererTest, OpaqueBackground) {
   std::unique_ptr<ClearCountingContext> context_owned(new ClearCountingContext);
   ClearCountingContext* context = context_owned.get();
 
+  auto provider = TestContextProvider::Create(std::move(context_owned));
+  provider->BindToCurrentThread();
+
   FakeOutputSurfaceClient output_surface_client;
   std::unique_ptr<OutputSurface> output_surface(
-      FakeOutputSurface::Create3d(std::move(context_owned)));
-  CHECK(output_surface->BindToClient(&output_surface_client));
+      FakeOutputSurface::Create3d(std::move(provider)));
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -705,10 +713,13 @@ TEST_F(GLRendererTest, TransparentBackground) {
   std::unique_ptr<ClearCountingContext> context_owned(new ClearCountingContext);
   ClearCountingContext* context = context_owned.get();
 
+  auto provider = TestContextProvider::Create(std::move(context_owned));
+  provider->BindToCurrentThread();
+
   FakeOutputSurfaceClient output_surface_client;
   std::unique_ptr<OutputSurface> output_surface(
-      FakeOutputSurface::Create3d(std::move(context_owned)));
-  CHECK(output_surface->BindToClient(&output_surface_client));
+      FakeOutputSurface::Create3d(std::move(provider)));
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -739,10 +750,13 @@ TEST_F(GLRendererTest, OffscreenOutputSurface) {
   std::unique_ptr<ClearCountingContext> context_owned(new ClearCountingContext);
   ClearCountingContext* context = context_owned.get();
 
+  auto provider = TestContextProvider::Create(std::move(context_owned));
+  provider->BindToCurrentThread();
+
   FakeOutputSurfaceClient output_surface_client;
   std::unique_ptr<OutputSurface> output_surface(
-      FakeOutputSurface::CreateOffscreen(std::move(context_owned)));
-  CHECK(output_surface->BindToClient(&output_surface_client));
+      FakeOutputSurface::CreateOffscreen(std::move(provider)));
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -795,10 +809,13 @@ TEST_F(GLRendererTest, ActiveTextureState) {
       new TextureStateTrackingContext);
   TextureStateTrackingContext* context = context_owned.get();
 
+  auto provider = TestContextProvider::Create(std::move(context_owned));
+  provider->BindToCurrentThread();
+
   FakeOutputSurfaceClient output_surface_client;
   std::unique_ptr<OutputSurface> output_surface(
-      FakeOutputSurface::Create3d(std::move(context_owned)));
-  CHECK(output_surface->BindToClient(&output_surface_client));
+      FakeOutputSurface::Create3d(std::move(provider)));
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -876,10 +893,13 @@ TEST_F(GLRendererTest, ShouldClearRootRenderPass) {
       new NoClearRootRenderPassMockContext);
   NoClearRootRenderPassMockContext* mock_context = mock_context_owned.get();
 
+  auto provider = TestContextProvider::Create(std::move(mock_context_owned));
+  provider->BindToCurrentThread();
+
   FakeOutputSurfaceClient output_surface_client;
   std::unique_ptr<OutputSurface> output_surface(
-      FakeOutputSurface::Create3d(std::move(mock_context_owned)));
-  CHECK(output_surface->BindToClient(&output_surface_client));
+      FakeOutputSurface::Create3d(std::move(provider)));
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -961,10 +981,13 @@ class ScissorTestOnClearCheckingGLES2Interface : public TestGLES2Interface {
 TEST_F(GLRendererTest, ScissorTestWhenClearing) {
   auto gl_owned = base::MakeUnique<ScissorTestOnClearCheckingGLES2Interface>();
 
+  auto provider = TestContextProvider::Create(std::move(gl_owned));
+  provider->BindToCurrentThread();
+
   FakeOutputSurfaceClient output_surface_client;
-  std::unique_ptr<OutputSurface> output_surface(FakeOutputSurface::Create3d(
-      TestContextProvider::Create(std::move(gl_owned))));
-  CHECK(output_surface->BindToClient(&output_surface_client));
+  std::unique_ptr<OutputSurface> output_surface(
+      FakeOutputSurface::Create3d(std::move(provider)));
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -1034,9 +1057,12 @@ TEST_F(GLRendererTest, NoDiscardOnPartialUpdates) {
   auto gl_owned = base::MakeUnique<DiscardCheckingGLES2Interface>();
   auto* gl = gl_owned.get();
 
+  auto provider = TestContextProvider::Create(std::move(gl_owned));
+  provider->BindToCurrentThread();
+
   FakeOutputSurfaceClient output_surface_client;
-  auto output_surface = FakeOutputSurface::Create3d(std::move(gl_owned));
-  CHECK(output_surface->BindToClient(&output_surface_client));
+  auto output_surface = FakeOutputSurface::Create3d(std::move(provider));
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -1115,8 +1141,8 @@ TEST_F(GLRendererTest, DrawFramePreservesFramebuffer) {
   // has finished.
   FakeOutputSurfaceClient output_surface_client;
   std::unique_ptr<FakeOutputSurface> output_surface(
-      FakeOutputSurface::Create3d(TestWebGraphicsContext3D::Create()));
-  CHECK(output_surface->BindToClient(&output_surface_client));
+      FakeOutputSurface::Create3d());
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -1453,11 +1479,12 @@ class OutputSurfaceMockContext : public TestWebGraphicsContext3D {
 
 class MockOutputSurface : public OutputSurface {
  public:
-  MockOutputSurface()
-      : OutputSurface(TestContextProvider::Create(
-            base::MakeUnique<StrictMock<OutputSurfaceMockContext>>())) {}
+  explicit MockOutputSurface(scoped_refptr<ContextProvider> provider)
+      : OutputSurface(std::move(provider)) {}
   virtual ~MockOutputSurface() {}
 
+  void BindToClient(OutputSurfaceClient*) override {}
+
   MOCK_METHOD0(EnsureBackbuffer, void());
   MOCK_METHOD0(DiscardBackbuffer, void());
   MOCK_METHOD4(Reshape,
@@ -1480,22 +1507,29 @@ class MockOutputSurface : public OutputSurface {
 
 class MockOutputSurfaceTest : public GLRendererTest {
  protected:
-  virtual void SetUp() {
+  void SetUp() override {
+    auto context = base::MakeUnique<StrictMock<OutputSurfaceMockContext>>();
+    context_ = context.get();
+    auto provider = TestContextProvider::Create(std::move(context));
+    provider->BindToCurrentThread();
+    output_surface_ =
+        base::MakeUnique<StrictMock<MockOutputSurface>>(std::move(provider));
+
     FakeOutputSurfaceClient output_surface_client_;
-    CHECK(output_surface_.BindToClient(&output_surface_client_));
+    output_surface_->BindToClient(&output_surface_client_);
 
     shared_bitmap_manager_.reset(new TestSharedBitmapManager());
     resource_provider_ = FakeResourceProvider::Create(
-        output_surface_.context_provider(), shared_bitmap_manager_.get());
+        output_surface_->context_provider(), shared_bitmap_manager_.get());
 
-    renderer_.reset(new FakeRendererGL(&settings_, &output_surface_,
+    renderer_.reset(new FakeRendererGL(&settings_, output_surface_.get(),
                                        resource_provider_.get()));
-    EXPECT_CALL(output_surface_, GetOverlayCandidateValidator()).Times(1);
+    EXPECT_CALL(*output_surface_, GetOverlayCandidateValidator()).Times(1);
     renderer_->Initialize();
 
-    EXPECT_CALL(output_surface_, EnsureBackbuffer()).Times(1);
+    EXPECT_CALL(*output_surface_, EnsureBackbuffer()).Times(1);
     renderer_->SetVisible(true);
-    Mock::VerifyAndClearExpectations(&output_surface_);
+    Mock::VerifyAndClearExpectations(output_surface_.get());
   }
 
   void SwapBuffers() { renderer_->SwapBuffers(std::vector<ui::LatencyInfo>()); }
@@ -1510,15 +1544,15 @@ class MockOutputSurfaceTest : public GLRendererTest {
     AddQuad(render_pass, gfx::Rect(viewport_size), SK_ColorGREEN);
     render_pass->has_transparent_background = transparent;
 
-    EXPECT_CALL(output_surface_, EnsureBackbuffer()).WillRepeatedly(Return());
+    EXPECT_CALL(*output_surface_, EnsureBackbuffer()).WillRepeatedly(Return());
 
-    EXPECT_CALL(output_surface_,
+    EXPECT_CALL(*output_surface_,
                 Reshape(viewport_size, device_scale_factor, _, transparent))
         .Times(1);
 
-    EXPECT_CALL(output_surface_, BindFramebuffer()).Times(1);
+    EXPECT_CALL(*output_surface_, BindFramebuffer()).Times(1);
 
-    EXPECT_CALL(*Context(), drawElements(_, _, _, _)).Times(1);
+    EXPECT_CALL(*context_, drawElements(_, _, _, _)).Times(1);
 
     renderer_->DecideRenderPassAllocationsForFrame(
         render_passes_in_draw_order_);
@@ -1526,15 +1560,10 @@ class MockOutputSurfaceTest : public GLRendererTest {
                          gfx::ColorSpace(), viewport_size);
   }
 
-  OutputSurfaceMockContext* Context() {
-    return static_cast<OutputSurfaceMockContext*>(
-        static_cast<TestContextProvider*>(output_surface_.context_provider())
-            ->TestContext3d());
-  }
-
   RendererSettings settings_;
   FakeOutputSurfaceClient output_surface_client_;
-  StrictMock<MockOutputSurface> output_surface_;
+  OutputSurfaceMockContext* context_ = nullptr;
+  std::unique_ptr<StrictMock<MockOutputSurface>> output_surface_;
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager_;
   std::unique_ptr<ResourceProvider> resource_provider_;
   std::unique_ptr<FakeRendererGL> renderer_;
@@ -1542,14 +1571,14 @@ class MockOutputSurfaceTest : public GLRendererTest {
 
 TEST_F(MockOutputSurfaceTest, BackbufferDiscard) {
   // Drop backbuffer on hide.
-  EXPECT_CALL(output_surface_, DiscardBackbuffer()).Times(1);
+  EXPECT_CALL(*output_surface_, DiscardBackbuffer()).Times(1);
   renderer_->SetVisible(false);
-  Mock::VerifyAndClearExpectations(&output_surface_);
+  Mock::VerifyAndClearExpectations(output_surface_.get());
 
   // Restore backbuffer on show.
-  EXPECT_CALL(output_surface_, EnsureBackbuffer()).Times(1);
+  EXPECT_CALL(*output_surface_, EnsureBackbuffer()).Times(1);
   renderer_->SetVisible(true);
-  Mock::VerifyAndClearExpectations(&output_surface_);
+  Mock::VerifyAndClearExpectations(output_surface_.get());
 }
 
 class TestOverlayProcessor : public OverlayProcessor {
@@ -1601,7 +1630,7 @@ TEST_F(GLRendererTest, DontOverlayWithCopyRequests) {
   FakeOutputSurfaceClient output_surface_client;
   std::unique_ptr<FakeOutputSurface> output_surface(
       FakeOutputSurface::Create3d());
-  CHECK(output_surface->BindToClient(&output_surface_client));
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -1749,16 +1778,17 @@ TEST_F(GLRendererTest, OverlaySyncTokensAreProcessed) {
       new WaitSyncTokenCountingContext);
   WaitSyncTokenCountingContext* context = context_owned.get();
 
+  auto provider = TestContextProvider::Create(std::move(context_owned));
+  provider->BindToCurrentThread();
+
   MockOverlayScheduler overlay_scheduler;
-  scoped_refptr<TestContextProvider> context_provider =
-      TestContextProvider::Create(std::move(context_owned));
-  context_provider->support()->SetScheduleOverlayPlaneCallback(base::Bind(
+  provider->support()->SetScheduleOverlayPlaneCallback(base::Bind(
       &MockOverlayScheduler::Schedule, base::Unretained(&overlay_scheduler)));
 
   FakeOutputSurfaceClient output_surface_client;
   std::unique_ptr<OutputSurface> output_surface(
-      FakeOutputSurface::Create3d(context_provider));
-  CHECK(output_surface->BindToClient(&output_surface_client));
+      FakeOutputSurface::Create3d(std::move(provider)));
+  output_surface->BindToClient(&output_surface_client);
 
   std::unique_ptr<SharedBitmapManager> shared_bitmap_manager(
       new TestSharedBitmapManager());
@@ -1842,10 +1872,13 @@ class GLRendererPartialSwapTest : public GLRendererTest {
     auto gl_owned = base::MakeUnique<PartialSwapMockGLES2Interface>();
     auto* gl = gl_owned.get();
 
+    auto provider = TestContextProvider::Create(std::move(gl_owned));
+    provider->BindToCurrentThread();
+
     FakeOutputSurfaceClient output_surface_client;
     std::unique_ptr<FakeOutputSurface> output_surface(
-        FakeOutputSurface::Create3d(std::move(gl_owned)));
-    CHECK(output_surface->BindToClient(&output_surface_client));
+        FakeOutputSurface::Create3d(std::move(provider)));
+    output_surface->BindToClient(&output_surface_client);
 
     std::unique_ptr<ResourceProvider> resource_provider =
         FakeResourceProvider::Create(output_surface->context_provider(),
@@ -1923,6 +1956,7 @@ class GLRendererWithMockContextTest : public ::testing::Test {
     context_support_ptr_ = context_support.get();
     auto context_provider = TestContextProvider::Create(
         TestWebGraphicsContext3D::Create(), std::move(context_support));
+    context_provider->BindToCurrentThread();
     output_surface_ = FakeOutputSurface::Create3d(std::move(context_provider));
     output_surface_->BindToClient(&output_surface_client_);
     resource_provider_ = FakeResourceProvider::Create(