Initial error handling code

include/core/SkFlattenable.h

Index: include/core/SkFlattenable.h
diff --git a/include/core/SkFlattenable.h b/include/core/SkFlattenable.h
index 58e69fd416cb049b56d881942f6f77ebb676e93d..f204141d8f0932e40a6bef475817d7944963cf67 100644
--- a/include/core/SkFlattenable.h
+++ b/include/core/SkFlattenable.h
@@ -16,7 +16,7 @@ class SkFlattenableReadBuffer;
 class SkFlattenableWriteBuffer;
 
 #define SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(flattenable) \
-        SkFlattenable::Registrar(#flattenable, flattenable::CreateProc);
+        SkFlattenable::Registrar(#flattenable, flattenable::CreateProc, flattenable::IsA);
 
 #define SK_DECLARE_FLATTENABLE_REGISTRAR_GROUP() static void InitializeFlattenables();
 
@@ -29,12 +29,52 @@ class SkFlattenableWriteBuffer;
 #define SK_DECLARE_UNFLATTENABLE_OBJECT() \
     virtual Factory getFactory() SK_OVERRIDE { return NULL; }
 
-#define SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(flattenable) \
+/** For SkFlattenable derived objects that can be serialized without type checking
+  */
+#define SK_DEFINE_FLATTENABLE_SERIALIZABLE(flattenable) \

[mtklein, 2013/10/07 19:29:56]

Maybe SK_DEFINE_FLATTENABLE_DESERIALIZATION_PROCS?  _SERIALIZABLE is a little
vague, particularly given that these are method for deserialization.  +1 to
DECLARE -> DEFINE.

[sugoi1, 2013/10/08 20:23:10]

Done.

     virtual Factory getFactory() SK_OVERRIDE { return CreateProc; } \
     static SkFlattenable* CreateProc(SkFlattenableReadBuffer& buffer) { \
         return SkNEW_ARGS(flattenable, (buffer)); \
     }
 
+/** For SkFlattenable derived objects that need type checking
+  */
+#define SK_DEFINE_FLATTENABLE_TYPE_CHECKING(flattenable) \
+    static bool IsA(const char* name) { \
+        return (strcmp(flattenable::GetName(), name) == 0) || INHERITED::IsA(name); \

[sugoi1, 2013/10/07 15:54:08]

These type checks are now done using string comparisons.

+    } \
+    virtual const char* getFlattenableName() SK_OVERRIDE { \
+        return flattenable::GetName(); \
+    }
+
+/** For SkFlattenable derived objects with a valid type
+    This macros would typically be used by virtual classes, which need a type, but can't
+    be constructed or serialized directly without going through a derived class
+  */
+#define SK_DEFINE_FLATTENABLE_TYPE(flattenable) \

[mtklein, 2013/10/07 19:29:56]

Is there any case where we need to call SK_DEFINE_FLATTENABLE_TYPE_CHECKING
where we can't just call SK_DEFINE_FLATTENABLE_TYPE?  Why is GetName() separate?
 Can we merge these together into SK_DEFINE_FLATTENABLE_TYPE_CHECKING_PROCS?

[sugoi1, 2013/10/08 20:23:10]

Yeah, I had already done this locally.

+    static const char* GetName() { \
+        return #flattenable; \
+    } \
+    SK_DEFINE_FLATTENABLE_TYPE_CHECKING(flattenable)
+
+/** For SkFlattenable derived objects defined within the skia library
+  */
+#define SK_DEFINE_FLATTENABLE_SERIALIZABLE_TYPE(flattenable) \

[mtklein, 2013/10/07 19:29:56]

I think I may have been unclear before.  It looks like you're building up enough
levels of macros here so that each implementation class needs to call just one.
I appreciate the abstraction, but I'd prefer it the other way around.

I'd rather define just the very lowest-level macros here and have each class
call one, two, three, four, however many macros are needed to instantiate the
features that the class needs, ideally with names that make each feature
obvious.

E.g, unfold
  SK_DEFINE_FLATTENABLE_SERIALIZABLE_TYPE(foo)
into
  SK_DEFINE_FLATTENABLE_DESERIALIZATION_PROCS(foo)
  SK_DEFINE_FLATTENABLE_TYPE_CHECKING_PROCS(foo)

Ideally I'd like to be able to read one of the flattenable's headers and
understand why we've called each of the macros we call, without too much
cross-referencing into this file.  As it is right now I'm worried we're just
going to have our brains shut down as soon as we stumble across one of the many
similarly named SK_DEFINE_FLATTENABLE_SERIALZIABLE_TYPE_PROCS_CHECKING_GROUP
macros.  SK_DEFINE_FLATTENABLE_TYPE AND SK_DEFINE_FLATTENABLE_SERIALIZABLE_TYPE
in particular sound "the same" to me.

If our existing names aren't helpful, I'm strongly in favor of doing what you've
done with SK_DECLARE_PUBLIC_FLATETNABLE_DESERIALIZATION_PROCS, changing them for
the better with backwards compatibility.

[sugoi1, 2013/10/08 20:23:10]

Done.

+    SK_DEFINE_FLATTENABLE_SERIALIZABLE(flattenable) \
+    SK_DEFINE_FLATTENABLE_TYPE(flattenable)
+
+/** Deprecated, remove once noone is using it anymore
+  */
+#define SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(flattenable) \
+    SK_DEFINE_FLATTENABLE_SERIALIZABLE_TYPE(flattenable)
+    
+/** This is for an instantiatable SkFlattenable derived class
+    with derived classes available in header files
+  */
+#define SK_DECLARE_FLATTENABLE_GROUP_TYPE(flattenable) \
+    SK_DEFINE_FLATTENABLE_TYPE(flattenable) \
+    SK_DECLARE_FLATTENABLE_REGISTRAR_GROUP()
+
 /** \class SkFlattenable
 
  SkFlattenable is the base class for objects that need to be flattened
@@ -43,9 +83,11 @@ class SkFlattenableWriteBuffer;
  */
 class SK_API SkFlattenable : public SkRefCnt {
 public:
+
     SK_DECLARE_INST_COUNT(SkFlattenable)
 
     typedef SkFlattenable* (*Factory)(SkFlattenableReadBuffer&);
+    typedef bool (*TypeCheck)(const char*);
 
     SkFlattenable() {}
 
@@ -57,12 +99,45 @@ public:
 
     static Factory NameToFactory(const char name[]);
     static const char* FactoryToName(Factory);
-    static void Register(const char name[], Factory);
+
+    static void Register(const char name[], Factory, TypeCheck);
+
+    /** Returns the class id
+        GetName() methods on classes descending from SkFlattenable should return
+        the name associated with that class, or SkFlattenable::kNoneType if

[mtklein, 2013/10/07 19:29:56]

kNoneType?  comment generally needs an update?

[sugoi1, 2013/10/08 20:23:10]

Done.

+        they're not part of the subset of SkFlattenables that can be used with
+        SkFlattenable::TypeToFactory(). Typycally, SK_DEFINE_FLATTENABLE_TYPE() will
+        define GetName(), getFlattenableType() and IsA() in the derived class.
+        * Note that SK_DEFINE_FLATTENABLE_TYPE() is included in other macros that include
+          the "_TYPE" suffix, so most classes will have these declared by default.

[mtklein, 2013/10/07 19:29:56]

I get that this seems convenient, but the implicit definition of the type
checking code is going to be confusing.

[sugoi1, 2013/10/08 20:23:10]

Done. I did leave it in SK_DEFINE_FLATTENABLE_DESERIALIZATION_PROCS(), since
type checking will be mandatory to deserialize.

+          External classes can use SK_DEFINE_FLATTENABLE_EXTERN(), which will do the same,
+          but set the class type to SkFlattenable::kNone_Type.
+      */
+    static const char* GetName() { return "SkFlattenable"; }

[mtklein, 2013/10/07 19:29:56]

GetTypeName?

[sugoi1, 2013/10/08 20:23:10]

Done.

+
+    /** Returns the class id of an object

[mtklein, 2013/10/07 19:29:56]

class id of an object -> name of the object's class?

[sugoi1, 2013/10/08 20:23:10]

Done.

+      */
+    virtual const char* getFlattenableName() = 0;

[mtklein, 2013/10/07 19:29:56]

getTypeName()?

[sugoi1, 2013/10/08 20:23:10]

Done.

+
+    /** Returns whether the current object is of the given type

[mtklein, 2013/10/07 19:29:56]

There's no current object here?  How about

/** Returns whether this class is itself or inherits from a class with this type
name.
 *  (SkFlattenable is the root of this type checking system.)
 */

[sugoi1, 2013/10/08 20:23:10]

Done.

+      */
+    static bool IsA(const char* name) {
+        return strcmp(GetName(), name) == 0;
+    }
+
+    /** Checks if typeA is a typeB
+      * For example, if typeA is kSkImageFilter and typeB is kSkFlattenable, this would return true

[mtklein, 2013/10/07 19:29:56]

"SkImageFilter" and "SkFlattenable"?

[sugoi1, 2013/10/08 20:23:10]

Done.

+      * Inversely, if typeA is kSkFlattenable and typeB is kSkImageFilter, this would return false
+      */
+    static bool TypeIsA(const char* nameA, const char* nameB) {
+        TypeCheck typeCheck = NameToTypeCheck(nameA);
+        return typeCheck ? (*typeCheck)(nameB) : false;
+    }
 
     class Registrar {
     public:
-        Registrar(const char name[], Factory factory) {
-            SkFlattenable::Register(name, factory);
+        Registrar(const char name[], Factory factory, TypeCheck typeCheck) {
+            SkFlattenable::Register(name, factory, typeCheck);
         }
     };
 
@@ -76,6 +151,7 @@ protected:
 
 private:
     static void InitializeFlattenables();
+    static TypeCheck NameToTypeCheck(const char name[]);
 
     friend class SkGraphics;
     friend class SkFlattenableWriteBuffer;