Update SkFlattenable buffers to be more modular.

Update SkFlattenable buffers to be more modular.

This CL is an effort to stage the conversion to named
parameters for all SkFlattenable commands. This particular
stage only does the following two things...

1. Move flattenable buffers from SkFlattenable.h into
   their own header.
2. Update and Add new read write methods for better clarity
   and convenience.

BUG=

Committed: https://code.google.com/p/skia/source/detail?r=4980

[DerekS, 2012-08-01 20:00:01]

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenable.h
File include/core/SkFlattenable.h (right):

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenable.h#new...
include/core/SkFlattenable.h:15: class SkFlattenableMapping;
remove this

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffers.h
File include/core/SkFlattenableBuffers.h (right):

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffer...
include/core/SkFlattenableBuffers.h:14: #include "SkPaint.h"
clean this up

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffer...
include/core/SkFlattenableBuffers.h:22: class SkIRect;
struct not class

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffer...
include/core/SkFlattenableBuffers.h:29: class SkRect;
struct not class

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffer...
include/core/SkFlattenableBuffers.h:37: #if SK_ALLOW_STATIC_GLOBAL_INITIALIZERS
remove this

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffer...
include/core/SkFlattenableBuffers.h:144: virtual uint32_t readBinary(void*
value) = 0;
readByteArray

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffer...
include/core/SkFlattenableBuffers.h:150: // helpers to get info about arrays and
binary data
explain that this only peeks into data structure

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffer...
include/core/SkFlattenableBuffers.h:152: virtual uint32_t getBinaryLength() = 0;
remove me

[Leon, 2012-08-01 21:08:15]

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffers.h
File include/core/SkFlattenableBuffers.h (right):

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffer...
include/core/SkFlattenableBuffers.h:13: #include "SkPoint.h"
Minor: not alphabetical

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffer...
include/core/SkFlattenableBuffers.h:152: virtual uint32_t getBinaryLength() = 0;
It seems non-obvious that these peek and readBinary (and read*array) actually
consume the int. Maybe that's okay though.

http://codereview.appspot.com/6448095/diff/1/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

http://codereview.appspot.com/6448095/diff/1/include/core/SkImageFilter.h#new...
include/core/SkImageFilter.h:17: struct SkIPoint;
Ideally this should be alphabetical.

http://codereview.appspot.com/6448095/diff/1/src/core/SkBitmapProcShader.cpp
File src/core/SkBitmapProcShader.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/core/SkBitmapProcShader.cpp#...
src/core/SkBitmapProcShader.cpp:38: fState.fTileModeX = buffer.read32();
Why is this not readUInt?

http://codereview.appspot.com/6448095/diff/1/src/core/SkBitmapProcShader.cpp#...
src/core/SkBitmapProcShader.cpp:75: buffer.write32(fState.fTileModeX);
Why not writeUInt?

http://codereview.appspot.com/6448095/diff/1/src/core/SkData.cpp
File src/core/SkData.cpp (left):

http://codereview.appspot.com/6448095/diff/1/src/core/SkData.cpp#oldcode133
src/core/SkData.cpp:133: if (0 == fSize) {
Is the check for a zero size no longer necessary?

http://codereview.appspot.com/6448095/diff/1/src/core/SkData.cpp
File src/core/SkData.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/core/SkData.cpp#newcode243
src/core/SkData.cpp:243: buffer.writeInt(fCount);
Why did this change? I don't understand the between writeInt and write32 (and I
acknowledge both already existed). The comment says "printf in hex"?

http://codereview.appspot.com/6448095/diff/1/src/core/SkOrderedWriteBuffer.cpp
File src/core/SkOrderedWriteBuffer.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/core/SkOrderedWriteBuffer.cp...
src/core/SkOrderedWriteBuffer.cpp:12:
SkOrderedWriteBuffer::SkOrderedWriteBuffer(size_t minSize) :
For initializer lists we typically put the colon or comma at the start of the
line.

http://codereview.appspot.com/6448095/diff/1/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/core/SkPaint.cpp#newcode1931
src/core/SkPaint.cpp:1931: uint32_t* ptr =
buffer.getOrderedBinaryBuffer()->reserve(kPODPaintSize);
What if getOrderedBinaryBuffer returns NULL?

http://codereview.appspot.com/6448095/diff/1/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/core/SkPicturePlayback.cpp#n...
src/core/SkPicturePlayback.cpp:272:
buffer.getWriter32()->writeMatrix((*fMatrices)[i]);
Why not call

buffer.writeMatrix()

instead?

http://codereview.appspot.com/6448095/diff/1/src/core/SkPicturePlayback.cpp#n...
src/core/SkPicturePlayback.cpp:440:
buffer.getReader32()->readMatrix(&fMatrices->writableAt(i));
buffer.readMatrix()?

http://codereview.appspot.com/6448095/diff/1/src/effects/SkColorMatrixFilter.cpp
File src/effects/SkColorMatrixFilter.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/effects/SkColorMatrixFilter....
src/effects/SkColorMatrixFilter.cpp:304: buffer.writeScalarArray(fMatrix.fMat,
20);
Doesn't necessarily need to be in this change list, but why aren't we using a
constant for 20?

