Move SkPDFStream back to SkStream to save memory.

Move SkPDFStream back to SkStream to save memory.

SkPDFStream stores data as a SkStreamRewindable to minimize
deep duplication and memory overhead.

SkStreamToStreamRewindable function to deal with fact that
SkTypeface returns a SkStream.

BUG=skia:2743
Committed: https://skia.googlesource.com/skia/+/e322482f4d82bc704e40b8c89843f2ea5b6ddc04

[hal.canary, 2014-07-11 20:52:55]

I hope this helps with the sk_out_of_memory issues on the Nexus4.

[bungeman-skia, 2014-07-11 21:43:39]

Long term we need to have all of our interfaces actually take the right sort of
stream interface.

https://codereview.chromium.org/387863005/diff/20001/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.chromium.org/387863005/diff/20001/src/core/SkStream.cpp#ne...
src/core/SkStream.cpp:911: SkStreamRewindable*
SkStreamToStreamRewindable(SkStream* stream) {
I would prefer this to be SkStreamRewindableFromSkStream so that 1) it looks
more like a constructor call and 2) the parts of the name are closer to the
values they refer to. On the other hand, the other functions here use the
backward 'To' form, so I can't be too ornery.

https://codereview.chromium.org/387863005/diff/20001/src/core/SkStream.cpp#ne...
src/core/SkStream.cpp:922: SkAssertResult(stream->rewind());
I wouldn't assert this, just because a stream has a length doesn't imply that
it's rewindable (though I must admit that it seems unlikely). I think I would
just 'stream->rewind()' before this 'if' block and remove the
'SkAssertResult(stream->rewind());' below (line 929).

Then, if 'hasPosition' you can subtract that from the length.

https://codereview.chromium.org/387863005/diff/20001/src/core/SkStream.cpp#ne...
src/core/SkStream.cpp:924: SkAssertResult(stream->rewind());
I see no reason to try to leave this in a known state, since other paths do not.
I wouldn't bother with this and just comment that the passed stream is left at
an unknown position.

https://codereview.chromium.org/387863005/diff/20001/src/core/SkStream.cpp#ne...
src/core/SkStream.cpp:937: SkAssertResult(stream->rewind());
Same as above (line 924), I see no reason to try to rewind anything just to
leave the stream in a known state.

https://codereview.chromium.org/387863005/diff/20001/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.chromium.org/387863005/diff/20001/src/pdf/SkPDFStream.h#ne...
src/pdf/SkPDFStream.h:30: *  @param stream The data part of the stream.  Will be
duplicate()d.
This seems to want to go with the constructor below?

[hal.canary, 2014-07-12 00:58:49]

https://codereview.chromium.org/387863005/diff/20001/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.chromium.org/387863005/diff/20001/src/core/SkStream.cpp#ne...
src/core/SkStream.cpp:911: SkStreamRewindable*
SkStreamToStreamRewindable(SkStream* stream) {
On 2014/07/11 21:43:38, bungeman1 wrote:
> I would prefer this to be SkStreamRewindableFromSkStream so that 1) it looks
> more like a constructor call and 2) the parts of the name are closer to the
> values they refer to. On the other hand, the other functions here use the
> backward 'To' form, so I can't be too ornery.

Done.

https://codereview.chromium.org/387863005/diff/20001/src/core/SkStream.cpp#ne...
src/core/SkStream.cpp:922: SkAssertResult(stream->rewind());
On 2014/07/11 21:43:38, bungeman1 wrote:
> I wouldn't assert this, just because a stream has a length doesn't imply that
> it's rewindable (though I must admit that it seems unlikely). I think I would
> just 'stream->rewind()' before this 'if' block and remove the
> 'SkAssertResult(stream->rewind());' below (line 929).
> 
> Then, if 'hasPosition' you can subtract that from the length.

Done.

