Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce Skia utilities to support PassRefPtr <-> sk_sp interoperability:

 * fromSkSp(sk_sp<T>)       - for adopting sk_sps into PassRefPtr
                              (when transferring ownership from Skia->Blink)
 * toSkSp(PassRefPtr<T>)    - for releasing PassRefPtr into sk_sp
                              (when transferring ownership from Blink->Skia)
 * toSkSp(const RefPtr<T>&) - for creating a new sk_sp ref
                              (when sharing ownership with Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}

Committed: https://crrev.com/09fd8033ec57166b9f7a24903bef368fd3da8206
Cr-Commit-Position: refs/heads/master@{#381971}

[f(malita), 2016-03-13 02:19:24]

Description was changed from

==========
Switch Blink SkShader clients to sk_sp APIs

WIP

BUG=skia:5077
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs





BUG=skia:5077
==========

[f(malita), 2016-03-16 13:44:13]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs





BUG=skia:5077
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>&&)       - for adopting sk_sps rvals into PassRevPtr
                                (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>&&) - for adopting PassRefPtr rvals into sk_sp
                                (when transferring ownership from Blink->Skia)

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia and
Skia now expects non-const SkPicture args).


BUG=skia:5077
==========

[f(malita), 2016-03-16 13:56:15]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>&&)       - for adopting sk_sps rvals into PassRevPtr
                                (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>&&) - for adopting PassRefPtr rvals into sk_sp
                                (when transferring ownership from Blink->Skia)

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia and
Skia now expects non-const SkPicture args).


BUG=skia:5077
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>&&)       - for adopting sk_sps rvals into PassRevPtr
                                (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>&&) - for adopting PassRefPtr rvals into sk_sp
                                (when transferring ownership from Blink->Skia)

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia and
Skia now expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@chromium.org,senorblanco@chromium.org,schenney@ch...
==========

[f(malita), 2016-03-16 13:56:15]

fmalita@chromium.org changed reviewers:
+ jbroman@chromium.org, reed@chromium.org, schenney@chromium.org,
senorblanco@chromium.org

[f(malita), 2016-03-16 13:58:21]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>&&)       - for adopting sk_sps rvals into PassRevPtr
                                (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>&&) - for adopting PassRefPtr rvals into sk_sp
                                (when transferring ownership from Blink->Skia)

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia and
Skia now expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@chromium.org,senorblanco@chromium.org,schenney@ch...
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>&&)       - for adopting sk_sps rvals into PassRevPtr
                                (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>&&) - for adopting PassRefPtr rvals into sk_sp
                                (when transferring ownership from Blink->Skia)

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia and
Skia now expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org
==========

[f(malita), 2016-03-16 13:58:21]

fmalita@chromium.org changed reviewers:
+ reed@google.com
- reed@chromium.org, schenney@chromium.org

[f(malita), 2016-03-16 13:59:10]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>&&)       - for adopting sk_sps rvals into PassRevPtr
                                (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>&&) - for adopting PassRefPtr rvals into sk_sp
                                (when transferring ownership from Blink->Skia)

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia and
Skia now expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>&&)       - for adopting sk_sps rvals into PassRevPtr
                                (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>&&) - for adopting PassRefPtr rvals into sk_sp
                                (when transferring ownership from Blink->Skia)

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org
==========

[f(malita), 2016-03-16 14:01:15]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>&&)       - for adopting sk_sps rvals into PassRevPtr
                                (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>&&) - for adopting PassRefPtr rvals into sk_sp
                                (when transferring ownership from Blink->Skia)

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>&&)       - for adopting sk_sps rvals into PassRevPtr
                                (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>&&) - for adopting PassRefPtr rvals into sk_sp
                                (when transferring ownership from Blink->Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org
==========

[f(malita), 2016-03-16 14:06:45]

Alternative approach to sk_sps in Blink: rather than using the new smart pointer
type directly (as attempted in https://codereview.chromium.org/1779833002/),
make it easily convertible to/from PassRefPtr, let Blink continue using its
native wrappers internally, and only update the API boundaries.

[f(malita), 2016-03-16 14:09:33]

fmalita@chromium.org changed reviewers:
+ bungeman@chromium.org

[jbroman, 2016-03-16 14:43:08]

I'm a little uneasy about the name "adoptRef" because I have such strong
associations with the meaning of that function, but I guess this is conceptually
similar enough that I'm OK with sharing that name.

https://codereview.chromium.org/1789063005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h (right):

https://codereview.chromium.org/1789063005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h:187: //   is
preferred when feasible because it also skips the adopt/move op)
Why? The adopt/move will presumably be elided by the compiler.

Is this just for readability reasons? (If so, is it necessary to call this out,
since presumably all reviewers are trying to make the code readable?)

https://codereview.chromium.org/1789063005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h:199: template
<typename T> PassRefPtr<T> adoptRef(sk_sp<T>&& sp)
I also think taking the sk_sp by value is probably simpler (and this is the
usual way of passing C++11-style smart pointers). It would also allow this code
to be valid (with an implicit copy):

sk_sp<SkShader> shader;
RefPtr<SkShader> shader2 = adoptRef(shader);
// ownership is shared; would have been transferred if
adoptRef(std::move(shader)) were used

The extra ref that is required is taken when constructing the sk_sp argument,
and the ref count doesn't thrash.

https://codereview.chromium.org/1789063005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h:204: template
<typename T> sk_sp<T> adoptSkSp(PassRefPtr<T>&& ref)
To be consistent with other usage of PassRefPtr, I think this should probably
just be PassRefPtr<T> without the rvalue reference. PassRefPtr is already
basically an rvalue-emulation type anyhow.

This should also make the argument accept a RefPtr due to the implicit
conversion, so you should then be able to write in case (c) above::

RefPtr<SkShader> m_shader = adoptRef(SkShader::MakeFoo(...));
paint.setShader(adoptSkSp(m_shader));

instead of needing to explicitly construct a PassRefPtr or extract the raw ptr.

[f(malita), 2016-03-16 16:51:45]

On 2016/03/16 14:43:08, jbroman wrote:
> I'm a little uneasy about the name "adoptRef" because I have such strong
> associations with the meaning of that function, but I guess this is
conceptually
> similar enough that I'm OK with sharing that name.

Somewhat tangential: ideally, PassRefPtr should be freely convertible to/from
sk_sp (same as it behaves wrt RefPtr),  but adding Skia conversion
operators/ctors to WTF::PassRefPtr smells.  Overloaded adoption helpers is the
best I can think of at this point.

https://codereview.chromium.org/1789063005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h (right):

https://codereview.chromium.org/1789063005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h:187: //   is
preferred when feasible because it also skips the adopt/move op)
On 2016/03/16 14:43:08, jbroman wrote:
> Why? The adopt/move will presumably be elided by the compiler.