http://codereview.appspot.com/6448095/diff/1/src/effects/SkColorMatrixFilter....
src/effects/SkColorMatrixFilter.cpp:309: // TODO: buffer.readScalars(array, N)?
TODO can go away.

http://codereview.appspot.com/6448095/diff/1/src/effects/SkLayerRasterizer.cpp
File src/effects/SkLayerRasterizer.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/effects/SkLayerRasterizer.cp...
src/effects/SkLayerRasterizer.cpp:148: #if 0
Why do we still need this stuff?

http://codereview.appspot.com/6448095/diff/1/src/images/SkImageRef.cpp
File src/images/SkImageRef.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/images/SkImageRef.cpp#newcod...
src/images/SkImageRef.cpp:181: fSampleSize = buffer.readInt();
Why not readUInt?

http://codereview.appspot.com/6448095/diff/1/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/pipe/SkGPipeRead.cpp#newcode147
src/pipe/SkGPipeRead.cpp:147: SkOrderedReadBuffer*  fReader;
Should align with the others

http://codereview.appspot.com/6448095/diff/1/src/pipe/SkGPipeRead.cpp#newcode666
src/pipe/SkGPipeRead.cpp:666: uint32_t op32 = reader.getReader32()->readU32();
Why not reader.readUInt()?

http://codereview.appspot.com/6448095/diff/1/src/ports/SkImageRef_ashmem.cpp
File src/ports/SkImageRef_ashmem.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/ports/SkImageRef_ashmem.cpp#...
src/ports/SkImageRef_ashmem.cpp:231: sk_free(uri);
Should this be inside the if block?

[DerekS, 2012-08-02 14:03:28]

I'll follow this up with a new patch that addresses the comments from both
myself and scroggo.

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffers.h
File include/core/SkFlattenableBuffers.h (right):

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffer...
include/core/SkFlattenableBuffers.h:13: #include "SkPoint.h"
On 2012/08/01 21:08:15, scroggo-work wrote:
> Minor: not alphabetical

Done.

http://codereview.appspot.com/6448095/diff/1/include/core/SkFlattenableBuffer...
include/core/SkFlattenableBuffers.h:152: virtual uint32_t getBinaryLength() = 0;
On 2012/08/01 21:08:15, scroggo-work wrote:
> It seems non-obvious that these peek and readBinary (and read*array) actually
> consume the int. Maybe that's okay though.

I updated this comment to make it clear it doesn't change the buffer state.

http://codereview.appspot.com/6448095/diff/1/include/core/SkImageFilter.h
File include/core/SkImageFilter.h (right):

http://codereview.appspot.com/6448095/diff/1/include/core/SkImageFilter.h#new...
include/core/SkImageFilter.h:17: struct SkIPoint;
On 2012/08/01 21:08:15, scroggo-work wrote:
> Ideally this should be alphabetical.

Done.

http://codereview.appspot.com/6448095/diff/1/src/core/SkBitmapProcShader.cpp
File src/core/SkBitmapProcShader.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/core/SkBitmapProcShader.cpp#...
src/core/SkBitmapProcShader.cpp:38: fState.fTileModeX = buffer.read32();
On 2012/08/01 21:08:15, scroggo-work wrote:
> Why is this not readUInt?

Done.

http://codereview.appspot.com/6448095/diff/1/src/core/SkBitmapProcShader.cpp#...
src/core/SkBitmapProcShader.cpp:75: buffer.write32(fState.fTileModeX);
On 2012/08/01 21:08:15, scroggo-work wrote:
> Why not writeUInt?

Done.

http://codereview.appspot.com/6448095/diff/1/src/core/SkData.cpp
File src/core/SkData.cpp (left):

http://codereview.appspot.com/6448095/diff/1/src/core/SkData.cpp#oldcode133
src/core/SkData.cpp:133: if (0 == fSize) {
On 2012/08/01 21:08:15, scroggo-work wrote:
> Is the check for a zero size no longer necessary?

It was never really necessary as malloc(0) returns NULL, but I can add it back
if it makes things more clear.

http://codereview.appspot.com/6448095/diff/1/src/core/SkData.cpp
File src/core/SkData.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/core/SkData.cpp#newcode243
src/core/SkData.cpp:243: buffer.writeInt(fCount);
On 2012/08/01 21:08:15, scroggo-work wrote:
> Why did this change? I don't understand the between writeInt and write32 (and
I
> acknowledge both already existed). The comment says "printf in hex"? 

In the ordered binary case they both end up doing the exact same thing. However,
for things like the JSON buffer if you call write 32 we will print/store the hex
value instead of the integer.

http://codereview.appspot.com/6448095/diff/1/src/core/SkOrderedWriteBuffer.cpp
File src/core/SkOrderedWriteBuffer.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/core/SkOrderedWriteBuffer.cp...
src/core/SkOrderedWriteBuffer.cpp:12:
SkOrderedWriteBuffer::SkOrderedWriteBuffer(size_t minSize) :
On 2012/08/01 21:08:15, scroggo-work wrote:
> For initializer lists we typically put the colon or comma at the start of the
> line.

