sk_sp for gpu.

sk_sp for Ganesh.

Convert use of GrFragmentProcessor, GrGeometryProcessor, and
GrXPFactory to sk_sp. This clarifies ownership and should
reduce reference count churn by moving ownership.
 
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041113004

Committed: https://skia.googlesource.com/skia/+/06ca8ec87cf6fab57cadd043a5ac18c4154a4129

[bungeman-skia, 2016-06-07 22:25:27]

Description was changed from

==========
sk_sp for gpu.
==========

to

==========
sk_sp for gpu.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041113004
==========

[bungeman-skia, 2016-06-07 22:25:49]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-07 22:26:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041113004/1

[bungeman-skia, 2016-06-07 22:27:07]

bungeman@google.com changed reviewers:
+ bsalomon@google.com

[bungeman-skia, 2016-06-07 22:27:09]

At Patch Set 1 this is sure to still have issues and not be landable if anyone
else is using any of this. However, it did compile locally for me, and I'd like
you to take a look at it and make sure it generally makes sense before I put too
much more time into it.

[commit-bot: I haz the power, 2016-06-07 22:33:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-07 22:33:47]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[reed1, 2016-06-08 10:29:42]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2016-06-08 10:29:42]

reed@google.com changed required reviewers:
+ bsalomon@google.com

[reed1, 2016-06-08 10:29:43]

lgtm

[bungeman-skia, 2016-06-08 16:03:44]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-08 16:03:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041113004/20001

[commit-bot: I haz the power, 2016-06-08 16:30:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-08 16:30:02]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2016-06-08 17:05:20]

https://codereview.chromium.org/2041113004/diff/20001/src/gpu/GrFragmentProce...
File src/gpu/GrFragmentProcessor.cpp (right):

https://codereview.chromium.org/2041113004/diff/20001/src/gpu/GrFragmentProce...
src/gpu/GrFragmentProcessor.cpp:337: reinterpret_cast<const
sk_sp<GrFragmentProcessor>*>(fChildProcessors.begin()),
This is the ugliest cast.

https://codereview.chromium.org/2041113004/diff/20001/src/gpu/GrFragmentProce...
src/gpu/GrFragmentProcessor.cpp:357: std::vector<sk_sp<GrFragmentProcessor>>
replacementSeries;
I'm currently using vector instead of SkTArray because SkTArray doesn't have the
equivalent of 'reserve' (SkTDArray has setCount). Also, there may or may not be
a nicer way to do this.

[bungeman-skia, 2016-06-08 18:22:17]

https://codereview.chromium.org/2041113004/diff/20001/include/core/SkColorFil...
File include/core/SkColorFilter.h (right):

https://codereview.chromium.org/2041113004/diff/20001/include/core/SkColorFil...
include/core/SkColorFilter.h:155: virtual sk_sp<GrFragmentProcessor>
asFragmentProcessor(GrContext*) const;
The issue here is that 'return nullptr;' with sk_sp implicitly logically turns
into

auto tmp = sk_sp<GrFragmentProcessor>(nullptr);
return_value = std::move(tmp);
tmp.~sk_sp<GrFragmentProcessor>();
return return_value;

which means destroying the local, which means that GrFragmentProcessor->unref()
needs to be accessible. Of course, the compiler never actually generates code
which does all of this, but the type system says that it has to be there.

I think the solution this issue (which shows up in a few places) is just to
define these out of line.

https://codereview.chromium.org/2041113004/diff/20001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/2041113004/diff/20001/include/core/SkRefCnt.h...
include/core/SkRefCnt.h:268: constexpr sk_sp(std::nullptr_t) : fPtr(nullptr) {}
This allows sk_sp<T>() to be a compile time static. unique_ptr also does this.

https://codereview.chromium.org/2041113004/diff/20001/src/core/SkLocalMatrixS...
File src/core/SkLocalMatrixShader.h (right):

