Allow passing ownership with SkAutoTUnref

Allow passing ownership with SkAutoTUnref

In the past, SkAutoTUnref would take ownership of a ref-counted
object, and in order to pass ownership elsewhere, you would pull the
raw pointer out of the SkAutoTUnref, and pass that around. This led
to requiring manual ->ref() calls still when you wanted to share the
reference with the callsite.

Now SkAutoTUnref includes a copy constructor and operator= that
allow it to pass a ref-counted object into another SkAutoTUnref, and
keep a reference to the underlying object in both SkAutoTUnref
instances. Then, when both instances are destroyed, the underlying
object is as well.

[Stephen White, 2012-11-27 01:52:21]

https://codereview.appspot.com/6849109/diff/1/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.appspot.com/6849109/diff/1/include/core/SkRefCnt.h#newcode249
include/core/SkRefCnt.h:249: static SkRefPtr<T> StealRef(T* obj) {
Bikeshed:  Couldn't we just call this AdoptRef?

https://codereview.appspot.com/6849109/diff/1/include/core/SkRefCnt.h#newcode266
include/core/SkRefCnt.h:266: template<typename T> static inline SkRefPtr<T>
SkAdoptAndIncrementRef(T* obj) {
This one seems really ungainly.  You're not actually adopting in this case. 
This sounds more like SkSharedCustody() to me.  :)

Since there are still some callers which need these semantics, perhaps we should
just leave the original constructor as-is, and get rid of this flavour. 
SkRefPtr<T>(foo) does sounds like you're ref'ing it.

[danakj, 2012-11-27 02:40:13]

https://codereview.appspot.com/6849109/diff/1/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.appspot.com/6849109/diff/1/include/core/SkRefCnt.h#newcode249
include/core/SkRefCnt.h:249: static SkRefPtr<T> StealRef(T* obj) {
On 2012/11/27 01:52:22, Stephen White wrote:
> Bikeshed:  Couldn't we just call this AdoptRef?

Sure.

https://codereview.appspot.com/6849109/diff/1/include/core/SkRefCnt.h#newcode266
include/core/SkRefCnt.h:266: template<typename T> static inline SkRefPtr<T>
SkAdoptAndIncrementRef(T* obj) {
On 2012/11/27 01:52:22, Stephen White wrote:
> This one seems really ungainly.  You're not actually adopting in this case. 
> This sounds more like SkSharedCustody() to me.  :)

Haha, yes :)

> Since there are still some callers which need these semantics, perhaps we
should
> just leave the original constructor as-is, and get rid of this flavour. 
> SkRefPtr<T>(foo) does sounds like you're ref'ing it.

I dislike using the constructor because most of the places where its getting
used are in other constructor locations, and then you just have

Construtor(Pointer *p)
  : fP(p) {
}

And what about that says there was a reference increment?

I also prefer this because it makes new code think about it a bit more. The
constructor looks like it's the default/normal path. But it should be an
exception (eventually this thing should probably just disappear).

I'd rather keep this method, but I agree the name is a bit fuzzy. I'm having
trouble inventing something much better though..

SkIncrementAndMakeRefPtr()?
SkIncrementAndAdoptRef() is it better in a different order?

[danakj, 2012-11-27 03:18:07]

On 2012/11/27 02:40:13, danakj wrote:
> https://codereview.appspot.com/6849109/diff/1/include/core/SkRefCnt.h
> File include/core/SkRefCnt.h (right):
> 
>
https://codereview.appspot.com/6849109/diff/1/include/core/SkRefCnt.h#newcode249
> include/core/SkRefCnt.h:249: static SkRefPtr<T> StealRef(T* obj) {
> On 2012/11/27 01:52:22, Stephen White wrote:
> > Bikeshed:  Couldn't we just call this AdoptRef?
> 
> Sure.
> 
>
https://codereview.appspot.com/6849109/diff/1/include/core/SkRefCnt.h#newcode266
> include/core/SkRefCnt.h:266: template<typename T> static inline SkRefPtr<T>
> SkAdoptAndIncrementRef(T* obj) {
> On 2012/11/27 01:52:22, Stephen White wrote:
> > This one seems really ungainly.  You're not actually adopting in this case. 
> > This sounds more like SkSharedCustody() to me.  :)
> 
> Haha, yes :)
> 
> > Since there are still some callers which need these semantics, perhaps we
> should
> > just leave the original constructor as-is, and get rid of this flavour. 
> > SkRefPtr<T>(foo) does sounds like you're ref'ing it.
> 
> I dislike using the constructor because most of the places where its getting
> used are in other constructor locations, and then you just have
> 
> Construtor(Pointer *p)
>   : fP(p) {
> }
> 
> And what about that says there was a reference increment?
> 
> I also prefer this because it makes new code think about it a bit more. The
> constructor looks like it's the default/normal path. But it should be an
> exception (eventually this thing should probably just disappear).
> 
> I'd rather keep this method, but I agree the name is a bit fuzzy. I'm having
> trouble inventing something much better though..
> 
> SkIncrementAndMakeRefPtr()?
> SkIncrementAndAdoptRef() is it better in a different order?

I could also just remove it and have call-sites ref() before SkAdoptRef() in
these cases.

[reed1, 2012-11-27 14:06:01]

https://codereview.appspot.com/6849109/diff/4001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.appspot.com/6849109/diff/4001/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:258: 
Lets keep these in the SkRefPtr scope for now, since we have other ref counting
helpers, and SkAdoptRef is pretty generic sounding.

https://codereview.appspot.com/6849109/diff/4001/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:264: 
This one is wacky, since it is exactly what the old constructor did. Do we
really need it instead of the constructor? If so, lets move it into the class
scope.

[reed1, 2012-11-27 14:07:26]

Many many of the changes to the .cpp files could be done with our existing
SkAutoTUnref, as we do in lots of other skia .cpp files. Do we really need to
use/update SkRefPtr for these scoped use cases?

[TomH, 2012-11-27 15:53:24]

On 2012/11/27 14:07:26, reed1 wrote:
> Many many of the changes to the .cpp files could be done with our existing
> SkAutoTUnref, as we do in lots of other skia .cpp files. Do we really need to
> use/update SkRefPtr for these scoped use cases?

This. PDF has been a problematic user of Skia reference-counting; if we're
touching it, let's get it aligned with mainstream Skia idiom.

[Stephen White, 2012-11-27 16:48:45]

On 2012/11/27 15:53:24, TomH wrote:
> On 2012/11/27 14:07:26, reed1 wrote:
> > Many many of the changes to the .cpp files could be done with our existing
> > SkAutoTUnref, as we do in lots of other skia .cpp files. Do we really need
to
> > use/update SkRefPtr for these scoped use cases?
> 
> This. PDF has been a problematic user of Skia reference-counting; if we're
> touching it, let's get it aligned with mainstream Skia idiom.

I think the problem is the idiom doesn't cover all use cases.  In particular,
it's not very good for ownership transfer.

See https://codereview.chromium.org/11418117/ for the problem that Dana is
trying to solve.

[reed1, 2012-11-27 16:55:23]

Looks like in that case, we don't want adopt either, as the receiver and the
giver both want to be owners.

I would suggest we look at updating the PDF code to match the rest of skia, and
separately we can look at ways to allow for safe assignment of SkRefCnt objects
(other than the macro that currently exists.)

[danakj, 2012-11-27 17:09:17]

On 2012/11/27 16:55:23, reed1 wrote:
> Looks like in that case, we don't want adopt either, as the receiver and the
> giver both want to be owners.

We do want Adopt at the boundary to cc/, where we turn a raw pointer into a
SkRefPtr. Then internally, it passes SkRefPtr&'s around, so it gets owned in
both places.

Adopt is for making SkRefPtr's our of raw pointers only. Once you have an
SkRefPtr, you'd always want to pass it around, and things will just work.

> I would suggest we look at updating the PDF code to match the rest of skia,
and
> separately we can look at ways to allow for safe assignment of SkRefCnt
objects
> (other than the macro that currently exists.)

I'm just trying to make cc/ automatically refcount at the right places with a
class like SkRefPtr.  The existing class is frustrating because it leaves a
reference outside the class that requires manual bookkeeping. I'm trying to make
the class into an idiom where you give a raw pointer to this class, and then it
"just works" and the object is cleaned up when all references to the SkRefPtr
(and any copies of it) are no more.

If SkRefPtr isn't a desirable construct within Skia, then we should probably
nuke it, and we can make something with those behaviour characteristics in cc/
where we do want it.

[reed1, 2012-11-27 17:22:53]

I think making your own, for now, is most expedient. That gives you the freedom
to get exactly what you need, and Skia can watch and learn, in case we later
want to roll all or parts of what you create into our core.

[Stephen White, 2012-11-27 18:00:10]

On 2012/11/27 17:22:53, reed1 wrote:
> I think making your own, for now, is most expedient. That gives you the
freedom
> to get exactly what you need, and Skia can watch and learn, in case we later
> want to roll all or parts of what you create into our core.

I'd really rather not have three different ways of doing the same thing across
two codebases.  Having many different "local customs" makes things overly
confusing, especially for new developers trying to figure out which custom to
follow.

As a compromise, could we just add a static SkRefPtr<T>::AdoptRef() for now, and
leave the ref'ing constructor as-is?

[reed1, 2012-11-27 18:24:34]

Skia has two, and we are working to get it down to one. Adding more to SkRefPtr
seems a step in the wrong direction.

Perhaps we can add an assignment method to SkAutoTUnref.

[Stephen White, 2012-11-27 18:30:29]

On 2012/11/27 18:24:34, reed1 wrote:
> Skia has two, and we are working to get it down to one. Adding more to
SkRefPtr
> seems a step in the wrong direction.
> 
> Perhaps we can add an assignment method to SkAutoTUnref.

Personally I find SkRefPtr the right direction, and more like most other
codebases I've seen (principle of least surprise).  I'd rather get rid of
SkAutoTUnref than add new semantics to it that make it behave unlike its name.

[reed1, 2012-11-27 18:34:23]

SkAutoTUnref is used in over 90 files. Lets keep that as Skia's pattern for now.

[danakj, 2012-11-27 18:35:45]

On 2012/11/27 18:30:29, Stephen White wrote:
> On 2012/11/27 18:24:34, reed1 wrote:
> > Skia has two, and we are working to get it down to one. Adding more to
> SkRefPtr
> > seems a step in the wrong direction.
> > 
> > Perhaps we can add an assignment method to SkAutoTUnref.
> 
> Personally I find SkRefPtr the right direction, and more like most other
> codebases I've seen (principle of least surprise).  I'd rather get rid of
> SkAutoTUnref than add new semantics to it that make it behave unlike its name.

It seems like with copy+ref operations, SkAutoTUnref becomes equivalent to
SkRefPtr without the SkIncrementAndAdopt() (and without some API sugar).

At this point it seems like we all want the same thing, just different names.
- SkAutoTUnref with copy+ref
- SkRefPtr with Adopt() and drop the AdoptAndIncrement() (maybe drop it later)

[danakj, 2012-11-27 20:03:54]

Updated.

The patch is larger, but the parts of interest are:
include/core/SkRefCnt.h: Updated the SkAutoTUnref class.
tests/RefCntTest.cpp: Added unit tests for the class.

The rest of the changes are mostly changing implicit conversions from
SKAutoTUnref<T> to T* into explicit conversions. This is explained in the CL
description.

[danakj, 2012-11-27 20:05:17]

On 2012/11/27 20:03:54, danakj wrote:
> Updated.
> 
> The patch is larger, but the parts of interest are:
> include/core/SkRefCnt.h: Updated the SkAutoTUnref class.
> tests/RefCntTest.cpp: Added unit tests for the class.
> 
> The rest of the changes are mostly changing implicit conversions from
> SKAutoTUnref<T> to T* into explicit conversions. This is explained in the CL
> description.

Oh, I meant to add:

We have a lot of places that pass the raw pointer with .get() since that was
what was happening before. Many of those places are passing ownership however,
so they should pass the SkAutoTUnref instead of the raw pointer in the future,
but this requires changing the receiving methods to match.

[reed1, 2012-11-27 20:23:36]

I vote against removing the implicit conversion to T*. Passing a ptr or passing
a ref to SkAutoTUnref are both ambiguous w.r.t. whether the receiving is going
to take a ref on the object.

[danakj, 2012-11-27 20:34:01]

On 2012/11/27 20:23:36, reed1 wrote:
> I vote against removing the implicit conversion to T*. Passing a ptr or
passing
> a ref to SkAutoTUnref are both ambiguous w.r.t. whether the receiving is going
> to take a ref on the object.

Ok. Not sure I agree. In the latter case, the called function implicitly takes a
reference. It may or may not drop it when the function leaves. Functionally,
they can be equivalent, but making "passing a raw pointer" explicit, the code
becomes clear that the caller is or isn't meaning to keep their own reference.

For example in GrContent::createTexture, it creates a texture object. Passes the
raw pointer off to the cache. And then returns the raw pointer. It reads like
the return value is going to be deleted by the SkAutoTUnref object going out of
scope.

Anyhow, patch has been updated to put the operator T* back. PTAL.

[reed1, 2012-11-27 20:42:11]

works for me, with nit about location and dox.

https://codereview.appspot.com/6849109/diff/11003/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.appspot.com/6849109/diff/11003/include/core/SkRefCnt.h#new...
include/core/SkRefCnt.h:180: // Copy and assign take a reference on the new
class instance.
Lets move this up next to the other constructor, and have its own dox (so a
separate dox for assignment).

[bsalomon, 2012-11-27 20:51:43]

On 2012/11/27 20:42:11, reed1 wrote:
> works for me, with nit about location and dox.
> 
> https://codereview.appspot.com/6849109/diff/11003/include/core/SkRefCnt.h
> File include/core/SkRefCnt.h (right):
> 
>
https://codereview.appspot.com/6849109/diff/11003/include/core/SkRefCnt.h#new...
> include/core/SkRefCnt.h:180: // Copy and assign take a reference on the new
> class instance.
> Lets move this up next to the other constructor, and have its own dox (so a
> separate dox for assignment).

I like the assignment operator. I can imagine using it in op= of classes that
have SkAutoTUnref members.

What's the use case for the copy-cons? I'm a little concerned about removing
SkNoncopyable because it makes it possible to write functions that take
SkAutoTUnref as a param. Is that something we want?

[danakj, 2012-11-27 20:55:50]

On 2012/11/27 20:51:43, bsalomon wrote:
> On 2012/11/27 20:42:11, reed1 wrote:
> > works for me, with nit about location and dox.
> > 
> > https://codereview.appspot.com/6849109/diff/11003/include/core/SkRefCnt.h
> > File include/core/SkRefCnt.h (right):
> > 
> >
>
https://codereview.appspot.com/6849109/diff/11003/include/core/SkRefCnt.h#new...
> > include/core/SkRefCnt.h:180: // Copy and assign take a reference on the new
> > class instance.
> > Lets move this up next to the other constructor, and have its own dox (so a
> > separate dox for assignment).
> 
> I like the assignment operator. I can imagine using it in op= of classes that
> have SkAutoTUnref members.
> 
> What's the use case for the copy-cons? I'm a little concerned about removing
> SkNoncopyable because it makes it possible to write functions that take
> SkAutoTUnref as a param. Is that something we want?

That is exactly the use case I am going for:
http://code.google.com/searchframe#OAMlx_jo-ck/src/cc/layer_impl.cc&exact_pac...

This will take a SkAutoTUnref<SkImageFilter> instead of an SkImageFilter*. This
creates equivalent behaviour to scoped_refptr, similar to WTF::RefPtr.

[danakj, 2012-11-27 20:58:21]

https://codereview.appspot.com/6849109/diff/11003/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.appspot.com/6849109/diff/11003/include/core/SkRefCnt.h#new...
include/core/SkRefCnt.h:180: // Copy and assign take a reference on the new
class instance.
On 2012/11/27 20:42:12, reed1 wrote:
> Lets move this up next to the other constructor, and have its own dox (so a
> separate dox for assignment).

Done.

[reed1, 2012-11-27 21:01:45]

void LayerImpl::setFilter(SkImageFilter* filter);

IMHO, for a skia call, this looks exactly right. It means the caller need not
have already constructed a wrapper class, but it may (given implicit conversion
to T*). If the implementation of LayerImpl wants to use a container to hold its
reference, that is fine, but that need not trickle up to the signature of the
API.

[bsalomon, 2012-11-27 21:02:02]

On 2012/11/27 20:55:50, danakj wrote:
> On 2012/11/27 20:51:43, bsalomon wrote:
> > On 2012/11/27 20:42:11, reed1 wrote:
> > > works for me, with nit about location and dox.
> > > 
> > > https://codereview.appspot.com/6849109/diff/11003/include/core/SkRefCnt.h
> > > File include/core/SkRefCnt.h (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/6849109/diff/11003/include/core/SkRefCnt.h#new...
> > > include/core/SkRefCnt.h:180: // Copy and assign take a reference on the
new
> > > class instance.
> > > Lets move this up next to the other constructor, and have its own dox (so
a
> > > separate dox for assignment).
> > 
> > I like the assignment operator. I can imagine using it in op= of classes
that
> > have SkAutoTUnref members.
> > 
> > What's the use case for the copy-cons? I'm a little concerned about removing
> > SkNoncopyable because it makes it possible to write functions that take
> > SkAutoTUnref as a param. Is that something we want?
> 
> That is exactly the use case I am going for:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/cc/layer_impl.cc&exact_pac...
> 
> This will take a SkAutoTUnref<SkImageFilter> instead of an SkImageFilter*.
This
> creates equivalent behaviour to scoped_refptr, similar to WTF::RefPtr.

Wouldn't this work:
Make LayerImpl have SkAutoTUnref<SkImageFilter> m_filter instead of a bare ptr.
setFilter still takes a bare ptr, but it calls m_filter.reset(filter).
Remove the SkSafeUnref from ~LayerImpl.

[danakj, 2012-11-27 21:13:39]

For reference, here is the chromium-dev thread about why operator T* is bad.

https://groups.google.com/a/chromium.org/forum/?fromgroups#!searchin/chromium...

[danakj, 2012-11-27 21:17:15]

On Tue, Nov 27, 2012 at 4:01 PM,  <reed@google.com> wrote:
> void LayerImpl::setFilter(SkImageFilter* filter);
>
> IMHO, for a skia call, this looks exactly right. It means the caller
> need not have already constructed a wrapper class, but it may (given
> implicit conversion to T*). If the implementation of LayerImpl wants to
> use a container to hold its reference, that is fine, but that need not
> trickle up to the signature of the API.

Yes, given the access to the raw pointer, it is possible to pass
around raw pointers. The goal is to never touch the raw pointer and
have to think about ownership. The SkAutoTUnref object acts as the
pointer surrogate, and everything is accessed through it via
operator->. When the raw pointer is used, it is always while
pretending it is not ref counted at all.

[bsalomon, 2012-11-27 21:24:54]

On 2012/11/27 21:13:39, danakj wrote:
> For reference, here is the chromium-dev thread about why operator T* is bad.
> 
>
https://groups.google.com/a/chromium.org/forum/?fromgroups#%21searchin/chromi...

IIUC, the problems identified in the first message from Will don't apply to
SkAutoTUnref in its current incarnation because it can't be a return value or
parameter because it inherits from SkNoncopyable. So it seems to me like we
would be adding the problem by removing SkNoncopyable and then fixing it
differently by removing the T* operator.

[danakj, 2012-11-27 21:31:57]

On 2012/11/27 21:24:54, bsalomon wrote:
> On 2012/11/27 21:13:39, danakj wrote:
> > For reference, here is the chromium-dev thread about why operator T* is bad.
> > 
> >
>
https://groups.google.com/a/chromium.org/forum/?fromgroups#%2521searchin/chro...
> 
> IIUC, the problems identified in the first message from Will don't apply to
> SkAutoTUnref in its current incarnation because it can't be a return value or
> parameter because it inherits from SkNoncopyable. So it seems to me like we
> would be adding the problem by removing SkNoncopyable and then fixing it
> differently by removing the T* operator.

Well, that's why I proposed we remove the operator T*. Making the class copyable
allows this class to be used similar to all other ref-counting wrappers in the
chromium project (scoped_refptr and WTF::RefPtr). Treating this one differently
seems really bad. I would also argue that reset() isn't a good idea, and we
should not allow passing a raw pointer when it's going to be ref'd ever.

For instance, these do different things, where fThing is a SkAutoTUnref<Thing>:

void foo(Thing* thing) {
fThing = SKAutoTUnref<Thing>(thing); // steals the reference from the caller.
}

void foo(Thing* thing) {
fThing.reset(thing); // takes a new reference.
}

It's still easy to shoot oneself in the foot, and requires thinking about the
reference counts. That's why we'd forbid treating raw pointers are reffable in
cc/, and always pass the SkAutoTUnref object instead.

[reed1, 2012-11-27 21:36:34]

Every SkPaint method for ref'able objects returns a raw pointer, so forbidding
raw ptrs in cc/ files might get tedious w/o lots of added wrappers.

Lets land the assignment operator, as that has garnered general support. That
way we can spend lots more time debating the constructor question w/o holding up
assignment.

[reed1, 2012-11-27 21:40:11]

just a note:

SkAutoTUnref never appears as part of Skia's public API. This is by design, and
is meant (among other reasons) to not impose a given container syntax or
semantic on any of its callers. All of the getters and setters have well defined
(and consistent modulo the pdf code) behavior. Not saying other modules need to
mirror this at all, just wanted to help explain the design framework Skia is
coming from.

[bsalomon, 2012-11-27 21:43:45]

On 2012/11/27 21:31:57, danakj wrote:
> On 2012/11/27 21:24:54, bsalomon wrote:
> > On 2012/11/27 21:13:39, danakj wrote:
> > > For reference, here is the chromium-dev thread about why operator T* is
bad.
> > > 
> > >
> >
>
https://groups.google.com/a/chromium.org/forum/?fromgroups#%252521searchin/ch...
> > 
> > IIUC, the problems identified in the first message from Will don't apply to
> > SkAutoTUnref in its current incarnation because it can't be a return value
or
> > parameter because it inherits from SkNoncopyable. So it seems to me like we
> > would be adding the problem by removing SkNoncopyable and then fixing it
> > differently by removing the T* operator.
> 
> Well, that's why I proposed we remove the operator T*. Making the class
copyable
> allows this class to be used similar to all other ref-counting wrappers in the
> chromium project (scoped_refptr and WTF::RefPtr). Treating this one
differently
> seems really bad. I would also argue that reset() isn't a good idea, and we
> should not allow passing a raw pointer when it's going to be ref'd ever.
> 
> For instance, these do different things, where fThing is a
SkAutoTUnref<Thing>:
> 
> void foo(Thing* thing) {
> fThing = SKAutoTUnref<Thing>(thing); // steals the reference from the caller.
> }
> 
> void foo(Thing* thing) {
> fThing.reset(thing); // takes a new reference.
> }
> 

Not arguing the WK vs Skia ref conventions as I'm not very familiar with the WK
ref counting, but I think this example is incorrect. These do the same thing;
reset() doesn't take a reference.


> It's still easy to shoot oneself in the foot, and requires thinking about the
> reference counts. That's why we'd forbid treating raw pointers are reffable in
> cc/, and always pass the SkAutoTUnref object instead.

[danakj, 2012-11-27 21:43:47]

On 2012/11/27 21:36:34, reed1 wrote:
> Every SkPaint method for ref'able objects returns a raw pointer, so forbidding
> raw ptrs in cc/ files might get tedious w/o lots of added wrappers.

Right, things change at the Skia API boundary, and we tend to wrap any
differences in methods/classes to abstract that.

> Lets land the assignment operator, as that has garnered general support. That
> way we can spend lots more time debating the constructor question w/o holding
up
> assignment.

I'd really rather discuss this as a whole. Having an assignment operator is
useful, but not enough. I would still write another class to wrap this
difference and allow us to use ref-counted skia objects the same way as
ref-counted objects wrapped in scoped_refptr.

What is the compelling reason here to have behaviour that differs from
scoped_refptr?

[bungeman, 2012-11-27 21:44:26]

Much of this may already have been said, but here are my notes that I've been
making as this has gone along.

https://codereview.appspot.com/6849109/diff/4001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.appspot.com/6849109/diff/4001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:158: SkRefPtr<SkMemoryStream> staticStream;
SkAutoTUnref<SkMemoryStream>

https://codereview.appspot.com/6849109/diff/4001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:163: staticStream = SkAdoptRef(new SkMemoryStream(srcLen +
1));
staticStream.reset(new SkMemoryStream(srcLen + 1));

