add setter on SkPaint that takes sk_sp

src/core/SkPaint.cpp

 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkPaint.h"
 #include "SkAutoKern.h"
 #include "SkChecksum.h"
@@ skipping 26 matching lines @@
 
 static inline uint32_t set_clear_mask(uint32_t bits, bool cond, uint32_t mask) {
     return cond ? bits | mask : bits & ~mask;
 }
 
 // define this to get a printf for out-of-range parameter in setters
 // e.g. setTextSize(-1)
 //#define SK_REPORT_API_RANGE_CHECK
 
 SkPaint::SkPaint() {
-    fTypeface    = nullptr;
-    fPathEffect  = nullptr;
-    fShader      = nullptr;
-    fXfermode    = nullptr;
-    fMaskFilter  = nullptr;
-    fColorFilter = nullptr;
-    fRasterizer  = nullptr;
-    fLooper      = nullptr;
-    fImageFilter = nullptr;
-
     fTextSize   = SkPaintDefaults_TextSize;
     fTextScaleX = SK_Scalar1;
     fTextSkewX  = 0;
     fColor      = SK_ColorBLACK;
     fWidth      = 0;
     fMiterLimit = SkPaintDefaults_MiterLimit;
 
     // Zero all bitfields, then set some non-zero defaults.
     fBitfieldsUInt           = 0;
     fBitfields.fFlags        = SkPaintDefaults_Flags;
     fBitfields.fCapType      = kDefault_Cap;
     fBitfields.fJoinType     = kDefault_Join;
     fBitfields.fTextAlign    = kLeft_Align;
     fBitfields.fStyle        = kFill_Style;
     fBitfields.fTextEncoding = kUTF8_TextEncoding;
     fBitfields.fHinting      = SkPaintDefaults_Hinting;
 }
 
