Add skia::RefPtr class to wrap ref counted classes from Skia.

Add skia::RefPtr class to wrap ref counted classes from Skia.

This class behaves like scoped_refptr and wraps the raw SkRefCnt subclass
so that we don't have to call ref() and unref() directly on the pointers.

Tested by:
unit_tests:RefPtrTest.ReferenceCounting
unit_tests:RefPtrTest.Construct
unit_tests:RefPtrTest.DeclareAndAssign
unit_tests:RefPtrTest.Assign
unit_tests:RefPtrTest.Upcast

BUG=163454
NOTRY=true

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

[danakj, 2012-11-28 22:08:06]

https://codereview.appspot.com/6849109/ has gone nowhere except to prompt the
removal of SkRefPtr from skia...

So here we go again. Here's SkiaRefPtr complete with unit tests.

[jamesr, 2012-11-28 22:20:54]

https://codereview.chromium.org/11418217/diff/2001/cc/skia_refptr.h
File cc/skia_refptr.h (right):

https://codereview.chromium.org/11418217/diff/2001/cc/skia_refptr.h#newcode13
cc/skia_refptr.h:13: class SkiaRefPtr {
This doesn't belong in cc/

[danakj, 2012-11-29 02:21:41]

PTAL. Moved it to skia/ext

[danakj, 2012-11-29 04:17:21]

In the last upload, I got rid of every call to ->ref() and ->unref() in
chromium. The patch looks a bit scarier than it ought to, due to all the files.

The diffs are pretty straight-forward, and the part of interest is really
skia/ext/refptr.h and skia/ext/refptr_unittest.cc

[danakj, 2012-11-29 04:17:58]

senorblanco can you please review skia/
jamesr can you please review cc/

and i'll find owners for the rest once those are happy.

[jamesr, 2012-11-29 07:34:05]

lgtm for cc/, content/renderer/, and webkit/ although I think Stephen should
look over the cc changes as well for sanity

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

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr.h#newcode12
skia/ext/refptr.h:12: template<typename T>
could you add some class-level comments indicating how this is supposed to be
used, perhaps with some example code?

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr.h#newcode33
skia/ext/refptr.h:33: // take ownership.
this comment's repeated  and i can't really tell what it means here

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr.h#newcode51
skia/ext/refptr.h:51: // Convert the RefPtr<T> into a RefPtr<P>.
could you just make operator= accept RefPtr<U>s and then rely on
SkRefCnt_SafeAssign() compiling to make sure the types are convertable?

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr_unittest.cc
File skia/ext/refptr_unittest.cc (right):

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr_unittest.c...
skia/ext/refptr_unittest.cc:16: {
why this scope? there are no locals outside of this scope that have destructors

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr_unittest.c...
skia/ext/refptr_unittest.cc:124: }  // namespace cc
namespace skia

[Stephen White, 2012-11-29 18:42:04]

Thanks for taking this on!

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

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr.h#newcode26
skia/ext/refptr.h:26: RefPtr(T* ptr) : ptr_(ptr) {}
Out of curiosity, why did you decide on a public constructor for this one? 
Wouldn't it be better to have an explicit RefPtr::Adopt(), a la WebKit?

I like Adopt() because it follows the "making wrong code look wrong" principle: 
if I see a "new Foo" or Foo::Create() without an Adopt, my spidey sense tingles.
 The current implementation also seems like it would make problems like this
harder to spot:

skia::RefPtr<T> foo(otherRefPtr.get());

instead of

skia::RefPtr<T> foo(otherRefPtr);

[danakj, 2012-11-29 21:37:11]

PTAL! We have skia::AdoptRef() now :)

And I replaced all SkAutoTUnref's as well.

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

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr.h#newcode12
skia/ext/refptr.h:12: template<typename T>
On 2012/11/29 07:34:06, jamesr wrote:
> could you add some class-level comments indicating how this is supposed to be
> used, perhaps with some example code?

Done.

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr.h#newcode26
skia/ext/refptr.h:26: RefPtr(T* ptr) : ptr_(ptr) {}
On 2012/11/29 18:42:04, Stephen White wrote:
> Out of curiosity, why did you decide on a public constructor for this one? 
> Wouldn't it be better to have an explicit RefPtr::Adopt(), a la WebKit?
> 
> I like Adopt() because it follows the "making wrong code look wrong"
principle: 
> if I see a "new Foo" or Foo::Create() without an Adopt, my spidey sense
tingles.
>  The current implementation also seems like it would make problems like this
> harder to spot:
> 
> skia::RefPtr<T> foo(otherRefPtr.get());
> 
> instead of
> 
> skia::RefPtr<T> foo(otherRefPtr);

