cc: Move RasterBuffer implementations from ResourceProvider to RasterWorkerPool implementations.

cc/resources/gpu_raster_worker_pool.cc

Index: cc/resources/gpu_raster_worker_pool.cc
diff --git a/cc/resources/gpu_raster_worker_pool.cc b/cc/resources/gpu_raster_worker_pool.cc
index 8dc5adcfe04c7d48f29cb72e8461f34b9ad9237e..3003d03bc7d951ff20ec6666344501c645b53f98 100644
--- a/cc/resources/gpu_raster_worker_pool.cc
+++ b/cc/resources/gpu_raster_worker_pool.cc
@@ -6,13 +6,54 @@
 
 #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(
@@ -143,12 +184,15 @@ void GpuRasterWorkerPool::CheckForCompletedTasks() {
   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() {