Experimentally, the first example above does not elide the move ctor - so using
the non-inlined version incurs an extra move op.

But move is cheap and IIRC Ben mentioned we may even expect it to be optimized
out, so it's possibly premature to worry about it at this point.

Removed the comment.

> Is this just for readability reasons?

I actually find the non-inlined version more readable :)

https://codereview.chromium.org/1789063005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h:199: template
<typename T> PassRefPtr<T> adoptRef(sk_sp<T>&& sp)
On 2016/03/16 14:43:08, jbroman wrote:
> I also think taking the sk_sp by value is probably simpler (and this is the
> usual way of passing C++11-style smart pointers). It would also allow this
code
> to be valid (with an implicit copy):
> 
> sk_sp<SkShader> shader;
> RefPtr<SkShader> shader2 = adoptRef(shader);
> // ownership is shared; would have been transferred if
> adoptRef(std::move(shader)) were used
> 
> The extra ref that is required is taken when constructing the sk_sp argument,
> and the ref count doesn't thrash.

We've had a little discussion regarding val vs. rval ref arguments internally
(for Skia setter params).  The new WK style guide for example recommends using
rval ref params when the ownership transfer is guaranteed to occur (setters or
adopting in this case): https://webkit.org/blog/5381/refptr-basics/

I kind of like the explicitness of forcing clients to create rvals - it catches
the case of someone forgetting to std::move in your example above, plus it
avoids the whole uncertainty about pass-by-val copy elision/optimization.

It is more restrictive though, possibly confusing, and likely a premature
optimzation.

Switched to pass-by-val.

https://codereview.chromium.org/1789063005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h:204: template
<typename T> sk_sp<T> adoptSkSp(PassRefPtr<T>&& ref)
On 2016/03/16 14:43:08, jbroman wrote:
> To be consistent with other usage of PassRefPtr, I think this should probably
> just be PassRefPtr<T> without the rvalue reference. PassRefPtr is already
> basically an rvalue-emulation type anyhow.
> 
> This should also make the argument accept a RefPtr due to the implicit
> conversion, so you should then be able to write in case (c) above::
> 
> RefPtr<SkShader> m_shader = adoptRef(SkShader::MakeFoo(...));
> paint.setShader(adoptSkSp(m_shader));
> 
> instead of needing to explicitly construct a PassRefPtr or extract the raw
ptr.

Makes sense, done.

[f(malita), 2016-03-16 16:52:53]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>&&)       - for adopting sk_sps rvals into PassRevPtr
                                (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>&&) - for adopting PassRefPtr rvals into sk_sp
                                (when transferring ownership from Blink->Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>)       - for adopting sk_sps rvals into PassRevPtr
                              (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>) - for adopting PassRefPtr rvals into sk_sp
                              (when transferring ownership from Blink->Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org
==========

[jbroman, 2016-03-16 17:47:02]

lgtm

https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Gradient.cpp (right):

https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Gradient.cpp:259:
paint.setShader(adoptSkSp<SkShader>(refShader()));
Does this not work without the explicit type parameter?

[danakj, 2016-03-16 17:54:53]

https://codereview.chromium.org/1789063005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h (right):

https://codereview.chromium.org/1789063005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h:199: template
<typename T> PassRefPtr<T> adoptRef(sk_sp<T>&& sp)
On 2016/03/16 16:51:45, f(malita) wrote:
> On 2016/03/16 14:43:08, jbroman wrote:
> > I also think taking the sk_sp by value is probably simpler (and this is the
> > usual way of passing C++11-style smart pointers). It would also allow this
> code
> > to be valid (with an implicit copy):
> > 
> > sk_sp<SkShader> shader;
> > RefPtr<SkShader> shader2 = adoptRef(shader);
> > // ownership is shared; would have been transferred if
> > adoptRef(std::move(shader)) were used
> > 
> > The extra ref that is required is taken when constructing the sk_sp
argument,
> > and the ref count doesn't thrash.
> 
> We've had a little discussion regarding val vs. rval ref arguments internally
> (for Skia setter params).  The new WK style guide for example recommends using
> rval ref params when the ownership transfer is guaranteed to occur (setters or
> adopting in this case): https://webkit.org/blog/5381/refptr-basics/
> 
> I kind of like the explicitness of forcing clients to create rvals - it
catches
> the case of someone forgetting to std::move in your example above, plus it
> avoids the whole uncertainty about pass-by-val copy elision/optimization.
> 
> It is more restrictive though, possibly confusing, and likely a premature
> optimzation.
> 
> Switched to pass-by-val.

(google style guide also says not to do write && except for move
constructor/operator= and perfect forwarding, which we (sorta, but will?) follow
in blink too)

[f(malita), 2016-03-16 18:17:43]

On 2016/03/16 17:54:53, danakj wrote:
> 
> (google style guide also says not to do write && except for move
> constructor/operator= and perfect forwarding, which we (sorta, but will?)
follow
> in blink too)

Thanks, good to know.

That means the current Skia setter direction is well aligned with Chromium/Blink
style and we should keep it this way :)

