Allow SkStream::peek() to partially succeed

src/core/SkStream.cpp

Index: src/core/SkStream.cpp
diff --git a/src/core/SkStream.cpp b/src/core/SkStream.cpp
index 05867d91bfe53191ce65e15de1dc8d23d49a9c5a..e475aa799c470a290ec4d174f3936d813c9a96c1 100644
--- a/src/core/SkStream.cpp
+++ b/src/core/SkStream.cpp
@@ -367,18 +367,22 @@ size_t SkMemoryStream::read(void* buffer, size_t size) {
     return size;
 }
 
-bool SkMemoryStream::peek(void* buffer, size_t size) const {
+size_t SkMemoryStream::peek(void* buffer, size_t size) const {
     SkASSERT(buffer != nullptr);
-    const size_t position = fOffset;
-    if (size > fData->size() - position) {
-        // The stream is not large enough to satisfy this request.
-        return false;
+
+    if (this->isAtEnd()) {

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

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.

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

Removed.

+        return 0;
     }
+
+    SkASSERT(fData->size() > fOffset);

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

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.

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

Removed.

+    const size_t position = fOffset;

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

It seems this be clearer (or at least more consistent) if 'position' were named
'currentOffset' or something like that.

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

Done.

+    size = SkTMin(size, fData->size() - position);

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

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.

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

Done.

+
     SkMemoryStream* nonConstThis = const_cast<SkMemoryStream*>(this);
     SkDEBUGCODE(const size_t bytesRead =) nonConstThis->read(buffer, size);
     SkASSERT(bytesRead == size);
     nonConstThis->fOffset = position;
-    return true;
+    return size;
 }
 
 bool SkMemoryStream::isAtEnd() const {
@@ -725,11 +729,17 @@ public:
         return fOffset == fSize;
     }
 
-    bool peek(void* buff, size_t size) const override {
+    size_t peek(void* buff, size_t size) const override {
         SkASSERT(buff != nullptr);
-        if (fOffset + size > fSize) {
-            return false;
+
+        if (this->isAtEnd()) {

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

As above, this appears to be unneeded.

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

Removed

+            return 0;
         }
+
+        SkASSERT(fSize > fOffset);

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

And 'SkASSERT(fSize >= fOffset);' should be a general invariant of this class
(note isAtEnd already depends on this).

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

Removed

+        size = SkTMin(size, fSize - fOffset);
+
+        const size_t totalBytesToPeek = size;

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

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]

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

         char* buffer = static_cast<char*>(buff);
         const SkDynamicMemoryWStream::Block* current = fCurrent;
         size_t currentOffset = fCurrentOffset;
@@ -743,7 +753,7 @@ public:
             current = current->fNext;
             currentOffset = 0;
         }
-        return true;
+        return totalBytesToPeek;
     }
 
     bool rewind() override {