Make SkStream *not* ref counted.

Make SkStream *not* ref counted.

SkStream is a stateful object, so it does not make sense for it to have
multiple owners. Make SkStream inherit directly from SkNoncopyable.

Update methods which previously called SkStream::ref() (e.g.
SkImageDecoder::buildTileIndex() and SkFrontBufferedStream::Create(),
which required the existing owners to call SkStream::unref()) to take
ownership of their SkStream parameters and delete when done (including
on failure).

Switch all SkAutoTUnref<SkStream>s to SkAutoTDelete<SkStream>s. In some
cases this means heap allocating streams that were previously stack
allocated.

Respect ownership rules of SkTypeface::CreateFromStream() and
SkImageDecoder::buildTileIndex().

Update the comments for exceptional methods which do not affect the
ownership of their SkStream parameters (e.g.
SkPicture::CreateFromStream() and SkTypeface::Deserialize()) to be
explicit about ownership.

Remove test_stream_life, which tested that buildTileIndex() behaved
correctly when SkStream was a ref counted object. The test does not
make sense now that it is not.

In SkPDFStream, remove the SkMemoryStream member. Instead of using it,
create a new SkMemoryStream to pass to fDataStream (which is now an
SkAutoTDelete).

Make other pdf rasterizers behave like SkPDFDocumentToBitmap.

SkPDFDocumentToBitmap delete the SkStream, so do the same in the
following pdf rasterizers:
    
SkPopplerRasterizePDF
SkNativeRasterizePDF
SkNoRasterizePDF

Requires a change to Android, which currently treats SkStreams as ref
counted objects.

Committed: https://skia.googlesource.com/skia/+/a1193e4b0e34a7e4e1bd33e9708d7341679f8321

[bungeman-skia, 2015-01-15 22:44:54]

bungeman@google.com changed reviewers:
+ bungeman@google.com

[bungeman-skia, 2015-01-15 22:44:56]

https://codereview.chromium.org/849103004/diff/100001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/849103004/diff/100001/gm/gmmain.cpp#newcode1097
gm/gmmain.cpp:1097: documentStream->duplicate(), &pdfBitmap);
I think you can just rewind and detach here, since it isn't being used later.
write_document above doesn't take ownership of the stream (or else we wouldn't
have it here).

https://codereview.chromium.org/849103004/diff/100001/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.chromium.org/849103004/diff/100001/include/core/SkStream.h...
include/core/SkStream.h:50: SK_DECLARE_INST_COUNT(SkStream)
The entire point of this is to instance count ref countable objects, so we
should just remove it (and the include above).

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontHost_Fr...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontHost_Fr...
src/ports/SkFontHost_FreeType.cpp:243: // assumes ownership of the stream, will
call unref() when its done
Comment, 'will delete when its done'.

[bungeman-skia, 2015-01-15 22:56:04]

Looks like SkFontConfigTypeface.h and SkDWriteFontFileStream are the next files
which need to be touched.

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontHost_fo...
File src/ports/SkFontHost_fontconfig.cpp (right):

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontHost_fo...
src/ports/SkFontHost_fontconfig.cpp:157: stream->ref();
At this point I think these two lines can be removed and just assert(false) for
the time being. This was written at a time when Chromium/Blink usage was in flux
and there wasn't anything we could rely on, so this was a last ditch "make it
mostly work until it might crash somewhere else".

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontMgr_fon...
File src/ports/SkFontMgr_fontconfig.cpp (right):

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontMgr_fon...
src/ports/SkFontMgr_fontconfig.cpp:381: , fStream(SkRef(stream))
Looks like this line needs to take ownership instead of ref'ing.

[scroggo, 2015-01-16 19:13:29]

scroggo@google.com changed reviewers:
+ halcanary@google.com

[scroggo, 2015-01-16 19:13:37]

Adding Hal, primarily to look at PDF stuff (but feel free to look at other
things as well).

In some cases, I've updated to take ownership, but I wonder if we should just
make it not affect the ownership. e.g. SkFrontBufferedStream's use case never
needs to hang on to the stream, so it *could* leave deletion up to the caller.

On 2015/01/15 22:56:04, bungeman1 wrote:
> Looks like SkFontConfigTypeface.h and SkDWriteFontFileStream are the next
files
> which need to be touched.

I think I already fixed SkDWriteFontFileStream, and I've now fixed
SkFontConfigTypeface.h

