Adds asynchronous unzip functions to ZipReader

third_party/zlib/google/zip_reader_unittest.cc

Index: third_party/zlib/google/zip_reader_unittest.cc
diff --git a/third_party/zlib/google/zip_reader_unittest.cc b/third_party/zlib/google/zip_reader_unittest.cc
index 09efd9a7a669ec0707853111f8394ed9998a61d9..304d41d221475e7555e46af65561e5dd6658ed77 100644
--- a/third_party/zlib/google/zip_reader_unittest.cc
+++ b/third_party/zlib/google/zip_reader_unittest.cc
@@ -7,12 +7,14 @@
 #include <set>
 #include <string>
 
+#include "base/bind.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "base/md5.h"
 #include "base/path_service.h"
 #include "base/platform_file.h"
+#include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -21,6 +23,8 @@
 
 namespace {
 
+const static std::string kQuuxExpectedMD5 = "d1ae4ac8a17a0e09317113ab284b57a6";
+
 // Wrap PlatformFiles in a class so that we don't leak them in tests.
 class PlatformFileWrapper {
  public:
@@ -61,6 +65,36 @@ class PlatformFileWrapper {
   base::PlatformFile file_;
 };
 
+class MockUnzipListener : public base::SupportsWeakPtr<MockUnzipListener> {

[satorux1, 2013/12/11 06:45:11]

Please add a class comment.

[Drew Haven, 2013/12/11 18:20:53]

Done.

+ public:
+  MockUnzipListener() 
+      : success_calls(0),
+        failure_calls(0),
+        progress_calls(0),
+        current_progress(0) {
+  }
+
+  void OnUnzipSuccess() {

[satorux1, 2013/12/11 06:45:11]

function comment is missing. maybe something like

// Used as success_callback for ExtractCurrentEntryToFilePathAsync()

[Drew Haven, 2013/12/11 18:20:53]

Done.

+    success_calls++;
+  }
+
+  void OnUnzipFailure() {
+    failure_calls++;
+  }
+
+  void OnUnzipProgress(int64 progress) {
+    progress_calls++;
+    EXPECT_GT(progress, current_progress);

[satorux1, 2013/12/11 06:45:11]

This may be a matter of taste, but I think EXPECT* should usually only be used
in TEST functions (logic: test failures from EXPECT* in random places are hard
to debug, hence should usually be avoided).

Here, maybe you can make it a DCHECK.

[Drew Haven, 2013/12/11 18:20:53]

True.  At least a DCHECK gives you a nice stack trace.

+    current_progress = progress;
+  }
+
+  int success_calls;

[satorux1, 2013/12/11 06:45:11]

success_calls_

[Drew Haven, 2013/12/11 18:20:53]

Done.

+  int failure_calls;
+  int progress_calls;
+
+  int64 current_progress;

[satorux1, 2013/12/11 06:45:11]

member variables should be private. Please add getters for them.

[Drew Haven, 2013/12/11 18:20:53]

Done.

I'm really confused why this is preferred over an eight-line GMock instance. 
Granted, with the new setup GMock wouldn't work as well, but assuming we were
still making a listener subclass.

[satorux1, 2013/12/12 08:13:01]

Non-GMock code is a bit more verbose but anyone can read. GMock version is more
succinct but harder to read for someone who's not familiar with GMock. I think
it's a reasonable choice not to use GMock in a project where not all folks are
familiar with GMock and some folks feel very strongly about use of GMock. :)

+};
+
 }   // namespace
 
 namespace zip {
@@ -113,6 +147,15 @@ class ZipReaderTest : public PlatformTest {
     return true;
   }
 
+  void CompareFileAndMD5(const base::FilePath& path,
+                         const std::string expected_md5) {
+    // Read the output file and compute the MD5.
+    std::string output;
+    ASSERT_TRUE(base::ReadFileToString(path, &output));
+    const std::string md5 = base::MD5String(output);
+    EXPECT_EQ(expected_md5, md5);

[satorux1, 2013/12/11 06:45:11]

Instead of calling ASSERT and EXPECT here, please return a boolean and do

EXPECT_TRUE(CompareFileAndMD5(...);

This way is nicer because the line number in the test failure report will point
us to the call site of CompareFileAndMD5(). In this file, there is only one call
site of CompareFileAndMD5(), but if there are many, the line number info is very
helpful.

[Drew Haven, 2013/12/11 18:20:53]

Done.

+  }
+
   // The path to temporary directory used to contain the test operations.
   base::FilePath test_dir_;
   // The path to the test data directory where test.zip etc. are located.
@@ -128,6 +171,8 @@ class ZipReaderTest : public PlatformTest {
   std::set<base::FilePath> test_zip_contents_;
 
   base::ScopedTempDir temp_dir_;
+
+  base::MessageLoop message_loop_;
 };
 
 TEST_F(ZipReaderTest, Open_ValidZipFile) {
@@ -220,8 +265,7 @@ TEST_F(ZipReaderTest, ExtractCurrentEntryToFilePath_RegularFile) {
   ASSERT_TRUE(base::ReadFileToString(test_dir_.AppendASCII("quux.txt"),
                                      &output));
   const std::string md5 = base::MD5String(output);
-  const std::string kExpectedMD5 = "d1ae4ac8a17a0e09317113ab284b57a6";
-  EXPECT_EQ(kExpectedMD5, md5);
+  EXPECT_EQ(kQuuxExpectedMD5, md5);
   // quux.txt should be larger than kZipBufSize so that we can exercise
   // the loop in ExtractCurrentEntry().
   EXPECT_LT(static_cast<size_t>(internal::kZipBufSize), output.size());
@@ -241,8 +285,7 @@ TEST_F(ZipReaderTest, PlatformFileExtractCurrentEntryToFilePath_RegularFile) {
   ASSERT_TRUE(base::ReadFileToString(test_dir_.AppendASCII("quux.txt"),
                                      &output));
   const std::string md5 = base::MD5String(output);
-  const std::string kExpectedMD5 = "d1ae4ac8a17a0e09317113ab284b57a6";
-  EXPECT_EQ(kExpectedMD5, md5);
+  EXPECT_EQ(kQuuxExpectedMD5, md5);
   // quux.txt should be larger than kZipBufSize so that we can exercise
   // the loop in ExtractCurrentEntry().
   EXPECT_LT(static_cast<size_t>(internal::kZipBufSize), output.size());
@@ -264,8 +307,7 @@ TEST_F(ZipReaderTest, PlatformFileExtractCurrentEntryToFd_RegularFile) {
   ASSERT_TRUE(base::ReadFileToString(test_dir_.AppendASCII("quux.txt"),
                                      &output));
   const std::string md5 = base::MD5String(output);
-  const std::string kExpectedMD5 = "d1ae4ac8a17a0e09317113ab284b57a6";
-  EXPECT_EQ(kExpectedMD5, md5);
+  EXPECT_EQ(kQuuxExpectedMD5, md5);
   // quux.txt should be larger than kZipBufSize so that we can exercise
   // the loop in ExtractCurrentEntry().
   EXPECT_LT(static_cast<size_t>(internal::kZipBufSize), output.size());
@@ -296,8 +338,7 @@ TEST_F(ZipReaderTest, ExtractCurrentEntryIntoDirectory_RegularFile) {
   ASSERT_TRUE(base::ReadFileToString(
       test_dir_.AppendASCII("foo/bar/quux.txt"), &output));
   const std::string md5 = base::MD5String(output);
-  const std::string kExpectedMD5 = "d1ae4ac8a17a0e09317113ab284b57a6";
-  EXPECT_EQ(kExpectedMD5, md5);
+  EXPECT_EQ(kQuuxExpectedMD5, md5);
 }
 
 TEST_F(ZipReaderTest, current_entry_info_RegularFile) {
@@ -428,4 +469,64 @@ TEST_F(ZipReaderTest, OpenFromString) {
   EXPECT_EQ(std::string("This is a test.\n"), actual);
 }
 
+// Verifies that the asynchronous extraction to a file works.
+TEST_F(ZipReaderTest, ExtractToFileAsync_RegularFile) {
+  MockUnzipListener listener;
+
+  ZipReader reader;
+  base::FilePath target_file = test_dir_.AppendASCII("quux.txt");
+  base::FilePath target_path(FILE_PATH_LITERAL("foo/bar/quux.txt"));
+  ASSERT_TRUE(reader.Open(test_zip_file_));
+  ASSERT_TRUE(reader.LocateAndOpenEntry(target_path));
+  reader.ExtractCurrentEntryToFilePathAsync(
+      target_file,
+      base::Bind(&MockUnzipListener::OnUnzipSuccess,
+                 listener.AsWeakPtr()),
+      base::Bind(&MockUnzipListener::OnUnzipFailure,
+                 listener.AsWeakPtr()),
+      base::Bind(&MockUnzipListener::OnUnzipProgress,
+                 listener.AsWeakPtr()));
+
+  EXPECT_EQ(0, listener.success_calls);
+  EXPECT_EQ(0, listener.failure_calls);
+  EXPECT_EQ(0, listener.progress_calls);
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, listener.success_calls);
+  EXPECT_EQ(0, listener.failure_calls);
+  EXPECT_GE(listener.progress_calls, 1);
+
+  CompareFileAndMD5(target_file, kQuuxExpectedMD5);
+
+  int64 file_size;

[satorux1, 2013/12/11 06:45:11]

= 0;

to make it initialized. doesn't matter here, but it's a good practice to avoid
uninitialized variables 

[Drew Haven, 2013/12/11 18:20:53]

Done.

+  ASSERT_TRUE(base::GetFileSize(target_file, &file_size));
+
+  EXPECT_EQ(listener.current_progress, file_size);
+}
+
+// Verifies that the asynchronous extraction to a file works.
+TEST_F(ZipReaderTest, ExtractToFileAsync_Directory) {
+  MockUnzipListener listener;
+
+  ZipReader reader;
+  base::FilePath target_path(FILE_PATH_LITERAL("foo/"));
+  ASSERT_TRUE(reader.Open(test_zip_file_));
+  ASSERT_TRUE(reader.LocateAndOpenEntry(target_path));
+  reader.ExtractCurrentEntryToFilePathAsync(
+      target_path,
+      base::Bind(&MockUnzipListener::OnUnzipSuccess,
+                 listener.AsWeakPtr()),
+      base::Bind(&MockUnzipListener::OnUnzipFailure,
+                 listener.AsWeakPtr()),
+      base::Bind(&MockUnzipListener::OnUnzipProgress,
+                 listener.AsWeakPtr()));
+
+  EXPECT_EQ(1, listener.success_calls);
+  EXPECT_EQ(0, listener.failure_calls);
+  EXPECT_GE(0, listener.progress_calls);
+
+  ASSERT_TRUE(base::DirectoryExists(target_path));
+}
+
 }  // namespace zip