-SkPaint::SkPaint(const SkPaint& src) {
+SkPaint::SkPaint(const SkPaint& src)
+#define COPY_CTR(field) field(src.field)

[bungeman-skia, 2016/03/06 04:50:05]

Note that for simplicity and uniformity, COPY_CTR can just be renamed COPY, the
existing COPY below be removed, and just do everything in the member initializer
list. Then you don't need two names and everything looks the same.
Interestingly, it looks like you already did basically this in the
operator=(const&) below.

[reed1, 2016/03/06 20:06:17]

Agreed about unity. I think CTR is clearer (to me) than COPY, but I'll go with
COPY.

+    : COPY_CTR(fTypeface)
+    , COPY_CTR(fPathEffect)
+    , COPY_CTR(fShader)
+    , COPY_CTR(fXfermode)
+    , COPY_CTR(fMaskFilter)
+    , COPY_CTR(fColorFilter)
+    , COPY_CTR(fRasterizer)
+    , COPY_CTR(fLooper)
+    , COPY_CTR(fImageFilter)
+{
 #define COPY(field) field = src.field
-#define REF_COPY(field) field = SkSafeRef(src.field)
-
-    REF_COPY(fTypeface);
-    REF_COPY(fPathEffect);
-    REF_COPY(fShader);
-    REF_COPY(fXfermode);
-    REF_COPY(fMaskFilter);
-    REF_COPY(fColorFilter);
-    REF_COPY(fRasterizer);
-    REF_COPY(fLooper);
-    REF_COPY(fImageFilter);
-
     COPY(fTextSize);
     COPY(fTextScaleX);
     COPY(fTextSkewX);
     COPY(fColor);
     COPY(fWidth);
     COPY(fMiterLimit);
     COPY(fBitfields);
 
 #undef COPY
-#undef REF_COPY
+#undef COPY_CTR
 }
 
 SkPaint::SkPaint(SkPaint&& src) {
 #define MOVE(field) field = std::move(src.field)
 #define REF_MOVE(field) field = src.field; src.field = nullptr

[bungeman-skia, 2016/03/06 04:50:05]

REF_MOVE needs to be removed and all the REF_MOVE below just turn into MOVE.
Indeed, this should end up like the copy constructor above, but with MOVE
instead of COPY, with everything done in the member initializer list. This is
probably the source of the leak, as the assignment here is actually calling the
copy assignment operator. It looks like you already did this with move
assignment operator.

[bungeman-skia, 2016/03/06 14:03:40]

I shouldn't write these things so late at night. Of course this should not leak.
However, this is calling the copy assignment operator so it's not a very
efficient move.

[reed1, 2016/03/06 20:06:17]

Doh! I inverted the intended use. Will move all of them.

 
     REF_MOVE(fTypeface);
     REF_MOVE(fPathEffect);
     REF_MOVE(fShader);
     REF_MOVE(fXfermode);
     REF_MOVE(fMaskFilter);
     REF_MOVE(fColorFilter);
     REF_MOVE(fRasterizer);
     REF_MOVE(fLooper);
     REF_MOVE(fImageFilter);
 
     MOVE(fTextSize);
     MOVE(fTextScaleX);
     MOVE(fTextSkewX);
     MOVE(fColor);
     MOVE(fWidth);
     MOVE(fMiterLimit);
     MOVE(fBitfields);
 
 #undef MOVE
 #undef REF_MOVE
 }
 
-SkPaint::~SkPaint() {
-    SkSafeUnref(fTypeface);
-    SkSafeUnref(fPathEffect);
-    SkSafeUnref(fShader);
-    SkSafeUnref(fXfermode);
-    SkSafeUnref(fMaskFilter);
-    SkSafeUnref(fColorFilter);
-    SkSafeUnref(fRasterizer);
-    SkSafeUnref(fLooper);
-    SkSafeUnref(fImageFilter);
-}
+SkPaint::~SkPaint() {}
 
 SkPaint& SkPaint::operator=(const SkPaint& src) {
     if (this == &src) {
         return *this;
     }
 
 #define COPY(field) field = src.field
-#define REF_COPY(field) SkSafeUnref(field); field = SkSafeRef(src.field)
 
-    REF_COPY(fTypeface);
-    REF_COPY(fPathEffect);
-    REF_COPY(fShader);
-    REF_COPY(fXfermode);
-    REF_COPY(fMaskFilter);
-    REF_COPY(fColorFilter);
-    REF_COPY(fRasterizer);
-    REF_COPY(fLooper);
-    REF_COPY(fImageFilter);
-
+    COPY(fTypeface);
+    COPY(fPathEffect);
+    COPY(fShader);
+    COPY(fXfermode);
+    COPY(fMaskFilter);
+    COPY(fColorFilter);
+    COPY(fRasterizer);
+    COPY(fLooper);
+    COPY(fImageFilter);
     COPY(fTextSize);
     COPY(fTextScaleX);
     COPY(fTextSkewX);
     COPY(fColor);
     COPY(fWidth);
     COPY(fMiterLimit);
     COPY(fBitfields);
 
     return *this;
 
 #undef COPY
-#undef REF_COPY
 }
 
 SkPaint& SkPaint::operator=(SkPaint&& src) {
     if (this == &src) {
         return *this;
     }
 
 #define MOVE(field) field = std::move(src.field)
-#define REF_MOVE(field) SkSafeUnref(field); field = src.field; src.field = nullptr
 
-    REF_MOVE(fTypeface);
-    REF_MOVE(fPathEffect);
-    REF_MOVE(fShader);
-    REF_MOVE(fXfermode);
-    REF_MOVE(fMaskFilter);
-    REF_MOVE(fColorFilter);
-    REF_MOVE(fRasterizer);
-    REF_MOVE(fLooper);
-    REF_MOVE(fImageFilter);
-
+    MOVE(fTypeface);
+    MOVE(fPathEffect);
+    MOVE(fShader);
+    MOVE(fXfermode);
+    MOVE(fMaskFilter);
+    MOVE(fColorFilter);
+    MOVE(fRasterizer);
+    MOVE(fLooper);
+    MOVE(fImageFilter);
     MOVE(fTextSize);
     MOVE(fTextScaleX);
     MOVE(fTextSkewX);
     MOVE(fColor);
     MOVE(fWidth);
     MOVE(fMiterLimit);
     MOVE(fBitfields);
 
     return *this;
 
 #undef MOVE
-#undef REF_MOVE
 }
 
 bool operator==(const SkPaint& a, const SkPaint& b) {
 #define EQUAL(field) (a.field == b.field)
     return EQUAL(fTypeface)
         && EQUAL(fPathEffect)
         && EQUAL(fShader)
         && EQUAL(fXfermode)
         && EQUAL(fMaskFilter)
         && EQUAL(fColorFilter)
@@ skipping 174 matching lines @@
         fBitfields.fTextEncoding = encoding;
     } else {
 #ifdef SK_REPORT_API_RANGE_CHECK
         SkDebugf("SkPaint::setTextEncoding(%d) out of range\n", encoding);
 #endif
     }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
-SkTypeface* SkPaint::setTypeface(SkTypeface* font) {
-    SkRefCnt_SafeAssign(fTypeface, font);
-    return font;
-}
+#define MOVE_FIELD(Field) void SkPaint::set##Field(sk_sp<Sk##Field> f) { f##Field = std::move(f); }
+MOVE_FIELD(Typeface)
+MOVE_FIELD(Rasterizer)
+MOVE_FIELD(ImageFilter)
+MOVE_FIELD(Shader)
+MOVE_FIELD(ColorFilter)
+MOVE_FIELD(Xfermode)
+MOVE_FIELD(PathEffect)
+MOVE_FIELD(MaskFilter)
+#undef MOVE_FIELD
+void SkPaint::setLooper(sk_sp<SkDrawLooper> looper) { fLooper = std::move(looper); }
 
-SkRasterizer* SkPaint::setRasterizer(SkRasterizer* r) {
-    SkRefCnt_SafeAssign(fRasterizer, r);
-    return r;
-}
+#define SET_PTR(Field)                              \
+    Sk##Field* SkPaint::set##Field(Sk##Field* f) {  \
+        this->f##Field.reset(SkSafeRef(f));         \
+        return f;                                   \
+    }
+SET_PTR(Typeface)
+SET_PTR(Rasterizer)
+SET_PTR(ImageFilter)
+SET_PTR(Shader)
+SET_PTR(ColorFilter)
+SET_PTR(Xfermode)
+SET_PTR(PathEffect)
+SET_PTR(MaskFilter)
+#undef SET_PTR
 
 SkDrawLooper* SkPaint::setLooper(SkDrawLooper* looper) {
-    SkRefCnt_SafeAssign(fLooper, looper);
+    fLooper.reset(SkSafeRef(looper));
     return looper;
 }
 