Speaking of Blink refptr/passrefptr, do you happen to know what's the long term
plan?  Are we going to switch to cc smart pointers at some point or modernize
refptr (and drop passrefptr, like WK did)?

https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Gradient.cpp (right):

https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Gradient.cpp:259:
paint.setShader(adoptSkSp<SkShader>(refShader()));
On 2016/03/16 17:47:02, jbroman wrote:
> Does this not work without the explicit type parameter?

Doh, it sure does for PassRefPtr.  I got carried away with the RefPtr pattern.

Done.

[jbroman, 2016-03-16 18:20:46]

On 2016/03/16 at 18:17:43, fmalita wrote:
> On 2016/03/16 17:54:53, danakj wrote:
> > 
> > (google style guide also says not to do write && except for move
> > constructor/operator= and perfect forwarding, which we (sorta, but will?)
follow
> > in blink too)
> 
> Thanks, good to know.
> 
> That means the current Skia setter direction is well aligned with
Chromium/Blink style and we should keep it this way :)
> 
> Speaking of Blink refptr/passrefptr, do you happen to know what's the long
term plan?  Are we going to switch to cc smart pointers at some point or
modernize refptr (and drop passrefptr, like WK did)?
> 
>
https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/Gradient.cpp (right):
> 
>
https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/Gradient.cpp:259:
paint.setShader(adoptSkSp<SkShader>(refShader()));
> On 2016/03/16 17:47:02, jbroman wrote:
> > Does this not work without the explicit type parameter?
> 
> Doh, it sure does for PassRefPtr.  I got carried away with the RefPtr pattern.
> 
> Done.

We could probably add a (const RefPtr<T>&) overload to adoptRef to avoid the
explicit template parameter in that case since it's so common, but I don't feel
strongly either way.

[jbroman, 2016-03-16 18:22:25]

On 2016/03/16 at 18:20:46, jbroman wrote:
> On 2016/03/16 at 18:17:43, fmalita wrote:
> > On 2016/03/16 17:54:53, danakj wrote:
> > > 
> > > (google style guide also says not to do write && except for move
> > > constructor/operator= and perfect forwarding, which we (sorta, but will?)
follow
> > > in blink too)
> > 
> > Thanks, good to know.
> > 
> > That means the current Skia setter direction is well aligned with
Chromium/Blink style and we should keep it this way :)
> > 
> > Speaking of Blink refptr/passrefptr, do you happen to know what's the long
term plan?  Are we going to switch to cc smart pointers at some point or
modernize refptr (and drop passrefptr, like WK did)?
> > 
> >
https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/platform/graphics/Gradient.cpp (right):
> > 
> >
https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/graphics/Gradient.cpp:259:
paint.setShader(adoptSkSp<SkShader>(refShader()));
> > On 2016/03/16 17:47:02, jbroman wrote:
> > > Does this not work without the explicit type parameter?
> > 
> > Doh, it sure does for PassRefPtr.  I got carried away with the RefPtr
pattern.
> > 
> > Done.
> 
> We could probably add a (const RefPtr<T>&) overload to adoptRef to avoid the
explicit template parameter in that case since it's so common, but I don't feel
strongly either way.

[jbroman, 2016-03-16 18:25:12]

On 2016/03/16 at 18:20:46, jbroman wrote:
> On 2016/03/16 at 18:17:43, fmalita wrote:
> > On 2016/03/16 17:54:53, danakj wrote:
> > > 
> > > (google style guide also says not to do write && except for move
> > > constructor/operator= and perfect forwarding, which we (sorta, but will?)
follow
> > > in blink too)
> > 
> > Thanks, good to know.
> > 
> > That means the current Skia setter direction is well aligned with
Chromium/Blink style and we should keep it this way :)
> > 
> > Speaking of Blink refptr/passrefptr, do you happen to know what's the long
term plan?  Are we going to switch to cc smart pointers at some point or
modernize refptr (and drop passrefptr, like WK did)?

I know yutak is working on an OwnPtr -> unique_ptr migration. You could ask him
if there's a plan for RefPtr (I haven't heard one yet), but I expect it will at
least be changed to look more like normal C++11 smart pointers.

> >
https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/platform/graphics/Gradient.cpp (right):
> > 
> >
https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/graphics/Gradient.cpp:259:
paint.setShader(adoptSkSp<SkShader>(refShader()));
> > On 2016/03/16 17:47:02, jbroman wrote:
> > > Does this not work without the explicit type parameter?
> > 
> > Doh, it sure does for PassRefPtr.  I got carried away with the RefPtr
pattern.
> > 
> > Done.
> 
> We could probably add a (const RefPtr<T>&) overload to adoptRef to avoid the
explicit template parameter in that case since it's so common, but I don't feel
strongly either way.

