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 0b21abcc764c7aed2bd67f9a83c5162df3e222a9..80928e44f84d7d7cc50cda0fa7fd2616c29212ee 100644
--- a/include/core/SkFlattenable.h
+++ b/include/core/SkFlattenable.h
@@ -27,14 +27,52 @@ class SkFlattenableWriteBuffer;
     }
 
 #define SK_DECLARE_UNFLATTENABLE_OBJECT() \
-    virtual Factory getFactory() SK_OVERRIDE { return NULL; }; \
+    virtual Factory getFactory() SK_OVERRIDE { return NULL; };
+
+#define SK_DEFINE_FLATTENABLE_TYPE_FCNT(flattenable) \

[mtklein, 2013/09/24 22:52:18]

OK, I give up.  What's FCNT stand for / short for?

[sugoi1, 2013/09/25 21:15:27]

Sorry, typo, I meant FNCT, as in function. The name is pretty generic. I wasn't
particularly inspired for that macro :)

+    static bool IsA(SkFlattenable::Type type) { \
+        return (flattenable::GetType() == type) || INHERITED::IsA(type); \
+    } \
+    virtual SkFlattenable::Type getFlattenableType() SK_OVERRIDE { \
+        return flattenable::GetType(); \
+    }
 
-#define SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(flattenable) \
+#define SK_DEFINE_FLATTENABLE_TYPE(flattenable) \

[mtklein, 2013/09/24 22:52:18]

Normally I love macros which cleverly do things like k##flattenable, but I think
as a rule of thumb it should probably only be done when both the macro's
definition and all uses fit on the same screen in the same file.  (Yes, if you
look you'll find places where I've broken this rule myself in the last week...) 
Given that this is spread out over many files and the reader is unlikely to know
what's going on, I think I'd prefer to read

SK_DEFINE_FLATTENABLE_TYPE(SkFoo, SkFlattenable::kSkFoo)

[sugoi1, 2013/09/25 21:15:27]

I not against this, but I sort of like the fact that the macro user didn't have
to care about the type and that it would be automatically created for him. The
reason why I hadn't done this is that you have to add you enum inside
SkFlattenable::Type anyway, so the compiler will tell you what's going on even
if you just write SK_DEFINE_FLATTENABLE_TYPE(SkFoo) since it won't compile until
you add your enum value here.

+    static SkFlattenable::Type GetType() { \
+        return SkFlattenable::k##flattenable; \
+    } \
+    SK_DEFINE_FLATTENABLE_TYPE_FCNT(flattenable)
+
+/** Only for SkFlattenable derived objects that have implementations hidden in a cpp file
+  */
+#define SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_UTILS() \

[mtklein, 2013/09/24 22:52:18]

May SK_DECLARE_FLATTENABLE_DESERIALIZATION and
SK_DEFINE_FLATTENABLE_DESERIALIZATION for the next one?  What are you trying to
convey with PUBLIC here?  Without seeing this file, I assumed it literally would
be some macro wrapped with a  "public: " and "private: ".

Maybe even we want these to be

SK_{DECLARE,DEFINE}_API_FOR_SK_FLATTENABLE_SERIALIZATION(class)

[sugoi1, 2013/09/25 21:15:27]

I didn't invent the PUBLIC name, it was there before, I simply cut it into
pieces, but I can rename the old one too.

+    static SkFlattenable::Factory GetFactory(SkFlattenable::Type); \
+    static SkFlattenable::TypeCheck GetTypeCheck(SkFlattenable::Type);
+
+#define SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION(flattenable) \
     virtual Factory getFactory() SK_OVERRIDE { return CreateProc; } \
     static SkFlattenable* CreateProc(SkFlattenableReadBuffer& buffer) { \
         return SkNEW_ARGS(flattenable, (buffer)); \
     }
 
+/** For SkFlattenable derived objects defined within the skia library
+  */
+#define SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(flattenable) \

[mtklein, 2013/09/24 22:52:18]

I think we'll be better off in the long term if we cut out these two
middle-level macros.  The fewer SK_DESOMETHING_SOMETHING_ABOUT_SERIALIZATION
macros we have, the more comprehensible this is going to be in the end, even if
that means we need to spend 3 or 4 lines expressing what each class can or can't
do.

[sugoi1, 2013/09/25 21:15:27]

All right, I'll try to combine them into fewer macros so that each class has
less macros in its declaration.

