Use skia::RefPtr to avoid having to manually refcount GraphicsContextState::m_looper.

Use skia::RefPtr to avoid having to manually refcount GraphicsContextState::m_looper.

BUG=251744, 254509
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154263

[danakj, 2013-06-19 15:47:13]

https://codereview.chromium.org/17448009/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/GraphicsContextState.h (right):

https://codereview.chromium.org/17448009/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/GraphicsContextState.h:130: SkRefPtr<SkDrawLooper>
m_looper;
I believe you want to use SKAutoTUnref instead. SkRefPtr is the less loved of
the two and I think will go away.

[jbroman, 2013-06-19 15:50:11]

On 2013/06/19 15:47:13, danakj wrote:
>
https://codereview.chromium.org/17448009/diff/1/Source/core/platform/graphics...
> File Source/core/platform/graphics/GraphicsContextState.h (right):
> 
>
https://codereview.chromium.org/17448009/diff/1/Source/core/platform/graphics...
> Source/core/platform/graphics/GraphicsContextState.h:130:
SkRefPtr<SkDrawLooper>
> m_looper;
> I believe you want to use SKAutoTUnref instead. SkRefPtr is the less loved of
> the two and I think will go away.

Source? That makes me sad, since the semantics of the two are *not* the same.
SkAutoTUnref doesn't ref things that are assigned to it (and syntactically,
doesn't implement operator=).

[danakj, 2013-06-19 15:53:50]

On Wed, Jun 19, 2013 at 11:50 AM, <jbroman@chromium.org> wrote:

