Refactor read and write buffers.

Refactor read and write buffers.

Eliminates SkFlattenable{Read,Write}Buffer, promoting SkOrdered{Read,Write}Buffer
a step each in the hierarchy.

What used to be this:

SkFlattenableWriteBuffer -> SkOrderedWriteBuffer
SkFlattenableReadBuffer  -> SkOrderedReadBuffer
SkFlattenableReadBuffer  -> SkValidatingReadBuffer

is now

SkWriteBuffer
SkReadBuffer -> SkValidatingReadBuffer

Benefits:
  - code is simpler, names are less wordy
  - the generic SkFlattenableFooBuffer code in SkPaint was incorrect; removed
  - write buffers are completely devirtualized, important for record speed

This refactoring was mostly mechanical.  You aren't going to find anything
interesting in files with less than 10 lines changed.

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=13245

[mtklein_C, 2014-01-29 19:31:13]

Believe it or not, this is all in the name of improving picture recording speed.

[reed1, 2014-01-29 19:39:34]

+scrogg : pipe implications (if any)
+derek : didn't we make the writer virtual for JSON/XML backends?

[mtklein, 2014-01-29 19:47:00]

On 2014/01/29 19:39:34, reed1 wrote:
> +scrogg : pipe implications (if any)
> +derek : didn't we make the writer virtual for JSON/XML backends?

I'll let Derek correct me if I'm wrong, but I think we agree that we did make it
virtual for JSON/XML backends, but it's not actually very helpful toward that
goal.  We'd have to call everything in exactly the same order and with the exact
same arguments that SkOrderedWriteBuffer expects anyway, so it doesn't really
buy us any flexibility.  If we're going to do JSON, XML, or even proto or mojo,
order won't matter, but we will certainly want to encode some of our structs
differently (e.g. enums here, strings there; bitfields here, unfolded there;
defaults implicit here, explicit there; etc.).  we'll have to write against a
separate interface from SkFlattenableWriteBuffer no matter what we do.

[scroggo, 2014-01-29 20:07:19]

The changes to pipe look fine to me.

The overall CL lgtm also.

https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:8: typedef SkReadBuffer
SkFlattenableReadBuffer;
Do we need this typedef? Is there any reason not to replace clients of
SkFlattenableReadBuffer with SkReadBuffer?

https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp...
src/pipe/SkGPipeRead.cpp:219: SkReadBuffer*      fReader;
nit: fReader no longer lines up with other members.

[mtklein, 2014-01-29 20:13:45]

On 2014/01/29 20:07:19, scroggo wrote:
> The changes to pipe look fine to me.
> 
> The overall CL lgtm also.
> 
>
https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
> File include/core/SkFlattenableBuffers.h (right):
> 
>
https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
> include/core/SkFlattenableBuffers.h:8: typedef SkReadBuffer
> SkFlattenableReadBuffer;
> Do we need this typedef? Is there any reason not to replace clients of
> SkFlattenableReadBuffer with SkReadBuffer?
> 
> https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp
> File src/pipe/SkGPipeRead.cpp (right):
> 
>
https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp...
> src/pipe/SkGPipeRead.cpp:219: SkReadBuffer*      fReader;
> nit: fReader no longer lines up with other members.

Derek:
Actually, having a JSON/Other backend would allow us to not keep the same order
as there would be no expectation that a flattenable written with any other
revision of Skia would be able to be deserialized on any other revision. 
However, I do like the idea of having the more persistent format use our public
API instead of being entrenched in our core code.  My only concern is ripping
this out before the other approach has been tested, but I think the risk is
small.

Is there a measurable speed difference going from to a non-virtual flattenable
interface?  If so, I think that would trump any concern I have about removing
the code now.

[mtklein, 2014-01-29 20:29:55]

On 2014/01/29 20:13:45, mtklein wrote:
> On 2014/01/29 20:07:19, scroggo wrote:
> > The changes to pipe look fine to me.
> > 
> > The overall CL lgtm also.
> > 
> >
>
https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
> > File include/core/SkFlattenableBuffers.h (right):
> > 
> >
>
https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
> > include/core/SkFlattenableBuffers.h:8: typedef SkReadBuffer
> > SkFlattenableReadBuffer;
> > Do we need this typedef? Is there any reason not to replace clients of
> > SkFlattenableReadBuffer with SkReadBuffer?
> > 
> >
https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp
> > File src/pipe/SkGPipeRead.cpp (right):
> > 
> >
>
https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp...
> > src/pipe/SkGPipeRead.cpp:219: SkReadBuffer*      fReader;
> > nit: fReader no longer lines up with other members.
> 
> Derek:
> Actually, having a JSON/Other backend would allow us to not keep the same
order
> as there would be no expectation that a flattenable written with any other
> revision of Skia would be able to be deserialized on any other revision. 
> However, I do like the idea of having the more persistent format use our
public
> API instead of being entrenched in our core code.  My only concern is ripping
> this out before the other approach has been tested, but I think the risk is
> small.
> 
> Is there a measurable speed difference going from to a non-virtual flattenable
> interface?  If so, I think that would trump any concern I have about removing
> the code now.