https://codereview.chromium.org/387863005/diff/20001/src/core/SkStream.cpp#ne...
src/core/SkStream.cpp:924: SkAssertResult(stream->rewind());
On 2014/07/11 21:43:38, bungeman1 wrote:
> I see no reason to try to leave this in a known state, since other paths do
not.
> I wouldn't bother with this and just comment that the passed stream is left at
> an unknown position.

Done.

https://codereview.chromium.org/387863005/diff/20001/src/core/SkStream.cpp#ne...
src/core/SkStream.cpp:937: SkAssertResult(stream->rewind());
On 2014/07/11 21:43:38, bungeman1 wrote:
> Same as above (line 924), I see no reason to try to rewind anything just to
> leave the stream in a known state.

Done.

https://codereview.chromium.org/387863005/diff/20001/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.chromium.org/387863005/diff/20001/src/pdf/SkPDFStream.h#ne...
src/pdf/SkPDFStream.h:30: *  @param stream The data part of the stream.  Will be
duplicate()d.
On 2014/07/11 21:43:38, bungeman1 wrote:
> This seems to want to go with the constructor below?

Done.

[reed1, 2014-07-12 11:05:05]

https://codereview.chromium.org/387863005/diff/40001/src/core/SkStreamPriv.h
File src/core/SkStreamPriv.h (right):

https://codereview.chromium.org/387863005/diff/40001/src/core/SkStreamPriv.h#...
src/core/SkStreamPriv.h:41: *  otherwise copying to memory).
What happens with subsequent reads to the newly returned object? Do they affect
the cursor in the original stream?

[hal.canary, 2014-07-14 14:34:11]

I modified the code and the documentation to be as consistent as possible.

https://codereview.chromium.org/387863005/diff/40001/src/core/SkStreamPriv.h
File src/core/SkStreamPriv.h (right):

https://codereview.chromium.org/387863005/diff/40001/src/core/SkStreamPriv.h#...
src/core/SkStreamPriv.h:41: *  otherwise copying to memory).
On 2014/07/12 11:05:05, reed1 wrote:
> What happens with subsequent reads to the newly returned 
> object? Do they affect the cursor in the original stream?

Good question.  Before http://crrev.com/340783013, the PDF backend got a
SkStream from SkTypeface::openStream() and and rewinded it after each read.

[bungeman-skia, 2014-07-14 15:05:00]

https://codereview.chromium.org/387863005/diff/60001/src/core/SkStreamPriv.h
File src/core/SkStreamPriv.h (right):

https://codereview.chromium.org/387863005/diff/60001/src/core/SkStreamPriv.h#...
src/core/SkStreamPriv.h:43: *  calls rewind() on the input stream before
returning.
There doesn't seem to be any actionable information here for the caller, as they
don't know what succeeded and what didn't?

[hal.canary, 2014-07-14 15:08:52]

https://codereview.chromium.org/387863005/diff/60001/src/core/SkStreamPriv.h
File src/core/SkStreamPriv.h (right):

https://codereview.chromium.org/387863005/diff/60001/src/core/SkStreamPriv.h#...
src/core/SkStreamPriv.h:43: *  calls rewind() on the input stream before
returning.
On 2014/07/14 15:05:00, bungeman1 wrote:
> There doesn't seem to be any actionable information here for the caller, as
they
> don't know what succeeded and what didn't? 

Done.

[bungeman-skia, 2014-07-14 15:10:23]

lgtm at PS5

[hal.canary, 2014-07-14 15:23:02]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-07-14 15:23:58]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/halcanary@google.com/387863005/80001

[commit-bot: I haz the power, 2014-07-14 15:37:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Mac10.8-Clang-x86-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86-Release-Trybot/b...)
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

[hal.canary, 2014-07-14 15:41:50]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2014-07-14 15:50:13]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-07-14 15:50:35]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/halcanary@google.com/387863005/100001

[commit-bot: I haz the power, 2014-07-14 16:12:18]

Change committed as e322482f4d82bc704e40b8c89843f2ea5b6ddc04