Prototype code that turns any/every flattenable into JSON

include/core/SkWriteBuffer.h

Index: include/core/SkWriteBuffer.h
diff --git a/include/core/SkWriteBuffer.h b/include/core/SkWriteBuffer.h
index 07d6fd1175d7fc193fefcee6f485594781e82b68..bf9c455f6c894f4117f503def1c9b2b1933dd5d2 100644
--- a/include/core/SkWriteBuffer.h
+++ b/include/core/SkWriteBuffer.h
@@ -26,15 +26,56 @@ class SkRefCntSet;
 

[mtklein, 2016/05/02 13:55:15]

We might try moving this whole file to include/private?

 class SkWriteBuffer {
 public:
+    SkWriteBuffer() {}
+    virtual ~SkWriteBuffer() {}
+
+    virtual bool isCrossProcess() const = 0;

[reed1, 2016/05/02 12:44:39]

Can/should we consider making this non-virtual, and always storing a flags field
in the base-class?

[mtklein, 2016/05/02 13:55:15]

If it's at all possible I'd prefer an interface here to an abstract base class.

[reed1, 2016/05/02 14:04:42]

What are the advantages? I was motivated to just simplify cases where subclasses
would replicate the same logic (and reduce the shear number of virtuals). Not
critical, seems like a small plus.

+
+    virtual void writeByteArray(const void* data, size_t size) = 0;
+    virtual void writeDataAsByteArray(SkData* data) = 0;

[reed1, 2016/05/02 12:44:39]

does the "SkData" version need to be virtual?

[mtklein, 2016/05/02 13:55:15]

Making these virtual gives us finer type information in the debugger.

E.g. this shows up as "xx_data": "..." rather than "xx_byteArray": "...".

I don't think this is particularly important... if "xx_byteArray" is unclear, 
the best answer is to thread through the full field name.

[reed1, 2016/05/02 14:04:42]

Yea, I thought about that too, but thinking further ahead, I think that
distinction seems more like an impl detail of the caller, and not conveying
(much) information to the reader (the reader object/client, not the code
reviewer).

As another example, we could tag the buffer that a block came from a stream and
not from an array, but to the reader object, I think they are the same.

[Brian Osman, 2016/05/02 14:49:12]

As discussed - I had split it off to capture the entry-point used. There's only
one user of read/writeData, though, so it really comes down to the pure
interface vs. abstract base class, I think. An earlier version of my change has
some shared code in the base-class, and I ended up moving towards an
interface... I don't have a real strong opinion either way, except that we be
consistent (share where it makes sense, or not at all).

[mtklein, 2016/05/02 15:04:43]

My preference for an interface here is mostly because I'm tired of working with
class hierarchies where there is a complex relationship between the base class
and sub class, like SkCanvas and or SkReadBuffer.  It's hard to write a correct
subclass of those types.  Usually these things only get complicated when the
base class maintains state itself.

I don't have any objection to adding (really, keeping) stateless non-virtual
aliases to reduce the surface of the interface, e.g.

void writeDataAsByteArray(SkData* data) {
   this->writeByteArray(data->data(), data->size());
}

+    virtual void writeBool(bool value) = 0;;

[mtklein, 2016/05/02 13:55:14]

Stray ;.

+    virtual void writeScalar(SkScalar value) = 0;
+    virtual void writeScalarArray(const SkScalar* value, uint32_t count) = 0;
+    virtual void writeInt(int32_t value) = 0;
+    virtual void writeIntArray(const int32_t* value, uint32_t count) = 0;
+    virtual void writeUInt(uint32_t value) = 0;
+    virtual void write32(int32_t value) = 0;

[reed1, 2016/05/02 12:44:39]

How is this different from writeInt? Lets either simplify or document why we
have both.

[Brian Osman, 2016/05/02 14:49:12]

They appear to be totally interchangeable in their use-cases. The underlying
writer also has write32 (which is used at this level for all 32-bit integral
types), but also has writeInt, etc... We can definitely narrow down, though it
may be better to do that in a follow-up change, because of how much calling code
will be touched.

[mtklein, 2016/05/02 15:04:43]

Seems like another fine candidate to alias on the interface, then later remove
if we still care.

+    virtual void writeString(const char* value) = 0;
+    virtual void writeEncodedString(const void* value, size_t byteLength,

[reed1, 2016/05/02 12:44:39]

Do we use writeEncodedString?

[mtklein, 2016/05/02 13:55:15]

No.

+                                    SkPaint::TextEncoding encoding) = 0;
+    virtual void writeFunctionPtr(void* ptr) = 0;

[reed1, 2016/05/02 12:44:39]

do we still need writeFunctionPtr?

[mtklein, 2016/05/02 13:55:14]

No.  Its one use, in tests/ImageFilterTest.cpp, is both incorrect and
unnecessary.

+
+    virtual void writeFlattenable(const SkFlattenable* flattenable) = 0;
+    virtual void writeColor(const SkColor& color) = 0;

[reed1, 2016/05/02 12:44:39]

take param by value?

[Brian Osman, 2016/05/02 14:49:12]

Sure.

[mtklein, 2016/05/02 15:04:43]

I suspect points and rects are also faster to pass by value, fwiw.

+    virtual void writeColorArray(const SkColor* color, uint32_t count) = 0;
+    virtual void writePoint(const SkPoint& point) = 0;
+    virtual void writePointArray(const SkPoint* point, uint32_t count) = 0;
+    virtual void writeMatrix(const SkMatrix& matrix) = 0;
+    virtual void writeIRect(const SkIRect& rect) = 0;
+    virtual void writeRect(const SkRect& rect) = 0;
+    virtual void writeRegion(const SkRegion& region) = 0;
+    virtual void writePath(const SkPath& path) = 0;
+    virtual size_t writeStream(SkStream* stream, size_t length) = 0;

[reed1, 2016/05/02 12:44:39]

doe writeStream need to be virtual? Doesn't it just devolve into writeByteArray?

[mtklein, 2016/05/02 13:55:15]

Yes; no.  If we devolve into writeByteArray(), that requires that we copy the
stream to a temporary contiguous buffer.  writeStream() avoids that extra copy.

[reed1, 2016/05/02 14:04:42]

Good point about efficiency. I wonder if that could be inverted, and we just ask
each stream subclass to efficiently write themselves to the buffer? Just
thinking out loud (and thinking about sharing common code/logic).

[mtklein, 2016/05/02 15:04:42]

We've got more streams than write buffers, so I think we'd end up with more,
more complex code that way.
Given that we pass a 'length' parameter through here, the method we've got is
probably the one best way
for all streams: allocate length bytes, then read() that many from the stream.

+    virtual void writeBitmap(const SkBitmap& bitmap) = 0;

[reed1, 2016/05/02 12:44:39]

Does writeBitmap need to be virtual? i.e. does it produce something different
than what writeImage produces?

[mtklein, 2016/05/02 13:55:15]

Yes, today writeBitmap() serializes some formats as raw pixels, while
writeImage() skips them.

[reed1, 2016/05/02 14:04:42]

Yea, but that distinction seems more historical than important (if it is
important, I would like to understand more). I'm fine if we leave this as is for
now, and investigate the differences in a separate thread/CL.

[mtklein, 2016/05/02 15:04:42]

Agreed.  Generally I want to always do
SkImage::MakeFromBitmap(bitmap)->encode(), and similar.  Not very interested in
thinking of SkBitmaps as special.

+    virtual void writeImage(const SkImage*) = 0;
+    virtual void writeTypeface(SkTypeface* typeface) = 0;
+    virtual void writePaint(const SkPaint& paint) = 0;
+};
+
+/**
+ * Concrete implementation that serializes to a flat binary blob.
+ */
+class SkBinaryWriteBuffer final : public SkWriteBuffer {
+public:
     enum Flags {
-        kCrossProcess_Flag  = 1 << 0,
+        kCrossProcess_Flag = 1 << 0,
     };
 
-    SkWriteBuffer(uint32_t flags = 0);
-    SkWriteBuffer(void* initialStorage, size_t storageSize, uint32_t flags = 0);
-    ~SkWriteBuffer();
+    SkBinaryWriteBuffer(uint32_t flags = 0);
+    SkBinaryWriteBuffer(void* initialStorage, size_t storageSize, uint32_t flags = 0);
+    ~SkBinaryWriteBuffer();
 
-    bool isCrossProcess() const {
+    bool isCrossProcess() const override {
         return SkToBool(fFlags & kCrossProcess_Flag);
     }
 
@@ -45,41 +86,40 @@ public:
 
     size_t bytesWritten() const { return fWriter.bytesWritten(); }
 
-    void writeByteArray(const void* data, size_t size);
-    void writeDataAsByteArray(SkData* data) { this->writeByteArray(data->data(), data->size()); }
-    void writeBool(bool value);
-    void writeScalar(SkScalar value);
-    void writeScalarArray(const SkScalar* value, uint32_t count);
-    void writeInt(int32_t value);
-    void writeIntArray(const int32_t* value, uint32_t count);
-    void writeUInt(uint32_t value);
-    void write32(int32_t value);
-    void writeString(const char* value);
-    void writeEncodedString(const void* value, size_t byteLength, SkPaint::TextEncoding encoding);
-    void writeFunctionPtr(void* ptr) { fWriter.writePtr(ptr); }
-
-    void writeFlattenable(const SkFlattenable* flattenable);
-    void writeColor(const SkColor& color);
-    void writeColorArray(const SkColor* color, uint32_t count);
-    void writePoint(const SkPoint& point);
-    void writePointArray(const SkPoint* point, uint32_t count);
-    void writeMatrix(const SkMatrix& matrix);
-    void writeIRect(const SkIRect& rect);
-    void writeRect(const SkRect& rect);
-    void writeRegion(const SkRegion& region);
-    void writePath(const SkPath& path);
-    size_t writeStream(SkStream* stream, size_t length);
-    void writeBitmap(const SkBitmap& bitmap);
-    void writeImage(const SkImage*);
-    void writeTypeface(SkTypeface* typeface);
-    void writePaint(const SkPaint& paint) { paint.flatten(*this); }
+    void writeByteArray(const void* data, size_t size) override;
+    void writeDataAsByteArray(SkData* data) override;
+    void writeBool(bool value) override;
+    void writeScalar(SkScalar value) override;
+    void writeScalarArray(const SkScalar* value, uint32_t count) override;
+    void writeInt(int32_t value) override;
+    void writeIntArray(const int32_t* value, uint32_t count) override;
+    void writeUInt(uint32_t value) override;
+    void write32(int32_t value) override;
+    void writeString(const char* value) override;
+    void writeEncodedString(const void* value, size_t byteLength,
+                            SkPaint::TextEncoding encoding) override;
+    void writeFunctionPtr(void* ptr) override;
+
+    void writeFlattenable(const SkFlattenable* flattenable) override;
+    void writeColor(const SkColor& color) override;
+    void writeColorArray(const SkColor* color, uint32_t count) override;
+    void writePoint(const SkPoint& point) override;
+    void writePointArray(const SkPoint* point, uint32_t count) override;
+    void writeMatrix(const SkMatrix& matrix) override;
+    void writeIRect(const SkIRect& rect) override;
+    void writeRect(const SkRect& rect) override;
+    void writeRegion(const SkRegion& region) override;
+    void writePath(const SkPath& path) override;
+    size_t writeStream(SkStream* stream, size_t length) override;
+    void writeBitmap(const SkBitmap& bitmap) override;
+    void writeImage(const SkImage*) override;
+    void writeTypeface(SkTypeface* typeface) override;
+    void writePaint(const SkPaint& paint) override;
 
     bool writeToStream(SkWStream*);
     void writeToMemory(void* dst) { fWriter.flatten(dst); }
 
     SkFactorySet* setFactoryRecorder(SkFactorySet*);
-
-    SkRefCntSet* getTypefaceRecorder() const { return fTFSet; }
     SkRefCntSet* setTypefaceRecorder(SkRefCntSet*);
 
     /**