Simplify flattening to just write enough to call the factory

include/core/SkImageFilter.h

 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #ifndef SkImageFilter_DEFINED
 #define SkImageFilter_DEFINED
 
@@ skipping 187 matching lines @@
     static Cache* GetExternalCache();
 
     SK_DEFINE_FLATTENABLE_TYPE(SkImageFilter)
 
 protected:
     class Common {
     public:
         Common() {}
         ~Common();
 
+        /**
+         *  Attempt to unflatten the cropRect and the expected number of input filters.
+         *  If any number of input filters is valid, pass -1.
+         *  If this fails (i.e. corrupt buffer or contents) then return false and common will
+         *  be left uninitialized.
+         *  If this returns true, then inputCount() is the number of found input filters, each
+         *  of which may be NULL or a valid imagefilter.
+         */
         bool unflatten(SkReadBuffer&, int expectedInputs = -1);
 
-        CropRect        cropRect() const { return fCropRect; }
+        const CropRect& cropRect() const { return fCropRect; }
         int             inputCount() const { return fInputs.count(); }
         SkImageFilter** inputs() const { return fInputs.get(); }
         uint32_t        uniqueID() const { return fUniqueID; }
 
+        SkImageFilter*  inputAt(int index) const { return fInputs[index]; }
+
         // If the caller wants a copy of the inputs, call this and it will transfer ownership
         // of the unflattened input filters to the caller. This is just a short-cut for copying
         // the inputs, calling ref() on each, and then waiting for Common's destructor to call
         // unref() on each.
         void detachInputs(SkImageFilter** inputs);
 
     private:
         CropRect fCropRect;
         // most filters accept at most 2 input-filters
         SkAutoSTArray<2, SkImageFilter*> fInputs;
         uint32_t fUniqueID;
 
         void allocInputs(int count);
     };
 
     SkImageFilter(int inputCount, SkImageFilter** inputs, const CropRect* cropRect = NULL);
 
     virtual ~SkImageFilter();
 
     /**
      *  Constructs a new SkImageFilter read from an SkReadBuffer object.
      *
      *  @param inputCount    The exact number of inputs expected for this SkImageFilter object.
      *                       -1 can be used if the filter accepts any number of inputs.
      *  @param rb            SkReadBuffer object from which the SkImageFilter is read.
      */
     explicit SkImageFilter(int inputCount, SkReadBuffer& rb);
 
-    virtual void flatten(SkWriteBuffer& wb) const SK_OVERRIDE;
+    virtual void flatten(SkWriteBuffer&) const SK_OVERRIDE;
 
     /**
      *  This is the virtual which should be overridden by the derived class
      *  to perform image filtering.
      *
      *  src is the original primitive bitmap. If the filter has a connected
      *  input, it should recurse on that input and use that in place of src.
      *
      *  The matrix is the current matrix on the canvas.
      *
@@ skipping 59 matching lines @@
     bool usesSrcInput() const { return fUsesSrcInput; }
 
     typedef SkFlattenable INHERITED;
     int fInputCount;
     SkImageFilter** fInputs;
     bool fUsesSrcInput;
     CropRect fCropRect;
     uint32_t fUniqueID; // Globally unique
 };
 
+/**
+ *  Helper to unflatten the common data, and return NULL if we fail.
+ *
+ *  Declares a local variable 'common', and expects a SkReadBuffer named 'buffer'
+ */
+#define SK_IMAGEFILTER_UNFLATTEN_COMMON(expectedCount)      \

[Stephen White, 2014/08/18 18:17:26]

This macro is too much magic, IMHO: putting a variable unnamed at
the call site in the caller's scope, and an early return that you
don't see at the call site.

At a minimum, I'd like to see the variable name in the macro:

#define SK_IMAGEFILTER_UNFLATTEN_COMMON(expectedCount, var)
  ....

But to be honest, without a macro, this is only 4 lines of code. I 
would be ok with duplicating those 4 lines in each filter to 
improve readability at the call site, and to make the early-
return explicit.

[reed1, 2014/08/18 22:39:16]

A downside of copy/pasting 4 lines everywhere is the chance for a typo, and/or
forgetting to do the early return on failure.
I think the compromise suggestion of passing the var name makes good sense.
Done.

+    Common common;                                          \
+    do {                                                    \
+        if (!common.unflatten(buffer, expectedCount)) {     \
+            return NULL;                                    \
+        }                                                   \
+    } while (0)
+
 #endif