Right, order wouldn't matter in the end.  But the encodings would still be
completely coupled, to the point where we can't change anything in our internal
format because it'd change the JSON version, and our JSON version will be filled
with packed bitfields instead of readable strings because that's what we
writeUInt'd.  There's no way to have a stable format and an unstable format use
the same interface.  If we tried to use this, we'd be locked into having two
different concrete encodings of the same schema forever.

I'd like to rip this out mostly because it gets in the way of improving picture
recording performance, and because it's clear we can't maintain both versions
(non-ordered buffers cannot encode or decode a correct picture today).  Stable
pictures are way secondary in importance, but this change certainly won't hurt. 
Even if we can't use only-public API for a stable picture, we can't use any of
what we're throwing away here.  Stable pictures have to stay decoupled from
flattening for flattening to remain flexible and for stable pictures to remain
stable.

This will bring only a slight performance improvement right now.  We're
decreasing each effect flattening call from N virtual dispatches to N-1 (I think
N=2 or 3), and decreasing typefaces from 1 to 0.  The real benefits will come in
follow up when it's easier to think about the code.

[mtklein, 2014-01-29 20:38:28]

https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:8: typedef SkReadBuffer
SkFlattenableReadBuffer;
On 2014/01/29 20:07:21, scroggo wrote:
> Do we need this typedef? Is there any reason not to replace clients of
> SkFlattenableReadBuffer with SkReadBuffer?

We don't, but Chromium does.  Once I can clean that up, this whole file gets
deleted.

https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp...
src/pipe/SkGPipeRead.cpp:219: SkReadBuffer*      fReader;
On 2014/01/29 20:07:21, scroggo wrote:
> nit: fReader no longer lines up with other members.

Done.

[djsollen, 2014-01-29 20:53:10]

lgtm

[scroggo, 2014-01-30 16:13:33]

On 2014/01/29 20:38:28, mtklein wrote:
>
https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
> File include/core/SkFlattenableBuffers.h (right):
> 
>
https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
> include/core/SkFlattenableBuffers.h:8: typedef SkReadBuffer
> SkFlattenableReadBuffer;
> On 2014/01/29 20:07:21, scroggo wrote:
> > Do we need this typedef? Is there any reason not to replace clients of
> > SkFlattenableReadBuffer with SkReadBuffer?
> 
> We don't, but Chromium does.  Once I can clean that up, this whole file gets
> deleted.

Great. Isn't our typical pattern to add a compile flag to keep the old behavior?
> 
> https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp
> File src/pipe/SkGPipeRead.cpp (right):
> 
>
https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp...
> src/pipe/SkGPipeRead.cpp:219: SkReadBuffer*      fReader;
> On 2014/01/29 20:07:21, scroggo wrote:
> > nit: fReader no longer lines up with other members.
> 
> Done.

[mtklein, 2014-01-30 16:17:07]

On 2014/01/30 16:13:33, scroggo wrote:
> On 2014/01/29 20:38:28, mtklein wrote:
> >
>
https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
> > File include/core/SkFlattenableBuffers.h (right):
> > 
> >
>
https://codereview.chromium.org/134163010/diff/60001/include/core/SkFlattenab...
> > include/core/SkFlattenableBuffers.h:8: typedef SkReadBuffer
> > SkFlattenableReadBuffer;
> > On 2014/01/29 20:07:21, scroggo wrote:
> > > Do we need this typedef? Is there any reason not to replace clients of
> > > SkFlattenableReadBuffer with SkReadBuffer?
> > 
> > We don't, but Chromium does.  Once I can clean that up, this whole file gets
> > deleted.
> 
> Great. Isn't our typical pattern to add a compile flag to keep the old
behavior?
> > 
> >
https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp
> > File src/pipe/SkGPipeRead.cpp (right):
> > 
> >
>
https://codereview.chromium.org/134163010/diff/60001/src/pipe/SkGPipeRead.cpp...
> > src/pipe/SkGPipeRead.cpp:219: SkReadBuffer*      fReader;
> > On 2014/01/29 20:07:21, scroggo wrote:
> > > nit: fReader no longer lines up with other members.
> > 
> > Done.

Yep.  I'm not sure we need an additional compilation guard beyond "#include the
old file to get the old behavior"?

[reed1, 2014-01-30 17:55:45]

lgtm

[commit-bot: I haz the power, 2014-01-30 18:09:18]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/134163010/120001

[commit-bot: I haz the power, 2014-01-30 18:59:21]

Change committed as 13245

[commit-bot: I haz the power, 2014-01-30 19:02:16]

CQ bit was unchecked on CL. Ignoring.