https://codereview.chromium.org/849103004/diff/100001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/849103004/diff/100001/gm/gmmain.cpp#newcode1097
gm/gmmain.cpp:1097: documentStream->duplicate(), &pdfBitmap);
On 2015/01/15 22:44:55, bungeman1 wrote:
> I think you can just rewind and detach here, since it isn't being used later.
> write_document above doesn't take ownership of the stream (or else we wouldn't
> have it here).

Actually, that won't work. This loops through the rasterizer functions, and the
mac one (SkPDFDocumentToBitmap - unfortunately that one does not make it clear
that it is mac specific...) takes ownership and deletes the stream.

I updated the others to match SkPDFDocumentToBitmap, since it seemed more like
something we actually use.. Alternatively, I could have updated the mac one to
behave like the others.

(The change description explains this change.)

https://codereview.chromium.org/849103004/diff/100001/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.chromium.org/849103004/diff/100001/include/core/SkStream.h...
include/core/SkStream.h:50: SK_DECLARE_INST_COUNT(SkStream)
On 2015/01/15 22:44:55, bungeman1 wrote:
> The entire point of this is to instance count ref countable objects, so we
> should just remove it (and the include above).

Oh, right, of course.

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontHost_Fr...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontHost_Fr...
src/ports/SkFontHost_FreeType.cpp:243: // assumes ownership of the stream, will
call unref() when its done
On 2015/01/15 22:44:55, bungeman1 wrote:
> Comment, 'will delete when its done'.

Done.

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontHost_fo...
File src/ports/SkFontHost_fontconfig.cpp (right):

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontHost_fo...
src/ports/SkFontHost_fontconfig.cpp:157: stream->ref();
On 2015/01/15 22:56:04, bungeman1 wrote:
> At this point I think these two lines can be removed and just assert(false)
for
> the time being. This was written at a time when Chromium/Blink usage was in
flux
> and there wasn't anything we could rely on, so this was a last ditch "make it
> mostly work until it might crash somewhere else".

Definitely need to delete ref(); just hadn't tested on linux yet.

Will delete the rewind as well.

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontMgr_fon...
File src/ports/SkFontMgr_fontconfig.cpp (right):

https://codereview.chromium.org/849103004/diff/100001/src/ports/SkFontMgr_fon...
src/ports/SkFontMgr_fontconfig.cpp:381: , fStream(SkRef(stream))
On 2015/01/15 22:56:04, bungeman1 wrote:
> Looks like this line needs to take ownership instead of ref'ing.

Done.

[bungeman-skia, 2015-01-16 19:24:52]

SkDWriteFontFileStream.cpp will also need some changes, it at least calls SkRef
on an SkStream.

[scroggo, 2015-01-16 20:14:48]

On 2015/01/16 19:24:52, bungeman1 wrote:
> SkDWriteFontFileStream.cpp will also need some changes, it at least calls
SkRef
> on an SkStream.

Done.

[hal.canary, 2015-01-16 23:32:56]

On 2015/01/16 19:13:37, scroggo wrote:
> Adding Hal, primarily to look at PDF stuff (but feel free to look at other
> things as well).


pdf: lgtm

this is a very good thing.

[bungeman-skia, 2015-01-20 21:37:05]

I believe that SkEmptyFontMgr::onCreateFromStream needs to delete the SkStream
passed to it.

[bungeman-skia, 2015-01-20 21:41:49]

https://codereview.chromium.org/849103004/diff/360001/src/fonts/SkFontMgr_fon...
File src/fonts/SkFontMgr_fontconfig.cpp (right):

https://codereview.chromium.org/849103004/diff/360001/src/fonts/SkFontMgr_fon...
src/fonts/SkFontMgr_fontconfig.cpp:311: return NULL;
Looks like we need to delete the stream here?

https://codereview.chromium.org/849103004/diff/360001/src/fonts/SkFontMgr_fon...
src/fonts/SkFontMgr_fontconfig.cpp:319: SkAutoTDelete<SkStream>
stream(SkStream::NewFromFile(path));
I don't think this guy should be deleting the stream, as it should be passing
ownership to createFromStream.

https://codereview.chromium.org/849103004/diff/360001/src/ports/SkFontMgr_fon...
File src/ports/SkFontMgr_fontconfig.cpp (right):

https://codereview.chromium.org/849103004/diff/360001/src/ports/SkFontMgr_fon...
src/ports/SkFontMgr_fontconfig.cpp:822: return NULL;
Here we need to delete the stream.

[scroggo, 2015-01-20 22:19:09]

On 2015/01/20 21:37:05, bungeman1 wrote:
> I believe that SkEmptyFontMgr::onCreateFromStream needs to delete the SkStream
> passed to it.

Done.

