cc: Move RasterBuffer implementations from ResourceProvider to RasterWorkerPool implementations.

cc/resources/gpu_raster_worker_pool.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/resources/gpu_raster_worker_pool.h"
 
 #include "base/debug/trace_event.h"
 #include "cc/output/context_provider.h"
+#include "cc/resources/raster_buffer.h"
 #include "cc/resources/resource.h"
 #include "cc/resources/resource_provider.h"
 #include "cc/resources/scoped_gpu_raster.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
+#include "third_party/skia/include/core/SkSurface.h"
 #include "third_party/skia/include/gpu/GrContext.h"
+#include "third_party/skia/include/utils/SkNullCanvas.h"
 
 namespace cc {
+namespace {
+
+class RasterBufferImpl : public RasterBuffer {

[vmpstr, 2014/09/16 22:15:55]

<nit>
Can we come up with a name that isn't using Impl somehow. I know this is a more
proper way of naming this type of class, but given that we have the whole
impl-thread thing, I think this makes it somewhat confusing. Maybe
ScopedRasterBuffer, since it locks/unlocks things in the ctor/dtor (or
LockedRasterBuffer, RasterBufferAutoLock)?

I think we should also file a bug and/or decide whether we allow FooImpl in cc
as an implementation class of Foo, or whether things like PictureLayerImpl have
to get a better name
</nit>

[reveman, 2014/09/17 14:40:56]

RasterBufferImpl is a more compact way of writing
GpuRasterWorkerPoolRasterBuffer in this case. We could just name it that,
although I find it a bit annoying that we can't use the "Impl" suffix in a way
that's consistent with the rest of the chromium code.
I don't think PictureLayerImpl is too bad. Yes, there's no explicit PictureLayer
interface but it's in some sense implementing a "picture layer".

We could instead use PictureLayerHost on the main thread and PictureLayer on the
compositor thread. That would probably be a bit more consistent with the rest of
chromium but LayerTreeHostImpl would become a bit confusing. Not sure what's the
best solution is. I think we should wait until all non impl-side painting code
has been removed before we make any changes like this though.

+ public:
+  RasterBufferImpl(ResourceProvider* resource_provider,
+                   const Resource* resource)
+      : resource_provider_(resource_provider),
+        resource_(resource),
+        surface_(resource_provider->LockForWriteToSkSurface(resource->id())) {}
+  virtual ~RasterBufferImpl() {
+    resource_provider_->UnlockForWriteToSkSurface(resource_->id());
+  }
+
+  // Overridden from RasterBuffer:
+  virtual skia::RefPtr<SkCanvas> AcquireSkCanvas() OVERRIDE {
+    skia::RefPtr<SkCanvas> canvas = surface_
+                                        ? skia::SharePtr(surface_->getCanvas())
+                                        : skia::AdoptRef(SkCreateNullCanvas());

[vmpstr, 2014/09/16 22:15:55]

The previous code did "return skia::RefPtr<SkCanvas>();" type of thing. Is this
equivalent? That is to say, if the return value was used in DCHECKs or ifs,
would it evaluate to the same thing?

[reveman, 2014/09/17 14:40:56]

The return value couldn't be used in DCHECKs as we need to be able to handle
failures here. It will happen in situations where we've lost the context.

In the past we had to conditionally skip raster, upload, etc in context lost
situations. By handling this here and still returning a valid canvas the
difference in code paths for context lost situations is reduced and that's in my
experience very valuable as significantly different code paths for lost context
handling often end up with bugs as they are never as well tested as the normal
code paths.

+
+    // Balanced with restore() call in ReleaseSkCanvas. save()/restore() calls
+    // are needed to ensure that canvas returns to its previous state after use.
+    canvas->save();
+    return canvas;
+  }
+  virtual void ReleaseSkCanvas(const skia::RefPtr<SkCanvas>& canvas) OVERRIDE {
+    // Balanced with save() call in AcquireSkCanvas.
+    canvas->restore();
+  }
+
+ private:
+  ResourceProvider* resource_provider_;
+  const Resource* resource_;
+  SkSurface* surface_;
+
+  DISALLOW_COPY_AND_ASSIGN(RasterBufferImpl);
+};
+
+}  // namespace
 
 // static
 scoped_ptr<RasterWorkerPool> GpuRasterWorkerPool::Create(
     base::SequencedTaskRunner* task_runner,
     ContextProvider* context_provider,
     ResourceProvider* resource_provider) {
   return make_scoped_ptr<RasterWorkerPool>(new GpuRasterWorkerPool(
       task_runner, context_provider, resource_provider));
 }
 
@@ skipping 110 matching lines @@
 
     task->WillComplete();
     task->CompleteOnOriginThread(this);
     task->DidComplete();
 
     task->RunReplyOnOriginThread();
   }
   completed_tasks_.clear();
 }
 
-RasterBuffer* GpuRasterWorkerPool::AcquireBufferForRaster(RasterTask* task) {
-  return resource_provider_->AcquireGpuRasterBuffer(task->resource()->id());
+scoped_ptr<RasterBuffer> GpuRasterWorkerPool::AcquireBufferForRaster(
+    const Resource* resource) {
+  return make_scoped_ptr<RasterBuffer>(
+      new RasterBufferImpl(resource_provider_, resource));
 }
 
-void GpuRasterWorkerPool::ReleaseBufferForRaster(RasterTask* task) {
-  resource_provider_->ReleaseGpuRasterBuffer(task->resource()->id());
+void GpuRasterWorkerPool::ReleaseBufferForRaster(
+    scoped_ptr<RasterBuffer> buffer) {
+  // Nothing to do here. RasterBufferImpl destructor cleans up after itself.
 }
 
 void GpuRasterWorkerPool::OnRasterFinished() {
   TRACE_EVENT0("cc", "GpuRasterWorkerPool::OnRasterFinished");
 
   DCHECK(raster_tasks_pending_);
   raster_tasks_pending_ = false;
   client_->DidFinishRunningTasks();
 }
 
@@ skipping 21 matching lines @@
   TRACE_EVENT0("cc", "GpuRasterWorkerPool::RunTasksOnOriginThread");
 
   DCHECK(run_tasks_on_origin_thread_pending_);
   run_tasks_on_origin_thread_pending_ = false;
 
   ScopedGpuRaster gpu_raster(context_provider_);
   task_graph_runner_->RunUntilIdle();
 }
 
 }  // namespace cc