I was having difficulty making an Adopt() method that would deal well with
superclasses.

// This wouldn't compile because no implicit upcasting.
skia::RefPtr<SkShader> s = skia::Adopt(new SkSpecializedShader);

So I ended up with way too much typing:
skia::RefPtr<SkShader> s = skia::RefPtr<SkShader>::Adopt(new
SkSpecializedShader);

Almost 80 cols of boilerplate, which I found hard to justify. So I went with a
constructor.

However, jamesr's comment on PassAs() here points out a great way to solve this.

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr.h#newcode33
skia/ext/refptr.h:33: // take ownership.
On 2012/11/29 07:34:06, jamesr wrote:
> this comment's repeated  and i can't really tell what it means here

This, and the constructor, take a raw skia pointer, and steal that reference to
take ownership of the raw pointer.

Did you miss the T* parameter here, or is there something I can do to word this
better?

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr.h#newcode51
skia/ext/refptr.h:51: // Convert the RefPtr<T> into a RefPtr<P>.
On 2012/11/29 07:34:06, jamesr wrote:
> could you just make operator= accept RefPtr<U>s and then rely on
> SkRefCnt_SafeAssign() compiling to make sure the types are convertable?

Yaaaaa!! thanks!

[danakj, 2012-11-29 21:38:01]

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

https://codereview.chromium.org/11418217/diff/6027/skia/ext/refptr.h#newcode33
skia/ext/refptr.h:33: // take ownership.
On 2012/11/29 21:37:11, danakj wrote:
> On 2012/11/29 07:34:06, jamesr wrote:
> > this comment's repeated  and i can't really tell what it means here
> 
> This, and the constructor, take a raw skia pointer, and steal that reference
to
> take ownership of the raw pointer.
> 
> Did you miss the T* parameter here, or is there something I can do to word
this
> better?

Actually with AdoptRef() this problem goes away :)

[danakj, 2012-11-29 21:49:42]

I missed AdoptRef() in a couple unit test files when assigning, so I've updated
the CL and rebased for the bots.

[danakj, 2012-11-29 22:12:46]

I need to split this CL up. I'll do just skia/ on this one.

[danakj, 2012-11-29 22:38:53]

@senorblanco: This CL is just the skia/ bits (and a minimal change outside that
falls out from it).

There's still some Sk raw pointers, that are tricky to deal with, but they all
end up going back into skia, and we don't ref() or unref() them ourselves. See
vector_canvas.h for example. I wanted to make the SkDevice be a skia::RefPtr,
but we take it out of a skia object, and then want to pass it into this other
one. There's "ownership" comments along the way, but we would need a way to
stick it into the skia::RefPtr without adopting, or we'll be stealing a
reference from the source owner.

[Stephen White, 2012-11-29 23:00:33]

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