by which I mean adoptSkSp, of course

[danakj, 2016-03-16 18:34:34]

On Wed, Mar 16, 2016 at 11:25 AM, <jbroman@chromium.org> wrote:

> On 2016/03/16 at 18:20:46, jbroman wrote:
> > On 2016/03/16 at 18:17:43, fmalita wrote:
> > > On 2016/03/16 17:54:53, danakj wrote:
> > > >
> > > > (google style guide also says not to do write && except for move
> > > > constructor/operator= and perfect forwarding, which we (sorta, but
> will?)
> follow
> > > > in blink too)
> > >
> > > Thanks, good to know.
> > >
> > > That means the current Skia setter direction is well aligned with
> Chromium/Blink style and we should keep it this way :)
> > >
> > > Speaking of Blink refptr/passrefptr, do you happen to know what's the
> long
> term plan? Are we going to switch to cc smart pointers at some point or
> modernize refptr (and drop passrefptr, like WK did)?
>
> I know yutak is working on an OwnPtr -> unique_ptr migration. You could
> ask him
> if there's a plan for RefPtr (I haven't heard one yet), but I expect it
> will at
> least be changed to look more like normal C++11 smart pointers.
>

hehe, I wrote this basically then saw your reply. So, ya that ^^


>
>
> > >
>
>
https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/platform/graphics/Gradient.cpp (right):
> > >
> > >
>
>
https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/platform/graphics/Gradient.cpp:259:
> paint.setShader(adoptSkSp<SkShader>(refShader()));
> > > On 2016/03/16 17:47:02, jbroman wrote:
> > > > Does this not work without the explicit type parameter?
> > >
> > > Doh, it sure does for PassRefPtr. I got carried away with the RefPtr
> pattern.
> > >
> > > Done.
> >
> > We could probably add a (const RefPtr<T>&) overload to adoptRef to avoid
> the
> explicit template parameter in that case since it's so common, but I don't
> feel
> strongly either way.
>
> by which I mean adoptSkSp, of course
>
> https://codereview.chromium.org/1789063005/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2016-03-16 18:42:21]

On Wed, Mar 16, 2016 at 11:25 AM, <jbroman@chromium.org> wrote:

> On 2016/03/16 at 18:20:46, jbroman wrote:
> > On 2016/03/16 at 18:17:43, fmalita wrote:
> > > On 2016/03/16 17:54:53, danakj wrote:
> > > >
> > > > (google style guide also says not to do write && except for move
> > > > constructor/operator= and perfect forwarding, which we (sorta, but
> will?)
> follow
> > > > in blink too)
> > >
> > > Thanks, good to know.
> > >
> > > That means the current Skia setter direction is well aligned with
> Chromium/Blink style and we should keep it this way :)
> > >
> > > Speaking of Blink refptr/passrefptr, do you happen to know what's the
> long
> term plan? Are we going to switch to cc smart pointers at some point or
> modernize refptr (and drop passrefptr, like WK did)?
>
> I know yutak is working on an OwnPtr -> unique_ptr migration. You could
> ask him
> if there's a plan for RefPtr (I haven't heard one yet), but I expect it
> will at
> least be changed to look more like normal C++11 smart pointers.
>

hehe, I wrote this basically then saw your reply. So, ya that ^^


>
>
> > >
>
>
https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/platform/graphics/Gradient.cpp (right):
> > >
> > >
>
>
https://codereview.chromium.org/1789063005/diff/100001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/platform/graphics/Gradient.cpp:259:
> paint.setShader(adoptSkSp<SkShader>(refShader()));
> > > On 2016/03/16 17:47:02, jbroman wrote:
> > > > Does this not work without the explicit type parameter?
> > >
> > > Doh, it sure does for PassRefPtr. I got carried away with the RefPtr
> pattern.
> > >
> > > Done.
> >
> > We could probably add a (const RefPtr<T>&) overload to adoptRef to avoid
> the
> explicit template parameter in that case since it's so common, but I don't
> feel
> strongly either way.
>
> by which I mean adoptSkSp, of course
>
> https://codereview.chromium.org/1789063005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reed1, 2016-03-16 19:39:17]

lgtm

[f(malita), 2016-03-16 19:50:48]

On 2016/03/16 18:25:12, jbroman wrote:
> On 2016/03/16 at 18:20:46, jbroman wrote:
> > We could probably add a (const RefPtr<T>&) overload to adoptRef to avoid the
> explicit template parameter in that case since it's so common, but I don't
feel
> strongly either way.
> 
> by which I mean adoptSkSp, of course

I'm a bit hesitant to use "adopt" for that flavor, as it may suggest we're
somehow stealing the ref from a const RefPtr.

Maybe shareSkSp or refSkSp?

Either way, we can bikeshed the name and add that separately (going to land this
to unblock one of Mike's CLs).

[f(malita), 2016-03-16 19:51:49]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2016-03-16 19:51:50]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1789063005/#ps120001
(title: "drop unneeded adoptSkSp type arg")

[danakj, 2016-03-16 19:52:07]

On Wed, Mar 16, 2016 at 12:50 PM, <fmalita@chromium.org> wrote:

