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

third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h

Index: third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h
diff --git a/third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h b/third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h
index 3c3781bc87a3a63ba77324fec711ceb68d8e1fdb..9cb06f92a26ad9f942dcad7a3fe4efe85b11fb1f 100644
--- a/third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h
+++ b/third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h
@@ -38,6 +38,7 @@
 #include "platform/graphics/Image.h"
 #include "platform/transforms/AffineTransform.h"
 #include "third_party/skia/include/core/SkCanvas.h"
+#include "third_party/skia/include/core/SkRefCnt.h"
 #include "wtf/MathExtras.h"
 
 namespace blink {
@@ -126,6 +127,85 @@ inline SkCanvas::SrcRectConstraint WebCoreClampingModeToSkiaRectConstraint(Image
         : SkCanvas::kFast_SrcRectConstraint;
 }
 
+
+// Skia's smart pointer APIs are preferable over their legacy raw pointer counterparts.
+// The following helpers ensure interoperability between Skia's SkRefCnt wrapper sk_sp<T> and
+// Blink's RefPtr<T>/PassRefPtr<T>.
+//
+//   - adoptRef(sk_sp<T>&&):       adopts an sk_sp rvalue into a PassRefPtr (to be used when
+//                                 transferring ownership from Skia to Blink).
+//   - adoptSkSp(PassRefPtr<T>&&): adopts a PassRefPtr rvalue into a sk_sp (to be used when
+//                                 transferring ownership from Blink to Skia).
+//
+// General guidelines
+//
+// When receiving ref counted objects from Skia:
+//
+//   1) use sk_sp-based Skia factories if available (e.g. SkShader::MakeFoo() instead of
+//      SkShader::CreateFoo())
+//
+//   2) use sk_sp<T> locals for temporary objects (to be immediately transferred back to Skia)
+//
+//   3) use RefPtr<T>/PassRefPtr<T> for objects to be retained in Blink, use
+//      adoptRef(sk_sp<T>&&) to convert
+//
+// When passing ref counted objects to Skia:
+//
+//   1) use sk_sk-based Skia APIs when available (e.g. SkPaint::setShader(sk_sp<SkShader>)
+//      instead of SkPaint::setShader(SkShader*))
+//
+//   2) if the object ownership is being passed to Skia, use std::move(sk_sp<T>) or
+//      adoptSkSp(PassRefPtr<T>&&) to transfer without refcount churn
+//
+//   3) if the object ownership is shared with Skia (Blink retains a reference), use
+//      sk_ref_sp(RefPtr<T>::get())
+//
+// Example (creating a SkShader and setting it on SkPaint):
+//
+// a) legacy/old style
+//
+//     RefPtr<SkShader> shader = adoptRef(SkShader::CreateFoo(...));
+//     paint.setShader(shader.get());
+//
+//  (Note: the legacy approach introduces refcount churn as Skia grabs a ref while Blink is
+//   temporarily holding on to its own)
+//
+// b) new style, ownership transferred
+//
+//     // using Skia smart pointer locals
+//     sk_sp<SkShader> shader = SkShader::MakeFoo(...);
+//     paint.setShader(std::move(shader));
+//
+//     // using Blink smart pointer locals
+//     RefPtr<SkShader> shader = adoptRef(SkShader::MakeFoo(...));
+//     paint.setShader(adoptSkSp(shader.release());
+//
+//     // using no locals
+//     paint.setShader(SkShader::MakeFoo(...));
+//
+//  (Note: while all these variants avoid ref count churn, the last/inlined-factory version
+//   is preferred when feasible because it also skips the adopt/move op)

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

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?)

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

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.
I actually find the non-inlined version more readable :)

+//
+// c) new style, shared ownership
+//
+//     // using sk_ref_sp
+//     RefPtr<SkShader> m_shader = adoptRef(SkShader::MakeFoo(...));
+//     paint.setShader(sk_ref_sp(m_shader.get()));
+//
+//     // using a temp PassRefPtr and sk_sp adoption
+//     RefPtr<SkShader> m_shader = adoptRef(SkShader::MakeFoo(...));
+//     paint.setShader(adoptSkSp(PassRefPtr<SkShader>(m_shader)));
+//
+template <typename T> PassRefPtr<T> adoptRef(sk_sp<T>&& sp)

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

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.

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

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.

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

(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)

+{
+    return adoptRef(sp.release());
+}
+
+template <typename T> sk_sp<T> adoptSkSp(PassRefPtr<T>&& ref)

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

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]

Makes sense, done.

+{
+    return sk_sp<T>(ref.leakRef());
+}
+
 } // namespace blink
 
 #endif  // SkiaUtils_h