https://codereview.chromium.org/2041113004/diff/20001/src/core/SkLocalMatrixS...
src/core/SkLocalMatrixShader.h:31: sk_sp<GrFragmentProcessor>
asFragmentProcessor(
If the body of this were out of line, we could just use the forward declare in
SkShader.h.

https://codereview.chromium.org/2041113004/diff/20001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/2041113004/diff/20001/src/core/SkShader.cpp#n...
src/core/SkShader.cpp:21: #include "GrFragmentProcessor.h"
Hmmm... not sure this is valid in a cpp file without guards.
However, we need to have a sk_sp<GrFragmentProcessor> to return, which means
GrFragmentProcessor->unref() needs to exist to compile time.

This need for complete types is annoying.

https://codereview.chromium.org/2041113004/diff/20001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/2041113004/diff/20001/src/core/SkXfermode.cpp...
src/core/SkXfermode.cpp:1242: #if SK_SUPPORT_GPU
These includes should go at the top of the file, especially since apparently
there is a block of code which needs this above.

[bsalomon, 2016-06-08 19:50:50]

https://codereview.chromium.org/2041113004/diff/20001/include/core/SkColorFil...
File include/core/SkColorFilter.h (right):

https://codereview.chromium.org/2041113004/diff/20001/include/core/SkColorFil...
include/core/SkColorFilter.h:155: virtual sk_sp<GrFragmentProcessor>
asFragmentProcessor(GrContext*) const;
On 2016/06/08 18:22:16, bungeman-skia wrote:
> The issue here is that 'return nullptr;' with sk_sp implicitly logically turns
> into
> 
> auto tmp = sk_sp<GrFragmentProcessor>(nullptr);
> return_value = std::move(tmp);
> tmp.~sk_sp<GrFragmentProcessor>();
> return return_value;
> 
> which means destroying the local, which means that
GrFragmentProcessor->unref()
> needs to be accessible. Of course, the compiler never actually generates code
> which does all of this, but the type system says that it has to be there.
> 
> I think the solution this issue (which shows up in a few places) is just to
> define these out of line.

sgtm

https://codereview.chromium.org/2041113004/diff/20001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/2041113004/diff/20001/include/core/SkRefCnt.h...
include/core/SkRefCnt.h:268: constexpr sk_sp(std::nullptr_t) : fPtr(nullptr) {}
On 2016/06/08 18:22:16, bungeman-skia wrote:
> This allows sk_sp<T>() to be a compile time static. unique_ptr also does this.

w00t

https://codereview.chromium.org/2041113004/diff/20001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/2041113004/diff/20001/src/core/SkShader.cpp#n...
src/core/SkShader.cpp:21: #include "GrFragmentProcessor.h"
On 2016/06/08 18:22:16, bungeman-skia wrote:
> Hmmm... not sure this is valid in a cpp file without guards.
> However, we need to have a sk_sp<GrFragmentProcessor> to return, which means
> GrFragmentProcessor->unref() needs to exist to compile time.
> 
> This need for complete types is annoying.

I suppose that if this is a problem we could do a "using GrFragmentProcessor = 
SkRefCnt;" in the non-GPU build.

https://codereview.chromium.org/2041113004/diff/20001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/2041113004/diff/20001/src/core/SkXfermode.cpp...
src/core/SkXfermode.cpp:1242: #if SK_SUPPORT_GPU
On 2016/06/08 18:22:16, bungeman-skia wrote:
> These includes should go at the top of the file, especially since apparently
> there is a block of code which needs this above.

Yep, I think the placing of includes intermixed with source is regrettable.

[bungeman-skia, 2016-06-08 20:49:45]

Description was changed from

==========
sk_sp for gpu.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041113004
==========

to

==========
sk_sp for Ganesh.

Convert use of GrFragmentProcessor, GrGeometryProcessor, and
GrXPFactory to sk_sp. This clarifies ownership and should
reduce reference count churn by moving ownership.
 
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041113004
==========

[bungeman-skia, 2016-06-08 20:50:34]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-08 20:50:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041113004/60001

[commit-bot: I haz the power, 2016-06-08 21:23:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-08 21:23:17]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2016-06-08 21:55:39]

https://codereview.chromium.org/2041113004/diff/60001/src/core/SkLocalMatrixS...
File src/core/SkLocalMatrixShader.h (right):

https://codereview.chromium.org/2041113004/diff/60001/src/core/SkLocalMatrixS...
src/core/SkLocalMatrixShader.h:16: #include "GrFragmentProcessor.h"
Should be a forward declaration.

[bungeman-skia, 2016-06-08 22:00:56]

I think that at Patch Set 5 this should be working. Please take a look.

[bungeman-skia, 2016-06-09 03:07:06]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-09 03:07:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041113004/100001

[commit-bot: I haz the power, 2016-06-09 03:21:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-09 03:21:07]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-06-09 13:42:14]

lgtm

[bungeman-skia, 2016-06-09 14:58:57]

The CQ bit was checked by bungeman@google.com

[bungeman-skia, 2016-06-09 14:58:58]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/2041113004/#ps100001
(title: "Reserve correctly.")

[commit-bot: I haz the power, 2016-06-09 14:59:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041113004/100001

[commit-bot: I haz the power, 2016-06-09 15:01:06]

Description was changed from

==========
sk_sp for Ganesh.

Convert use of GrFragmentProcessor, GrGeometryProcessor, and
GrXPFactory to sk_sp. This clarifies ownership and should
reduce reference count churn by moving ownership.
 
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041113004
==========

to

==========
sk_sp for Ganesh.

Convert use of GrFragmentProcessor, GrGeometryProcessor, and
GrXPFactory to sk_sp. This clarifies ownership and should
reduce reference count churn by moving ownership.
 
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2041113004

Committed:
https://skia.googlesource.com/skia/+/06ca8ec87cf6fab57cadd043a5ac18c4154a4129
==========

[commit-bot: I haz the power, 2016-06-09 15:01:08]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/06ca8ec87cf6fab57cadd043a5ac18c4154a4129