Allow SkStream::peek() to partially succeed

Allow SkStream::peek() to partially succeed

If the stream can peek less than requested, peek that amount. Return
the number of bytes peeked.

This simplifies crrev.com/1472123002. For a stream that is smaller than
14 bytes, it can successfully peek, meaning the client will not need to
fall back to read() + rewind(), which may fail if the stream can peek
but not rewind.

This CL revives code from patch set 3 of crrev.com/1044953002, where I
initially introduced peek() (including tests).

Add a test for SkFrontBufferedStream that verifies that peeking does
not make rewind() fail (i.e. by reading past the internal buffer).

BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/d61c384342aaf7facf31b948db98acc32d81d21b

[scroggo, 2015-12-04 22:25:13]

scroggo@google.com changed reviewers:
+ bungeman@google.com, msarett@google.com, reed@google.com

[reed1, 2015-12-07 14:43:58]

Do we have to comment-out the parameter names (hard to read) ?

[msarett, 2015-12-07 15:41:03]

On 2015/12/07 14:43:58, reed1 wrote:
> Do we have to comment-out the parameter names (hard to read) ?

Android enables Wunused-parameter and Skia is compiled to treat warnings as
errors
in Android.  I think commenting out the names is necessary to include SkStream.h
in
Android.

lgtm

[scroggo, 2015-12-07 16:03:45]

On 2015/12/07 15:41:03, msarett wrote:
> On 2015/12/07 14:43:58, reed1 wrote:
> > Do we have to comment-out the parameter names (hard to read) ?
> 
> Android enables Wunused-parameter and Skia is compiled to treat warnings as
> errors
> in Android.  I think commenting out the names is necessary to include
SkStream.h
> in
> Android.

That's correct. It's kind of annoying, because parameters *are* allowed even if
unused if there's no definition (i.e. the definition is elsewhere, or it's
abstract).

FWIW, elsewhere in this same file we also comment out parameter names e.g. 

    virtual bool seek(size_t /*position*/) { return false; }

(there we do not have spaces between the name and the comment markers. Should
this match?)

Alternatively, we can leave the names out entirely. This means the comments no
longer have a parameter name to reference, but we can specify a name in the
comment or just refer to the type? Let me know what you prefer.
> 
> lgtm

