Fixes for SkPictureShader.

src/core/SkPictureShader.cpp

 /*
  * Copyright 2014 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkPictureShader.h"
 
 #include "SkBitmap.h"
 #include "SkBitmapProcShader.h"
 #include "SkCanvas.h"
 #include "SkMatrixUtils.h"
 #include "SkPicture.h"
 #include "SkReadBuffer.h"
 
 #if SK_SUPPORT_GPU
 #include "GrContext.h"
 #endif
 
 SkPictureShader::SkPictureShader(SkPicture* picture, TileMode tmx, TileMode tmy)
-    : fPicture(picture)
+    : fPicture(SkRef(picture))
     , fTmx(tmx)
-    , fTmy(tmy) {
-    SkSafeRef(fPicture);
-}
+    , fTmy(tmy) { }
 
 SkPictureShader::SkPictureShader(SkReadBuffer& buffer)
         : INHERITED(buffer) {
     fTmx = static_cast<SkShader::TileMode>(buffer.read32());
     fTmy = static_cast<SkShader::TileMode>(buffer.read32());
     if (buffer.readBool()) {
         fPicture = SkPicture::CreateFromBuffer(buffer);
     } else {
-        fPicture = NULL;
+        // For backwards compatibility, create a dummy picture. Older versions allowed

[scroggo, 2014/04/15 19:15:49]

This code is only for handling already existing SKPs. If chromium is not yet
using this class, this code can be removed.

[f(malita), 2014/04/15 19:40:40]

Yes, should be safe to remove.

[scroggo, 2014/04/15 20:33:20]

Done.

+        // creating an SkPictureShader with a NULL SkPicture.
+        SkPictureRecorder factory;
+        factory.beginRecording(0, 0);
+        fPicture = factory.endRecording();
     }
 }
 
 SkPictureShader::~SkPictureShader() {
-    SkSafeUnref(fPicture);
+    fPicture->unref();
 }
 
 SkPictureShader* SkPictureShader::Create(SkPicture* picture, TileMode tmx, TileMode tmy) {
+    if (!picture || 0 == picture->width() || 0 == picture->height()) {
+        return NULL;
+    }
     return SkNEW_ARGS(SkPictureShader, (picture, tmx, tmy));
 }
 
 void SkPictureShader::flatten(SkWriteBuffer& buffer) const {
     this->INHERITED::flatten(buffer);
 
     buffer.write32(fTmx);
     buffer.write32(fTmy);
     buffer.writeBool(NULL != fPicture);
     if (fPicture) {
         fPicture->flatten(buffer);
     }
 }
 
 SkShader* SkPictureShader::buildBitmapShader(const SkMatrix& matrix) const {
-    if (!fPicture || (0 == fPicture->width() && 0 == fPicture->height())) {
-        return NULL;
-    }
+    // FIXME: Older versions of this class allowed a picture with 0 width or
+    // height, but would bail out here. Allow a 0 width/height to support
+    // reading an SkPictureShader from an SKP. Note that we will still return
+    // NULL in that case due to an empty tileSize. If/when we can disregard
+    // old SKPs, it is safe to assert the picture's width and height are
+    // greater than zero.

[f(malita), 2014/04/15 19:40:40]

It's safe to disregard old SKPs - the shader is not used at this point.

[scroggo, 2014/04/15 20:33:20]

Done.

+    SkASSERT(fPicture); // && fPicture->width() > 0 && fPicture->height() > 0);
 
     SkMatrix m;
     if (this->hasLocalMatrix()) {
         m.setConcat(matrix, this->getLocalMatrix());
     } else {
         m = matrix;
     }
 
     // Use a rotation-invariant scale
     SkPoint scale;
@@ skipping 137 matching lines @@
 
 #if SK_SUPPORT_GPU
 GrEffectRef* SkPictureShader::asNewEffect(GrContext* context, const SkPaint& paint) const {
     SkAutoTUnref<SkShader> bitmapShader(this->buildBitmapShader(context->getMatrix()));
     if (!bitmapShader) {
         return NULL;
     }
     return bitmapShader->asNewEffect(context, paint);
 }
 #endif