Fixes for SkPictureShader.

Update comment in header to make it more clear that the picture
should be unaltered after creating the shader. We want our shaders
to be immutable, and this supports that.

Make the factory return NULL if the shader would have never drawn
anyway i.e. for a null picture or picture with no width/height.

Addresses comments I brought up in
https://codereview.chromium.org/221923007/#msg16.

BUG=skia:1976
R=reed@google.com, fmalita@chromium.org, robertphillips@google.com

Author: scroggo@google.com

Review URL: https://codereview.chromium.org/238253005

git-svn-id: http://skia.googlecode.com/svn/trunk@14288 2bbb7eff-a529-9590-31e7-b0007b416f81

diff --git a/gyp/tests.gypi b/gyp/tests.gypi
index 5c3ce94..e6c6bb0 100644 (file)
--- a/gyp/tests.gypi
+++ b/gyp/tests.gypi
@@ -127,6 +127,7 @@
     '../tests/PathTest.cpp',
     '../tests/PathUtilsTest.cpp',
     '../tests/PictureTest.cpp',
+    '../tests/PictureShaderTest.cpp',
     '../tests/PictureUtilsTest.cpp',
     '../tests/PixelRefTest.cpp',
     '../tests/PointTest.cpp',

diff --git a/include/core/SkShader.h b/include/core/SkShader.h
index 076ecf5..6566e69 100644 (file)
--- a/include/core/SkShader.h
+++ b/include/core/SkShader.h
@@ -349,7 +349,9 @@ public:
     /** Call this to create a new shader that will draw with the specified picture.
      *
      *  @param src  The picture to use inside the shader (if not NULL, its ref count
-     *              is incremented).
+     *              is incremented). The SkPicture must not be changed after
+     *              successfully creating a picture shader.
+     *              FIXME: src cannot be const due to SkCanvas::drawPicture
      *  @param tmx  The tiling mode to use when sampling the bitmap in the x-direction.
      *  @param tmy  The tiling mode to use when sampling the bitmap in the y-direction.
      *  @return     Returns a new shader object. Note: this function never returns null.

diff --git a/src/core/SkPictureShader.cpp b/src/core/SkPictureShader.cpp
index 15df3a3..bf31285 100644 (file)
--- a/src/core/SkPictureShader.cpp
+++ b/src/core/SkPictureShader.cpp
@@ -19,28 +19,25 @@
 #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;
-    }
+    fPicture = SkPicture::CreateFromBuffer(buffer);
 }
 
 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));
 }
 
@@ -49,16 +46,11 @@ void SkPictureShader::flatten(SkWriteBuffer& buffer) const {
 
     buffer.write32(fTmx);
     buffer.write32(fTmy);
-    buffer.writeBool(NULL != fPicture);
-    if (fPicture) {
-        fPicture->flatten(buffer);
-    }
+    fPicture->flatten(buffer);
 }
 
 bool SkPictureShader::buildBitmapShader(const SkMatrix& matrix) const {
-    if (!fPicture || (0 == fPicture->width() && 0 == fPicture->height())) {
-        return false;
-    }
+    SkASSERT(fPicture && fPicture->width() > 0 && fPicture->height() > 0);
 
     SkMatrix m;
     if (this->hasLocalMatrix()) {

diff --git a/tests/PictureShaderTest.cpp b/tests/PictureShaderTest.cpp
new file mode 100644 (file)
index 0000000..17ef5b5
--- /dev/null
+++ b/tests/PictureShaderTest.cpp
@@ -0,0 +1,26 @@
+/*
+ * 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 "SkPicture.h"
+#include "SkPictureRecorder.h"
+#include "SkShader.h"
+#include "Test.h"
+
+// Test that attempting to create a picture shader with a NULL picture or
+// empty picture returns NULL.
+DEF_TEST(PictureShader_empty, reporter) {
+    SkShader* shader = SkShader::CreatePictureShader(NULL,
+            SkShader::kClamp_TileMode, SkShader::kClamp_TileMode);
+    REPORTER_ASSERT(reporter, NULL == shader);
+
+    SkPictureRecorder factory;
+    factory.beginRecording(0, 0, NULL, 0);
+    SkAutoTUnref<SkPicture> picture(factory.endRecording());
+    shader = SkShader::CreatePictureShader(picture.get(),
+            SkShader::kClamp_TileMode, SkShader::kClamp_TileMode);
+    REPORTER_ASSERT(reporter, NULL == shader);
+}