> On 2013/06/19 15:47:13, danakj wrote:
>
> https://codereview.chromium.**org/17448009/diff/1/Source/**
>
core/platform/graphics/**GraphicsContextState.h<https://codereview.chromium.org/17448009/diff/1/Source/core/platform/graphics/GraphicsContextState.h>
>
>> File Source/core/platform/graphics/**GraphicsContextState.h (right):
>>
>
>
> https://codereview.chromium.**org/17448009/diff/1/Source/**
>
core/platform/graphics/**GraphicsContextState.h#**newcode130<https://codereview.chromium.org/17448009/diff/1/Source/core/platform/graphics/GraphicsContextState.h#newcode130>
>
>> Source/core/platform/graphics/**GraphicsContextState.h:130:
>>
> SkRefPtr<SkDrawLooper>
>
>> m_looper;
>> I believe you want to use SKAutoTUnref instead. SkRefPtr is the less
>> loved of
>> the two and I think will go away.
>>
>
> Source? That makes me sad, since the semantics of the two are *not* the
> same.
> SkAutoTUnref doesn't ref things that are assigned to it (and syntactically,
> doesn't implement operator=).


Yes. Well, if you can I'd really suggest you use skia::RefPtr in skia/ext
everywhere in blink as we do in chromium.

Source for SkRefPtr discussion:
https://codereview.appspot.com/6849109/#msg13

With conclusion and such: https://chromiumcodereview.appspot.com/11418217

[jbroman, 2013-06-19 17:43:12]

Hmm, OK. Switched to skia::RefPtr.

[jamesr, 2013-06-19 17:45:25]

https://codereview.chromium.org/17448009/diff/6001/Source/core/platform/graph...
File Source/core/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/17448009/diff/6001/Source/core/platform/graph...
Source/core/platform/graphics/GraphicsContext.h:41: #include "skia/ext/refptr.h"
this creates a cycle between the chromium and blink repositories, which is bad.
There are a few other places that #include "skia/ext/...", but none for
refptr.h. I think skia::RefPtr is better than any of the types in the skia
repository proper, but probably not enough better to add this complexity.

[jbroman, 2013-06-19 18:12:09]

On 2013/06/19 17:45:25, jamesr wrote:
>
https://codereview.chromium.org/17448009/diff/6001/Source/core/platform/graph...
> File Source/core/platform/graphics/GraphicsContext.h (right):
> 
>
https://codereview.chromium.org/17448009/diff/6001/Source/core/platform/graph...
> Source/core/platform/graphics/GraphicsContext.h:41: #include
"skia/ext/refptr.h"
> this creates a cycle between the chromium and blink repositories, which is
bad.
> There are a few other places that #include "skia/ext/...", but none for
> refptr.h. I think skia::RefPtr is better than any of the types in the skia
> repository proper, but probably not enough better to add this complexity.

There are already 13 uses of skia/ext/ from Blink, yes, but I don't disagree
with your point.

I suppose the options are:
1. Drop this change and don't make any similar changes -- manual refcounting is
fine.
2. Use SkAutoTUnref, and accept its weird semantics in this case (it manages
only the unref side of things).
3. Add a yet another clone of SkRefPtr/skia::RefPtr in
platform/graphics/skia/SkiaUtils.h or somewhere similar, matching wtf style.

I guess 1 is probably the path-of-least-resistance, but smart pointers are a
Good Thing(TM) for clean and correct code, and it's a little disappointing.

[danakj, 2013-06-19 18:17:42]

On Wed, Jun 19, 2013 at 2:12 PM, <jbroman@chromium.org> wrote:

> On 2013/06/19 17:45:25, jamesr wrote:
>
> https://codereview.chromium.**org/17448009/diff/6001/Source/**
>
core/platform/graphics/**GraphicsContext.h<https://codereview.chromium.org/17448009/diff/6001/Source/core/platform/graphics/GraphicsContext.h>
>
>> File Source/core/platform/graphics/**GraphicsContext.h (right):
>>
>
>
> https://codereview.chromium.**org/17448009/diff/6001/Source/**
>
core/platform/graphics/**GraphicsContext.h#newcode41<https://codereview.chromium.org/17448009/diff/6001/Source/core/platform/graphics/GraphicsContext.h#newcode41>
>
>> Source/core/platform/graphics/**GraphicsContext.h:41: #include
>>
> "skia/ext/refptr.h"
>
>> this creates a cycle between the chromium and blink repositories, which is
>>
> bad.
>
>> There are a few other places that #include "skia/ext/...", but none for
>> refptr.h. I think skia::RefPtr is better than any of the types in the skia
>> repository proper, but probably not enough better to add this complexity.
>>
>
> There are already 13 uses of skia/ext/ from Blink, yes, but I don't
> disagree
> with your point.
>
> I suppose the options are:
> 1. Drop this change and don't make any similar changes -- manual
> refcounting is
> fine.
> 2. Use SkAutoTUnref, and accept its weird semantics in this case (it
> manages
> only the unref side of things).
> 3. Add a yet another clone of SkRefPtr/skia::RefPtr in
> platform/graphics/skia/**SkiaUtils.h or somewhere similar, matching wtf
> style.
>
> I guess 1 is probably the path-of-least-resistance, but smart pointers are
> a
> Good Thing(TM) for clean and correct code, and it's a little disappointing.
>

I think 2 > 1.

I also like 3 in the meantime cuz smart pointers are nice. I'm not sure if
it would be awesome or horrific at the blink-chromium API boundary. Perhaps
we could have skia/ext/refptr.h include the blink one, and auto convert?


>
>
https://codereview.chromium.**org/17448009/<https://codereview.chromium.org/1...

[Stephen Chennney, 2013-06-20 17:03:39]

I would support (3), although I would create a new file instead of adding it in
SkiaUtils.

Can we name the class SkRefPtr, or SkiaRefPtr?

Could we convince the Skia team to add more Chrome/Blink friendly ref counting
to Skia proper?

[danakj, 2013-06-20 17:08:27]

On Thu, Jun 20, 2013 at 1:03 PM, <schenney@chromium.org> wrote:

> I would support (3), although I would create a new file instead of adding
> it in
> SkiaUtils.
>
> Can we name the class SkRefPtr, or SkiaRefPtr?
>
> Could we convince the Skia team to add more Chrome/Blink friendly ref
> counting
> to Skia proper?
>

That's what was attempted here:
https://codereview.appspot.com/6849109/#msg13 It was the opinion of the
team at the time that Skia should not provide APIs for this. Of course, I'd
be really happy to roll src/skia/ext/refptr,h into skia proper as well, but
am not hopeful of seeing that happen, given the number of senior engineers
who participated in the conversation last time without that result.


>
>
https://codereview.chromium.**org/17448009/<https://codereview.chromium.org/1...
>

[danakj, 2013-06-20 17:09:40]

On Thu, Jun 20, 2013 at 1:08 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, Jun 20, 2013 at 1:03 PM, <schenney@chromium.org> wrote:
>
>> I would support (3), although I would create a new file instead of adding
>> it in
>> SkiaUtils.
>>
>> Can we name the class SkRefPtr, or SkiaRefPtr?
>>
>
+1 for SkiaRefPtr (doesn't conflict with skia's own SkRefPtr, and is close
to chromium's skia::RefPtr which it would behave like).


>
>> Could we convince the Skia team to add more Chrome/Blink friendly ref
>> counting
>> to Skia proper?
>>
>
> That's what was attempted here:
> https://codereview.appspot.com/6849109/#msg13 It was the opinion of the
> team at the time that Skia should not provide APIs for this. Of course, I'd
> be really happy to roll src/skia/ext/refptr,h into skia proper as well, but
> am not hopeful of seeing that happen, given the number of senior engineers
> who participated in the conversation last time without that result.
>
>
>>
>>
https://codereview.chromium.**org/17448009/<https://codereview.chromium.org/1...
>>
>
>

[jbroman, 2013-06-21 19:14:58]

Just to provide one more option:

4. In https://codereview.appspot.com/6844116, reed@ added aliases for Skia's
ref/unref to make them compatible with scoped_ptr. Maybe they would be accept a
similar alias deref -> unref, which should make all Skia objects "just work"
with WTF::RefPtr/WTF::PassRefPtr. (Notably both WTF::RefCounted and SkRefCnt
have initial refcount 1.) This would probably add the least code and least
confusing (we already manage non-refcounted Skia objects with WTF::OwnPtr).

[danakj, 2013-06-24 15:23:46]

On Fri, Jun 21, 2013 at 3:14 PM, <jbroman@chromium.org> wrote:

> Just to provide one more option:
>
> 4. In
https://codereview.appspot.**com/6844116<https://codereview.appspot.com/6844116>,
> reed@ added aliases for Skia's
> ref/unref to make them compatible with scoped_ptr. Maybe they would be
> accept a
> similar alias deref -> unref, which should make all Skia objects "just
> work"
> with WTF::RefPtr/WTF::PassRefPtr. (Notably both WTF::RefCounted and
> SkRefCnt
> have initial refcount 1.) This would probably add the least code and least
> confusing (we already manage non-refcounted Skia objects with WTF::OwnPtr).
>

At first glance, this seems reasonable. PassRefPtr has adoptRef to take
ownership of a reference, and a public constructor to add a ref (similar to
AdoptRef and ShareRef in skia::RefPtr).


>
>
https://codereview.chromium.**org/17448009/<https://codereview.chromium.org/1...
>

[jbroman, 2013-07-04 15:37:12]

PTAL.

[Stephen Chennney, 2013-07-08 14:51:58]

Shouldn't this be OwnPtr? Or can Skia hold a ref count on it and keep it alive?

[jbroman, 2013-07-08 14:57:07]

On 2013/07/08 14:51:58, Stephen Chenney wrote:
> Shouldn't this be OwnPtr? Or can Skia hold a ref count on it and keep it
alive?

Skia should be able to ref this object and expect its lifetime to be
correspondingly extended (for instance, if it is recording to an SkPicture it
may do this). A RefPtr holds the same kind of ref that Skia objects do –
assuring the expected lifetime semantics.

[jbroman, 2013-07-15 20:07:46]

On 2013/07/08 14:57:07, jbroman wrote:
> On 2013/07/08 14:51:58, Stephen Chenney wrote:
> > Shouldn't this be OwnPtr? Or can Skia hold a ref count on it and keep it
> alive?
> 
> Skia should be able to ref this object and expect its lifetime to be
> correspondingly extended (for instance, if it is recording to an SkPicture it
> may do this). A RefPtr holds the same kind of ref that Skia objects do –
> assuring the expected lifetime semantics.

ping

[Stephen Chennney, 2013-07-15 21:13:47]

On 2013/07/15 20:07:46, jbroman wrote:
> On 2013/07/08 14:57:07, jbroman wrote:
> > On 2013/07/08 14:51:58, Stephen Chenney wrote:
> > > Shouldn't this be OwnPtr? Or can Skia hold a ref count on it and keep it
> > alive?
> > 
> > Skia should be able to ref this object and expect its lifetime to be
> > correspondingly extended (for instance, if it is recording to an SkPicture
it
> > may do this). A RefPtr holds the same kind of ref that Skia objects do –
> > assuring the expected lifetime semantics.
> 
> ping

Oops. Sorry. LGTM.

[commit-bot: I haz the power, 2013-07-15 21:14:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/17448009/24001

[commit-bot: I haz the power, 2013-07-16 00:44:51]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-07-16 02:45:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/17448009/24001

[commit-bot: I haz the power, 2013-07-16 03:19:04]

Change committed as 154263