New ZipReader::ExtractCurrentEntryToString API.

third_party/zlib/google/zip_reader.cc

Index: third_party/zlib/google/zip_reader.cc
diff --git a/third_party/zlib/google/zip_reader.cc b/third_party/zlib/google/zip_reader.cc
index aa85fa3baf22c34035a96b7aa9c24e4e130eb1ee..4bd4038748ede187bdbc6f4e8561036c7a41764b 100644
--- a/third_party/zlib/google/zip_reader.cc
+++ b/third_party/zlib/google/zip_reader.cc
@@ -344,6 +344,80 @@ bool ZipReader::ExtractCurrentEntryToFd(const int fd) {
 }
 #endif  // defined(OS_POSIX)
 
+bool ZipReader::ExtractCurrentEntryToString(
+    size_t max_read_bytes,
+    std::string* output) const {
+  DCHECK(output);
+  DCHECK(zip_file_);
+  DCHECK(max_read_bytes != 0);
+
+  if (current_entry_info()->is_directory()) {
+    output->clear();

[satorux1, 2014/05/23 23:45:47]

i think we should always call output->clear() even if it's not a directory
because |output| may contain something.

[João Eiras, 2014/06/02 16:41:07]

It's my personal style to avoid changing output parameters in case the function
fails.

Ahead, the loop uses a new string (std::string contents) and in case of success
swaps with the output arg.

Are you fine with that ? 

[satorux1, 2014/06/03 08:17:40]

That's fine by me.

+    return true;
+  }
+
+  const int open_result = unzOpenCurrentFile(zip_file_);
+  if (open_result != UNZ_OK)
+    return false;
+
+  // The original_size() is the best hint for the real size, so it saves
+  // doing reallocations for the common case when the uncompressed size is
+  // correct. However, we need to assume that the uncompressed size could be
+  // incorrect therefore this function needs to read as much data as possible.
+  std::string contents;
+  contents.reserve(std::min<size_t>(
+      max_read_bytes, current_entry_info()->original_size()) + 1);

[satorux1, 2014/05/23 23:45:47]

i think we don't need +1

[João Eiras, 2014/06/02 16:41:07]

Was there for unzReadCurrentFile() to tell if it hit the eof on the first call.

+
+  size_t buffer_index = 0;
+  bool success = true;
+  while (true) {
+    // This just sets size() to be equal to capacity() so the chars which are read

[satorux1, 2014/05/23 23:45:47]

line should be < 80 chars

+    // ahead will not be erased by calling resize() later.
+    contents.resize(contents.capacity());

[satorux1, 2014/05/23 23:45:47]

hmm, this looks complex. I understand that you are trying to avoid copying
bytes, but I think the cost for copying bytes is small enough compared to the 
expensive computation required for uncompression.

I'd suggest to simply append() data, like:

char buffer[kZipBufSize];
while (true) {
   bytes_read = unzReadCurrentFile(zip_file_, buf, kZipBufiSize];
   if (bytes_read == 0) {
     break;
   } else if (bytes_read < 0) {
     success = false;
     break;
  } else {
     if (contents.size() + bytes_read > max_read_bytes) {
       success = false;
       break;
    } 
     contents.append(buffer, kZipBufSize);
  }
}

This way, you don't have to worry about resizing and overrunning |contents|.

[João Eiras, 2014/06/02 16:41:07]

It does look a bit overly complicated, but then my concern was for the common
case (the original size being properly set) the loop body only runs once and
there's only one call to unzReadCurrentFile() which fills the buffer in one go.
But I'll accept your change.

+
+    const size_t space_available = contents.capacity() - buffer_index;
+    const int bytes_to_read = std::min<int>(INT_MAX, space_available);
+    const int bytes_read = unzReadCurrentFile(
+        zip_file_,
+        &(contents[buffer_index]),
+        bytes_to_read);
+    DCHECK(bytes_read <= bytes_to_read);
+
+    if (bytes_read == 0) {
+      contents.resize(buffer_index);
+      // Reached the end of the file.
+      break;
+    } else if (bytes_read < 0) {
+      // If num_bytes_read < 0, then it's a specific UNZ_* error code.
+      success = false;
+      break;
+    } else {  // if (num_bytes_read > 0)
+      buffer_index += bytes_read;
+
+      if (buffer_index > max_read_bytes) {
+        success = false;
+        break;
+      }
+
+      if (bytes_read == bytes_to_read) {
+        // Filled up the buffer, no more space left, need to resize.
+        if (static_cast<size_t>(bytes_to_read) == space_available)
+          contents.reserve(contents.capacity() + internal::kZipBufSize);
+      } else {
+        contents.resize(buffer_index);
+        // Read less bytes than asked. That means it reached end of file.
+        break;
+      }
+    }
+  }
+
+  unzCloseCurrentFile(zip_file_);
+  if (success)
+    output->swap(contents);
+
+  return success;
+}
+
 bool ZipReader::OpenInternal() {
   DCHECK(zip_file_);