-SkImageFilter* SkPaint::setImageFilter(SkImageFilter* imageFilter) {
-    SkRefCnt_SafeAssign(fImageFilter, imageFilter);
-    return imageFilter;
+SkXfermode* SkPaint::setXfermodeMode(SkXfermode::Mode mode) {
+    fXfermode.reset(SkXfermode::Create(mode));
+    return fXfermode.get();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
 static SkScalar mag2(SkScalar x, SkScalar y) {
     return x * x + y * y;
 }
 
 static bool tooBig(const SkMatrix& m, SkScalar ma2max) {
     return  mag2(m[SkMatrix::kMScaleX], m[SkMatrix::kMSkewY]) > ma2max
@@ skipping 1296 matching lines @@
     SkDescriptor*       desc = ad.getDesc();
 
     write_out_descriptor(desc, rec, pe, &peBuffer, mf, &mfBuffer, ra, &raBuffer, descSize);
 
     SkASSERT(descSize == desc->getLength());
 
 #ifdef TEST_DESC
     test_desc(rec, pe, &peBuffer, mf, &mfBuffer, ra, &raBuffer, desc, descSize);
 #endif
 
-    proc(fTypeface, desc, context);
+    proc(fTypeface.get(), desc, context);
 }
 
 SkGlyphCache* SkPaint::detachCache(const SkSurfaceProps* surfaceProps,
                                    FakeGamma fakeGamma,
                                    const SkMatrix* deviceMatrix) const {
     SkGlyphCache* cache;
     this->descriptorProc(surfaceProps, fakeGamma, deviceMatrix, DetachDescProc, &cache);
     return cache;
 }
 
@@ skipping 236 matching lines @@
         this->setMaskFilter(nullptr);
         this->setColorFilter(nullptr);
         this->setRasterizer(nullptr);
         this->setLooper(nullptr);
         this->setImageFilter(nullptr);
     }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
-SkShader* SkPaint::setShader(SkShader* shader) {
-    SkRefCnt_SafeAssign(fShader, shader);
-    return shader;
-}
-
-SkColorFilter* SkPaint::setColorFilter(SkColorFilter* filter) {
-    SkRefCnt_SafeAssign(fColorFilter, filter);
-    return filter;
-}
-
-SkXfermode* SkPaint::setXfermode(SkXfermode* mode) {
-    SkRefCnt_SafeAssign(fXfermode, mode);
-    return mode;
-}
-
-SkXfermode* SkPaint::setXfermodeMode(SkXfermode::Mode mode) {
-    SkSafeUnref(fXfermode);
-    fXfermode = SkXfermode::Create(mode);
-    return fXfermode;
-}
-
-SkPathEffect* SkPaint::setPathEffect(SkPathEffect* effect) {
-    SkRefCnt_SafeAssign(fPathEffect, effect);
-    return effect;
-}
-
-SkMaskFilter* SkPaint::setMaskFilter(SkMaskFilter* filter) {
-    SkRefCnt_SafeAssign(fMaskFilter, filter);
-    return filter;
-}
-
-///////////////////////////////////////////////////////////////////////////////
-
 bool SkPaint::getFillPath(const SkPath& src, SkPath* dst, const SkRect* cullRect,
                           SkScalar resScale) const {
     SkStrokeRec rec(*this, resScale);
 
     const SkPath* srcPtr = &src;
     SkPath tmpPath;
 
     if (fPathEffect && fPathEffect->filterPath(&tmpPath, src, &rec, cullRect)) {
         srcPtr = &tmpPath;
     }
@@ skipping 357 matching lines @@
     // TODO: check if we should allow imagefilters to broadcast that they don't affect alpha
     // ala colorfilters
     return imf != nullptr;
 }
 
 bool SkPaint::nothingToDraw() const {
     if (fLooper) {
         return false;
     }
     SkXfermode::Mode mode;
-    if (SkXfermode::AsMode(fXfermode, &mode)) {
+    if (SkXfermode::AsMode(fXfermode.get(), &mode)) {
         switch (mode) {
             case SkXfermode::kSrcOver_Mode:
             case SkXfermode::kSrcATop_Mode:
             case SkXfermode::kDstOut_Mode:
             case SkXfermode::kDstOver_Mode:
             case SkXfermode::kPlus_Mode:
                 if (0 == this->getAlpha()) {
-                    return !affects_alpha(fColorFilter) && !affects_alpha(fImageFilter);
+                    return !affects_alpha(fColorFilter.get()) && !affects_alpha(fImageFilter.get());
                 }
                 break;
             case SkXfermode::kDst_Mode:
                 return true;
             default:
                 break;
         }
     }
     return false;
 }
 
 uint32_t SkPaint::getHash() const {
     // We're going to hash 10 pointers and 7 32-bit values, finishing up with fBitfields,
     // so fBitfields should be 10 pointers and 6 32-bit values from the start.
     static_assert(offsetof(SkPaint, fBitfields) == 9 * sizeof(void*) + 6 * sizeof(uint32_t),
                   "SkPaint_notPackedTightly");
     return SkChecksum::Murmur3(reinterpret_cast<const uint32_t*>(this),
                                offsetof(SkPaint, fBitfields) + sizeof(fBitfields));
 }