LGTM Notes from conversation: *SVG should go to experimental *Most places fixed up here should ...
13 years, 6 months ago
(2011-06-24 17:22:07 UTC)
#2
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).
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 ...
13 years, 6 months ago
(2011-06-24 19:06:36 UTC)
#5
On 2011/06/24 19:06:36, Steve VanDeBogart wrote: > Thanks > > LGTM with comment. > > ...
13 years, 6 months ago
(2011-06-24 19:09:57 UTC)
#6
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.
> >
Still LGTM. On 2011/06/24 19:09:57, reed1 wrote: > On 2011/06/24 19:06:36, Steve VanDeBogart wrote: > ...
13 years, 6 months ago
(2011-06-24 19:51:05 UTC)
#8
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.
> > >
Issue 4657046: replace detach/getStream apis on dynamicwstream with SkData
(Closed)
Created 13 years, 6 months ago by reed1
Modified 13 years, 6 months ago
Reviewers: Steve VanDeBogart, bungeman
Base URL: http://skia.googlecode.com/svn/trunk/
Comments: 10