New Zip::ZipFromMemory API.

third_party/zlib/google/zip_reader.h

Index: third_party/zlib/google/zip_reader.h
diff --git a/third_party/zlib/google/zip_reader.h b/third_party/zlib/google/zip_reader.h
index 5f0a16785de3bb51b304eb8723521c1acb839d63..66aba1e2130bc16dc7053a82a2fffd624745a1cc 100644
--- a/third_party/zlib/google/zip_reader.h
+++ b/third_party/zlib/google/zip_reader.h
@@ -21,6 +21,12 @@
 #include "third_party/zlib/contrib/minizip/unzip.h"
 #endif
 
+template <class T> class scoped_refptr;
+
+namespace base {
+class RefCountedMemory;
+}
+
 namespace zip {
 
 // This class is used for reading zip files. A typical use case of this
@@ -180,6 +186,23 @@ class ZipReader {
   bool ExtractCurrentEntryToFd(int fd);
 #endif
 
+  // Extracts the current entry into memory. If the current file is a directory,
+  // NULL is stored in the |output| parameter. If the current file is a file,
+  // |output| is never set to NULL. Returns true on success.
+  // OpenCurrentEntryInZip() must be called beforehand.
+  // Note that EntryInfo::original_size() can return a different value than the
+  // real size of the uncompressed contents. Therefore, to prevent excessive
+  // memory usage, the maximum amount of data to read has to be specified in
+  // |max_read_bytes|. If the size of the uncompressed data is bigger than
+  // max_read_bytes then false is returned. |max_read_bytes| must be positive
+  // non-zero.
+  bool ExtractCurrentEntryToRefCountedMemory(
+      int max_read_bytes,
+      base::RefCountedMemory** output) const;

[satorux1, 2014/02/26 04:05:47]

I found this API complex. RefCountedMemory is tricky to use. I'd like it just to
be std::string.

[João Eiras, 2014/02/26 17:35:38]

Tricky in what way ?

I'd have to warn that files inside zip archives can be quite bit. The typical
semantic for std::string is to copy and use them as if they were small pieces of
text, not big blobs like multi-megabyte images. Using RefCountedMemory makes
sure that it is a blob that won't be copied that easily.
It's quite convenient to ask the FILE thread to extract a file from an archive,
and return the contents to the other thread from which the request originated
(ok, ok, using RefCountedString::TakeString is easy).
That is exactly why I preferred this type.  What else would you suggest that
implies the buffer returned is a big blob that should not be trivially copied ?

The same applies to ZipFromMemory: making copies of the input buffers would be
quite overkill. It's quite trivial to wrap a buffer with a RefCountedMemory
object. The other way, converting a raw buffer, a vector or something else to
std::string would require a copy.

[satorux1, 2014/02/27 06:57:00]

Reference counting in general is tricky. It makes the ownership of an object
unclear (i.e. who owns the last reference of that object?), which may cause
hard-to-debug memory related issues (ex. cyclic references, and an object living
too long because of the last reference held unexpectedly).
Not sure if I understand your correctly. If you want to make it uncopiable, can
your client code make it hard to do so?

For instance, suppose we have a function UncompressToMemory() that takes
std::string* as output parameter, you can do something like:

  std::string* output = new std::string;
  PostTaskAndReply(
      BrowserThread::FILE,
      FROM_HERE,
      base::Bind(&UncompressToMemory(file_name, output),
      base::Bind(&DidUncompressToMemory, output));


// This function will receive the uncompressed data in |output|
void DidUncompressToMemory(std::string* raw_output) {
   scoped_ptr<std::string> output(raw_output);
   // Then, |output| owns the uncompressed data and it's uncopiable.
   ...
  
}

This is an idiom often used in Chrome code.

Note that you could also use PostTaskAndReplyWithResult() but the return value
is copied (the compiler may be able to do return value optimization to get rid
of the copy, but it might not).
I'm also confused here. Looking at the code,
ZipReader::ExtractCurrentEntryToRefCountedMemory() internally uses
std::vector<unsigned char> (should probably be std::string) for accumulating
uncompressed data. What's wrong with this function takes std::string* and
accumulate uncompressed data into that string object via the pointer? The
PostTaskAndReply() idiom I presented above would just work fine with this API.

Am I misunderstanding something?

[satorux1, 2014/02/27 16:39:03]

Maybe your concern was when the input data was big? If so, scoped_ptr<> can also
solve that problem. Suppose we have CompressFromMemoryToMemory() which takes a
large blog as the input data, you can do:


   // input_data is a scoped_ptr<std::string>
   std::string* output = new std::string;
   PostTaskAndReply(
       BrowserThread::FILE,
       FROM_HERE,
       base::Bind(&CompressFromMemoryToMemory(base::Passed(&input_data),
output),
       base::Bind(&DidCompressFromMemoryToMemory, output));

void CompressFromMemory(scoped_ptr<std::string>input_data, std::string* output)
{
   ...
}

Using base::Passed(&input_data), the input data is passed to the callback of
CompressFromMemoryToMemory() without copying the large data.
 

+  bool ExtractCurrentEntryToRefCountedMemory(
+      int max_read_bytes,
+      scoped_refptr<base::RefCountedMemory>* output) const;

[satorux1, 2014/02/26 04:05:47]

scoped_refptr<> should only be used when an object belongs to multiple threads.
I think scoped_refptr<> should be avoided here.

Besides, function overloading is not allowed.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

+
   // Returns the current entry info. Returns NULL if the current entry is
   // not yet opened. OpenCurrentEntryInZip() must be called beforehand.
   EntryInfo* current_entry_info() const {