SkPDF: Refactor SkPDFObject heiararchy.

src/pdf/SkPDFTypes.h

Index: src/pdf/SkPDFTypes.h
diff --git a/src/pdf/SkPDFTypes.h b/src/pdf/SkPDFTypes.h
index 76e191134cef4f7a954453d4f152c050e70b416b..10635e793b6d4d542ae39a8b68d83aa9a8a7870a 100644
--- a/src/pdf/SkPDFTypes.h
+++ b/src/pdf/SkPDFTypes.h
@@ -1,4 +1,3 @@
-
 /*
  * Copyright 2010 The Android Open Source Project
  *
@@ -10,7 +9,6 @@
 #ifndef SkPDFTypes_DEFINED
 #define SkPDFTypes_DEFINED
 
-#include "SkPDFTypes.h"
 #include "SkRefCnt.h"
 #include "SkScalar.h"
 #include "SkString.h"
@@ -28,10 +26,11 @@ class SkWStream;
     A PDF Object is the base class for primitive elements in a PDF file.  A
     common subtype is used to ease the use of indirect object references,
     which are common in the PDF format.
+
 */
 class SkPDFObject : public SkRefCnt {
 public:
-    SK_DECLARE_INST_COUNT(SkPDFObject)
+    SK_DECLARE_INST_COUNT(SkPDFObject);
 
     /** Subclasses must implement this method to print the object to the
      *  PDF file.
@@ -55,170 +54,192 @@ private:
     typedef SkRefCnt INHERITED;
 };
 
-/** \class SkPDFObjRef
+////////////////////////////////////////////////////////////////////////////////
 
-    An indirect reference to a PDF object.
-*/
-class SkPDFObjRef : public SkPDFObject {
+/**
+   A SkPDFUnion is a non-virtualized implementation of the
+   non-compound, non-specialized PDF Object types: Name, String,
+   Number, Boolean.
+ */
+class SkPDFUnion {
 public:
-    SK_DECLARE_INST_COUNT(SkPDFObjRef)
+    // move

[tomhudson, 2015/04/24 21:32:29]

This comment tells me nothing. Please improve.
(Getting rid of is acceptable, but since I haven't a clue why this function
exists I'd rather see an explanation here.)

[hal.canary, 2015/04/25 13:41:02]

Done.  Please let me know if you need more information to clarify.

+    SkPDFUnion move() { return static_cast<SkPDFUnion&&>(*this); }
+    SkPDFUnion(SkPDFUnion&& other);
+    SkPDFUnion& operator=(SkPDFUnion&& other);
 
-    /** Create a reference to an existing SkPDFObject.
-     *  @param obj The object to reference.
-     */
-    explicit SkPDFObjRef(SkPDFObject* obj);
-    virtual ~SkPDFObjRef();
+    ~SkPDFUnion();
 
-    // The SkPDFObject interface.
-    virtual void emitObject(SkWStream* stream,
-                            const SkPDFObjNumMap& objNumMap,
-                            const SkPDFSubstituteMap& substitutes) override;
-    virtual void addResources(SkPDFObjNumMap*,
-                              const SkPDFSubstituteMap&) const override;
+    /** The following nine functions are the standard way of creating
+        SkPDFUnion objects. */
 
-private:
-    SkAutoTUnref<SkPDFObject> fObj;
+    static SkPDFUnion Int(int32_t);
 
-    typedef SkPDFObject INHERITED;
-};
+    static SkPDFUnion Bool(bool);
 
-/** \class SkPDFInt
+    static SkPDFUnion Scalar(SkScalar);
 
-    An integer object in a PDF.
-*/
-class SkPDFInt : public SkPDFObject {
-public:
-    SK_DECLARE_INST_COUNT(SkPDFInt)
+    /** These two functions do NOT take ownership of ptr, and do NOT
+        copy the string.  Suitable for passing in static const
+        strings. For example:
+          SkPDFUnion n = SkPDFUnion::Name("Length");
+          SkPDFUnion u = SkPDFUnion::String("Identity"); */
 
-    /** Create a PDF integer (usually for indirect reference purposes).
-     *  @param value An integer value between 2^31 - 1 and -2^31.
-     */
-    explicit SkPDFInt(int32_t value);
-    virtual ~SkPDFInt();
+    /** SkPDFUnion::Name(const char*) assumes that the passed string
+        is already a valid name (that is: it has no control or
+        whitespace characters).  This will not copy the name. */
+    static SkPDFUnion Name(const char*);
 
-    // The SkPDFObject interface.
-    virtual void emitObject(SkWStream* stream,
-                            const SkPDFObjNumMap& objNumMap,
-                            const SkPDFSubstituteMap& substitutes) override;
+    /** SkPDFUnion::String will encode the passed string.  This will
+        not copy the name. */
+    static SkPDFUnion String(const char*);
 
-private:
-    int32_t fValue;
+    /** SkPDFUnion::Name(const SkString&) does not assume that the
+        passed string is already a valid name and it will escape the
+        string. */
+    static SkPDFUnion Name(const SkString&);
 
-    typedef SkPDFObject INHERITED;
-};
+    /** SkPDFUnion::String will encode the passed string. */
+    static SkPDFUnion String(const SkString&);
 
-/** \class SkPDFBool
+    /** This function DOES take ownership of the object. E.g.
+          SkAutoTUnref<SkPDFDict> dict(new SkPDFDict);
+          dict->insert(.....);
+          SkPDFUnion u = SkPDFUnion::Object(dict.detach()) */
+    static SkPDFUnion Object(SkPDFObject*);
 
-    An boolean value in a PDF.
-*/
-class SkPDFBool : public SkPDFObject {
-public:
-    SK_DECLARE_INST_COUNT(SkPDFBool)
+    /** This function DOES take ownership of the object. E.g.
+          SkAutoTUnref<SkPDFBitmap> image(
+                 SkPDFBitmap::Create(fCanon, bitmap));
+          SkPDFUnion u = SkPDFUnion::ObjRef(image.detach()) */
+    static SkPDFUnion ObjRef(SkPDFObject*);
 
-    /** Create a PDF boolean.
-     *  @param value true or false.
-     */
-    explicit SkPDFBool(bool value);
-    virtual ~SkPDFBool();
+    /** These two non-virtual methods mirror SkPDFObject's
+        corresponding virtuals. */
+    void emitObject(SkWStream*,
+                    const SkPDFObjNumMap&,
+                    const SkPDFSubstituteMap&) const;
+    void addResources(SkPDFObjNumMap*, const SkPDFSubstituteMap&) const;
 
-    // The SkPDFObject interface.
-    virtual void emitObject(SkWStream* stream,
-                            const SkPDFObjNumMap& objNumMap,
-                            const SkPDFSubstituteMap& substitutes) override;
+    /** Given an arbitrary string, convert it to a valid name. */
+    static SkString EscapeName(const char*, size_t len);
+    /** Most names need no escaping */
+    static bool IsValidName(const char*);

[tomhudson, 2015/04/24 21:32:30]

Musing for future consideration: do we ever use this other than in a construct
something like:

SkString name = ...
if (!SkPDFUnion::IsValidName(name.c_str())) {
  name = SkPDFUnion::EscapeName(name.c_str(), strlen);
}

[hal.canary, 2015/04/25 13:41:02]

Actually, in every case we create a Name object, we either KNOW it is a valid
name, in which case we ASSERT(IsValidName("name'))  If we don't know, we always
escape it.

Since this function is not used outside of this class, I'm removing it from the
header.

+
+    bool isName() const;
 
 private:
-    bool fValue;
+    union {
+        int32_t fIntValue;
+        bool fBoolValue;
+        SkScalar fScalarValue;
+        const char* fStaticString;
+        char fSkString[sizeof(SkString)];
+        SkPDFObject* fObject;
+    };
+    enum class Type : char {
+        /** It is an error to call emitObject() or addResources() on an
+            kDestroyed object. */
+        kDestroyed = 0,
+        kInt,
+        kBool,
+        kScalar,
+        kName,
+        kString,
+        kNameSkS,
+        kStringSkS,
+        kObjRef,
+        kObject,
+    };
+    Type fType;
 
-    typedef SkPDFObject INHERITED;
+    SkPDFUnion(Type);
+    SkPDFUnion& operator=(const SkPDFUnion&) = delete;
+    SkPDFUnion(const SkPDFUnion&) = delete;
 };
+SK_COMPILE_ASSERT(sizeof(SkString) == sizeof(void*), SkString_size);
 
-/** \class SkPDFScalar
+////////////////////////////////////////////////////////////////////////////////
 
-    A real number object in a PDF.
-*/
-class SkPDFScalar : public SkPDFObject {
+/**  This class is a SkPDFUnion with SkPDFObject virtuals attached. */
+// TODO(halcanary): 99% of the uses of this class should be
+// transitioned to using a bare SkPDFUnion inside an array or dict.
+class SkPDFAtom : public SkPDFObject {
 public:
-    SK_DECLARE_INST_COUNT(SkPDFScalar)
-
-    /** Create a PDF real number.
-     *  @param value A real value.
-     */
-    explicit SkPDFScalar(SkScalar value);
-    virtual ~SkPDFScalar();
-
-    static void Append(SkScalar value, SkWStream* stream);
+    void emitObject(SkWStream* stream,
+                    const SkPDFObjNumMap& objNumMap,
+                    const SkPDFSubstituteMap& substitutes) final;
+    void addResources(SkPDFObjNumMap*, const SkPDFSubstituteMap&) const final;
 
-    // The SkPDFObject interface.
-    virtual void emitObject(SkWStream* stream,
-                            const SkPDFObjNumMap& objNumMap,
-                            const SkPDFSubstituteMap& substitutes) override;
+protected:
+    SkPDFAtom(SkPDFUnion&& v) : fValue(v.move()) {}
 
 private:
-    SkScalar fValue;
-
+    const SkPDFUnion fValue;
     typedef SkPDFObject INHERITED;
 };
 
-/** \class SkPDFString
+/** The following six classes exist only to ease transition to SkPDFUnion. */
+class SkPDFObjRef : public SkPDFAtom {
+public:
+    SK_DECLARE_INST_COUNT(SkPDFObjRef);
+    explicit SkPDFObjRef(SkPDFObject* obj)
+        : INHERITED(SkPDFUnion::ObjRef(SkRef(obj))) {}
+    typedef SkPDFAtom INHERITED;
+};
 
-    A string object in a PDF.
-*/
-class SkPDFString : public SkPDFObject {
+class SkPDFInt : public SkPDFAtom {
 public:
-    SK_DECLARE_INST_COUNT(SkPDFString)
+    SK_DECLARE_INST_COUNT(SkPDFInt);
+    explicit SkPDFInt(int32_t value) : INHERITED(SkPDFUnion::Int(value)) {}
+    typedef SkPDFAtom INHERITED;
+};
 
-    /** Create a PDF string. Maximum length (in bytes) is 65,535.
-     *  @param value A string value.
-     */
-    explicit SkPDFString(const char value[]);
-    explicit SkPDFString(const SkString& value);
+class SkPDFBool : public SkPDFAtom {
+public:
+    SK_DECLARE_INST_COUNT(SkPDFBool);
+    explicit SkPDFBool(bool value) : INHERITED(SkPDFUnion::Bool(value)) {}
+    typedef SkPDFAtom INHERITED;
+};
 
-    virtual ~SkPDFString();
+class SkPDFScalar : public SkPDFAtom {
+public:
+    SK_DECLARE_INST_COUNT(SkPDFScalar);
+    explicit SkPDFScalar(SkScalar value)
+        : INHERITED(SkPDFUnion::Scalar(value)) {}
+    static void Append(SkScalar value, SkWStream* stream);
+    typedef SkPDFAtom INHERITED;
+};
 
-    // The SkPDFObject interface.
-    virtual void emitObject(SkWStream* stream,
-                            const SkPDFObjNumMap& objNumMap,
-                            const SkPDFSubstituteMap& substitutes) override;
+class SkPDFString : public SkPDFAtom {
+public:
+    SK_DECLARE_INST_COUNT(SkPDFString);
+    explicit SkPDFString(const char value[])
+        : INHERITED(SkPDFUnion::String(value)) {}
+    explicit SkPDFString(const SkString& value)
+        : INHERITED(SkPDFUnion::String(value)) {}
 
     static SkString FormatString(const char* input, size_t len);
-private:
-    static const size_t kMaxLen = 65535;
 
-    const SkString fValue;
+    static const size_t kMaxLen = 65535;
 
-    typedef SkPDFObject INHERITED;
+private:
+    typedef SkPDFAtom INHERITED;
 };
 
-/** \class SkPDFName
-
-    A name object in a PDF.
-*/
-class SkPDFName : public SkPDFObject {
+class SkPDFName : public SkPDFAtom {
 public:
-    SK_DECLARE_INST_COUNT(SkPDFName)
-
-    /** Create a PDF name object. Maximum length is 127 bytes.
-     *  @param value The name.
-     */
-    explicit SkPDFName(const char name[]);
-    explicit SkPDFName(const SkString& name);
-    virtual ~SkPDFName();
+    SK_DECLARE_INST_COUNT(SkPDFName);
+    /** Create a PDF name object. Maximum length is 127 bytes. */
+    explicit SkPDFName(const char name[])
+        : INHERITED(SkPDFUnion::Name(SkString(name))) {}
+    explicit SkPDFName(const SkString& name)
+        : INHERITED(SkPDFUnion::Name(name)) {}
 
-    bool operator==(const SkPDFName& b) const;
-
-    // The SkPDFObject interface.
-    virtual void emitObject(SkWStream* stream,
-                            const SkPDFObjNumMap& objNumMap,
-                            const SkPDFSubstituteMap& substitutes) override;
-
-private:
     static const size_t kMaxLen = 127;
 
-    const SkString fValue;
-
-    static SkString FormatName(const SkString& input);
-
-    typedef SkPDFObject INHERITED;
+private:
+    typedef SkPDFAtom INHERITED;
 };
 
 /** \class SkPDFArray
@@ -229,6 +250,8 @@ class SkPDFArray : public SkPDFObject {
 public:
     SK_DECLARE_INST_COUNT(SkPDFArray)
 
+    static const int kMaxLen = 8191;
+
     /** Create a PDF array. Maximum length is 8191.
      */
     SkPDFArray();
@@ -243,7 +266,7 @@ public:
 
     /** The size of the array.
      */
-    int size() { return fValue.count(); }
+    int size() const;
 
     /** Preallocate space for the given number of entries.
      *  @param length The number of array slots to preallocate.
@@ -254,27 +277,26 @@ public:
      *  @param value The value to add to the array.
      *  @return The value argument is returned.
      */
+    // DEPRECATED
     SkPDFObject* append(SkPDFObject* value);
 
-    /** Creates a SkPDFInt object and appends it to the array.
-     *  @param value The value to add to the array.
-     */
-    void appendInt(int32_t value);
-
-    /** Creates a SkPDFScalar object and appends it to the array.
+    /** Appends a value to the end of the array.
      *  @param value The value to add to the array.
      */
-    void appendScalar(SkScalar value);
-
-    /** Creates a SkPDFName object and appends it to the array.
-     *  @param value The value to add to the array.
-     */
-    void appendName(const char name[]);
+    void appendInt(int32_t);
+    void appendBool(bool);
+    void appendScalar(SkScalar);
+    void appendName(const char[]);
+    void appendName(const SkString&);
+    void appendString(const char[]);
+    void appendString(const SkString&);
+    /** appendObject and appendObjRef take ownership of the passed object */
+    void appendObject(SkPDFObject*);
+    void appendObjRef(SkPDFObject*);
 
 private:
-    static const int kMaxLen = 8191;
-    SkTDArray<SkPDFObject*> fValue;
-
+    SkTDArray<SkPDFUnion> fValues;
+    void append(SkPDFUnion&& value);
     typedef SkPDFObject INHERITED;
 };
 
@@ -313,104 +335,53 @@ public:
      *  @param value The value for this dictionary entry.
      *  @return The value argument is returned.
      */
+    // DEPRECATED
     SkPDFObject* insert(SkPDFName* key, SkPDFObject* value);
+    // DEPRECATED
+    SkPDFObject* insert(const char key[], SkPDFObject* value);
 
-    /** Add the value to the dictionary with the given key.  Refs value.  The
-     *  method will create the SkPDFName object.
+    /** Add the value to the dictionary with the given key.  Takes
+     *  ownership of the object;

[tomhudson, 2015/04/24 21:32:30]

Nit: interrupted comment edit?

[hal.canary, 2015/04/25 13:41:02]

Thinking in C.  ';' ends a statement.

      *  @param key   The text of the key for this dictionary entry.
      *  @param value The value for this dictionary entry.
-     *  @return The value argument is returned.
      */
-    SkPDFObject* insert(const char key[], SkPDFObject* value);
+    void insertObject(const char key[], SkPDFObject* value);
+    void insertObject(const SkString& key, SkPDFObject* value);
+    void insertObjRef(const char key[], SkPDFObject* value);
+    void insertObjRef(const SkString& key, SkPDFObject* value);
 
-    /** Add the int to the dictionary with the given key.
+    /** Add the value to the dictionary with the given key.
      *  @param key   The text of the key for this dictionary entry.
-     *  @param value The int value for this dictionary entry.
+     *  @param value The value for this dictionary entry.
      */
     void insertInt(const char key[], int32_t value);
-
-    /**
-     *  Calls insertInt() but asserts in debug builds that the value can be represented
-     *  by an int32_t.
-     */
-    void insertInt(const char key[], size_t value) {
-        this->insertInt(key, SkToS32(value));
-    }
-
-    /** Add the scalar to the dictionary with the given key.
-     *  @param key   The text of the key for this dictionary entry.
-     *  @param value The scalar value for this dictionary entry.
-     */
+    void insertInt(const char key[], size_t value);
     void insertScalar(const char key[], SkScalar value);
-
-    /** Add the name to the dictionary with the given key.
-     *  @param key   The text of the key for this dictionary entry.
-     *  @param name  The name for this dictionary entry.
-     */
-    void insertName(const char key[], const char name[]);
-
-    /** Add the name to the dictionary with the given key.
-     *  @param key   The text of the key for this dictionary entry.
-     *  @param name  The name for this dictionary entry.
-     */
-    void insertName(const char key[], const SkString& name) {
-        this->insertName(key, name.c_str());
-    }
+    void insertName(const char key[], const char nameValue[]);
+    void insertName(const char key[], const SkString& nameValue);
+    void insertString(const char key[], const char value[]);
+    void insertString(const char key[], const SkString& value);
 
     /** Remove all entries from the dictionary.
      */
     void clear();
 
-protected:
-    /** Use to remove a single key from the dictionary.
-     */
-    void remove(const char key[]);
-
-    /** Insert references to all of the key-value pairs from the other
-     *  dictionary into this one.
-     */
-    void mergeFrom(const SkPDFDict& other);
-
 private:
-    struct Rec {
-        SkPDFName* key;
-        SkPDFObject* value;
-        Rec(SkPDFName* k, SkPDFObject* v) : key(k), value(v) {}
+    struct Record {
+        SkPDFUnion fKey;
+        SkPDFUnion fValue;
     };
-
+    SkTDArray<Record> fRecords;
     static const int kMaxLen = 4095;
 
-    SkTDArray<struct Rec> fValue;
-
-    SkPDFObject* append(SkPDFName* key, SkPDFObject* value);
+    void set(const SkPDFUnion& name, const SkPDFUnion& value);
+    void set(SkPDFUnion&& name, SkPDFUnion&& value);
 
     typedef SkPDFObject INHERITED;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
 
-/** \class SkPDFSubstituteMap
-
-    The PDF Substitute Map manages substitute objects and owns the
-    substitutes.
-*/
-class SkPDFSubstituteMap : SkNoncopyable {
-public:
-    ~SkPDFSubstituteMap();
-    /** Set substitute object for the passed object.
-        Refs substitute.
-     */
-    void setSubstitute(SkPDFObject* original, SkPDFObject* substitute);
-
-    /** Find and return any substitute object set for the passed object. If
-     *  there is none, return the passed object.
-     */
-    SkPDFObject* getSubstitute(SkPDFObject* object) const;
-
-private:
-    SkTHashMap<SkPDFObject*, SkPDFObject*> fSubstituteMap;
-};
-
 /** \class SkPDFObjNumMap
 
     The PDF Object Number Map manages object numbers.  It is used to
@@ -436,4 +407,32 @@ private:
     SkTHashMap<SkPDFObject*, int32_t> fObjectNumbers;
 };
 
+////////////////////////////////////////////////////////////////////////////////
+
+/** \class SkPDFSubstituteMap
+
+    The PDF Substitute Map manages substitute objects and owns the
+    substitutes.
+*/
+class SkPDFSubstituteMap : SkNoncopyable {
+public:
+    ~SkPDFSubstituteMap();
+    /** Set substitute object for the passed object.
+        Refs substitute.
+     */
+    void setSubstitute(SkPDFObject* original, SkPDFObject* substitute);
+
+    /** Find and return any substitute object set for the passed object. If
+     *  there is none, return the passed object.
+     */
+    SkPDFObject* getSubstitute(SkPDFObject* object) const;
+
+    SkPDFObject* operator()(SkPDFObject* o) const {
+        return this->getSubstitute(o);
+    }
+
+private:
+    SkTHashMap<SkPDFObject*, SkPDFObject*> fSubstituteMap;
+};
+
 #endif