Only use skia::RefPtr for refcounting

skia/ext/refptr.h

Index: skia/ext/refptr.h
diff --git a/skia/ext/refptr.h b/skia/ext/refptr.h
index 514d453e034d69ccd10b96de8bb3a7f8666a93c0..660dc0a722d1d45fb8c356e1a1058181cb43061a 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,16 @@ 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());
 //   }
 //
+// If a Skia API passes a raw pointer that happens to be owned by Skia and

[vandebo (ex-Chrome), 2013/05/17 01:12:31]

Why is it important that it's owned by Skia?  The important point here is that
you want to take a new reference to the object and put it into the RefPtr.

[enne (OOO), 2013/05/17 01:23:34]

Skia starts its reference counting at ref count of 1, it is impossible to
differentiate between the case where a new object has been created (and needs to
be put in a ref ptr without being ref'd) and a case where this has already
happened (and so needs to be put in a ref ptr and be ref'd.)

The "owned by Skia" language is because of this and to differentiate from the
comments above in this file that recommend never passing raw pointers to Skia
refcounted objects because this difference is not clear.

+// already reference counted, then use ShareRef:
+//   skia::RefPtr<SkShader> shader = ShareRef(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 +88,27 @@ 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 a ref) or an
+  // already existing object with one owner (that does need a ref).
   explicit RefPtr(T* ptr) : ptr_(ptr) {}
 
   template<typename U>
   friend RefPtr<U> AdoptRef(U* ptr);
+
+  template<typename U>
+  friend RefPtr<U> ShareRef(U* ptr);
 };
 
+// For newly created raw pointers.
 template<typename T>
 RefPtr<T> AdoptRef(T* ptr) { return RefPtr<T>(ptr); }
 
+// For pointers that are already ref'd by at least one other object.

[piman, 2013/05/17 01:04:20]

nit: the description threw me off. I guess what matters is not that the pointer
is already ref'd (I read that as this class is responsible for releasing the
existing ref, which is the opposite of what it does), but that the pointer is
already fully owned. ShareRef is "reference neutral" in that it will release
exactly all refs that it takes (as opposed to AdoptRef which will release one
more).

[enne (OOO), 2013/05/17 01:23:34]

Do you have a suggestion for a better succinct comment to put here?

+template<typename T>
+RefPtr<T> ShareRef(T* ptr) { return RefPtr<T>(SkSafeRef(ptr)); }

[vandebo (ex-Chrome), 2013/05/17 01:12:31]

I'm not sure that ShareRef is the right name for this. ShareRef implies that
that the reference is shared some how. NewRef or AddRef seem like better names.

[enne (OOO), 2013/05/17 01:23:34]

It is shared.  As opposed to AdoptRef (where the caller is adopting the only
reference of a newly created object), ShareRef is "sharing" ptr with some
another object that has also ref'd it.

[vandebo (ex-Chrome), 2013/05/17 20:47:49]

It is sharing the object, the same way shared_ptr does, but it's not sharing the
reference, which is what "ShareRef" implies to me.  I agree that AdoptRef is
taking ownership of a currently unowned reference.  In this case though, you are
creating a new reference and taking ownership of it - there is no sharing (of
references) going on.  I previously suggested NewRef and AddRef, another option
is CreateRef, or I guess ShareObj, or SharePtr.

[enne (OOO), 2013/05/17 21:37:02]

I dislike NewRef, AddRef, and CreateRef because I think that's an implementation
detail.  I suspect many people seeing AdoptRef in a line of code would suspect
that it is also adding a ref, but it's an implementation detail that it actually
doesn't need to.

I'd really rather stick with a name that describes usage and not implementation.
 For instance, I like "adopt" quite a bit, because the usage metaphor is that
objects without any owners need to be adopted.  It also matches WebKit's
adoptPtr/adoptOwnPtr usage.

Between SharePtr and ShareObj, I like SharePtr better.  Would you like that I
change skia::AdoptRef to skia::AdoptPtr as well, or should that stay as
skia::AdoptRef?

[vandebo (ex-Chrome), 2013/05/18 00:06:02]

I disagree on both counts. I don't think it's an implementation detail, I think
it's the aim of the class to manage references and the caller should be fully
aware of the basic concepts.  AdoptRef is clearly reusing an existing reference.
 
Of course, you picked my least favorite option. I think this is the least
obvious name for what is happening of the names I suggested.  As I said above, I
think the caller should be aware of what's conceptually going on.
I think it should stay AdoptRef, since that's what it is doing. ;-)

+
 }  // namespace skia
 
 #endif  // SKIA_EXT_REFPTR_H_