[bungeman-skia, 2015-12-07 16:17:36]

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

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:373: if (this->isAtEnd()) {
This block seems redundant, though maybe an optimization for a low frequency
case (the user should already be checking this anyway if looping). The following
code will already read zero bytes and return 0 anyway.

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:377: SkASSERT(fData->size() > fOffset);
This assert depends on the above block to remove the 'fOffset == fData->size()'
case. 'SkASSERT(fData->size() >= fOffset);' is a general invariant of this class
anyway, if there were a nice syntax for pre/post condition invariant asserts,
this would be one. Such a thing would want to go into 'read' and 'seek', but
this seems like a strange place to put it.

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:378: const size_t position = fOffset;
It seems this be clearer (or at least more consistent) if 'position' were named
'currentOffset' or something like that.

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:379: size = SkTMin(size, fData->size() - position);
Is there a reason to calculate this? This is what 'read' will return, is it not?
The old test here was to prevent the overhead of doing a partial read but then
having to return false. With the new semantics, this is fine. Basically, the
SkDEBUGCODE can be removes (leaving the code) and the SkASSERT can be removed.

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:735: if (this->isAtEnd()) {
As above, this appears to be unneeded.

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:739: SkASSERT(fSize > fOffset);
And 'SkASSERT(fSize >= fOffset);' should be a general invariant of this class
(note isAtEnd already depends on this).

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:742: const size_t totalBytesToPeek = size;
Since we have to maintain size now to return it, I would prefer that the naming
here be changed. I'd like it to match the terms used in 'read', but those might
not be the best either. It may be clearer for 'size' here to be named
'bytesLeftToPeek' and 'totalBytesToPeek' here to be named 'bytesToPeek'.

[scroggo, 2015-12-07 16:52:02]

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

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:373: if (this->isAtEnd()) {
On 2015/12/07 16:17:36, bungeman1 wrote:
> This block seems redundant, though maybe an optimization for a low frequency
> case (the user should already be checking this anyway if looping). The
following
> code will already read zero bytes and return 0 anyway.

Removed.

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:377: SkASSERT(fData->size() > fOffset);
On 2015/12/07 16:17:36, bungeman1 wrote:
> This assert depends on the above block to remove the 'fOffset ==
fData->size()'
> case. 'SkASSERT(fData->size() >= fOffset);' is a general invariant of this
class
> anyway, if there were a nice syntax for pre/post condition invariant asserts,
> this would be one. Such a thing would want to go into 'read' and 'seek', but
> this seems like a strange place to put it.

Removed.

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:378: const size_t position = fOffset;
On 2015/12/07 16:17:36, bungeman1 wrote:
> It seems this be clearer (or at least more consistent) if 'position' were
named
> 'currentOffset' or something like that.

Done.

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:379: size = SkTMin(size, fData->size() - position);
On 2015/12/07 16:17:36, bungeman1 wrote:
> Is there a reason to calculate this? This is what 'read' will return, is it
not?
> The old test here was to prevent the overhead of doing a partial read but then
> having to return false. With the new semantics, this is fine. Basically, the
> SkDEBUGCODE can be removes (leaving the code) and the SkASSERT can be removed.

Done.

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:735: if (this->isAtEnd()) {
On 2015/12/07 16:17:36, bungeman1 wrote:
> As above, this appears to be unneeded.

Removed

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:739: SkASSERT(fSize > fOffset);
On 2015/12/07 16:17:36, bungeman1 wrote:
> And 'SkASSERT(fSize >= fOffset);' should be a general invariant of this class
> (note isAtEnd already depends on this).

Removed

https://codereview.chromium.org/1490923005/diff/1/src/core/SkStream.cpp#newco...
src/core/SkStream.cpp:742: const size_t totalBytesToPeek = size;
On 2015/12/07 16:17:36, bungeman1 wrote:
> Since we have to maintain size now to return it, I would prefer that the
naming
> here be changed. I'd like it to match the terms used in 'read', but those
might
> not be the best either. It may be clearer for 'size' here to be named
> 'bytesLeftToPeek' and 'totalBytesToPeek' here to be named 'bytesToPeek'.

Changed. I think I did as you requested; let me know if you meant something
else.

[bungeman-skia, 2015-12-07 17:33:02]

lgtm with nit

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

https://codereview.chromium.org/1490923005/diff/20001/include/core/SkStream.h...
include/core/SkStream.h:79: virtual size_t peek(void* /* buffer */, size_t /*
size */) const { return 0; }
nit: I think doing these like the others below (without the whitespace, like
/*foo*/) is a little less difficult to parse (especially without syntax
highlighting.

[scroggo, 2015-12-07 18:10:32]

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

https://codereview.chromium.org/1490923005/diff/20001/include/core/SkStream.h...
include/core/SkStream.h:79: virtual size_t peek(void* /* buffer */, size_t /*
size */) const { return 0; }
On 2015/12/07 17:33:01, bungeman1 wrote:
> nit: I think doing these like the others below (without the whitespace, like
> /*foo*/) is a little less difficult to parse (especially without syntax
> highlighting.

Done.

[bungeman-skia, 2015-12-07 18:35:03]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 18:35:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1490923005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1490923005/40001

[commit-bot: I haz the power, 2015-12-07 18:37:36]

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

[commit-bot: I haz the power, 2015-12-07 18:37:36]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[bungeman-skia, 2015-12-07 18:40:24]

https://codereview.chromium.org/1490923005/diff/40001/tests/StreamTest.cpp
File tests/StreamTest.cpp (right):

https://codereview.chromium.org/1490923005/diff/40001/tests/StreamTest.cpp#ne...
tests/StreamTest.cpp:225: return bytesRead - bytesPeeked;
SkToBool

[scroggo, 2015-12-07 18:43:40]

Mike, can I get an API review?

https://codereview.chromium.org/1490923005/diff/40001/tests/StreamTest.cpp
File tests/StreamTest.cpp (right):

https://codereview.chromium.org/1490923005/diff/40001/tests/StreamTest.cpp#ne...
tests/StreamTest.cpp:225: return bytesRead - bytesPeeked;
On 2015/12/07 18:40:23, bungeman1 wrote:
> SkToBool

Actually, this caught a bug! I meant to return a size_t.

[reed1, 2015-12-07 19:12:54]

lgtm

[scroggo, 2015-12-07 19:22:27]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-12-07 19:22:28]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
bungeman@google.com
Link to the patchset: https://codereview.chromium.org/1490923005/#ps60001
(title: "Use the correct return type")

[commit-bot: I haz the power, 2015-12-07 19:22:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1490923005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1490923005/60001

[commit-bot: I haz the power, 2015-12-07 19:37:15]

Description was changed from

==========
Allow SkStream::peek() to partially succeed

If the stream can peek less than requested, peek that amount. Return
the number of bytes peeked.

This simplifies crrev.com/1472123002. For a stream that is smaller than
14 bytes, it can successfully peek, meaning the client will not need to
fall back to read() + rewind(), which may fail if the stream can peek
but not rewind.

This CL revives code from patch set 3 of crrev.com/1044953002, where I
initially introduced peek() (including tests).

Add a test for SkFrontBufferedStream that verifies that peeking does
not make rewind() fail (i.e. by reading past the internal buffer).

BUG=skia:3257
==========

to

==========
Allow SkStream::peek() to partially succeed

If the stream can peek less than requested, peek that amount. Return
the number of bytes peeked.

This simplifies crrev.com/1472123002. For a stream that is smaller than
14 bytes, it can successfully peek, meaning the client will not need to
fall back to read() + rewind(), which may fail if the stream can peek
but not rewind.

This CL revives code from patch set 3 of crrev.com/1044953002, where I
initially introduced peek() (including tests).

Add a test for SkFrontBufferedStream that verifies that peeking does
not make rewind() fail (i.e. by reading past the internal buffer).

BUG=skia:3257

Committed:
https://skia.googlesource.com/skia/+/d61c384342aaf7facf31b948db98acc32d81d21b
==========

[commit-bot: I haz the power, 2015-12-07 19:37:15]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/d61c384342aaf7facf31b948db98acc32d81d21b