Collapse the initialize methods in WebGraphicsContext3DCommandBufferImpl

content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc

Index: content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
diff --git a/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc b/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
index dc5ee84b619d3c1be455a22443b35a0a03deac07..5beaa8cff033b799980adb2a51db07fd59138220 100644
--- a/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
+++ b/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
@@ -46,20 +46,17 @@ WebGraphicsContext3DCommandBufferImpl::WebGraphicsContext3DCommandBufferImpl(
     gfx::GpuPreference gpu_preference,
     bool automatic_flushes)
     : automatic_flushes_(automatic_flushes),
-      host_(host),
       surface_handle_(surface_handle),
       active_url_(active_url),
       gpu_preference_(gpu_preference),
-      weak_ptr_factory_(this) {
-  DCHECK(host);
+      host_(host) {
+  DCHECK(host_);
 }
 
 WebGraphicsContext3DCommandBufferImpl::
     ~WebGraphicsContext3DCommandBufferImpl() {
   if (real_gl_)
     real_gl_->SetLostContextCallback(base::Closure());
-
-  Destroy();
 }
 
 bool WebGraphicsContext3DCommandBufferImpl::MaybeInitializeGL(
@@ -68,46 +65,9 @@ bool WebGraphicsContext3DCommandBufferImpl::MaybeInitializeGL(
     scoped_refptr<gpu::gles2::ShareGroup> share_group,
     const gpu::gles2::ContextCreationAttribHelper& attributes,
     command_buffer_metrics::ContextType context_type) {
-  if (initialized_)
-    return true;
-
-  if (initialize_failed_)
-    return false;
-
+  DCHECK_EQ(!!shared_command_buffer, !!share_group);
   TRACE_EVENT0("gpu", "WebGfxCtx3DCmdBfrImpl::MaybeInitializeGL");
 
-  // TODO(vadimt): Remove ScopedTracker below once crbug.com/125248 is fixed.

[danakj, 2016/04/30 01:56:39]

Moved this out to GpuProcessTransportFactory, since this only applies to the
browser compositor and we already know it's tracking the time of
BindToCurrentThread, most of which is in the sync IPCs creating the
CommandBufferProxyImpl.

-  tracked_objects::ScopedTracker tracking_profile(
-      FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "125248 WebGraphicsContext3DCommandBufferImpl::MaybeInitializeGL"));
-
-  if (!CreateContext(memory_limits, shared_command_buffer,
-                     std::move(share_group), attributes, context_type)) {
-    Destroy();
-
-    initialize_failed_ = true;
-    return false;
-  }
-
-  real_gl_->SetLostContextCallback(
-      base::Bind(&WebGraphicsContext3DCommandBufferImpl::OnContextLost,
-                 // The callback is unset in the destructor.
-                 base::Unretained(this)));
-
-  real_gl_->TraceBeginCHROMIUM("WebGraphicsContext3D",
-                               "CommandBufferContext");
-
-  initialized_ = true;
-  return true;
-}
-
-bool WebGraphicsContext3DCommandBufferImpl::InitializeCommandBuffer(
-    gpu::CommandBufferProxyImpl* shared_command_buffer,
-    const gpu::gles2::ContextCreationAttribHelper& attributes,
-    command_buffer_metrics::ContextType context_type) {
-  if (!host_.get())
-    return false;
-
   DCHECK(attributes.buffer_preserved);
   std::vector<int32_t> serialized_attributes;
   attributes.Serialize(&serialized_attributes);
@@ -124,54 +84,31 @@ bool WebGraphicsContext3DCommandBufferImpl::InitializeCommandBuffer(
     command_buffer_metrics::UmaRecordContextInitFailed(context_type);
     return false;
   }
-
-  DVLOG_IF(1, gpu::error::IsError(command_buffer_->GetLastError()))

[danakj, 2016/04/30 01:56:39]

I can't see a way for last_state_ to be set to anything interesting after
creating the CommandBuffer. We do a sync call but don't do pass it a last_state_
pointer like we do in WaitForToken, so I dropped this print.

-      << "Context dead on arrival. Last error: "
-      << command_buffer_->GetLastError();
-  // Initialize the command buffer.
-  bool result = command_buffer_->Initialize();
-  LOG_IF(ERROR, !result) << "CommandBufferProxy::Initialize failed.";
-  if (!result)
+  if (!command_buffer_->Initialize()) {
+    DLOG(ERROR) << "CommandBufferProxy::Initialize failed.";
     command_buffer_metrics::UmaRecordContextInitFailed(context_type);
-  return result;
-}
-
-bool WebGraphicsContext3DCommandBufferImpl::CreateContext(
-    const gpu::SharedMemoryLimits& memory_limits,
-    gpu::CommandBufferProxyImpl* shared_command_buffer,
-    scoped_refptr<gpu::gles2::ShareGroup> share_group,
-    const gpu::gles2::ContextCreationAttribHelper& attributes,
-    command_buffer_metrics::ContextType context_type) {
-  TRACE_EVENT0("gpu", "WebGfxCtx3DCmdBfrImpl::CreateContext");
-  DCHECK_EQ(!!shared_command_buffer, !!share_group);
-
-  if (!InitializeCommandBuffer(shared_command_buffer, attributes,
-                               context_type)) {
-    LOG(ERROR) << "Failed to initialize command buffer.";
     return false;
   }
 
-  // Create the GLES2 helper, which writes the command buffer protocol.
+  // The GLES2 helper writes the command buffer protocol.
   gles2_helper_.reset(new gpu::gles2::GLES2CmdHelper(command_buffer_.get()));
+  gles2_helper_->SetAutomaticFlushes(automatic_flushes_);

[danakj, 2016/04/30 01:56:39]

I thot it was a bit weird that we set this after calling Initialize which
potentially makes it do more work, so I put this above Initialize so it's just
always true. It seems like this should be ok since Init calls AllocateRingBuffer
which does CalcImmediateEntries, and that's the only place this value is used.
And that's what SetAutomaticFlushes would do also after Initialize (tho here it
would just early out now so it's cheaper?) I also considered making it part of
the constructor. WDYT, is this ok?

   if (!gles2_helper_->Initialize(memory_limits.command_buffer_size)) {
-    LOG(ERROR) << "Failed to initialize GLES2CmdHelper.";
+    DLOG(ERROR) << "Failed to initialize GLES2CmdHelper.";

[danakj, 2016/04/30 01:56:39]

I turned all the LOGs into DLOGs which print when DCHECK_IS_ON so it's there on
the gpu bots.

     return false;
   }
 
-  if (!automatic_flushes_)
-    gles2_helper_->SetAutomaticFlushes(false);
-  // Create a transfer buffer used to copy resources between the renderer
+  // The transfer buffer is used to copy resources between the client
   // process and the GPU process.
   transfer_buffer_ .reset(new gpu::TransferBuffer(gles2_helper_.get()));
 
-  DCHECK(host_.get());
-
   const bool bind_generates_resource = attributes.bind_generates_resource;
   const bool lose_context_when_out_of_memory =
       attributes.lose_context_when_out_of_memory;
   const bool support_client_side_arrays = false;
 
-  // Create the object exposing the OpenGL API.
+  // The GLES2Implementation exposes the OpenGLES2 API, as well as the
+  // gpu::ContextSupport interface.
   real_gl_.reset(new gpu::gles2::GLES2Implementation(
       gles2_helper_.get(), std::move(share_group), transfer_buffer_.get(),
       bind_generates_resource, lose_context_when_out_of_memory,
@@ -180,10 +117,15 @@ bool WebGraphicsContext3DCommandBufferImpl::CreateContext(
                             memory_limits.min_transfer_buffer_size,
                             memory_limits.max_transfer_buffer_size,
                             memory_limits.mapped_memory_reclaim_limit)) {
-    LOG(ERROR) << "Failed to initialize GLES2Implementation.";
+    DLOG(ERROR) << "Failed to initialize GLES2Implementation.";
     return false;
   }
 
+  real_gl_->SetLostContextCallback(
+      base::Bind(&WebGraphicsContext3DCommandBufferImpl::OnContextLost,
+                 // The callback is unset in the destructor.
+                 base::Unretained(this)));
+  real_gl_->TraceBeginCHROMIUM("WebGraphicsContext3D", "CommandBufferContext");
   return true;
 }
 
@@ -194,35 +136,17 @@ bool WebGraphicsContext3DCommandBufferImpl::InitializeOnCurrentThread(
     const gpu::gles2::ContextCreationAttribHelper& attributes,
     command_buffer_metrics::ContextType context_type) {
   if (!MaybeInitializeGL(memory_limits, shared_command_buffer,
-                         std::move(share_group), attributes, context_type)) {
-    DLOG(ERROR) << "Failed to initialize context.";
+                         std::move(share_group), attributes, context_type))
     return false;
-  }
   if (gpu::error::IsError(command_buffer_->GetLastError())) {
-    LOG(ERROR) << "Context dead on arrival. Last error: "
-               << command_buffer_->GetLastError();
+    DLOG(ERROR) << "Context dead on arrival. Last error: "
+                << command_buffer_->GetLastError();
     return false;
   }
 
   return true;
 }
 
-void WebGraphicsContext3DCommandBufferImpl::Destroy() {
-  trace_gl_.reset();
-  real_gl_.reset();
-  transfer_buffer_.reset();
-  gles2_helper_.reset();
-  real_gl_.reset();
-  command_buffer_.reset();
-
-  host_ = nullptr;
-}
-
-gpu::ContextSupport*
-WebGraphicsContext3DCommandBufferImpl::GetContextSupport() {

[danakj, 2016/04/30 01:56:39]

This is just collatoral damage, as it's pointless since we return the
GLES2Implementation already. So just use that in the provider.

-  return real_gl_.get();
-}
-
 void WebGraphicsContext3DCommandBufferImpl::OnContextLost() {
   if (context_lost_callback_)
     context_lost_callback_->onContextLost();