https://codereview.appspot.com/6849109/diff/4001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:320: SkRefPtr<SkPDFArray> advanceArray = SkAdoptRef(
SkAutoTUnref<SkPDFArray> advanceArray(new SkPDFArray());

This x1000.

https://codereview.appspot.com/6849109/diff/4001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:850: fTypeface(SkAdoptAndIncrementRef(typeface)),
This style doesn't seem to have helped matters much. I still need to know which
SkAdopt* variation to use. There is no advantage here compared to fTypeface
being an SkAutoTUnref<SkTypeface> and this line being
fTypeface(SkSafeRef(typeface)).

https://codereview.appspot.com/6849109/diff/4001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:1004: addResource(pdfCmap.get());
1003 can be removed and line 1004 replaced with 

addResource(SkRef(pdfCmap.get())

However, this is really odd, why doesn't addResource do the ref? The only reason
I can think of is that usually there was an extra reference sitting around with
SkRefPtr to pass in anyway. Getting rid of SkRefPtr's extra ref means
addResource should be modified to work correctly.

This happens a number of times below as well.

https://codereview.appspot.com/6849109/diff/5014/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (left):

https://codereview.appspot.com/6849109/diff/5014/include/core/SkRefCnt.h#oldc...
include/core/SkRefCnt.h:150: template <typename T> class SkAutoTUnref :
SkNoncopyable {
These really should be non-copyable. We don't want to be able to ever pass them
as parameters or return them as values. The entire point of an SkAutoTUnref is
to make the reference local. Since they can never be passed we don't have to
worry about the reference being held leaking out anywhere.

https://codereview.appspot.com/6849109/diff/5014/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.appspot.com/6849109/diff/5014/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:155: SkAutoTUnref(const SkAutoTUnref& o) : fObj(o.fObj)
{
While it allows for use in std containers, we already have our own containers
and don't use the std ones. This also allows for these to be passed as
parameters (which should never be done).

https://codereview.appspot.com/6849109/diff/5014/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:187: SkAutoTUnref& operator=(const SkAutoTUnref& rp) {
It looks like this operator= is just syntactic sugar for

A.reset(SkSafeRef(B))

Except that it is also an expression which evaluates to A (the same could be
done in the above by making reset return *this). As such I don't have any issue
with it directly.

[danakj, 2012-11-27 21:48:10]

On 2012/11/27 21:43:45, bsalomon wrote:
> Not arguing the WK vs Skia ref conventions as I'm not very familiar with the
WK
> ref counting, but I think this example is incorrect. These do the same thing;
> reset() doesn't take a reference.

Ah, you're right. I read Unref as Ref. That's what I thought at first, but then
it seemed like reset was special.

That means in cc/layer_impl.cc, we'd have to call ->ref() on the pointer passed
in, then set it on m_filter. Both the caller and callee are meant to keep a
reference to the the underlying object.

[bsalomon, 2012-11-27 21:51:07]

On 2012/11/27 21:48:10, danakj wrote:
> On 2012/11/27 21:43:45, bsalomon wrote:
> > Not arguing the WK vs Skia ref conventions as I'm not very familiar with the
> WK
> > ref counting, but I think this example is incorrect. These do the same
thing;
> > reset() doesn't take a reference.
> 
> Ah, you're right. I read Unref as Ref. That's what I thought at first, but
then
> it seemed like reset was special.
> 
> That means in cc/layer_impl.cc, we'd have to call ->ref() on the pointer
passed
> in, then set it on m_filter. Both the caller and callee are meant to keep a
> reference to the the underlying object.

Yeah, that's how setters inside Skia are implemented in classes that use
SkAutoTUnref as members:
fThing->reset(SkSafeRef(thing));

[danakj, 2012-11-27 21:58:18]

On Tue, Nov 27, 2012 at 4:51 PM,  <bsalomon@google.com> wrote:
> On 2012/11/27 21:48:10, danakj wrote:
>>
>> On 2012/11/27 21:43:45, bsalomon wrote:
>> > Not arguing the WK vs Skia ref conventions as I'm not very familiar
>
> with the
>>
>> WK
>> > ref counting, but I think this example is incorrect. These do the
>
> same thing;
>>
>> > reset() doesn't take a reference.
>
>
>> Ah, you're right. I read Unref as Ref. That's what I thought at first,
>
> but then
>>
>> it seemed like reset was special.
>
>
>> That means in cc/layer_impl.cc, we'd have to call ->ref() on the
>
> pointer passed
>>
>> in, then set it on m_filter. Both the caller and callee are meant to
>
> keep a
>>
>> reference to the the underlying object.
>
>
> Yeah, that's how setters inside Skia are implemented in classes that use
> SkAutoTUnref as members:
> fThing->reset(SkSafeRef(thing));

Right, which is the whole thing I'm trying to avoid. I'd like
refcounting for SkImageFilter* to be implicit and decided by scope. If
we have to type it, then it's easy to mess up.

Mike: I didn't realize that SkAutoTUnref is not a public class. I
guess I'd want to make it one. I do feel like the internals of Skia
would benefit from not having to ever type ref() and unref(), but at
the moment I'm really just trying to have a class that wraps pointers
coming out of the public API. senorblanco@ suggested this class since
it's what Skia uses internally, and I think that makes a lot of sense.
Suddenly it makes us examine how ref-counting happens internally too,
since this is used there.

We could hash out what Skia should have available/desired patterns
internally, if you like, but maybe we can move forward for external
use too.

- Maybe it would make sense to have a subclass of SkAutoTUnref that is
for external use, and adds the operator=, copy-constructor, and drops
the operator T*.
- Personally, I think it would be nice if the Skia public API migrated
to passing out this class instead of raw pointers. That way call sites
don't have to think about (and maybe mess up) doing ref() explicitly
before they hold onto the pointer/stick it into this class.

[TomH, 2012-11-28 09:15:05]

On 2012/11/27 21:58:18, danakj wrote:
> Right, which is the whole thing I'm trying to avoid. I'd like
> refcounting for SkImageFilter* to be implicit and decided by scope. If
> we have to type it, then it's easy to mess up.
> 
> Mike: I didn't realize that SkAutoTUnref is not a public class. I
> guess I'd want to make it one. I do feel like the internals of Skia
> would benefit from not having to ever type ref() and unref(), but at
> the moment I'm really just trying to have a class that wraps pointers
> coming out of the public API. senorblanco@ suggested this class since
> it's what Skia uses internally, and I think that makes a lot of sense.
> Suddenly it makes us examine how ref-counting happens internally too,
> since this is used there.
> 
> We could hash out what Skia should have available/desired patterns
> internally, if you like, but maybe we can move forward for external
> use too.
> 
> - Maybe it would make sense to have a subclass of SkAutoTUnref that is
> for external use, and adds the operator=, copy-constructor, and drops
> the operator T*.
> - Personally, I think it would be nice if the Skia public API migrated
> to passing out this class instead of raw pointers. That way call sites
> don't have to think about (and maybe mess up) doing ref() explicitly
> before they hold onto the pointer/stick it into this class.


I'd like to take the opposite approach:
  Keep SkAutoTUnref private.
  Acknowledge that the Skia <-> Chrome interface is a transition point between
different projects, and so people need to be explicit about ownership. Accept
the Skia design decision *not* to have wrapped pointers coming out of the public
API.
  Find an owner to *fix* the bad code in Skia PDF so that well-intentioned
Chrome project members don't keep thinking it's how things should be done.

I volunteer to do this last when I'm back on the Skia team, but that's 10 months
away, and it's been festering too long - this seems to crop up every 6 months. I
don't know if I can justify carving out time from my current assignment within
the next month.

Junction independent projects with the simplest semantics possible, rather than
forcing complex semantics in one project to spread into all the others.

Tom