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

12 files changed, 187 insertions, 121 deletions

diff --git a/cc/cc_tests.gyp b/cc/cc_tests.gyp
index 7936688b..dc3d267 100644
--- 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
index 0fde59e..10c859e 100644
--- 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
index c5ddde2..7d92582 100644
--- 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
new file mode 100644
index 0000000..d3d92d8
--- /dev/null
+++ 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
new file mode 100644
index 0000000..4d82b76
--- /dev/null
+++ 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
index e4027ed..258ed90 100644
--- 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
index 514d453..a3900f6 100644
--- 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
index abfca90..a7a1911 100644
--- 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);
     }
diff --git a/ui/gfx/render_text.h b/ui/gfx/render_text.h
index 28739e00..1c7d3fc 100644
--- a/ui/gfx/render_text.h
+++ b/ui/gfx/render_text.h
@@ -14,6 +14,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/i18n/rtl.h"
 #include "base/string16.h"
+#include "skia/ext/refptr.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkPaint.h"
 #include "third_party/skia/include/core/SkRect.h"
@@ -76,7 +77,7 @@ class SkiaTextRenderer {
   bool started_drawing_;
   SkPaint paint_;
   SkRect bounds_;
-  SkRefPtr<SkShader> deferred_fade_shader_;
+  skia::RefPtr<SkShader> deferred_fade_shader_;
   SkScalar underline_thickness_;
   SkScalar underline_position_;
 
diff --git a/webkit/plugins/ppapi/ppapi_plugin_instance.cc b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
index 0bd76f4..9368e29 100644
--- a/webkit/plugins/ppapi/ppapi_plugin_instance.cc
+++ b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
@@ -1399,7 +1399,7 @@ int PluginInstance::PrintBegin(const WebPrintParams& print_params) {
   if (!num_pages)
     return 0;
   current_print_settings_ = print_settings;
-  canvas_ = NULL;
+  canvas_.clear();
   ranges_.clear();
   return num_pages;
 }
@@ -1417,7 +1417,7 @@ bool PluginInstance::PrintPage(int page_number, WebKit::WebCanvas* canvas) {
 #endif
   if (save_for_later) {
     ranges_.push_back(page_range);
-    canvas_ = canvas;
+    canvas_ = skia::SharePtr(canvas);
     return true;
   } else {
     return PrintPageHelper(&page_range, 1, canvas);
@@ -1456,7 +1456,7 @@ void PluginInstance::PrintEnd() {
   scoped_refptr<PluginInstance> ref(this);
   if (!ranges_.empty())
     PrintPageHelper(&(ranges_.front()), ranges_.size(), canvas_.get());
-  canvas_ = NULL;
+  canvas_.clear();
   ranges_.clear();
 
   DCHECK(plugin_print_interface_);
diff --git a/webkit/plugins/ppapi/ppapi_plugin_instance.h b/webkit/plugins/ppapi/ppapi_plugin_instance.h
index ae1da17..07f3657 100644
--- a/webkit/plugins/ppapi/ppapi_plugin_instance.h
+++ b/webkit/plugins/ppapi/ppapi_plugin_instance.h
@@ -44,13 +44,13 @@
 #include "ppapi/shared_impl/tracked_callback.h"
 #include "ppapi/thunk/ppb_gamepad_api.h"
 #include "ppapi/thunk/resource_creation_api.h"
+#include "skia/ext/refptr.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebCanvas.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebString.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebURLLoaderClient.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebURLResponse.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebPlugin.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebUserGestureToken.h"
-#include "third_party/skia/include/core/SkRefCnt.h"
 #include "ui/base/ime/text_input_type.h"
 #include "ui/gfx/rect.h"
 #include "webkit/plugins/ppapi/plugin_delegate.h"
@@ -733,7 +733,7 @@ class WEBKIT_PLUGINS_EXPORT PluginInstance :
   // to generate the entire PDF given the variables below:
   //
   // The most recently used WebCanvas, guaranteed to be valid.
-  SkRefPtr<WebKit::WebCanvas> canvas_;
+  skia::RefPtr<WebKit::WebCanvas> canvas_;
   // An array of page ranges.
   std::vector<PP_PrintPageNumberRange_Dev> ranges_;
 
diff --git a/webkit/renderer/compositor_bindings/web_layer_impl.cc b/webkit/renderer/compositor_bindings/web_layer_impl.cc
index 564a846..6c2e65f 100644
--- a/webkit/renderer/compositor_bindings/web_layer_impl.cc
+++ b/webkit/renderer/compositor_bindings/web_layer_impl.cc
@@ -162,8 +162,7 @@ void WebLayerImpl::setBackgroundFilters(const WebFilterOperations& filters) {
 }
 
 void WebLayerImpl::setFilter(SkImageFilter* filter) {
-  SkSafeRef(filter);  // Claim a reference for the compositor.
-  layer_->SetFilter(skia::AdoptRef(filter));
+  layer_->SetFilter(skia::SharePtr(filter));
 }
 
 void WebLayerImpl::setDebugName(WebKit::WebString name) {