Follow up to serialization validation code

src/pipe/SkGPipeRead.cpp

 
 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #include "SkBitmapHeap.h"
 #include "SkCanvas.h"
 #include "SkPaint.h"
 #include "SkGPipe.h"
 #include "SkGPipePriv.h"
 #include "SkReader32.h"
 #include "SkStream.h"
 
+#include "SkAnnotation.h"
 #include "SkColorFilter.h"
 #include "SkDrawLooper.h"
+#include "SkImageFilter.h"
 #include "SkMaskFilter.h"
 #include "SkOrderedReadBuffer.h"
 #include "SkPathEffect.h"
 #include "SkRasterizer.h"
 #include "SkRRect.h"
 #include "SkShader.h"
 #include "SkTypeface.h"
 #include "SkXfermode.h"
 
 static void set_paintflat(SkPaint* paint, SkFlattenable* obj, unsigned paintFlat) {
@@ skipping 24 matching lines @@
             paint->setXfermode((SkXfermode*)obj);
             break;
         case kAnnotation_PaintFlat:
             paint->setAnnotation((SkAnnotation*)obj);
             break;
         default:
             SkDEBUGFAIL("never gets here");
     }
 }
 
+static SkFlattenable* read_paintflat(SkFlattenableReadBuffer* reader, unsigned paintFlat) {

[reed1, 2013/09/13 15:42:56]

Seems like a lot of code to just check that paintFlat is < kCount_PaintFlags.
Why not just

if (paintFlat >= kCount_PaintFlags) {
    return NULL;
} else {
    return reader->readFlattenableT<SkFlattenable>();
}

I understand the future-idea-desire to have a runtime check, but this switch
statement accomplishes none of that afaik.

[sugoi1, 2013/09/13 16:53:44]

I'm not sure I understand. Each case uses a different class that can be used for
type checking. That's not equivalent to treating every case as a generic
SkFlattenable. Right now, if someone made a mistake and swapped 2 objects,
nothing would detect it.

+    SkASSERT(paintFlat < kCount_PaintFlats);
+    switch (paintFlat) {
+        case kColorFilter_PaintFlat:
+            return reader->readFlattenableT<SkColorFilter>();
+        case kDrawLooper_PaintFlat:
+            return reader->readFlattenableT<SkDrawLooper>();
+        case kMaskFilter_PaintFlat:
+            return reader->readFlattenableT<SkMaskFilter>();
+        case kPathEffect_PaintFlat:
+            return reader->readFlattenableT<SkPathEffect>();
+        case kRasterizer_PaintFlat:
+            return reader->readFlattenableT<SkRasterizer>();
+        case kShader_PaintFlat:
+            return reader->readFlattenableT<SkShader>();
+        case kImageFilter_PaintFlat:
+            return reader->readFlattenableT<SkImageFilter>();
+        case kXfermode_PaintFlat:
+            return reader->readFlattenableT<SkXfermode>();
+        case kAnnotation_PaintFlat:
+            return reader->readFlattenableT<SkAnnotation>();
+        default:
+            SkDEBUGFAIL("never gets here");
+    }
+    return NULL;
+}
+
 template <typename T> class SkRefCntTDArray : public SkTDArray<T> {
 public:
     ~SkRefCntTDArray() { this->unrefAll(); }
 };
 
 class SkGPipeState : public SkBitmapHeapReader {
 public:
     SkGPipeState();
     ~SkGPipeState();
 
@@ skipping 26 matching lines @@
 
     SkFlattenable* getFlat(unsigned index) const {
         if (0 == index) {
             return NULL;
         }
         return fFlatArray[index - 1];
     }
 
     void defFlattenable(PaintFlats pf, int index) {
         index--;
-        SkFlattenable* obj = fReader->readFlattenable();
+        SkFlattenable* obj = read_paintflat(fReader, pf);
         if (fFlatArray.count() == index) {
             *fFlatArray.append() = obj;
         } else {
             SkSafeUnref(fFlatArray[index]);
             fFlatArray[index] = obj;
         }
     }
 
     void defFactory(const char* name) {
         SkFlattenable::Factory factory = SkFlattenable::NameToFactory(name);
@@ skipping 732 matching lines @@
                 status = kReadAtom_Status;
                 break;
             }
     }
 
     if (bytesRead) {
         *bytesRead = reader.offset();
     }
     return status;
 }