New Zip::ZipFromMemory API.

third_party/zlib/google/zip_reader.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 #ifndef THIRD_PARTY_ZLIB_GOOGLE_ZIP_READER_H_
 #define THIRD_PARTY_ZLIB_GOOGLE_ZIP_READER_H_
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/platform_file.h"
 #include "base/time/time.h"
 
 #if defined(USE_SYSTEM_MINIZIP)
 #include <minizip/unzip.h>
 #else
 #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
 // class is to scan entries in a zip file and extract them. The code will
 // look like:
 //
 //   ZipReader reader;
 //   reader.Open(zip_file_path);
 //   while (reader.HasMore()) {
 //     reader.OpenCurrentEntryInZip();
@@ skipping 139 matching lines @@
   // timestamp is not valid, the timestamp will be set to the current time.
   bool ExtractCurrentEntryIntoDirectory(
       const base::FilePath& output_directory_path);
 
 #if defined(OS_POSIX)
   // Extracts the current entry by writing directly to a file descriptor.
   // Does not close the file descriptor. Returns true on success.
   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 {
     return current_entry_info_.get();
   }
 
   // Returns the number of entries in the zip file.
   // Open() must be called beforehand.
   int num_entries() const { return num_entries_; }
 
@@ skipping 18 matching lines @@
   scoped_ptr<EntryInfo> current_entry_info_;
 
   base::WeakPtrFactory<ZipReader> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ZipReader);
 };
 
 }  // namespace zip
 
 #endif  // THIRD_PARTY_ZLIB_GOOGLE_ZIP_READER_H_