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

src/pdf/SkPDFBitmap.cpp

Index: src/pdf/SkPDFBitmap.cpp
diff --git a/src/pdf/SkPDFBitmap.cpp b/src/pdf/SkPDFBitmap.cpp
index fdc33032a9c793d0ada23d4fdd6d5863599775b8..ed8ea40fb9858d425d5430a21958fd49487c7d02 100644
--- a/src/pdf/SkPDFBitmap.cpp
+++ b/src/pdf/SkPDFBitmap.cpp
@@ -382,13 +382,15 @@ 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;
@@ -404,22 +406,25 @@ 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
 
@@ -437,15 +442,17 @@ public:
     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());