https://codereview.chromium.org/11418217/diff/7151/skia/ext/refptr.h#newcode35
skia/ext/refptr.h:35: // wrap the raw pointer immediately in a skia::RefPtr and
always work with
Maybe mention Adopt here, instead of (or in addition to) wrap?

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas.cc
File skia/ext/vector_canvas.cc (left):

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas.cc#...
skia/ext/vector_canvas.cc:11: setDevice(device)->unref(); // Created with
refcount 1, and setDevice refs.
Not new to this patch, but IIRC setDevice() (the skia flavour) is deprecated.  I
wonder if we could just call through to the SkCanvas constructor which takes an
SkDevice?  It looks like PlatformCanvas (VectorCanvas's parent) is just an
SkCanvas now, although I've been wrong before.

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas_uni...
File skia/ext/vector_canvas_unittest.cc (right):

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas_uni...
skia/ext/vector_canvas_unittest.cc:733: skia::RefPtr<SkPathEffect> effect = new
SkDashPathEffect(
Shouldn't we be doing an AdoptRef here?

How is this working, in fact, if the constructor is private?  I was hoping that
code like this wouldn't work without the adopt.

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas_uni...
skia/ext/vector_canvas_unittest.cc:753: skia::RefPtr<SkPathEffect> effect = new
SkDashPathEffect(
Same here.

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas_uni...
skia/ext/vector_canvas_unittest.cc:771: skia::RefPtr<SkPathEffect> effect = new
SkDashPathEffect(
Same here (and below).

[Stephen White, 2012-11-29 23:11:46]

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas_uni...
File skia/ext/vector_canvas_unittest.cc (right):

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas_uni...
skia/ext/vector_canvas_unittest.cc:733: skia::RefPtr<SkPathEffect> effect = new
SkDashPathEffect(
In fact, it doesn't compile (yay!).  See win_rel unit_tests.

[danakj, 2012-11-29 23:15:52]

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

https://codereview.chromium.org/11418217/diff/7151/skia/ext/refptr.h#newcode35
skia/ext/refptr.h:35: // wrap the raw pointer immediately in a skia::RefPtr and
always work with
On 2012/11/29 23:00:33, Stephen White wrote:
> Maybe mention Adopt here, instead of (or in addition to) wrap?

Done.

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas.cc
File skia/ext/vector_canvas.cc (left):

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas.cc#...
skia/ext/vector_canvas.cc:11: setDevice(device)->unref(); // Created with
refcount 1, and setDevice refs.
On 2012/11/29 23:00:33, Stephen White wrote:
> Not new to this patch, but IIRC setDevice() (the skia flavour) is deprecated. 
I
> wonder if we could just call through to the SkCanvas constructor which takes
an
> SkDevice?  It looks like PlatformCanvas (VectorCanvas's parent) is just an
> SkCanvas now, although I've been wrong before.

Done.

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas_uni...
File skia/ext/vector_canvas_unittest.cc (right):

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas_uni...
skia/ext/vector_canvas_unittest.cc:733: skia::RefPtr<SkPathEffect> effect = new
SkDashPathEffect(
On 2012/11/29 23:11:47, Stephen White wrote:
> In fact, it doesn't compile (yay!).  See win_rel unit_tests.

Yeh, fixing!

https://codereview.chromium.org/11418217/diff/7151/skia/ext/vector_canvas_uni...
skia/ext/vector_canvas_unittest.cc:771: skia::RefPtr<SkPathEffect> effect = new
SkDashPathEffect(
On 2012/11/29 23:00:33, Stephen White wrote:
> Same here (and below).

Done.

[Stephen White, 2012-11-29 23:51:01]

LGTM.  Thanks for sticking with this!

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

https://codereview.chromium.org/11418217/diff/7151/skia/ext/refptr.h#newcode35
skia/ext/refptr.h:35: // wrap the raw pointer immediately in a skia::RefPtr and
always work with
On 2012/11/29 23:15:52, danakj wrote:
> On 2012/11/29 23:00:33, Stephen White wrote:
> > Maybe mention Adopt here, instead of (or in addition to) wrap?
> 
> Done.

Don't see any change here..

[danakj, 2012-11-29 23:51:47]

Thanks!

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

https://codereview.chromium.org/11418217/diff/7151/skia/ext/refptr.h#newcode35
skia/ext/refptr.h:35: // wrap the raw pointer immediately in a skia::RefPtr and
always work with
On 2012/11/29 23:51:01, Stephen White wrote:
> On 2012/11/29 23:15:52, danakj wrote:
> > On 2012/11/29 23:00:33, Stephen White wrote:
> > > Maybe mention Adopt here, instead of (or in addition to) wrap?
> > 
> > Done.
> 
> Don't see any change here..

Hrm.. lost in space I guess, will fix :)

[sky, 2012-11-29 23:57:31]

I'm adding Albert to this. I think he should take a look at skia/ext/refptr.h to
see if there is some way to share code with scoped_ptr. To my uninitiated eyes
the two seem very similar and I hate to duplicate logic. Especially when changes
are being done to scoped_ptr.

[danakj, 2012-11-30 00:05:57]

On 2012/11/29 23:57:31, sky wrote:
> I'm adding Albert to this. I think he should take a look at skia/ext/refptr.h
to
> see if there is some way to share code with scoped_ptr. To my uninitiated eyes
> the two seem very similar and I hate to duplicate logic. Especially when
changes
> are being done to scoped_ptr.

sgtm.

@sky can you please OWNERS review chrome/ ?

[awong, 2012-11-30 00:10:24]

This is actually closer to scoped_refptr<> rather than scoped_ptr<> because the
type is copyable and thus doesn't express single-ownership.

How must is skia/ext allowed to depend on chromium/base, and how much can we
modify the Skia interfaces? For instance, if the Skia code base were able to use
AddRef()/Release() rather than ref()/unref(), you could actually just directly
use scoped_refptr<> from chromium.

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

https://codereview.chromium.org/11418217/diff/13021/skia/ext/refptr.h#newcode43
skia/ext/refptr.h:43: RefPtr(const RefPtr<T>& other)
Remove the <T>

https://codereview.chromium.org/11418217/diff/13021/skia/ext/refptr.h#newcode58
skia/ext/refptr.h:58: RefPtr& operator=(const RefPtr<T>& other) {
Remove the <T>

https://codereview.chromium.org/11418217/diff/13021/skia/ext/refptr.h#newcode76
skia/ext/refptr.h:76: T& operator*() const { return static_cast<T&>(*ptr_); }
Why are we doing a static_cast<>?

[danakj, 2012-11-30 00:25:09]

I think skia/ already depends on base, I see it in the gyp.

However changing Skia APIs seems like a no go. That's how I ended up here in the
first place.

Would you subclass scoped_refptr and replace calls with ref() unref() ?

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

https://codereview.chromium.org/11418217/diff/13021/skia/ext/refptr.h#newcode43
skia/ext/refptr.h:43: RefPtr(const RefPtr<T>& other)
On 2012/11/30 00:10:25, awong wrote:
> Remove the <T>

Done.

https://codereview.chromium.org/11418217/diff/13021/skia/ext/refptr.h#newcode58
skia/ext/refptr.h:58: RefPtr& operator=(const RefPtr<T>& other) {
On 2012/11/30 00:10:25, awong wrote:
> Remove the <T>

Done.

https://codereview.chromium.org/11418217/diff/13021/skia/ext/refptr.h#newcode76
skia/ext/refptr.h:76: T& operator*() const { return static_cast<T&>(*ptr_); }
On 2012/11/30 00:10:25, awong wrote:
> Why are we doing a static_cast<>?

oh, i was toying with hiding ref() and unref() in the T* when returning it here.
I'll remove those.

[commit-bot: I haz the power, 2012-11-30 02:02:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418217/9060

[commit-bot: I haz the power, 2012-11-30 02:02:36]

Presubmit check for 11418217-9060 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/renderer/automation

Presubmit checks took 2.2s to calculate.

[danakj, 2012-11-30 02:04:42]

woops, pending sky@ review :)

[sky, 2012-12-03 15:25:04]

https://codereview.chromium.org/11418217/diff/10130/skia/ext/vector_canvas.h
File skia/ext/vector_canvas.h (right):

https://codereview.chromium.org/11418217/diff/10130/skia/ext/vector_canvas.h#...
skia/ext/vector_canvas.h:10: #include "skia/ext/refptr.h"
Do you really need this include here?

[danakj, 2012-12-03 16:52:37]

https://codereview.chromium.org/11418217/diff/10130/skia/ext/vector_canvas.h
File skia/ext/vector_canvas.h (right):

https://codereview.chromium.org/11418217/diff/10130/skia/ext/vector_canvas.h#...
skia/ext/vector_canvas.h:10: #include "skia/ext/refptr.h"
On 2012/12/03 15:25:04, sky wrote:
> Do you really need this include here?

No, I stared converting this API at one point, but will do it in a separate CL.
Removing.

[danakj, 2012-12-03 16:57:41]

PTAL

[Stephen White, 2012-12-03 17:37:52]

LGTM.

https://codereview.chromium.org/11418217/diff/12143/skia/ext/vector_platform_...
File skia/ext/vector_platform_device_emf_win.cc (right):

https://codereview.chromium.org/11418217/diff/12143/skia/ext/vector_platform_...
skia/ext/vector_platform_device_emf_win.cc:12: #include "skia/ext/refptr.h"
Nit:  can probably get rid of this.

[sky, 2012-12-03 18:09:58]

LGTM

[commit-bot: I haz the power, 2012-12-03 18:53:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418217/2074

[commit-bot: I haz the power, 2012-12-04 00:32:55]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-04 00:35:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418217/2074

[commit-bot: I haz the power, 2012-12-04 00:37:14]

Change committed as 170851