+    SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION(flattenable) \
+    SK_DEFINE_FLATTENABLE_TYPE(flattenable)
+
+/** For SkFlattenable derived objects defined outside of the skia (sample code, gms, etc).
+    Since these external object won't go through the ipc serialization, they are not given
+    a "Type" in the SkFlattenable::Type enum and there is no need to modify SkFlattenable.h
+    to add these derived classes. Note that the type is set to SkFlattenable::kNone here.
+  */
+#define SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS_EXTERN(flattenable) \
+    SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION(flattenable) \
+    static SkFlattenable::Type GetType() { \
+        return SkFlattenable::kNone; \
+    } \
+    SK_DEFINE_FLATTENABLE_TYPE_FCNT(flattenable)
+
 /** \class SkFlattenable
 
  SkFlattenable is the base class for objects that need to be flattened
@@ -43,9 +81,111 @@ class SkFlattenableWriteBuffer;
  */
 class SK_API SkFlattenable : public SkRefCnt {
 public:
+    enum Type {

[mtklein, 2013/09/24 22:52:18]

Do you have any opinion on whether we want to limit the types which are
compatible with this new code to just the image filters?  Is it easier to
support all SkFlattenables (which I'm assuming is this list?)?  Equivalent for
you from a security perspective?

[mtklein, 2013/09/24 22:52:18]

It'll help to add some comments here about how this is a closed set of
SkFlattenables that have special serializable properties: type-checking,
stability across binary versions, etc.

I think it's also _essential_ that we remind the reader very loudly that if you
reorder this enum the world will burn down (but appending new values is okay).

It'd also be nice to remind the reader and compiler how many bytes we use to
encode this (presumably 4?) with an SK_COMPILE_ASSERT(kTypeCount < something,
"Sorry, we're out of SkFlattenable::Types!").

[sugoi1, 2013/09/25 21:15:27]

I don't see any reason to exclude types from it. This allows type checking to
differentiate between "wrong type" and "complete garbage" (I'm not doing this in
this cl, but it's possible). It also makes it more future proof. If we'd depend
on this for security, it would mean that we'd open security breaches when adding
types in this enum, which would be pretty bad. I'm counting on type checking to
make the unused types irrelevant, and therefore not a security issue.

[sugoi1, 2013/09/25 21:15:27]

I added the comment about not reordering and a note about types, but I'll wait
until this is complete to see if I can add the compile assert. I'm not certain
how to handle (and if we have to handle) SkFlattenable classes declared outside
of skia yet. I'm hoping that won't be necessary.

+        kNone,

[mtklein, 2013/09/24 22:52:18]

Oh I so hate to ask you to do this, but you're going to need to fire up find and
replace.  Skia style wants this to look like

enum Type {
  kNone_Type,
  kSkFlattenable_Type,
  kBATShader_Type,
  ...
  kTypeCount
};

[sugoi1, 2013/09/25 21:15:27]

No problem, that's easy enough.

+        kSkFlattenable,
+        kBATShader,
+        kSk2DPathEffect,
+        kSk3DShader,
+        kSkAnnotation,
+        kSkArithmeticMode_scalar,
+        kSkAvoidXfermode,
+        kSkBicubicImageFilter,
+        kSkBitmapProcShader,
+        kSkBitmapSource,
+        kSkBlurDrawLooper,
+        kSkBlurImageFilter,
+        kSkBlurMaskFilterImpl,
+        kSkClearXfermode,
+        kSkColorFilter,
+        kSkColorFilterImageFilter,
+        kSkColorMatrixFilter,
+        kSkColorShader,
+        kSkColorTable,
+        kSkComposeImageFilter,
+        kSkComposePathEffect,
+        kSkComposeShader,
+        kSkCornerPathEffect,
+        kSkCosineMapper,
+        kSkData,
+        kSkDataPixelRef,
+        kSkDataSet,
+        kSkDataTable,
+        kSkDiffuseLightingImageFilter,
+        kSkDilateImageFilter,
+        kSkDiscreteMapper,
+        kSkDiscretePathEffect,
+        kSkDisplacementMapEffect,
+        kSkDistantLight,
+        kSkDownSampleImageFilter,
+        kSkDrawLooper,
+        kSkDropShadowImageFilter,
+        kSkDstInXfermode,
+        kSkDstOutXfermode,
+        kSkEmbossMaskFilter,
+        kSkEmptyShader,
+        kSkErodeImageFilter,
+        kSkFilterShader,
+        kSkImageFilter,
+        kSkImageRef_GlobalPool,
+        kSkKernel33MaskFilter,
+        kSkLayerDrawLooper,
+        kSkLayerRasterizer,
+        kSkLerpXfermode,
+        kSkLight,
+        kSkLightingColorFilter,
+        kSkLightingColorFilter_JustAdd,
+        kSkLightingColorFilter_JustMul,
+        kSkLightingColorFilter_NoPin,
+        kSkLightingColorFilter_SingleMul,
+        kSkLinearGradient,
+        kSkLine2DPathEffect,
+        kSkLumaMaskXfermode,
+        kSkLumaMaskXfermodeSrcOver,
+        kSkMagnifierImageFilter,
+        kSkMallocPixelRef,
+        kSkMaskFilter,
+        kSkMatrixConvolutionImageFilter,
+        kSkMergeImageFilter,
+        kSkModeColorFilter,
+        kSkOffsetImageFilter,
+        kSkPathEffect,
+        kSkPath1DPathEffect,
+        kSkPath2DPathEffect,
+        kSkPerlinNoiseShader,
+        kSkPixelRef,
+        kSkPixelXorXfermode,
+        kSkPointLight,
+        kSkProcCoeffXfermode,
+        kSkProcXfermode,
+        kSkRadialGradient,
+        kSkRasterizer,
+        kSkRectShaderImageFilter,
+        kSkShader,
+        kSkSpecularLightingImageFilter,
+        kSkSpotLight,
+        kSkSrcXfermode,
+        kSkStippleMaskFilter,
+        kSkSumPathEffect,
+        kSkSweepGradient,
+        kSkTable_ColorFilter,
+        kSkTableMaskFilter,
+        kSkTransparentShader,
+        kSkTriColorShader,
+        kSkTwoPointConicalGradient,
+        kSkTwoPointRadialGradient,
+        kSkUnitMapper,
+        kSkXfermode,
+        kSkXfermodeImageFilter,
+        kSrc_SkModeColorFilter,
+        kSrcOver_SkModeColorFilter,
+        kTypeCount
+    };

[mtklein, 2013/09/24 22:52:18]

Can you add a comment betwe kSrcOver_SkModeColorFilter and kTypeCount that says
something like "// New values go here"?  Not always obvious that the last guy is
a floating count.

[sugoi1, 2013/09/25 21:15:27]

Done.

+
     SK_DECLARE_INST_COUNT(SkFlattenable)
 
     typedef SkFlattenable* (*Factory)(SkFlattenableReadBuffer&);
+    typedef bool (*TypeCheck)(SkFlattenable::Type);
 
     SkFlattenable() {}
 
@@ -59,6 +199,32 @@ public:
     static const char* FactoryToName(Factory);
     static void Register(const char name[], Factory);
 
+    /** Returns the class id

[mtklein, 2013/09/24 22:52:18]

Let's flesh out this (and related comments):
  /** GetType methods on classes descending from SkFlattenable should return the
SkFlattenable::Type associated with that class, or SkFlattenable::kNoneType if
they're not part of the subset of SkFlattenables that can be used with
TypeToFactory.

  (Typically you'd use SK_MACRO_FOO to define GetType(), getFlattenableType()
and isA() in any subclass of SkFlattenable, or SK_MACRO_BAR will define them for
you to return kNone.)

Something like that?

[sugoi1, 2013/09/25 21:15:27]

Done.

+      */
+    static SkFlattenable::Type GetType() {
+        return SkFlattenable::kSkFlattenable;
+    }
+
+    /** Returns the class id of an object
+      */
+    virtual SkFlattenable::Type getFlattenableType() = 0;
+
+    /** Returns whether the current object is of the given type
+      */
+    static bool IsA(SkFlattenable::Type type) {
+        return (GetType() == type);
+    }
+
+    /** Returns the factory function required to create the given type
+      */
+    static Factory TypeToFactory(Type type);
+
+    /** Checks if typeA is a typeB
+      * For example, if typeA is kSkImageFilter and typeB is kSkFlattenable, this would return true
+      * Inversely, if typeA is kSkFlattenable and typeB is kSkImageFilter, this would return false
+      */
+    static bool TypeIsA(SkFlattenable::Type typeA, SkFlattenable::Type typeB);

[mtklein, 2013/09/24 22:52:18]

Does this need to be public?

[sugoi1, 2013/09/25 21:15:27]

It needs to be accessible inside SkValidatingReadBuffer::readFlattenable(), so
most likely this needs to be public.

+
     class Registrar {
     public:
         Registrar(const char name[], Factory factory) {