Simplify reference management in SkPDF

src/pdf/SkPDFTypes.h

Index: src/pdf/SkPDFTypes.h
diff --git a/src/pdf/SkPDFTypes.h b/src/pdf/SkPDFTypes.h
index b0ce0b97d0cdd4783e7cf3eadab5977d643860a9..49ba1124d55a15184ddda1451db5a2160d847c28 100644
--- a/src/pdf/SkPDFTypes.h
+++ b/src/pdf/SkPDFTypes.h
@@ -20,6 +20,9 @@
 class SkPDFCatalog;
 class SkWStream;
 
+class SkPDFObject;
+typedef SkTSet<SkPDFObject*> SkPDFObjectSet;

[mtklein, 2015/01/26 23:38:59]

I generally prefer writing out the full names of these things instead of
typedefing, unless you're leaving an option to easily swap between two
implementations or something.

I'm also pretty skeptical of SkTSet in general, but I see that really isn't the
point here.   It's not clear that we need to maintain sorted and insertion
order.  Might be something to look to simplify later. 

[hal.canary, 2015/02/09 23:35:01]

I'm not sure that I need SkTSet.  I really need an abstract interface:

    {
      bool add(SkPDFObject*); // return false if already known
    }

for addResources(), qhich I will add to SkPDFCatalog in a later CL.

Then SkTSet<SkPDFObject*> (or something like it) can be an implementation
detail of SkPDFCatalog.

Or maybe I'll just implement Map<SkPDFbject*, ObjectIndex> in a
later CL.

+
 /** \class SkPDFObject
 
     A PDF Object is the base class for primitive elements in a PDF file.  A
@@ -37,39 +40,16 @@ public:
      */
     virtual void emitObject(SkWStream* stream, SkPDFCatalog* catalog) = 0;
 
-    /** For non-primitive objects (i.e. objects defined outside this file),
-     *  this method will add to newResourceObjects any objects that this method
-     *  depends on, but not already in knownResourceObjects. This operates
-     *  recursively so if this object depends on another object and that object
-     *  depends on two more, all three objects will be added.
+    /**
+     *  An object's has a resource if it depends, via an indirect
+     *  reference on another object.  This method recursively adds
+     *  resouces for this object and for its resouces' resouces....

[mtklein, 2015/01/26 23:38:59]

This first sentence doesn't really make sense.  Is it

/** Adds all transitive dependencies of this object to resourceSet. */

?

It might make sense to mention why we pass catalog.  Can catalog be const*, or
const&?

[hal.canary, 2015/02/09 23:35:01]

Done.

      *
-     *  @param knownResourceObjects  The set of resources to be ignored.
-     *  @param newResourceObjects  The set to append dependant resources to.
-     */
-    virtual void getResources(const SkTSet<SkPDFObject*>& knownResourceObjects,
-                              SkTSet<SkPDFObject*>* newResourceObjects);
-
-    /** Static helper function to add a resource to a list.  The list takes
-     *  a reference.
-     * @param resource  The resource to add.
-     * @param list      The list to add the resource to.
-     */
-    static void AddResourceHelper(SkPDFObject* resource,
-                                  SkTDArray<SkPDFObject*>* list);
-
-    /** Static helper function to copy and reference the resources (and all
-     *   their subresources) into a new list.
-     * @param resources The resource list.
-     * @param newResourceObjects All the resource objects (recursively) used on
-     *                         the page are added to this array.  This gives
-     *                         the caller a chance to deduplicate resources
-     *                         across pages.
-     * @param knownResourceObjects  The set of resources to be ignored.
+     *  Ther default implementation does nothing and is therefore
+     *  suitable for "leaf" objects.

[mtklein, 2015/01/26 23:38:59]

Instead of writing this note, you could just put the implementation here, {}.

[hal.canary, 2015/02/09 23:35:01]

You shouldn't put virtuals in a header file.

[mtklein, 2015/02/09 23:47:17]

Why not?

      */
-    static void GetResourcesHelper(
-            const SkTDArray<SkPDFObject*>* resources,
-            const SkTSet<SkPDFObject*>& knownResourceObjects,
-            SkTSet<SkPDFObject*>* newResourceObjects);
+    virtual void addResources(SkPDFObjectSet* resourceSet,

[mtklein, 2015/01/26 23:38:59]

Can this method be const ?

+                              SkPDFCatalog* catalog);
 
 private:
     typedef SkRefCnt INHERITED;
@@ -91,6 +71,7 @@ public:
 
     // The SkPDFObject interface.
     virtual void emitObject(SkWStream* stream, SkPDFCatalog* catalog) SK_OVERRIDE;
+    virtual void addResources(SkPDFObjectSet*, SkPDFCatalog*) SK_OVERRIDE;
 
 private:
     SkAutoTUnref<SkPDFObject> fObj;
@@ -254,6 +235,7 @@ public:
 
     // The SkPDFObject interface.
     virtual void emitObject(SkWStream* stream, SkPDFCatalog* catalog) SK_OVERRIDE;
+    virtual void addResources(SkPDFObjectSet*, SkPDFCatalog*) SK_OVERRIDE;
 
     /** The size of the array.
      */
@@ -325,6 +307,7 @@ public:
 
     // The SkPDFObject interface.
     virtual void emitObject(SkWStream* stream, SkPDFCatalog* catalog) SK_OVERRIDE;
+    virtual void addResources(SkPDFObjectSet*, SkPDFCatalog*) SK_OVERRIDE;
 
     /** The size of the dictionary.
      */