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

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

 /*
  * Copyright (c) 2006,2007,2008, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 20 matching lines @@
 // All of the functions in this file should move to new homes and this file should be deleted.
 
 #ifndef SkiaUtils_h
 #define SkiaUtils_h
 
 #include "platform/PlatformExport.h"
 #include "platform/graphics/GraphicsTypes.h"
 #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 {
 
 class GraphicsContext;
 
 SkXfermode::Mode PLATFORM_EXPORT WebCoreCompositeToSkiaComposite(CompositeOperator, WebBlendMode = WebBlendModeNormal);
 CompositeOperator PLATFORM_EXPORT compositeOperatorFromSkia(SkXfermode::Mode);
 WebBlendMode PLATFORM_EXPORT blendModeFromSkia(SkXfermode::Mode);
 
@@ skipping 68 matching lines @@
 void drawPlatformFocusRing(const PrimitiveType&, SkCanvas*, SkColor, int width);
 
 // TODO(fmalita): remove in favor of direct SrcRectConstraint use.
 inline SkCanvas::SrcRectConstraint WebCoreClampingModeToSkiaRectConstraint(Image::ImageClampingMode clampMode)
 {
     return clampMode == Image::ClampImageToSourceRect
         ? SkCanvas::kStrict_SrcRectConstraint
         : 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