replace detach/getStream apis on dynamicwstream with SkData

[reed1, 2011-06-24 14:51:33]

start with the API changes in SkStream.h...

basically detach() and getStream() were fragile apis, and are replaced by using
SkData.

[bungeman, 2011-06-24 17:22:07]

LGTM

Notes from conversation:
*SVG should go to experimental
*Most places fixed up here should probably just use SkData directly and not
depend on a memory backed stream. Only SkStream and SkWStream should be passed
to functions. If one is passing in a SkDynamicMemoryWStream instead of just
SkStream, you probably want SkData instead.
SkMemoryStream/SkDynamicMemoryWStream should probably only ever be used for
SkData->SkStream/SkData->SkWStream conversion (as in SampleApp).

[Steve VanDeBogart, 2011-06-24 18:48:13]

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

http://codereview.appspot.com/4657046/diff/1/include/core/SkData.h#newcode78
include/core/SkData.h:78: static SkData* NewWithMalloc(const void* data, size_t
length);
This name is confusing.  NewFromMalloc?  NewWithFree?

http://codereview.appspot.com/4657046/diff/1/include/core/SkData.h#newcode107
include/core/SkData.h:107: class SkAutoDataUnref : SkNoncopyable {
This seems to be a symptom of the SkRefPtr + new requires an unref problem.  I
really would like to fix that, but I don't have a great idea.  SkPassRefPtr
seems like a reasonable idea, but not great.

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

http://codereview.appspot.com/4657046/diff/1/src/core/SkStream.cpp#newcode269
src/core/SkStream.cpp:269: if (copyData) {
This can just call setMemory.

http://codereview.appspot.com/4657046/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4657046/diff/1/src/pdf/SkPDFDevice.cpp#newcode1085
src/pdf/SkPDFDevice.cpp:1085: *  Can this return SkData instead of SkStream?
Yes, the result is immutable and the caller can wrap it in a stream if they want
to do something with it.

http://codereview.appspot.com/4657046/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4657046/diff/1/src/pdf/SkPDFFont.cpp#newcode627
src/pdf/SkPDFFont.cpp:627: // do we need pdfCmap->unref() here?
Nope, we pass the reference from new into fResources on the next line.

http://codereview.appspot.com/4657046/diff/1/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

http://codereview.appspot.com/4657046/diff/1/tests/PDFPrimitivesTest.cpp#newc...
tests/PDFPrimitivesTest.cpp:27: static bool
stream_equals(SkDynamicMemoryWStream* stream, size_t offset,
Can stream be a const ref?

[reed1, 2011-06-24 19:01:48]

uploaded new version

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

http://codereview.appspot.com/4657046/diff/1/include/core/SkData.h#newcode78
include/core/SkData.h:78: static SkData* NewWithMalloc(const void* data, size_t
length);
NewFromMalloc it is

On 2011/06/24 18:48:13, Steve VanDeBogart wrote:
> This name is confusing.  NewFromMalloc?  NewWithFree?

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

http://codereview.appspot.com/4657046/diff/1/src/core/SkStream.cpp#newcode269
src/core/SkStream.cpp:269: if (copyData) {
setMemory() calls fData->unref()

On 2011/06/24 18:48:13, Steve VanDeBogart wrote:
> This can just call setMemory.

http://codereview.appspot.com/4657046/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4657046/diff/1/src/pdf/SkPDFFont.cpp#newcode627
src/pdf/SkPDFFont.cpp:627: // do we need pdfCmap->unref() here?
removed comment

On 2011/06/24 18:48:13, Steve VanDeBogart wrote:
> Nope, we pass the reference from new into fResources on the next line.

[Steve VanDeBogart, 2011-06-24 19:06:36]

Thanks

LGTM with comment.

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

http://codereview.appspot.com/4657046/diff/1/src/core/SkStream.cpp#newcode269
src/core/SkStream.cpp:269: if (copyData) {
You could make it use SkSafeUnref and init fData to NULL.

On 2011/06/24 19:01:48, reed1 wrote:
> setMemory() calls fData->unref()
> 
> On 2011/06/24 18:48:13, Steve VanDeBogart wrote:
> > This can just call setMemory.
>

[reed1, 2011-06-24 19:09:57]

On 2011/06/24 19:06:36, Steve VanDeBogart wrote:
> Thanks
> 
> LGTM with comment.
> 
> http://codereview.appspot.com/4657046/diff/1/src/core/SkStream.cpp
> File src/core/SkStream.cpp (right):
> 
> http://codereview.appspot.com/4657046/diff/1/src/core/SkStream.cpp#newcode269
> src/core/SkStream.cpp:269: if (copyData) {
> You could make it use SkSafeUnref and init fData to NULL.

Thought about that, but then I think I'd have to check for null everywhere I use
fData. We can write a private helper method to do the copyData check and build
in a later CL.

> 
> On 2011/06/24 19:01:48, reed1 wrote:
> > setMemory() calls fData->unref()
> > 
> > On 2011/06/24 18:48:13, Steve VanDeBogart wrote:
> > > This can just call setMemory.
> >

[Steve VanDeBogart, 2011-06-24 19:51:05]

Still LGTM.

On 2011/06/24 19:09:57, reed1 wrote:
> On 2011/06/24 19:06:36, Steve VanDeBogart wrote:
> > Thanks
> > 
> > LGTM with comment.
> > 
> > http://codereview.appspot.com/4657046/diff/1/src/core/SkStream.cpp
> > File src/core/SkStream.cpp (right):
> > 
> >
http://codereview.appspot.com/4657046/diff/1/src/core/SkStream.cpp#newcode269
> > src/core/SkStream.cpp:269: if (copyData) {
> > You could make it use SkSafeUnref and init fData to NULL.
> 
> Thought about that, but then I think I'd have to check for null everywhere I
use
> fData. We can write a private helper method to do the copyData check and build
> in a later CL.

I don't think so - All the constructors would set fData.  This one would just
set it by way of setMemory()...

> 
> > 
> > On 2011/06/24 19:01:48, reed1 wrote:
> > > setMemory() calls fData->unref()
> > > 
> > > On 2011/06/24 18:48:13, Steve VanDeBogart wrote:
> > > > This can just call setMemory.
> > >