SkRecord bug fixes

Optional arguments to SkCanvas calls leaked refs (but not memory) because we
didn't destruct the optional objects (really, just SkPaint: other optional args
are all POD).  This adds Optional and PODArray, where Optional makes sure to
call the destructor.

I overlooked that SkPictureRecord really does call paint.computeFastBounds().
Do the same in SkRecordDraw.

BUG=skia:2378
R=reed@google.com, mtklein@google.com, tomhudson@google.com

Author: mtklein@chromium.org

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

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

2 files changed, 63 insertions, 20 deletions

diff --git a/src/record/SkRecordDraw.cpp b/src/record/SkRecordDraw.cpp
index f0d3772e9f..c6e85502a7 100644
--- a/src/record/SkRecordDraw.cpp
+++ b/src/record/SkRecordDraw.cpp
@@ -76,7 +76,7 @@ CAN_SKIP(ClipRegion);
 
 static bool can_skip_text(const SkCanvas& c, const SkPaint& p, SkScalar minY, SkScalar maxY) {
     // If we're drawing vertical text, none of the checks we're about to do make any sense.
-    // We use canComputeFastBounds as a proxy for "is this text going to be rectangular?".
+    // We'll need to call SkPaint::computeFastBounds() later, so bail out if that's not possible.
     if (p.isVerticalText() || !p.canComputeFastBounds()) {
         return false;
     }
@@ -88,7 +88,13 @@ static bool can_skip_text(const SkCanvas& c, const SkPaint& p, SkScalar minY, Sk
     SkDEBUGCODE(p.getFontMetrics(&metrics);)
     SkASSERT(-buffer <= metrics.fTop);
     SkASSERT(+buffer >= metrics.fBottom);
-    return c.quickRejectY(minY - buffer, maxY + buffer);
+
+    // Let the paint adjust the text bounds.  We don't care about left and right here, so we use
+    // 0 and 1 respectively just so the bounds rectangle isn't empty.
+    SkRect bounds;
+    bounds.set(0, -buffer, SK_Scalar1, buffer);
+    SkRect adjusted = p.computeFastBounds(bounds, &bounds);
+    return c.quickRejectY(minY + adjusted.fTop, maxY + adjusted.fBottom);
 }
 
 template <> bool Draw::canSkip(const SkRecords::DrawPosTextH& r) const {
diff --git a/src/record/SkRecords.h b/src/record/SkRecords.h
index 7958280e48..10806565d9 100644
--- a/src/record/SkRecords.h
+++ b/src/record/SkRecords.h
@@ -107,6 +107,31 @@ struct T {                                                                \
     A a; B b; C c; D d; E e;                                              \
 };
 
+// An Optional doesn't own the pointer's memory, but may need to destroy non-POD data.
+template <typename T>
+class Optional : SkNoncopyable {
+public:
+    Optional(T* ptr) : fPtr(ptr) {}
+    ~Optional() { if (fPtr) fPtr->~T(); }
+
+    operator T*() { return fPtr; }
+    operator const T*() const { return fPtr; }
+private:
+    T* fPtr;
+};
+
+// PODArray doesn't own the pointer's memory, and we assume the data is POD.
+template <typename T>
+class PODArray : SkNoncopyable {
+public:
+    PODArray(T* ptr) : fPtr(ptr) {}
+
+    operator T*() { return fPtr; }
+    operator const T*() const { return fPtr; }
+private:
+    T* fPtr;
+};
+
 // Like SkBitmap, but deep copies pixels if they're not immutable.
 // Using this, we guarantee the immutability of all bitmaps we record.
 class ImmutableBitmap {
@@ -126,13 +151,12 @@ private:
     SkBitmap fBitmap;
 };
 
-// Pointers here represent either an optional value or an array if accompanied by a count.
 // None of these records manages the lifetimes of pointers, except for DrawVertices handling its
 // Xfermode specially.
 
 RECORD0(Restore);
 RECORD1(Save, SkCanvas::SaveFlags, flags);
-RECORD3(SaveLayer, SkRect*, bounds, SkPaint*, paint, SkCanvas::SaveFlags, flags);
+RECORD3(SaveLayer, Optional<SkRect>, bounds, Optional<SkPaint>, paint, SkCanvas::SaveFlags, flags);
 
 static const unsigned kUnsetPopOffset = 0;
 RECORD2(PushCull, SkRect, rect, unsigned, popOffset);
@@ -147,33 +171,46 @@ RECORD3(ClipRect, SkRect, rect, SkRegion::Op, op, bool, doAA);
 RECORD2(ClipRegion, SkRegion, region, SkRegion::Op, op);
 
 RECORD1(Clear, SkColor, color);
-RECORD4(DrawBitmap, ImmutableBitmap, bitmap, SkScalar, left, SkScalar, top, SkPaint*, paint);
-RECORD3(DrawBitmapMatrix, ImmutableBitmap, bitmap, SkMatrix, matrix, SkPaint*, paint);
-RECORD4(DrawBitmapNine, ImmutableBitmap, bitmap, SkIRect, center, SkRect, dst, SkPaint*, paint);
+RECORD4(DrawBitmap, ImmutableBitmap, bitmap,
+                    SkScalar, left,
+                    SkScalar, top,
+                    Optional<SkPaint>, paint);
+RECORD3(DrawBitmapMatrix, ImmutableBitmap, bitmap, SkMatrix, matrix, Optional<SkPaint>, paint);
+RECORD4(DrawBitmapNine, ImmutableBitmap, bitmap,
+                        SkIRect, center,
+                        SkRect, dst,
+                        Optional<SkPaint>, paint);
 RECORD5(DrawBitmapRectToRect, ImmutableBitmap, bitmap,
-                              SkRect*, src,
+                              Optional<SkRect>, src,
                               SkRect, dst,
-                              SkPaint*, paint,
+                              Optional<SkPaint>, paint,
                               SkCanvas::DrawBitmapRectFlags, flags);
 RECORD3(DrawDRRect, SkRRect, outer, SkRRect, inner, SkPaint, paint);
 RECORD2(DrawOval, SkRect, oval, SkPaint, paint);
 RECORD1(DrawPaint, SkPaint, paint);
 RECORD2(DrawPath, SkPath, path, SkPaint, paint);
 RECORD4(DrawPoints, SkCanvas::PointMode, mode, size_t, count, SkPoint*, pts, SkPaint, paint);
-RECORD4(DrawPosText, char*, text, size_t, byteLength, SkPoint*, pos, SkPaint, paint);
-RECORD5(DrawPosTextH, char*, text,
+RECORD4(DrawPosText, PODArray<char>, text,
+                     size_t, byteLength,
+                     PODArray<SkPoint>, pos,
+                     SkPaint, paint);
+RECORD5(DrawPosTextH, PODArray<char>, text,
                       size_t, byteLength,
-                      SkScalar*, xpos,
+                      PODArray<SkScalar>, xpos,
                       SkScalar, y,
                       SkPaint, paint);
 RECORD2(DrawRRect, SkRRect, rrect, SkPaint, paint);
 RECORD2(DrawRect, SkRect, rect, SkPaint, paint);
-RECORD4(DrawSprite, ImmutableBitmap, bitmap, int, left, int, top, SkPaint*, paint);
-RECORD5(DrawText, char*, text, size_t, byteLength, SkScalar, x, SkScalar, y, SkPaint, paint);
-RECORD5(DrawTextOnPath, char*, text,
+RECORD4(DrawSprite, ImmutableBitmap, bitmap, int, left, int, top, Optional<SkPaint>, paint);
+RECORD5(DrawText, PODArray<char>, text,
+                  size_t, byteLength,
+                  SkScalar, x,
+                  SkScalar, y,
+                  SkPaint, paint);
+RECORD5(DrawTextOnPath, PODArray<char>, text,
                         size_t, byteLength,
                         SkPath, path,
-                        SkMatrix*, matrix,
+                        Optional<SkMatrix>, matrix,
                         SkPaint, paint);
 
 // This guy is so ugly we just write it manually.
@@ -201,11 +238,11 @@ struct DrawVertices {
 
     SkCanvas::VertexMode vmode;
     int vertexCount;
-    SkPoint* vertices;
-    SkPoint* texs;
-    SkColor* colors;
+    PODArray<SkPoint> vertices;
+    PODArray<SkPoint> texs;
+    PODArray<SkColor> colors;
     SkAutoTUnref<SkXfermode> xmode;
-    uint16_t* indices;
+    PODArray<uint16_t> indices;
     int indexCount;
     SkPaint paint;
 };