> On 2016/03/16 18:25:12, jbroman wrote:
> > On 2016/03/16 at 18:20:46, jbroman wrote:
> > > We could probably add a (const RefPtr<T>&) overload to adoptRef to
> avoid the
> > explicit template parameter in that case since it's so common, but I
> don't
> feel
> > strongly either way.
> >
> > by which I mean adoptSkSp, of course
>
> I'm a bit hesitant to use "adopt" for that flavor, as it may suggest we're
> somehow stealing the ref from a const RefPtr.
>
> Maybe shareSkSp or refSkSp?
>
> Either way, we can bikeshed the name and add that separately (going to
> land this
> to unblock one of Mike's CLs).
>

FWIW, usually when we convert to/from skia types we write something like
SkSpToRefPtr and RefPtrToSkSp.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-03-16 19:52:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1789063005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1789063005/120001

[danakj, 2016-03-16 19:58:22]

On Wed, Mar 16, 2016 at 12:50 PM, <fmalita@chromium.org> wrote:

> On 2016/03/16 18:25:12, jbroman wrote:
> > On 2016/03/16 at 18:20:46, jbroman wrote:
> > > We could probably add a (const RefPtr<T>&) overload to adoptRef to
> avoid the
> > explicit template parameter in that case since it's so common, but I
> don't
> feel
> > strongly either way.
> >
> > by which I mean adoptSkSp, of course
>
> I'm a bit hesitant to use "adopt" for that flavor, as it may suggest we're
> somehow stealing the ref from a const RefPtr.
>
> Maybe shareSkSp or refSkSp?
>
> Either way, we can bikeshed the name and add that separately (going to
> land this
> to unblock one of Mike's CLs).
>

FWIW, usually when we convert to/from skia types we write something like
SkSpToRefPtr and RefPtrToSkSp.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-03-16 22:10:53]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>)       - for adopting sk_sps rvals into PassRevPtr
                              (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>) - for adopting PassRefPtr rvals into sk_sp
                              (when transferring ownership from Blink->Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>)       - for adopting sk_sps rvals into PassRevPtr
                              (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>) - for adopting PassRefPtr rvals into sk_sp
                              (when transferring ownership from Blink->Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org
==========

[commit-bot: I haz the power, 2016-03-16 22:10:55]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-16 22:14:27]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>)       - for adopting sk_sps rvals into PassRevPtr
                              (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>) - for adopting PassRefPtr rvals into sk_sp
                              (when transferring ownership from Blink->Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).


BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>)       - for adopting sk_sps rvals into PassRevPtr
                              (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>) - for adopting PassRefPtr rvals into sk_sp
                              (when transferring ownership from Blink->Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}
==========

[commit-bot: I haz the power, 2016-03-16 22:14:28]

Patchset 7 (id:??) landed as
https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}

[fgorski, 2016-03-16 23:11:38]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1814473003/ by fgorski@chromium.org.

The reason for reverting is: Suspecting this patch to have caused the
compilation failure:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20non-Oilp...

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20non-Oilp....

[sof, 2016-03-17 06:11:37]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>)       - for adopting sk_sps rvals into PassRevPtr
                              (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>) - for adopting PassRefPtr rvals into sk_sp
                              (when transferring ownership from Blink->Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>)       - for adopting sk_sps rvals into PassRevPtr
                              (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>) - for adopting PassRefPtr rvals into sk_sp
                              (when transferring ownership from Blink->Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}
==========

[f(malita), 2016-03-17 14:12:50]

Non-Oilpan MSVC-only build borkage:

FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma/gomacc
"E:\b\depot_tools\win_toolchain\vs_files\a3796183a9fc4d22a687c5212b9c76dbd136d70d\VC\bin\amd64_x86\cl.exe"
/nologo /showIncludes /FC
@obj\third_party\WebKit\Source\core\layout\svg\webcore_svg.LayoutSVGResourcePaintServer.obj.rsp
/c
..\..\third_party\WebKit\Source\core\layout\svg\LayoutSVGResourcePaintServer.cpp
/Foobj\third_party\WebKit\Source\core\layout\svg\webcore_svg.LayoutSVGResourcePaintServer.obj
/Fdobj\third_party\WebKit\Source\core\webcore_svg.cc.pdb 
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\platform\heap\handle.h(1189):
error C2672: 'adoptRef': no matching overloaded function found
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\core\dom\document.h(221):
note: see reference to function template instantiation
'WTF::PassRefPtr<blink::Document> blink::adoptRefWillBeNoop<blink::Document>(T
*)' being compiled
        with
        [
            T=blink::Document
        ]
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\platform\heap\handle.h(1189):
error C2784: 'WTF::PassRefPtr<T> blink::adoptRef(sk_sp<T>)': could not deduce
template argument for 'sk_sp<T>' from 'blink::Document *'
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\platform\graphics\skia\skiautils.h(191):
note: see declaration of 'blink::adoptRef'

Apparently MSVC is not happy with the adoptRef overload being in a different
namespace (blink:: vs. WTF::) and gets fixated on the sk_sp version.

I ran some build tests in https://codereview.chromium.org/1810813002/ and moving
the overload to the original namespace (WTF::) seems to shut MSVC up.

PTAL

[jbroman, 2016-03-17 14:24:26]

