Adding release method to SkWriter32 and using it to not copy the ops data from SkPictureRecord to S…

Adding release method to SkWriter32 and using it to not copy the ops data from SkPictureRecord to SkPicturePlayback

BUG=skia:2125

[iancottrell, 2014-02-07 15:57:18]

Things to note on this cl:

I needed to make SkPicturePlayback take a mutable SkPictureRecord because it
modifies it by stealing the buffer now.
Is release the right name?
I think returning an SkData is the right approach, its safer, it's the type I
want in all the use cases we have so far, and it means you don't get the length
and the pointer separately.

[reed1, 2014-02-07 16:06:43]

https://codereview.chromium.org/155863005/diff/1/include/core/SkWriter32.h
File include/core/SkWriter32.h (right):

https://codereview.chromium.org/155863005/diff/1/include/core/SkWriter32.h#ne...
include/core/SkWriter32.h:238: SkData* release();
Modest vote for a more explicit name :

e.g.

releaseAsData() ?
detachAsData() ?  // seems like we use detach for this idea in other classes,
but maybe we use both

Also we should well-document that this also resets the state of the writer.

[iancottrell, 2014-02-07 17:01:47]

https://codereview.chromium.org/155863005/diff/1/include/core/SkWriter32.h
File include/core/SkWriter32.h (right):

https://codereview.chromium.org/155863005/diff/1/include/core/SkWriter32.h#ne...
include/core/SkWriter32.h:238: SkData* release();
On 2014/02/07 16:06:49, reed1 wrote:
> Modest vote for a more explicit name :
> 
> e.g.
> 
> releaseAsData() ?
> detachAsData() ?  // seems like we use detach for this idea in other classes,
> but maybe we use both
> 
> Also we should well-document that this also resets the state of the writer.  

Done.

[reed1, 2014-02-07 17:08:48]

api lgtm

[tomhudson, 2014-02-07 17:15:57]

lgtm

https://codereview.chromium.org/155863005/diff/60001/src/core/SkWriter32.cpp
File src/core/SkWriter32.cpp (right):

https://codereview.chromium.org/155863005/diff/60001/src/core/SkWriter32.cpp#...
src/core/SkWriter32.cpp:99: fData = NULL;
Nit: the constructor uses 0 for all 4 fields.
I think this usage of NULL is more in line with Skia style - worth fixing the
inconsistency?

[reed1, 2014-02-07 17:50:47]

On 2014/02/07 17:15:57, tomhudson wrote:
> lgtm
> 
> https://codereview.chromium.org/155863005/diff/60001/src/core/SkWriter32.cpp
> File src/core/SkWriter32.cpp (right):
> 
>
https://codereview.chromium.org/155863005/diff/60001/src/core/SkWriter32.cpp#...
> src/core/SkWriter32.cpp:99: fData = NULL;
> Nit: the constructor uses 0 for all 4 fields.
> I think this usage of NULL is more in line with Skia style - worth fixing the
> inconsistency?

+1 for using NULL for pointers

[iancottrell, 2014-02-11 13:28:39]

https://codereview.chromium.org/155863005/diff/60001/src/core/SkWriter32.cpp
File src/core/SkWriter32.cpp (right):

https://codereview.chromium.org/155863005/diff/60001/src/core/SkWriter32.cpp#...
src/core/SkWriter32.cpp:99: fData = NULL;
On 2014/02/07 17:15:58, tomhudson wrote:
> Nit: the constructor uses 0 for all 4 fields.
> I think this usage of NULL is more in line with Skia style - worth fixing the
> inconsistency?

Done.

[mtklein, 2014-02-11 13:31:12]

lgtm

https://codereview.chromium.org/155863005/diff/150001/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/155863005/diff/150001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:87: {
Not sure why we have this block around creating the opData.  Remove if you like?

[iancottrell, 2014-02-11 13:44:06]

https://codereview.chromium.org/155863005/diff/150001/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/155863005/diff/150001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:87: {
On 2014/02/11 13:31:12, mtklein wrote:
> Not sure why we have this block around creating the opData.  Remove if you
like?

Done. I presume it was to scope the old size and buffer variables.

[iancottrell, 2014-02-11 14:30:18]

The CQ bit was checked by iancottrell@google.com

[commit-bot: I haz the power, 2014-02-11 14:30:26]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/iancottrell@google.com/155863005/260001

[commit-bot: I haz the power, 2014-02-11 14:35:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-11 14:35:15]

Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
step(s) BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests,
BuildTools
http://108.170.219.164:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...

[iancottrell, 2014-03-12 11:09:47]

On 2014/02/11 14:35:15, I haz the power (commit-bot) wrote:
> Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
> step(s) BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests,
> BuildTools
>
http://108.170.219.164:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...


This cl has been abandoned, see https://codereview.chromium.org/167113003/ for
the new version.