Only use skia::RefPtr for refcounting

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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200989

[enne (OOO), 2013-05-17 00:47:44]

danakj: for cc/ and webkit/renderer/compositor/bindings/

vandebo: for skia/ext/ and printing/

piman: for webkit/plugins/ppapi/

xji: for ui/gfx/render_text.*

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

LGTM+nit

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h#newcode108
skia/ext/refptr.h:108: // For pointers that are already ref'd by at least one
other object.
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).

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

It's worth noting that Skia no long uses SkRefPtr.  With this CL, the last uses
of it will be removed and SkRefPtr can get deleted.  In Skia it has generally
been replaced by instances of SkAutoTUnref.  If SkAUtoTUnref has sufficient or
close to sufficient functionality, it might be better to replace skia::RefPtr
with it instead.

https://codereview.chromium.org/15004024/diff/1/printing/pdf_metafile_skia.cc
File printing/pdf_metafile_skia.cc (right):

https://codereview.chromium.org/15004024/diff/1/printing/pdf_metafile_skia.cc...
printing/pdf_metafile_skia.cc:33: skia::RefPtr<SkPDFDevice> current_page_;
SkAutoTUnref would also work here.

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h#newcode34
skia/ext/refptr.h:34: // If a Skia API passes a raw pointer that happens to be
owned by Skia and
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.

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h#newcode110
skia/ext/refptr.h:110: RefPtr<T> ShareRef(T* ptr) { return
RefPtr<T>(SkSafeRef(ptr)); }
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:33]

On 2013/05/17 01:12:30, vandebo wrote:
> It's worth noting that Skia no long uses SkRefPtr.  With this CL, the last
uses
> of it will be removed and SkRefPtr can get deleted.  In Skia it has generally
> been replaced by instances of SkAutoTUnref.  If SkAUtoTUnref has sufficient or
> close to sufficient functionality, it might be better to replace skia::RefPtr
> with it instead.

In my opinion, SkAutoTUnref is a disaster waiting to happen because it has an
implicit conversion operator to T*.

Additionally, isn't SkAutoTUnref supposed to be the equivalent of scoped_ptr,
where it's attempting to be a single ownership model rather than ref counting?
How is that a replacement?

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h#newcode34
skia/ext/refptr.h:34: // If a Skia API passes a raw pointer that happens to be
owned by Skia and
On 2013/05/17 01:12:31, vandebo wrote:
> 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.

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.

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h#newcode108
skia/ext/refptr.h:108: // For pointers that are already ref'd by at least one
other object.
On 2013/05/17 01:04:20, piman wrote:
> 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).

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

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h#newcode110
skia/ext/refptr.h:110: RefPtr<T> ShareRef(T* ptr) { return
RefPtr<T>(SkSafeRef(ptr)); }
On 2013/05/17 01:12:31, vandebo wrote:
> 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.

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.

[enne (OOO), 2013-05-17 01:24:42]

On 2013/05/17 01:23:33, enne wrote:
> Skia starts its reference counting at ref count of 1, it is 
> impossible to differentiate

...from a raw pointer, I mean.

[piman, 2013-05-17 01:42:13]

https://codereview.chromium.org/15004024/diff/1011/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/15004024/diff/1011/skia/ext/refptr.h#newcode108
skia/ext/refptr.h:108: // For pointers that are already ref'd by at least one
other object.
how about; "For pointers already owned. Doesn't take ownership of existing
references."

[danakj, 2013-05-17 13:52:54]

On Thu, May 16, 2013 at 9:12 PM, <vandebo@chromium.org> wrote:

> It's worth noting that Skia no long uses SkRefPtr.  With this CL, the last
> uses
> of it will be removed and SkRefPtr can get deleted.  In Skia it has
> generally
> been replaced by instances of SkAutoTUnref.  If SkAUtoTUnref has
> sufficient or
> close to sufficient functionality, it might be better to replace
> skia::RefPtr
> with it instead.
>

You can see https://codereview.appspot.com/6849109/ for all the context on
why we have skia::RefPtr as well as SkAutoTUnref. skia::RefPtr is safer,
and thus preferred.

[danakj, 2013-05-17 13:56:55]

LGTM

https://codereview.chromium.org/15004024/diff/1011/cc/resources/picture_pile_...
File cc/resources/picture_pile_impl_unittest.cc (right):

https://codereview.chromium.org/15004024/diff/1011/cc/resources/picture_pile_...
cc/resources/picture_pile_impl_unittest.cc:9: #include "skia/ext/refptr.h"
unused?

https://codereview.chromium.org/15004024/diff/1011/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/15004024/diff/1011/skia/ext/refptr.h#newcode108
skia/ext/refptr.h:108: // For pointers that are already ref'd by at least one
other object.
On 2013/05/17 01:42:13, piman wrote:
> how about; "For pointers already owned. Doesn't take ownership of existing
> references."

+1

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