Done.

http://codereview.appspot.com/6448095/diff/1/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/core/SkPaint.cpp#newcode1931
src/core/SkPaint.cpp:1931: uint32_t* ptr =
buffer.getOrderedBinaryBuffer()->reserve(kPODPaintSize);
On 2012/08/01 21:08:15, scroggo-work wrote:
> What if getOrderedBinaryBuffer returns NULL?

It isn't possible yet so I removed the code when I trying to shrink the CL, but
it doesn't hurt to have it and it makes it clear that this is making assumptions
about the underlying data.

http://codereview.appspot.com/6448095/diff/1/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/core/SkPicturePlayback.cpp#n...
src/core/SkPicturePlayback.cpp:272:
buffer.getWriter32()->writeMatrix((*fMatrices)[i]);
On 2012/08/01 21:08:15, scroggo-work wrote:
> Why not call
> 
> buffer.writeMatrix()
> 
> instead?

Done.

http://codereview.appspot.com/6448095/diff/1/src/core/SkPicturePlayback.cpp#n...
src/core/SkPicturePlayback.cpp:440:
buffer.getReader32()->readMatrix(&fMatrices->writableAt(i));
On 2012/08/01 21:08:15, scroggo-work wrote:
> buffer.readMatrix()?

Done.

http://codereview.appspot.com/6448095/diff/1/src/effects/SkColorMatrixFilter.cpp
File src/effects/SkColorMatrixFilter.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/effects/SkColorMatrixFilter....
src/effects/SkColorMatrixFilter.cpp:304: buffer.writeScalarArray(fMatrix.fMat,
20);
On 2012/08/01 21:08:15, scroggo-work wrote:
> Doesn't necessarily need to be in this change list, but why aren't we using a
> constant for 20?

I'll change it in a follow up CL

http://codereview.appspot.com/6448095/diff/1/src/effects/SkColorMatrixFilter....
src/effects/SkColorMatrixFilter.cpp:309: // TODO: buffer.readScalars(array, N)?
On 2012/08/01 21:08:15, scroggo-work wrote:
> TODO can go away.

Done.

http://codereview.appspot.com/6448095/diff/1/src/effects/SkLayerRasterizer.cpp
File src/effects/SkLayerRasterizer.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/effects/SkLayerRasterizer.cp...
src/effects/SkLayerRasterizer.cpp:148: #if 0
On 2012/08/01 21:08:15, scroggo-work wrote:
> Why do we still need this stuff?

This was a leftover from my refactoring. I can remove it.

http://codereview.appspot.com/6448095/diff/1/src/effects/SkRectShape.cpp
File src/effects/SkRectShape.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/effects/SkRectShape.cpp#newc...
src/effects/SkRectShape.cpp:86: fPaint.unflatten(buffer);
readPaint.

http://codereview.appspot.com/6448095/diff/1/src/images/SkImageRef.cpp
File src/images/SkImageRef.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/images/SkImageRef.cpp#newcod...
src/images/SkImageRef.cpp:181: fSampleSize = buffer.readInt();
On 2012/08/01 21:08:15, scroggo-work wrote:
> Why not readUInt?

the fSampleSize variable is an int

http://codereview.appspot.com/6448095/diff/1/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/pipe/SkGPipeRead.cpp#newcode147
src/pipe/SkGPipeRead.cpp:147: SkOrderedReadBuffer*  fReader;
On 2012/08/01 21:08:15, scroggo-work wrote:
> Should align with the others

Done.

http://codereview.appspot.com/6448095/diff/1/src/pipe/SkGPipeRead.cpp#newcode666
src/pipe/SkGPipeRead.cpp:666: uint32_t op32 = reader.getReader32()->readU32();
On 2012/08/01 21:08:15, scroggo-work wrote:
> Why not reader.readUInt()?

Done. this was leftover from an earlier refactor.

http://codereview.appspot.com/6448095/diff/1/src/ports/SkImageRef_ashmem.cpp
File src/ports/SkImageRef_ashmem.cpp (right):

http://codereview.appspot.com/6448095/diff/1/src/ports/SkImageRef_ashmem.cpp#...
src/ports/SkImageRef_ashmem.cpp:231: sk_free(uri);
On 2012/08/01 21:08:15, scroggo-work wrote:
> Should this be inside the if block?

Done.

[Leon, 2012-08-02 15:29:05]

http://codereview.appspot.com/6448095/diff/1/src/core/SkData.cpp
File src/core/SkData.cpp (left):

http://codereview.appspot.com/6448095/diff/1/src/core/SkData.cpp#oldcode133
src/core/SkData.cpp:133: if (0 == fSize) {
On 2012/08/02 14:03:28, DerekS wrote:
> On 2012/08/01 21:08:15, scroggo-work wrote:
> > Is the check for a zero size no longer necessary?
> 
> It was never really necessary as malloc(0) returns NULL, but I can add it back
> if it makes things more clear.

I do not know that it makes anything more clear. It does change things slightly,
though I cannot say which is better (if statement vs calling a couple
functions). It also means we now always set fReleaseProc to sk_free_releaseproc,
rather than setting it to NULL (which also should be fine).