SkPDF: clean up skpdfdocument, add static functions

src/pdf/SkPDFDocument.h

 
 /*
  * Copyright 2010 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #ifndef SkPDFDocument_DEFINED
 #define SkPDFDocument_DEFINED
 
 #include "SkAdvancedTypefaceMetrics.h"
 #include "SkRefCnt.h"
 #include "SkTDArray.h"
 #include "SkTemplates.h"
 
 class SkPDFCatalog;
 class SkPDFDevice;
 class SkPDFDict;
 class SkPDFPage;
 class SkPDFObject;
 class SkWStream;
 template <typename T> class SkTSet;
 
 /** \class SkPDFDocument
 
     A SkPDFDocument assembles pages together and generates the final PDF file.
 */
 class SkPDFDocument {

[mtklein, 2015/03/25 13:10:08]

It's starting to seem like SkPDFDocument doesn't need to exist.  It's becoming a
hanger for two static methods.  Is there more than one user of SkPDFDocument? 
If not, it seems like we should start migrating all this code to that single
user and delete this.

[hal.canary, 2015/03/25 13:19:45]

Options:
-  Keep this functionality together in a it's current .cpp
-  Rename the .cpp to SkPDFEmiter.cpp or something
-  move static functions to SkPDFDevice (already a very big file).

 public:
-    SkPDFDocument();
-    ~SkPDFDocument();
+    SkPDFDocument() {}
+    ~SkPDFDocument() { fPageDevices.unrefAll(); }

[mtklein, 2015/03/25 13:10:08]

This destructor seems like another good candidate to leave outlined.  It's not
really trivial work.  I guess it's okay if it's only called once, but then that
circles back to wondering why SkPDFDocument needs to exist.

[hal.canary, 2015/03/25 13:19:45]

It only gets called from one place (skdocument_pdf), and that class will take
direct ownership in a later cl.

 
     /** Output the PDF to the passed stream.  It is an error to call this (it
-     *  will return false and not modify stream) if no pages have been added
-     *  or there are pages missing (i.e. page 1 and 3 have been added, but not
-     *  page 2).
+     *  will return false and not modify stream) if pageDevices is empty.
+     *  No device pointer can be NULL.
+     *
+     *  @param pageDevices An array of pages, in order.  All pages
+     *                     should be created using the same SkPDFCanon.

[mtklein, 2015/03/25 13:10:08]

What happens if they're not?  Conflicting IDs?

I'm somewhat suspicious of classes which cannot enforce their own operational
invariants.  Seems like someone somewhere can enforce this structurally rather
than assert it, and this work should be done by them.

[hal.canary, 2015/03/25 13:19:45]

No de-duping across pages.  (a too-big pdf)
*You can not make this mistake from the public API.*

+     *                     TODO(halcanary): ASSERT this condition.
+     *  @param SkWStream   The writable output stream to send the PDF to.
+     */
+    static bool EmitPDF(const SkTDArray<SkPDFDevice*>& pageDevices, SkWStream*);
+
+    /** Output the PDF to the passed stream.  It is an error to call this (it
+     *  will return false and not modify stream) if no pages have been added.
      *
      *  @param stream    The writable output stream to send the PDF to.
      */
-    bool emitPDF(SkWStream* stream);
+    bool emitPDF(SkWStream* stream) const {
+        return SkPDFDocument::EmitPDF(fPageDevices, stream);
+    }
 
-    /** Append the passed pdf device to the document as a new page.  Returns
-     *  true if successful.  Will fail if the document has already been emitted.
+    /** Append the passed pdf device to the document as a new page.
      *
-     *  @param pdfDevice The page to add to this document.
+     *  @param pdfDevice The page to add to this document. All pages
+     *                   added to this document should be created
+     *                   using the same SkPDFCanon.
      */
-    bool appendPage(SkPDFDevice* pdfDevice) {
+    void appendPage(SkPDFDevice* pdfDevice) {
         fPageDevices.push(SkRef(pdfDevice));
-        return true;
     }
 
+    /** Get the count of unique font types used in the given pages.
+     */
+    static void GetCountOfFontTypes(
+            const SkTDArray<SkPDFDevice*>& pageDevices,
+            int counts[SkAdvancedTypefaceMetrics::kOther_Font + 1],
+            int* notSubsettableCount,
+            int* notEmbedddableCount);
+
     /** Get the count of unique font types used in the document.
      */
     void getCountOfFontTypes(
-        int counts[SkAdvancedTypefaceMetrics::kOther_Font + 1],
-        int* notSubsettableCount,
-        int* notEmbedddableCount) const;
+            int counts[SkAdvancedTypefaceMetrics::kOther_Font + 1],
+            int* notSubsettableCount,
+            int* notEmbedddableCount) const {
+        return SkPDFDocument::GetCountOfFontTypes(
+                fPageDevices, counts, notSubsettableCount, notEmbedddableCount);
+    }
 
 private:
     SkTDArray<SkPDFDevice*> fPageDevices;
 };
 
 #endif