https://codereview.chromium.org/849103004/diff/360001/src/fonts/SkFontMgr_fon...
File src/fonts/SkFontMgr_fontconfig.cpp (right):

https://codereview.chromium.org/849103004/diff/360001/src/fonts/SkFontMgr_fon...
src/fonts/SkFontMgr_fontconfig.cpp:311: return NULL;
On 2015/01/20 21:41:48, bungeman1 wrote:
> Looks like we need to delete the stream here?

Done with SkAutoTDelete.

https://codereview.chromium.org/849103004/diff/360001/src/fonts/SkFontMgr_fon...
src/fonts/SkFontMgr_fontconfig.cpp:319: SkAutoTDelete<SkStream>
stream(SkStream::NewFromFile(path));
On 2015/01/20 21:41:48, bungeman1 wrote:
> I don't think this guy should be deleting the stream, as it should be passing
> ownership to createFromStream.

Done.

https://codereview.chromium.org/849103004/diff/360001/src/ports/SkFontMgr_fon...
File src/ports/SkFontMgr_fontconfig.cpp (right):

https://codereview.chromium.org/849103004/diff/360001/src/ports/SkFontMgr_fon...
src/ports/SkFontMgr_fontconfig.cpp:822: return NULL;
On 2015/01/20 21:41:48, bungeman1 wrote:
> Here we need to delete the stream.

Done with SkAutoTDelete.

[bungeman-skia, 2015-01-20 22:23:54]

https://codereview.chromium.org/849103004/diff/380001/src/core/SkFontMgr.cpp
File src/core/SkFontMgr.cpp (right):

https://codereview.chromium.org/849103004/diff/380001/src/core/SkFontMgr.cpp#...
src/core/SkFontMgr.cpp:14: class SkStream;
Since we need to include now, should remove this forward decl.

[scroggo, 2015-01-20 22:25:15]

https://codereview.chromium.org/849103004/diff/380001/src/core/SkFontMgr.cpp
File src/core/SkFontMgr.cpp (right):

https://codereview.chromium.org/849103004/diff/380001/src/core/SkFontMgr.cpp#...
src/core/SkFontMgr.cpp:14: class SkStream;
On 2015/01/20 22:23:54, bungeman1 wrote:
> Since we need to include now, should remove this forward decl.

Done.

[bungeman, 2015-01-21 05:44:29]

https://codereview.chromium.org/849103004/diff/400001/src/ports/SkFontMgr_win...
File src/ports/SkFontMgr_win_dw.cpp (right):

https://codereview.chromium.org/849103004/diff/400001/src/ports/SkFontMgr_win...
src/ports/SkFontMgr_win_dw.cpp:84:
HR(SkDWriteFontFileStreamWrapper::Create(fStream, &stream));
fStream->duplicate()

This is an existing bug, but probably wasn't all that serious, given how this
has been called and is used by DW. Needs to be fixed now though.

[scroggo, 2015-01-21 14:56:07]

https://codereview.chromium.org/849103004/diff/400001/src/ports/SkFontMgr_win...
File src/ports/SkFontMgr_win_dw.cpp (right):

https://codereview.chromium.org/849103004/diff/400001/src/ports/SkFontMgr_win...
src/ports/SkFontMgr_win_dw.cpp:84:
HR(SkDWriteFontFileStreamWrapper::Create(fStream, &stream));
On 2015/01/21 05:44:29, bungeman wrote:
> fStream->duplicate()
> 
> This is an existing bug, but probably wasn't all that serious, given how this
> has been called and is used by DW. Needs to be fixed now though.

Done.

[bungeman-skia, 2015-01-21 17:01:12]

PS23 lgtm.

[scroggo, 2015-01-21 19:53:34]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-01-21 19:53:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/849103004/440001

[commit-bot: I haz the power, 2015-01-21 19:54:11]

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

[commit-bot: I haz the power, 2015-01-21 19:54:12]

Presubmit check for 849103004-440001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com')

Was the presubmit check useful? If not, run "git cl presubmit -v"
to figure out which PRESUBMIT.py was run, then run git blame
on the file to figure out who to ask for help.

[scroggo, 2015-01-21 19:55:07]

scroggo@google.com changed reviewers:
+ djsollen@google.com

[reed1, 2015-01-21 19:58:49]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-01-21 19:58:50]

lgtm

[scroggo, 2015-01-21 19:59:56]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-01-21 20:00:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/849103004/440001

[commit-bot: I haz the power, 2015-01-21 20:09:58]

Committed patchset #23 (id:440001) as
https://skia.googlesource.com/skia/+/a1193e4b0e34a7e4e1bd33e9708d7341679f8321