Only use skia::RefPtr for refcounting

For consistency and sanity in Chromium, only use skia::RefPtr in Chromium to ref count skia classes. SkRefPtr is unsafe to use for newly created objects because it refs the object that is passed to its constructor. skia::RefPtr makes this adoption explicit it via skia::AdoptRef and so is much clearer. This patch also adds a skia::ShareRef function which makes it explicit that the callsite is adopting a ref which is already owned somewhere else. Using AdoptRef vs. ShareRef seems much clearer than using SkRefPtr vs. skia::RefPtr. These are the remaining code sites that use internal Skia reference counted classes. Once these have been removed, then we can use a PRESUBMIT rule to prevent new uses from being added. BUG=none Review URL: https://chromiumcodereview.appspot.com/15004024 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@200989 0039d316-1c4b-4281-b951-d872f2087c98
marcosholgado · May 18, 2013 · 584abe7 · 584abe7
1 parent 5a26074
commit 584abe7
Show file tree

Hide file tree

Showing 12 changed files with 187 additions and 121 deletions.
diff --git a/cc/cc_tests.gyp b/cc/cc_tests.gyp
@@ -160,6 +160,8 @@
       'test/render_pass_test_utils.h',
       'test/scheduler_test_common.cc',
       'test/scheduler_test_common.h',
+      'test/skia_common.cc',
+      'test/skia_common.h',
       'test/test_web_graphics_context_3d.cc',
       'test/test_web_graphics_context_3d.h',
       'test/tiled_layer_test_common.cc',

diff --git a/cc/resources/picture_pile_impl_unittest.cc b/cc/resources/picture_pile_impl_unittest.cc
@@ -4,7 +4,9 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "cc/test/fake_picture_pile_impl.h"
+#include "cc/test/skia_common.h"
 #include "skia/ext/lazy_pixel_ref.h"
+#include "skia/ext/refptr.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkPixelRef.h"
 #include "third_party/skia/include/core/SkShader.h"