Note, I think this is a good change and I'm glad you're cleaning up the
remaining bits of the SkRefPtr mess. I'm only being a stickler for the nuances
because this is a somewhat subtle issue. Better precision in the names and
comments here is more likely to yield the right result down the road.

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h#newcode110
skia/ext/refptr.h:110: RefPtr<T> ShareRef(T* ptr) { return
RefPtr<T>(SkSafeRef(ptr)); }
On 2013/05/17 01:23:34, enne wrote:
> On 2013/05/17 01:12:31, vandebo wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/15004024/diff/12002/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/15004024/diff/12002/skia/ext/refptr.h#newcode34
skia/ext/refptr.h:34: // If a Skia API returns a raw pointer that already has an
owner, then use
The object under consideration is reference counted, it does not have a single
owner.  ShareRef does not enable sharing of references, it creates a new
reference.  Suggestion:

To take a scoped reference to an object which does not have a unowned reference
i.e. has all its references owned by other objects, use the "ShareRef" helper.

https://codereview.chromium.org/15004024/diff/12002/skia/ext/refptr.h#newcode94
skia/ext/refptr.h:94: // already existing object with one owner (that does to be
ref'd so that
typo: ...that does *need* to be ref'd...

https://codereview.chromium.org/15004024/diff/12002/skia/ext/refptr.h#newcode105
skia/ext/refptr.h:105: // For newly created raw pointers without any owners.
Technically the object doesn't need to be newly created.  It can be the result
of a function that returns a new reference.  Suggestion: For objects that have
an unowned reference (like newly create objects).

https://codereview.chromium.org/15004024/diff/12002/skia/ext/refptr.h#newcode109
skia/ext/refptr.h:109: // For pointers already owned. Doesn't take ownership of
existing references.
Suggestion: For objects that do not have unowned references (will ref the
object).

[enne (OOO), 2013-05-17 21:37:01]

vandebo: Thanks for all the feedback.  This is clearly a confusing bit of code,
so I appreciate trying to get the wording and naming to be more precise.  :)

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h#newcode110
skia/ext/refptr.h:110: RefPtr<T> ShareRef(T* ptr) { return
RefPtr<T>(SkSafeRef(ptr)); }
On 2013/05/17 20:47:49, vandebo wrote:
> On 2013/05/17 01:23:34, enne wrote:
> > On 2013/05/17 01:12:31, vandebo wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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?

https://codereview.chromium.org/15004024/diff/12002/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/15004024/diff/12002/skia/ext/refptr.h#newcode34
skia/ext/refptr.h:34: // If a Skia API returns a raw pointer that already has an
owner, then use
On 2013/05/17 20:47:49, vandebo wrote:
> The object under consideration is reference counted, it does not have a single
> owner.  ShareRef does not enable sharing of references, it creates a new
> reference.  Suggestion:
> 
> To take a scoped reference to an object which does not have a unowned
reference
> i.e. has all its references owned by other objects, use the "ShareRef" helper.

Incorporated most of that wording.

https://codereview.chromium.org/15004024/diff/12002/skia/ext/refptr.h#newcode94
skia/ext/refptr.h:94: // already existing object with one owner (that does to be
ref'd so that
On 2013/05/17 20:47:49, vandebo wrote:
> typo: ...that does *need* to be ref'd...

Done.

https://codereview.chromium.org/15004024/diff/12002/skia/ext/refptr.h#newcode105
skia/ext/refptr.h:105: // For newly created raw pointers without any owners.
On 2013/05/17 20:47:49, vandebo wrote:
> Technically the object doesn't need to be newly created.  It can be the result
> of a function that returns a new reference.  Suggestion: For objects that have
> an unowned reference (like newly create objects).

Done.

https://codereview.chromium.org/15004024/diff/12002/skia/ext/refptr.h#newcode109
skia/ext/refptr.h:109: // For pointers already owned. Doesn't take ownership of
existing references.
On 2013/05/17 20:47:49, vandebo wrote:
> Suggestion: For objects that do not have unowned references (will ref the
> object).

Incorporated that wording into this comment.

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

LGTM

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h
File skia/ext/refptr.h (right):

https://codereview.chromium.org/15004024/diff/1/skia/ext/refptr.h#newcode110
skia/ext/refptr.h:110: RefPtr<T> ShareRef(T* ptr) { return
RefPtr<T>(SkSafeRef(ptr)); }
On 2013/05/17 21:37:02, enne wrote:
> On 2013/05/17 20:47:49, vandebo wrote:
> > On 2013/05/17 01:23:34, enne wrote:
> > > On 2013/05/17 01:12:31, vandebo wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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 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.
 
> 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

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.

> change skia::AdoptRef to skia::AdoptPtr as well, or should that stay as
> skia::AdoptRef?

I think it should stay AdoptRef, since that's what it is doing. ;-)

[enne (OOO), 2013-05-18 00:11:52]

msw / xji: ui/gfx/render_text OWNERS review?

[msw, 2013-05-18 00:44:24]

ui/gfx/render_text.* LGTM

[commit-bot: I haz the power, 2013-05-18 02:47:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/15004024/18001

[commit-bot: I haz the power, 2013-05-18 09:40:27]

Change committed as 200989