On 2016/03/17 at 14:12:50, fmalita wrote:
> Non-Oilpan MSVC-only build borkage:
> 
> FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma/gomacc
"E:\b\depot_tools\win_toolchain\vs_files\a3796183a9fc4d22a687c5212b9c76dbd136d70d\VC\bin\amd64_x86\cl.exe"
/nologo /showIncludes /FC
@obj\third_party\WebKit\Source\core\layout\svg\webcore_svg.LayoutSVGResourcePaintServer.obj.rsp
/c
..\..\third_party\WebKit\Source\core\layout\svg\LayoutSVGResourcePaintServer.cpp
/Foobj\third_party\WebKit\Source\core\layout\svg\webcore_svg.LayoutSVGResourcePaintServer.obj
/Fdobj\third_party\WebKit\Source\core\webcore_svg.cc.pdb 
>
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\platform\heap\handle.h(1189):
error C2672: 'adoptRef': no matching overloaded function found
>
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\core\dom\document.h(221):
note: see reference to function template instantiation
'WTF::PassRefPtr<blink::Document> blink::adoptRefWillBeNoop<blink::Document>(T
*)' being compiled
>         with
>         [
>             T=blink::Document
>         ]
>
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\platform\heap\handle.h(1189):
error C2784: 'WTF::PassRefPtr<T> blink::adoptRef(sk_sp<T>)': could not deduce
template argument for 'sk_sp<T>' from 'blink::Document *'
>
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\platform\graphics\skia\skiautils.h(191):
note: see declaration of 'blink::adoptRef'
> 
> Apparently MSVC is not happy with the adoptRef overload being in a different
namespace (blink:: vs. WTF::) and gets fixated on the sk_sp version.
> 
> I ran some build tests in https://codereview.chromium.org/1810813002/ and
moving the overload to the original namespace (WTF::) seems to shut MSVC up.
> 
> PTAL

I'm not thrilled with reopening WTF in Source/platform/. How about just changing
the name? e.g. blink::adoptRefFromSkia?

[f(malita), 2016-03-17 17:05:43]

On 2016/03/17 14:24:26, jbroman wrote:
> I'm not thrilled with reopening WTF in Source/platform/. How about just
changing
> the name? e.g. blink::adoptRefFromSkia?

Ack.

Updated to adoptSkSp(sk_sp<T>), toSkSp(PassRefPtr<T>), toSkSp(const RefPtr<T>&).

[jbroman, 2016-03-17 17:19:13]

lgtm

