SkPDF: add drop() virtual to release resources early.

src/pdf/SkPDFBitmap.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkColorPriv.h"
 #include "SkData.h"
 #include "SkDeflate.h"
@@ skipping 364 matching lines @@
     stream->writeStream(asset.get(), asset->getLength());
     pdf_stream_end(stream);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 
 namespace {
 // This SkPDFObject only outputs the alpha layer of the given bitmap.
 class PDFAlphaBitmap final : public SkPDFObject {
 public:
-    PDFAlphaBitmap(const SkImage* image) : fImage(SkRef(image)) {}
+    PDFAlphaBitmap(const SkImage* image) : fImage(SkRef(image)) { SkASSERT(image); }

[tomhudson, 2016/03/18 13:59:38]

You don't want to do the force-to-be-nonnull-by-passing-in-a-ref thing here?

[hal.canary, 2016/03/18 21:18:07]

We rarely ever refer to SkImage types by reference.  later, we'll pass it in as
a sk_sp, anyways.

     ~PDFAlphaBitmap() {}

[tomhudson, 2016/03/18 13:59:38]

Since the base class has virtual functions, do we still need to be declaring the
destructor virtual?

[hal.canary, 2016/03/18 21:18:07]

it's a final class

[tomhudson, 2016/03/21 13:54:30]

This assertion is not sufficient for me to evaluate the accuracy of the design
decision. A google search leads me to e.g.
http://programmers.stackexchange.com/questions/284561/when-not-to-use-virtual...,
but that's vague/self-contradictory enough that I don't really think it suffices
for me to evaluate.

The *base class* destructor has to be virtual, right? We don't really mark
derived class destructors as override. We do want this destructor called if the
base class pointer is deleted. So it should be virtual?!

[mtklein, 2016/03/21 14:11:15]

Marking a class final prevents subclassing and also marks all its virtual
methods final, as if we'd written
   ~PDFAlphaBitmap() final {}
   ...
   void dump() final { ... }
etc.

When written explicitly, final is similar to override, in that it won't compile
unless the method was virtual.  So neither 
    void foo() final { ... }
    void bar() override { ... }
would compile here because foo() and bar() are not virtual methods on the parent
class.

In effect, we have marked ~(), emitObject() and dump() all as final, and
explicitly required that emitObject() and dump() override virtual methods.  We
could conceivably de-virtualize SkPDFObject's destructor and this code would
continue to compile, but it'd leak fImage.

For consistency, I'd suggest marking the destructor as override.  It doesn't
change anything except require SkPDFObject's destructor stays virtual.  Marking
destructors as override is a good habit to get into generally.

     void emitObject(SkWStream*  stream,
                     const SkPDFObjNumMap& objNumMap,
                     const SkPDFSubstituteMap& subs) const override {
+        SkASSERT(fImage);
         emit_image_xobject(stream, fImage.get(), true, nullptr, objNumMap, subs);
     }
+    void dump() override { fImage = nullptr; }
 
 private:
     sk_sp<const SkImage> fImage;
 };
 
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 
 namespace {
 class PDFDefaultBitmap final : public SkPDFObject {
 public:
     void emitObject(SkWStream* stream,
                     const SkPDFObjNumMap& objNumMap,
                     const SkPDFSubstituteMap& subs) const override {
+        SkASSERT(fImage);
         emit_image_xobject(stream, fImage.get(), false, fSMask, objNumMap, subs);
     }
     void addResources(SkPDFObjNumMap* catalog,
                       const SkPDFSubstituteMap& subs) const override {
+        SkASSERT(fImage);
         if (fSMask.get()) {
             SkPDFObject* obj = subs.getSubstitute(fSMask.get());
             SkASSERT(obj);
             catalog->addObjectRecursively(obj, subs);
         }
     }
+    void dump() override { fImage = nullptr; fSMask = nullptr; }
     PDFDefaultBitmap(const SkImage* image, SkPDFObject* smask)
-        : fImage(SkRef(image)), fSMask(smask) {}
+        : fImage(SkRef(image)), fSMask(smask) { SkASSERT(fImage); }

[tomhudson, 2016/03/18 13:59:38]

No destructor; do we need one since we've got virtuals?

[hal.canary, 2016/03/18 21:18:07]

it's a final class.

[mtklein, 2016/03/21 14:11:14]

(No _explicit_ destructor.  There is an implicitly-defined destructor here, and
it does get called when deleting from a base pointer as long as that base
pointer's destructor is virtual.  Same story as above... if we want to require
SkPDFObject's destructor is virtual, we could write ~PDFDefaultBitmap override
{}.)

 
 private:
     sk_sp<const SkImage> fImage;
-    const sk_sp<SkPDFObject> fSMask;
+    sk_sp<SkPDFObject> fSMask;
 };
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 
 namespace {
 /**
  *  This PDFObject assumes that its constructor was handed YUV or
  *  Grayscale JFIF Jpeg-encoded data that can be directly embedded
  *  into a PDF.
  */
 class PDFJpegBitmap final : public SkPDFObject {
 public:
     SkISize fSize;
     sk_sp<SkData> fData;
     bool fIsYUV;
     PDFJpegBitmap(SkISize size, SkData* data, bool isYUV)
-        : fSize(size), fData(SkRef(data)), fIsYUV(isYUV) {}
+        : fSize(size), fData(SkRef(data)), fIsYUV(isYUV) { SkASSERT(data); }
     void emitObject(SkWStream*,
                     const SkPDFObjNumMap&,
                     const SkPDFSubstituteMap&) const override;
+    void dump() override { fData = nullptr; }
 };
 
 void PDFJpegBitmap::emitObject(SkWStream* stream,
                                const SkPDFObjNumMap& objNumMap,
                                const SkPDFSubstituteMap& substituteMap) const {
+    SkASSERT(fData);
     SkPDFDict pdfDict("XObject");
     pdfDict.insertName("Subtype", "Image");
     pdfDict.insertInt("Width", fSize.width());
     pdfDict.insertInt("Height", fSize.height());
     if (fIsYUV) {
         pdfDict.insertName("ColorSpace", "DeviceRGB");
     } else {
         pdfDict.insertName("ColorSpace", "DeviceGray");
     }
     pdfDict.insertInt("BitsPerComponent", 8);
@@ skipping 43 matching lines @@
         }
     }
 
     SkPDFObject* smask =
             image_compute_is_opaque(image) ? nullptr : new PDFAlphaBitmap(image);
     #ifdef SK_PDF_IMAGE_STATS
     gRegularImageObjects.fetch_add(1);
     #endif
     return new PDFDefaultBitmap(image, smask);
 }