@@ -13,61 +15,6 @@
 namespace cc {
 namespace {
 
-class TestPixelRef : public SkPixelRef {
- public:
-  // Pure virtual implementation.
-  TestPixelRef(int width, int height)
-    : pixels_(new char[4 * width * height]) {}
-  virtual SkFlattenable::Factory getFactory() OVERRIDE { return NULL; }
-  virtual void* onLockPixels(SkColorTable** color_table) OVERRIDE {
-      return pixels_.get();
-  }
-  virtual void onUnlockPixels() OVERRIDE {}
-  virtual SkPixelRef* deepCopy(
-      SkBitmap::Config config,
-      const SkIRect* subset) OVERRIDE {
-    this->ref();
-    return this;
-  }
- private:
-  scoped_ptr<char[]> pixels_;
-};
-
-class TestLazyPixelRef : public skia::LazyPixelRef {
- public:
-  // Pure virtual implementation.
-  TestLazyPixelRef(int width, int height)
-    : pixels_(new char[4 * width * height]) {}
-  virtual SkFlattenable::Factory getFactory() OVERRIDE { return NULL; }
-  virtual void* onLockPixels(SkColorTable** color_table) OVERRIDE {
-      return pixels_.get();
-  }
-  virtual void onUnlockPixels() OVERRIDE {}
-  virtual bool PrepareToDecode(const PrepareParams& params) OVERRIDE {
-    return true;
-  }
-  virtual SkPixelRef* deepCopy(
-      SkBitmap::Config config,
-      const SkIRect* subset) OVERRIDE {
-    this->ref();
-    return this;
-  }
-  virtual void Decode() OVERRIDE {}
- private:
-  scoped_ptr<char[]> pixels_;
-};
-
-void CreateBitmap(gfx::Size size, const char* uri, SkBitmap* bitmap) {
-  SkAutoTUnref<TestLazyPixelRef> lazy_pixel_ref;
-  lazy_pixel_ref.reset(new TestLazyPixelRef(size.width(), size.height()));
-  lazy_pixel_ref->setURI(uri);
-
-  bitmap->setConfig(SkBitmap::kARGB_8888_Config,
-                    size.width(),
-                    size.height());
-  bitmap->setPixelRef(lazy_pixel_ref);
-}
-
 TEST(PicturePileImplTest, AnalyzeIsSolidUnscaled) {
   gfx::Size tile_size(100, 100);
   gfx::Size layer_bounds(400, 400);

diff --git a/cc/resources/picture_unittest.cc b/cc/resources/picture_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "cc/test/fake_content_layer_client.h"
+#include "cc/test/skia_common.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkDevice.h"
@@ -19,55 +20,6 @@
 namespace cc {
 namespace {
 
-class TestLazyPixelRef : public skia::LazyPixelRef {
- public:
-  // Pure virtual implementation.
-  TestLazyPixelRef(int width, int height)
-    : pixels_(new char[4 * width * height]) {}
-  virtual SkFlattenable::Factory getFactory() OVERRIDE { return NULL; }
-  virtual void* onLockPixels(SkColorTable** color_table) OVERRIDE {
-      return pixels_.get();
-  }
-  virtual void onUnlockPixels() OVERRIDE {}
-  virtual bool PrepareToDecode(const PrepareParams& params) OVERRIDE {
-    return true;
-  }
-  virtual SkPixelRef* deepCopy(
-      SkBitmap::Config config,
-      const SkIRect* subset) OVERRIDE {
-    this->ref();
-    return this;
-  }
-  virtual void Decode() OVERRIDE {}
- private:
-  scoped_ptr<char[]> pixels_;
-};
-
-void DrawPicture(unsigned char* buffer,
-                 gfx::Rect layer_rect,
-                 scoped_refptr<Picture> picture) {
-  SkBitmap bitmap;
-  bitmap.setConfig(SkBitmap::kARGB_8888_Config,
-                   layer_rect.width(),
-                   layer_rect.height());
-  bitmap.setPixels(buffer);
-  SkDevice device(bitmap);
-  SkCanvas canvas(&device);
-  canvas.clipRect(gfx::RectToSkRect(layer_rect));
-  picture->Raster(&canvas, layer_rect, 1.0f, false);
-}
-
-void CreateBitmap(gfx::Size size, const char* uri, SkBitmap* bitmap) {
-  SkAutoTUnref<TestLazyPixelRef> lazy_pixel_ref;
-  lazy_pixel_ref.reset(new TestLazyPixelRef(size.width(), size.height()));
-  lazy_pixel_ref->setURI(uri);
-
-  bitmap->setConfig(SkBitmap::kARGB_8888_Config,
-                    size.width(),
-                    size.height());
-  bitmap->setPixelRef(lazy_pixel_ref);
-}
-
 TEST(PictureTest, AsBase64String) {
   SkGraphics::Init();
 

diff --git a/cc/test/skia_common.cc b/cc/test/skia_common.cc
@@ -0,0 +1,82 @@
+// Copyright 2012 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/test/skia_common.h"
+
+#include "cc/resources/picture.h"
+#include "skia/ext/refptr.h"
+#include "third_party/skia/include/core/SkDevice.h"
+#include "ui/gfx/rect.h"
+#include "ui/gfx/skia_util.h"
+
+namespace cc {
+
+TestPixelRef::TestPixelRef(int width, int height)
+    : pixels_(new char[4 * width * height]) {}
+
+TestPixelRef::~TestPixelRef() {}
+
+SkFlattenable::Factory TestPixelRef::getFactory() { return NULL; }
+
+void* TestPixelRef::onLockPixels(SkColorTable** color_table) {
+  return pixels_.get();
+}
+
+SkPixelRef* TestPixelRef::deepCopy(
+    SkBitmap::Config config,
+    const SkIRect* subset) {
+  this->ref();
+  return this;
+}
+
+
+TestLazyPixelRef::TestLazyPixelRef(int width, int height)
+    : pixels_(new char[4 * width * height]) {}
+
+TestLazyPixelRef::~TestLazyPixelRef() {}
+
+SkFlattenable::Factory TestLazyPixelRef::getFactory() { return NULL; }
+
+void* TestLazyPixelRef::onLockPixels(SkColorTable** color_table) {
+  return pixels_.get();
+}
+
+bool TestLazyPixelRef::PrepareToDecode(const PrepareParams& params) {
+  return true;
+}
+
+SkPixelRef* TestLazyPixelRef::deepCopy(
+    SkBitmap::Config config,
+    const SkIRect* subset) {
+  this->ref();
+  return this;
+}
+
+void DrawPicture(unsigned char* buffer,
+                 gfx::Rect layer_rect,
+                 scoped_refptr<Picture> picture) {
+  SkBitmap bitmap;
+  bitmap.setConfig(SkBitmap::kARGB_8888_Config,
+                   layer_rect.width(),
+                   layer_rect.height());
+  bitmap.setPixels(buffer);
+  SkDevice device(bitmap);
+  SkCanvas canvas(&device);
+  canvas.clipRect(gfx::RectToSkRect(layer_rect));
+  picture->Raster(&canvas, layer_rect, 1.0f, false);
+}
+
+void CreateBitmap(gfx::Size size, const char* uri, SkBitmap* bitmap) {
+  skia::RefPtr<TestLazyPixelRef> lazy_pixel_ref =
+      skia::AdoptRef(new TestLazyPixelRef(size.width(), size.height()));
+  lazy_pixel_ref->setURI(uri);
+
+  bitmap->setConfig(SkBitmap::kARGB_8888_Config,
+                    size.width(),
+                    size.height());
+  bitmap->setPixelRef(lazy_pixel_ref.get());
+}
+
+
+}  // namespace cc
diff --git a/cc/test/skia_common.h b/cc/test/skia_common.h
@@ -0,0 +1,62 @@
+// Copyright 2012 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.
+
+#ifndef CC_TEST_SKIA_COMMON_H_
+#define CC_TEST_SKIA_COMMON_H_
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "skia/ext/lazy_pixel_ref.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "third_party/skia/include/core/SkFlattenable.h"
+
+namespace gfx {
+class Rect;
+class Size;
+}
+
+namespace cc {
+class Picture;
+
+class TestPixelRef : public SkPixelRef {
+ public:
+  TestPixelRef(int width, int height);
+  virtual ~TestPixelRef();
+
+  virtual SkFlattenable::Factory getFactory() OVERRIDE;
+  virtual void* onLockPixels(SkColorTable** color_table) OVERRIDE;
+  virtual void onUnlockPixels() OVERRIDE {}
+  virtual SkPixelRef* deepCopy(
+      SkBitmap::Config config,
+      const SkIRect* subset) OVERRIDE;
+ private:
+  scoped_ptr<char[]> pixels_;
+};
+
+class TestLazyPixelRef : public skia::LazyPixelRef {
+ public:
+  TestLazyPixelRef(int width, int height);
+  virtual ~TestLazyPixelRef();
+
+  virtual SkFlattenable::Factory getFactory() OVERRIDE;
+  virtual void* onLockPixels(SkColorTable** color_table) OVERRIDE;
+  virtual void onUnlockPixels() OVERRIDE {}
+  virtual bool PrepareToDecode(const PrepareParams& params) OVERRIDE;
+  virtual SkPixelRef* deepCopy(
+      SkBitmap::Config config,
+      const SkIRect* subset) OVERRIDE;
+  virtual void Decode() OVERRIDE {}
+ private:
+  scoped_ptr<char[]> pixels_;
+};
+
+void DrawPicture(unsigned char* buffer,
+                 gfx::Rect layer_rect,
+                 scoped_refptr<Picture> picture);
+
+void CreateBitmap(gfx::Size size, const char* uri, SkBitmap* bitmap);
+
+}  // namespace cc
+
+#endif  // CC_TEST_SKIA_COMMON_H_
diff --git a/printing/pdf_metafile_skia.cc b/printing/pdf_metafile_skia.cc
@@ -10,6 +10,7 @@
 #include "base/metrics/histogram.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/safe_numerics.h"
+#include "skia/ext/refptr.h"
 #include "skia/ext/vector_platform_device_skia.h"
 #include "third_party/skia/include/core/SkData.h"
 #include "third_party/skia/include/core/SkRefCnt.h"
@@ -29,7 +30,7 @@
 namespace printing {
 
 struct PdfMetafileSkiaData {
-  SkRefPtr<SkPDFDevice> current_page_;
+  skia::RefPtr<SkPDFDevice> current_page_;
   SkPDFDocument pdf_doc_;
   SkDynamicMemoryWStream pdf_stream_;
 #if defined(OS_MACOSX)
@@ -63,9 +64,9 @@ SkDevice* PdfMetafileSkia::StartPageForVectorCanvas(
   SkISize pdf_page_size = SkISize::Make(page_size.width(), page_size.height());
   SkISize pdf_content_size =
       SkISize::Make(content_area.width(), content_area.height());
-  SkRefPtr<SkPDFDevice> pdf_device =
-      new skia::VectorPlatformDeviceSkia(pdf_page_size, pdf_content_size,
-                                         transform);
+  skia::RefPtr<SkPDFDevice> pdf_device =
+      skia::AdoptRef(new skia::VectorPlatformDeviceSkia(
+          pdf_page_size, pdf_content_size, transform));
   data_->current_page_ = pdf_device;
   return pdf_device.get();
 }
@@ -93,7 +94,7 @@ bool PdfMetafileSkia::FinishDocument() {
   if (page_outstanding_)
     FinishPage();
 
-  data_->current_page_ = NULL;
+  data_->current_page_.clear();
 
   int font_counts[SkAdvancedTypefaceMetrics::kNotEmbeddable_Font + 1];
   data_->pdf_doc_.getCountOfFontTypes(font_counts);

diff --git a/skia/ext/refptr.h b/skia/ext/refptr.h
@@ -13,7 +13,7 @@ namespace skia {
 // this class to avoid dealing with the ref-counting and prevent leaks/crashes
 // due to ref-counting bugs.
 //
-// Example of Creating an SkShader* and setting it on a SkPaint:
+// Example of creating a new SkShader* and setting it on a SkPaint:
 //   skia::RefPtr<SkShader> shader = skia::AdoptRef(SkGradientShader::Create());
 //   paint.setShader(shader.get());
 //
@@ -25,12 +25,18 @@ namespace skia {
 //   }
 //   skia::RefPtr<SkShader> member_refptr_;
 //
-// When returning a ref-counted ponter, also return the skia::RefPtr instead. An
-// example method that creates an SkShader* and returns it:
+// When returning a ref-counted pointer, also return the skia::RefPtr instead.
+// An example method that creates an SkShader* and returns it:
 //   skia::RefPtr<SkShader> MakeAShader() {
 //     return skia::AdoptRef(SkGradientShader::Create());
 //   }
 //
+// To take a scoped reference to an object whose references are all owned
+// by other objects (i.e. does not have one that needs to be adopted) use the
+// skia::SharePtr helper:
+//
+//   skia::RefPtr<SkShader> shader = skia::SharePtr(paint.getShader());
+//
 // Never call ref() or unref() on the underlying ref-counted pointer. If you
 // AdoptRef() the raw pointer immediately into a skia::RefPtr and always work
 // with skia::RefPtr instances instead, the ref-counting will be taken care of
@@ -84,15 +90,29 @@ class RefPtr {
  private:
   T* ptr_;
 
+  // This function cannot be public because Skia starts its ref-counted
+  // objects at refcnt=1.  This makes it impossible to differentiate
+  // between a newly created object (that doesn't need to be ref'd) or an
+  // already existing object with one owner (that does need to be ref'd so that
+  // this RefPtr can also manage its lifetime).
   explicit RefPtr(T* ptr) : ptr_(ptr) {}
 
   template<typename U>
   friend RefPtr<U> AdoptRef(U* ptr);
+
+  template<typename U>
+  friend RefPtr<U> SharePtr(U* ptr);
 };
 
+// For objects that have an unowned reference (such as newly created objects).
 template<typename T>
 RefPtr<T> AdoptRef(T* ptr) { return RefPtr<T>(ptr); }
 
+// For objects that are already owned. This doesn't take ownership of existing
+// references and adds a new one.
+template<typename T>
+RefPtr<T> SharePtr(T* ptr) { return RefPtr<T>(SkSafeRef(ptr)); }
+
 }  // namespace skia
 
 #endif  // SKIA_EXT_REFPTR_H_
diff --git a/ui/gfx/render_text.cc b/ui/gfx/render_text.cc
@@ -224,7 +224,7 @@ void SkiaTextRenderer::DrawPosText(const SkPoint* pos,
     if (!paint_.isLCDRenderText() &&
         paint_.getShader() &&
         !paint_.getLooper()) {
-      deferred_fade_shader_ = paint_.getShader();
+      deferred_fade_shader_ = skia::SharePtr(paint_.getShader());
       paint_.setShader(NULL);
       canvas_skia_->saveLayer(&bounds_, NULL);
     }