Please also update the CL description and confirm that the latest patchset fixes
the build breakage that caused the revert.

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Gradient.h (right):

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Gradient.h:129: PassRefPtr<SkShader>
refShader();
nit: didn't notice this earlier, but do we have this addref-and-return pattern
elsewhere? I'd have expected either
- this returning SkShader* (and the caller using sk_ref_sp), or
- returning "const RefPtr<SkShader>&" (well, that's the shared_ptr-like pattern,
but we don't do this elsewhere in Blink), or
- having the method be "void updateShader()" and having the caller know that the
result is in m_shader (since it's in this class)

TBH, though, I don't feel strongly, especially since I lgtmed the earlier patch
with this. So if you like it the way it is, you can land without addressing this
nit.

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h (right):

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h:189: //    
RefPtr<SkShader> m_shader = adoptSkSp(SkShader::MakeFoo(...));
super-nit: maybe remove m_ as this doesn't look like it's a member in this
example?

[f(malita), 2016-03-17 17:41:24]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce a couple of Skia utilities to support PassRefPtr <-> sk_sp
interoperability:

 * adoptRef(sk_sp<T>)       - for adopting sk_sps rvals into PassRevPtr
                              (when transferring ownership from Skia->Blink)
 * adoptSkSp(PassRefPtr<T>) - for adopting PassRefPtr rvals into sk_sp
                              (when transferring ownership from Blink->Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce Skia utilities to support PassRefPtr <-> sk_sp interoperability:

 * adoptSkSp(sk_sp<T>)      - for adopting sk_sps into PassRefPtr
                              (when transferring ownership from Skia->Blink)
 * toSkSp(PassRefPtr<T>)    - for releasing PassRefPtr into sk_sp
                              (when transferring ownership from Blink->Skia)
 * toSkSp(const RefPtr<T>&) - for creating a new sk_sp ref
                              (when sharing ownership with Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}
==========

[Stephen White, 2016-03-17 17:50:40]

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Gradient.h (right):

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Gradient.h:129: PassRefPtr<SkShader>
refShader();
On 2016/03/17 17:19:13, jbroman wrote:
> nit: didn't notice this earlier, but do we have this addref-and-return pattern
> elsewhere? I'd have expected either
> - this returning SkShader* (and the caller using sk_ref_sp), or
> - returning "const RefPtr<SkShader>&" (well, that's the shared_ptr-like
pattern,
> but we don't do this elsewhere in Blink), or
> - having the method be "void updateShader()" and having the caller know that
the
> result is in m_shader (since it's in this class)
> 
> TBH, though, I don't feel strongly, especially since I lgtmed the earlier
patch
> with this. So if you like it the way it is, you can land without addressing
this
> nit.

I don't think we have ref...() as a verb elsewhere in Blink, and and that sounds
more like explicit ref/unref to me (which we want to avoid), so let's just
call it shader(), as it was before.

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImagePattern.cpp (right):

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImagePattern.cpp:36: return
adoptSkSp(SkShader::MakeColorShader(SK_ColorTRANSPARENT));
Bikeshed: I'd actually to avoid "adopt" here, ("skSpToPassRefPtr" or "fromSkSp"
or "toPassRefPtr" or something), since adopt to me means adopting a bare
pointer.

[f(malita), 2016-03-17 17:59:19]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce Skia utilities to support PassRefPtr <-> sk_sp interoperability:

 * adoptSkSp(sk_sp<T>)      - for adopting sk_sps into PassRefPtr
                              (when transferring ownership from Skia->Blink)
 * toSkSp(PassRefPtr<T>)    - for releasing PassRefPtr into sk_sp
                              (when transferring ownership from Blink->Skia)
 * toSkSp(const RefPtr<T>&) - for creating a new sk_sp ref
                              (when sharing ownership with Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce Skia utilities to support PassRefPtr <-> sk_sp interoperability:

 * fromSkSp(sk_sp<T>)       - for adopting sk_sps into PassRefPtr
                              (when transferring ownership from Skia->Blink)
 * toSkSp(PassRefPtr<T>)    - for releasing PassRefPtr into sk_sp
                              (when transferring ownership from Blink->Skia)
 * toSkSp(const RefPtr<T>&) - for creating a new sk_sp ref
                              (when sharing ownership with Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}
==========

[f(malita), 2016-03-17 18:04:42]

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Gradient.h (right):

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Gradient.h:129: PassRefPtr<SkShader>
refShader();
On 2016/03/17 17:19:13, jbroman wrote:
> nit: didn't notice this earlier, but do we have this addref-and-return pattern
> elsewhere? I'd have expected either
> - this returning SkShader* (and the caller using sk_ref_sp), or
> - returning "const RefPtr<SkShader>&" (well, that's the shared_ptr-like
pattern,
> but we don't do this elsewhere in Blink), or
> - having the method be "void updateShader()" and having the caller know that
the
> result is in m_shader (since it's in this class)

Probably not a common pattern in Blink.  I was planning to refactor this
similarly
to Pattern (which has the lazy logic in applyToPaint and not in the shader
builder)
but didn't want to pollute this CL too much.  Makes sense to leave alone for
now.

Done (switched back to the original rawptr API/your first suggestion).

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Gradient.h:129: PassRefPtr<SkShader>
refShader();
On 2016/03/17 17:50:40, Stephen White wrote:
> On 2016/03/17 17:19:13, jbroman wrote:
> > nit: didn't notice this earlier, but do we have this addref-and-return
pattern
> > elsewhere? I'd have expected either
> > - this returning SkShader* (and the caller using sk_ref_sp), or
> > - returning "const RefPtr<SkShader>&" (well, that's the shared_ptr-like
> pattern,
> > but we don't do this elsewhere in Blink), or
> > - having the method be "void updateShader()" and having the caller know that
> the
> > result is in m_shader (since it's in this class)
> > 
> > TBH, though, I don't feel strongly, especially since I lgtmed the earlier
> patch
> > with this. So if you like it the way it is, you can land without addressing
> this
> > nit.
> 
> I don't think we have ref...() as a verb elsewhere in Blink, and and that
sounds
> more like explicit ref/unref to me (which we want to avoid), so let's just
> call it shader(), as it was before.

Done.

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImagePattern.cpp (right):

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImagePattern.cpp:36: return
adoptSkSp(SkShader::MakeColorShader(SK_ColorTRANSPARENT));
On 2016/03/17 17:50:40, Stephen White wrote:
> Bikeshed: I'd actually to avoid "adopt" here, ("skSpToPassRefPtr" or
"fromSkSp"
> or "toPassRefPtr" or something), since adopt to me means adopting a bare
> pointer.

Since jbroman also suggested fromSkSp, fromSkSp it is :)

Done.

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h (right):

https://codereview.chromium.org/1789063005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h:189: //    
RefPtr<SkShader> m_shader = adoptSkSp(SkShader::MakeFoo(...));
On 2016/03/17 17:19:13, jbroman wrote:
> super-nit: maybe remove m_ as this doesn't look like it's a member in this
> example?

Done.

[f(malita), 2016-03-18 12:37:17]

PTAL

[jbroman, 2016-03-18 14:18:48]

still lgtm

[Stephen White, 2016-03-18 14:48:27]

Still LGTM too.

https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GeneratedImage.cpp (right):

https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GeneratedImage.cpp:57:
RefPtr<Pattern> picturePattern =
Pattern::createPicturePattern(tilePicture.release());
Out of curiosity, is this release() necessary? Or just to avoid churn?

https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Gradient.cpp (right):

https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Gradient.cpp:259:
paint.setShader(sk_ref_sp(shader()));
Perhaps m_gradient should be sk_sp<SkShader>, and shader() should return
sk_sp<Shader>, if it's only ever used for Skia calls. Then we could avoid the
fromSkSp() calls above, and the sk_ref_sp() here.

Doesn't have to be in this patch, though.

https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:311:
RefPtr<SkPicture> picture = adoptRef(m_pictureRecorder.endRecordingAsPicture());
Not related to this patch, but are there plans to make an sk_sp-ified version of
endRecordingAsPicture() over in Skia?

[reed1, 2016-03-18 14:58:15]

On 2016/03/18 14:48:27, Stephen White wrote:
> Still LGTM too.
> 
>
https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/GeneratedImage.cpp (right):
> 
>
https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/GeneratedImage.cpp:57:
> RefPtr<Pattern> picturePattern =
> Pattern::createPicturePattern(tilePicture.release());
> Out of curiosity, is this release() necessary? Or just to avoid churn?
> 
>
https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/Gradient.cpp (right):
> 
>
https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/Gradient.cpp:259:
> paint.setShader(sk_ref_sp(shader()));
> Perhaps m_gradient should be sk_sp<SkShader>, and shader() should return
> sk_sp<Shader>, if it's only ever used for Skia calls. Then we could avoid the
> fromSkSp() calls above, and the sk_ref_sp() here.

+1 

> 
> Doesn't have to be in this patch, though.
> 
>
https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):
> 
>
https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:311:
> RefPtr<SkPicture> picture =
adoptRef(m_pictureRecorder.endRecordingAsPicture());
> Not related to this patch, but are there plans to make an sk_sp-ified version
of
> endRecordingAsPicture() over in Skia?

Just landed this morning.

[f(malita), 2016-03-18 15:11:53]

https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GeneratedImage.cpp (right):

https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GeneratedImage.cpp:57:
RefPtr<Pattern> picturePattern =
Pattern::createPicturePattern(tilePicture.release());
On 2016/03/18 14:48:27, Stephen White wrote:
> Out of curiosity, is this release() necessary? Or just to avoid churn?

The latter: it would work fine without a release, but we'd be spinning the ref
count unnecessarily.

https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Gradient.cpp (right):

https://codereview.chromium.org/1789063005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Gradient.cpp:259:
paint.setShader(sk_ref_sp(shader()));
On 2016/03/18 14:48:27, Stephen White wrote:
> Perhaps m_gradient should be sk_sp<SkShader>, and shader() should return
> sk_sp<Shader>, if it's only ever used for Skia calls. Then we could avoid the
> fromSkSp() calls above, and the sk_ref_sp() here.
> 
> Doesn't have to be in this patch, though.

I'm not sure what the right balance for Blink's sk_sp-ization is, I guess we're
still in the phase of figuring the optimal mix :)

Initially I started down the path of "let's just use sk_sp for all Skia refs in
Blink", but that quickly exploded into massive API refactorings because Skia
objects are pervasive these days.  Adding Skia includes to a gazillion Blink
headers didn't seem right, so I decided to back off that path.

My current thought is to use sk_sp for local/private refs, but don't expose it
in Blink's internal APIs.

In this particular case, I think the shader is private so it should be fine to
convert without spilling into other Blink parts.

Will do that in a follow-up.

[f(malita), 2016-03-18 15:12:41]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2016-03-18 15:12:42]

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/1789063005/#ps220001
(title: "fromSkSp, review comments")

[commit-bot: I haz the power, 2016-03-18 15:12:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1789063005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1789063005/220001

[commit-bot: I haz the power, 2016-03-18 15:18:05]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce Skia utilities to support PassRefPtr <-> sk_sp interoperability:

 * fromSkSp(sk_sp<T>)       - for adopting sk_sps into PassRefPtr
                              (when transferring ownership from Skia->Blink)
 * toSkSp(PassRefPtr<T>)    - for releasing PassRefPtr into sk_sp
                              (when transferring ownership from Blink->Skia)
 * toSkSp(const RefPtr<T>&) - for creating a new sk_sp ref
                              (when sharing ownership with Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce Skia utilities to support PassRefPtr <-> sk_sp interoperability:

 * fromSkSp(sk_sp<T>)       - for adopting sk_sps into PassRefPtr
                              (when transferring ownership from Skia->Blink)
 * toSkSp(PassRefPtr<T>)    - for releasing PassRefPtr into sk_sp
                              (when transferring ownership from Blink->Skia)
 * toSkSp(const RefPtr<T>&) - for creating a new sk_sp ref
                              (when sharing ownership with Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}
==========

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

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-03-18 15:19:08]

Description was changed from

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce Skia utilities to support PassRefPtr <-> sk_sp interoperability:

 * fromSkSp(sk_sp<T>)       - for adopting sk_sps into PassRefPtr
                              (when transferring ownership from Skia->Blink)
 * toSkSp(PassRefPtr<T>)    - for releasing PassRefPtr into sk_sp
                              (when transferring ownership from Blink->Skia)
 * toSkSp(const RefPtr<T>&) - for creating a new sk_sp ref
                              (when sharing ownership with Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}
==========

to

==========
Add sk_sp helpers and switch Blink SkShader clients to the new APIs

Introduce Skia utilities to support PassRefPtr <-> sk_sp interoperability:

 * fromSkSp(sk_sp<T>)       - for adopting sk_sps into PassRefPtr
                              (when transferring ownership from Skia->Blink)
 * toSkSp(PassRefPtr<T>)    - for releasing PassRefPtr into sk_sp
                              (when transferring ownership from Blink->Skia)
 * toSkSp(const RefPtr<T>&) - for creating a new sk_sp ref
                              (when sharing ownership with Skia)

Update clients to use the new Skia shader factories ("makers" vs. prev
"creators"), and sk_sp-based setters.

A tangential change: deconstify SkPicture fields/args to align with the new
Skia APIs (SkPicture is inherently const - no non-cost methods - and Skia now
expects non-const SkPicture args).

BUG=skia:5077
R=jbroman@chromium.org,reed@google.com,senorblanco@chromium.org

Committed: https://crrev.com/2df3d4433fc756f1297c4e0ced6524cbfcba9571
Cr-Commit-Position: refs/heads/master@{#381563}

Committed: https://crrev.com/09fd8033ec57166b9f7a24903bef368fd3da8206
Cr-Commit-Position: refs/heads/master@{#381971}
==========

[commit-bot: I haz the power, 2016-03-18 15:19:10]

Patchset 12 (id:??) landed as
https://crrev.com/09fd8033ec57166b9f7a24903bef368fd3da8206
Cr-Commit-Position: refs/heads/master@{#381971}