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

content/browser/renderer_host/compositor_impl_android.cc

Index: content/browser/renderer_host/compositor_impl_android.cc
diff --git a/content/browser/renderer_host/compositor_impl_android.cc b/content/browser/renderer_host/compositor_impl_android.cc
index a8c57db192d8e3b8339e29811b4da3304c541210..c8363d6ea7311231d52b4ca09fa01948f58e8953 100644
--- a/content/browser/renderer_host/compositor_impl_android.cc
+++ b/content/browser/renderer_host/compositor_impl_android.cc
@@ -14,7 +14,6 @@
 #include "base/android/scoped_java_ref.h"
 #include "base/auto_reset.h"
 #include "base/bind.h"
-#include "base/cancelable_callback.h"
 #include "base/command_line.h"
 #include "base/containers/hash_tables.h"
 #include "base/lazy_instance.h"
@@ -213,18 +212,13 @@ void ExternalBeginFrameSource::OnVSync(base::TimeTicks frame_time,
 
 class AndroidOutputSurface : public cc::OutputSurface {
  public:
-  AndroidOutputSurface(
-      scoped_refptr<ContextProviderCommandBuffer> context_provider,
-      const base::Callback<void(gpu::Capabilities)>&
-          populate_gpu_capabilities_callback)
+  explicit AndroidOutputSurface(
+      scoped_refptr<ContextProviderCommandBuffer> context_provider)
       : cc::OutputSurface(std::move(context_provider)),
-        populate_gpu_capabilities_callback_(populate_gpu_capabilities_callback),
-        swap_buffers_completion_callback_(
-            base::Bind(&AndroidOutputSurface::OnSwapBuffersCompleted,
-                       base::Unretained(this))),
         overlay_candidate_validator_(
             new display_compositor::
-                CompositorOverlayCandidateValidatorAndroid()) {
+                CompositorOverlayCandidateValidatorAndroid()),
+        weak_ptr_factory_(this) {
     capabilities_.max_frames_pending = kMaxDisplaySwapBuffers;
   }
 
@@ -240,16 +234,13 @@ class AndroidOutputSurface : public cc::OutputSurface {
     }
   }
 
-  bool BindToClient(cc::OutputSurfaceClient* client) override {
-    if (!OutputSurface::BindToClient(client))
-      return false;
-
+  void BindToClient(cc::OutputSurfaceClient* client) override {
+    DCHECK(client);
+    DCHECK(!client_);
+    client_ = client;
     GetCommandBufferProxy()->SetSwapBuffersCompletionCallback(
-        swap_buffers_completion_callback_.callback());
-
-    populate_gpu_capabilities_callback_.Run(
-        context_provider_->ContextCapabilities());
-    return true;
+        base::Bind(&AndroidOutputSurface::OnSwapBuffersCompleted,
+                   weak_ptr_factory_.GetWeakPtr()));
   }
 
   void EnsureBackbuffer() override {}
@@ -305,13 +296,9 @@ class AndroidOutputSurface : public cc::OutputSurface {
   }
 
  private:
-  base::Callback<void(gpu::Capabilities)> populate_gpu_capabilities_callback_;
-  base::CancelableCallback<void(
-      const std::vector<ui::LatencyInfo>&,
-      gfx::SwapResult,
-      const gpu::GpuProcessHostedCALayerTreeParamsMac* params_mac)>
-      swap_buffers_completion_callback_;
+  cc::OutputSurfaceClient* client_ = nullptr;
   std::unique_ptr<cc::OverlayCandidateValidator> overlay_candidate_validator_;
+  base::WeakPtrFactory<AndroidOutputSurface> weak_ptr_factory_;
 };
 
 #if defined(ENABLE_VULKAN)
@@ -699,10 +686,12 @@ void CompositorImpl::OnGpuChannelEstablished(
           context_provider_command_buffer =
               static_cast<ContextProviderCommandBuffer*>(
                   context_provider.get());
+      context_provider_command_buffer->BindToCurrentThread();

[Khushal, 2016/10/25 01:10:02]

Btw, do we mean to do any fallbacks here? IIUC when this happens with the
CompositorFrameSink, it shouts back to the LTHC to make a new
CompositorFrameSink, and we try twice before giving up.
https://cs.chromium.org/chromium/src/content/browser/renderer_host/compositor...

[Khushal, 2016/10/25 01:28:35]

Nvm, just remembered that the context with the FrameSink given to the host and
the display is shared. Its weird that we have this logic here in
DidFailToInitializeCompositorFrameSink. Will this ever happen?

[danakj, 2016/10/25 18:58:39]

We don't check that the context is not dead-on-arrival here. This code is
depending on LTHI to check that instead, which it does so after we have set up
the lost context callback (in Display), so that we don't race between the two.

It's complicated and bad IMO but I think it's correct.

[Khushal, 2016/10/25 20:45:06]

Ohhh. Right CompositorFrameSink will still check this. Makes sense, thanks!

+      gpu_capabilities_ =
+          context_provider_command_buffer->ContextCapabilities();
+
       auto display_output_surface = base::MakeUnique<AndroidOutputSurface>(
-          context_provider_command_buffer,
-          base::Bind(&CompositorImpl::PopulateGpuCapabilities,
-                     base::Unretained(this)));
+          std::move(context_provider_command_buffer));
       InitializeDisplay(std::move(display_output_surface), nullptr,
                         std::move(context_provider));
       break;
@@ -746,11 +735,6 @@ void CompositorImpl::InitializeDisplay(
   host_->SetCompositorFrameSink(std::move(compositor_frame_sink));
 }
 
-void CompositorImpl::PopulateGpuCapabilities(
-    gpu::Capabilities gpu_capabilities) {
-  gpu_capabilities_ = gpu_capabilities;
-}
-
 void CompositorImpl::AddObserver(VSyncObserver* observer) {
   observer_list_.AddObserver(observer);
 }