[ZipFileCreator] Recursively zip folders

The ZipFileCreator service now recursively zips folders. Files App
doesn't need to enumerate all the files in folders to zip anymore. This
is done in a much more efficient way by ZipFileCreator.

When zipping a folder containing 100 subfolders with 1000 small
files each (for a total of 100,000 small files) on a kevin device,
the old system takes 144 seconds (listing files 103 seconds + zipping
files 41 seconds), whereas the new system takes only 26 seconds. This
is an improvement by a factor of 5.5.

BUG=chromium:912236
TEST=out/release/zlib_unittests
TEST=out/release/browser_tests --gtest_filter="ZipFileCreatorTest*

Change-Id: I049e25fbc365528c4d9ec12ac2afc58b4d036046
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2912233
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: François Degros <fdegros@chromium.org>
Cr-Commit-Position: refs/heads/master@{#886233}

chrome/services/file_util/public/cpp/zip_file_creator.cc

@@ -31,18 +31,19 @@ void BindDirectoryInBackground(
 
 }  // namespace
 
-ZipFileCreator::ZipFileCreator(
-    ResultCallback callback,
-    const base::FilePath& src_dir,
-    const std::vector<base::FilePath>& src_relative_paths,
-    const base::FilePath& dest_file)
+ZipFileCreator::ZipFileCreator(ResultCallback callback,
+                               base::FilePath src_dir,
+                               std::vector<base::FilePath> src_relative_paths,
+                               base::FilePath dest_file)
     : callback_(std::move(callback)),
-      src_dir_(src_dir),
-      src_relative_paths_(src_relative_paths),
-      dest_file_(dest_file) {
+      src_dir_(std::move(src_dir)),
+      src_relative_paths_(std::move(src_relative_paths)),
+      dest_file_(std::move(dest_file)) {
   DCHECK(callback_);
 }
 
+ZipFileCreator::~ZipFileCreator() = default;
+
 void ZipFileCreator::Start(
     mojo::PendingRemote<chrome::mojom::FileUtilService> service) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -56,8 +57,6 @@ void ZipFileCreator::Start(
                      std::move(service)));
 }
 
-ZipFileCreator::~ZipFileCreator() = default;
-
 void ZipFileCreator::CreateZipFile(
     mojo::PendingRemote<chrome::mojom::FileUtilService> service,
     base::File file) {
@@ -89,7 +88,7 @@ void ZipFileCreator::CreateZipFile(
   remote_zip_file_creator_.set_disconnect_handler(base::BindOnce(
       &ZipFileCreator::ReportDone, base::Unretained(this), false));
   remote_zip_file_creator_->CreateZipFile(
-      std::move(directory), src_dir_, src_relative_paths_, std::move(file),
+      std::move(directory), src_relative_paths_, std::move(file),
       base::BindOnce(&ZipFileCreator::ReportDone, base::Unretained(this)));
 }
 

chrome/services/file_util/public/cpp/zip_file_creator.h

@@ -28,9 +28,9 @@ class ZipFileCreator {
 
   // Creates a zip file from the specified list of files and directories.
   ZipFileCreator(ResultCallback callback,
-                 const base::FilePath& src_dir,
-                 const std::vector<base::FilePath>& src_relative_paths,
-                 const base::FilePath& dest_file);
+                 base::FilePath src_dir,
+                 std::vector<base::FilePath> src_relative_paths,
+                 base::FilePath dest_file);
 
   // Starts creating the zip file.
   //

chrome/services/file_util/public/cpp/zip_file_creator_browsertest.cc

@@ -94,13 +94,9 @@ IN_PROC_BROWSER_TEST_F(ZipFileCreatorTest, SomeFilesZip) {
   bool success = false;
   base::RunLoop run_loop;
 
-  std::vector<base::FilePath> paths;
-  paths.push_back(kDir1);
-  paths.push_back(kFile1);
-  paths.push_back(kFile2);
   (new ZipFileCreator(
        base::BindOnce(&TestCallback, &success, run_loop.QuitClosure()),
-       zip_base_dir(), paths, zip_archive_path()))
+       zip_base_dir(), {kDir1, kFile2}, zip_archive_path()))
       ->Start(LaunchService());
 
   run_loop.Run();
@@ -147,14 +143,14 @@ IN_PROC_BROWSER_TEST_F(ZipFileCreatorTest, ZipDirectoryWithManyFiles) {
   // root_dir/1
   // root_dir/1/1.txt -> Hello1/1
   // ...
-  // root_dir/1/7.txt -> Hello1/7
+  // root_dir/1/70.txt -> Hello1/70
   // root_dir/2
   // root_dir/2/1.txt -> Hello2/1
   // ...
-  // root_dir/2/7.txt -> Hello2/7
+  // root_dir/2/70.txt -> Hello2/70
   //...
   //...
-  // root_dir/10/7.txt -> Hello10/7
+  // root_dir/10/70.txt -> Hello10/70
 
   base::FilePath root_dir = zip_base_dir().Append("root_dir");
 
@@ -174,7 +170,7 @@ IN_PROC_BROWSER_TEST_F(ZipFileCreatorTest, ZipDirectoryWithManyFiles) {
       base::FilePath dir(std::to_string(i));
       ASSERT_TRUE(base::CreateDirectory(root_dir.Append(dir)));
       file_tree_content[dir] = std::string();
-      for (int j = 1; j <= 7; j++) {
+      for (int j = 1; j <= 70; j++) {
         base::FilePath file = dir.Append(std::to_string(j) + ".txt");
         std::string content =
             "Hello" + std::to_string(i) + "/" + std::to_string(j);
@@ -187,15 +183,14 @@ IN_PROC_BROWSER_TEST_F(ZipFileCreatorTest, ZipDirectoryWithManyFiles) {
   // Sanity check on the files created.
   constexpr size_t kEntryCount = 89 /* files under root dir */ +
                                  10 /* 1 to 10 dirs */ +
-                                 10 * 7 /* files under 1 to 10 dirs */;
+                                 10 * 70 /* files under 1 to 10 dirs */;
   DCHECK_EQ(kEntryCount, file_tree_content.size());
 
   bool success = false;
   base::RunLoop run_loop;
   (new ZipFileCreator(
        base::BindOnce(&TestCallback, &success, run_loop.QuitClosure()),
-       root_dir,
-       std::vector<base::FilePath>(),  // Empty means zip everything in dir.
+       root_dir, {},  // Empty means zip everything in root_dir.
        zip_archive_path()))
       ->Start(LaunchService());
 

chrome/services/file_util/public/mojom/zip_file_creator.mojom

@@ -3,7 +3,7 @@
 // found in the LICENSE file.
 
 // Zip file creator provided by the utility process and exposed by mojo
-// policy control to the chrome browser process on OS_CHROMEOS.
+// policy control to the Chrome browser process on OS_CHROMEOS.
 
 module chrome.mojom;
 
@@ -11,10 +11,14 @@ import "components/services/filesystem/public/mojom/directory.mojom";
 import "mojo/public/mojom/base/file.mojom";
 import "mojo/public/mojom/base/file_path.mojom";
 
+// Service that zips files and folders.
 interface ZipFileCreator {
-  // OS_CHROMEOS: Create a |zip_file| from a list of source files.
+  // Creates a ZIP file and adds all the files and directories specified in
+  // |source_relative_paths|.
+  // Folders are recursively explored.
+  // If |source_relative_paths| is empty, then the whole source directory is
+  // zipped.
   CreateZipFile(pending_remote<filesystem.mojom.Directory> source_dir_mojo,
-                mojo_base.mojom.FilePath source_dir,
                 array<mojo_base.mojom.FilePath> source_relative_paths,
                 mojo_base.mojom.File zip_file)
       => (bool success);

chrome/services/file_util/zip_file_creator.cc

@@ -135,7 +135,6 @@ ZipFileCreator::~ZipFileCreator() = default;
 
 void ZipFileCreator::CreateZipFile(
     mojo::PendingRemote<filesystem::mojom::Directory> source_dir_remote,
-    const base::FilePath& source_dir,
     const std::vector<base::FilePath>& source_relative_paths,
     base::File zip_file,
     CreateZipFileCallback callback) {
@@ -152,7 +151,7 @@ void ZipFileCreator::CreateZipFile(
 
   MojoFileAccessor file_accessor(std::move(source_dir_remote));
   const bool success = zip::Zip({
-      .src_dir = source_dir,
+      .file_accessor = &file_accessor,
       .dest_fd = zip_file.GetPlatformFile(),
       .src_files = source_relative_paths,
       .progress_callback =
@@ -161,7 +160,7 @@ void ZipFileCreator::CreateZipFile(
             return true;
           }),
       .progress_period = base::TimeDelta::FromMilliseconds(500),
-      .file_accessor = &file_accessor,
+      .recursive = true,
   });
   std::move(callback).Run(success);
 }

chrome/services/file_util/zip_file_creator.h

@@ -25,7 +25,6 @@ class ZipFileCreator : public chrome::mojom::ZipFileCreator {
   // chrome::mojom::ZipFileCreator:
   void CreateZipFile(
       mojo::PendingRemote<filesystem::mojom::Directory> source_dir_remote,
-      const base::FilePath& source_dir,
       const std::vector<base::FilePath>& source_relative_paths,
       base::File zip_file,
       CreateZipFileCallback callback) override;

third_party/zlib/google/zip.h

@@ -84,9 +84,13 @@ using FilterCallback = base::RepeatingCallback<bool(const base::FilePath&)>;
 
 // ZIP creation parameters and options.
 struct ZipParams {
-  // Source directory.
+  // Source directory. Ignored if |file_accessor| is set.
   base::FilePath src_dir;
 
+  // Abstraction around file system access used to read files.
+  // If left null, an implementation that accesses files directly is used.
+  FileAccessor* file_accessor = nullptr;  // Not owned
+
   // Destination file path.
   // Either dest_file or dest_fd should be set, but not both.
   base::FilePath dest_file;
@@ -126,10 +130,6 @@ struct ZipParams {
 
   // Should recursively add subdirectory contents?
   bool recursive = false;
-
-  // Abstraction around file system access used to read files. If left null, an
-  // implementation that accesses files directly is used.
-  FileAccessor* file_accessor = nullptr;  // Not owned
 };
 
 // Zip files specified into a ZIP archives. The source files and ZIP destination

third_party/zlib/google/zip_unittest.cc

@@ -567,10 +567,8 @@ TEST_F(ZipTest, ZipWithFileAccessor) {
   base::FilePath zip_file;
   ASSERT_TRUE(base::CreateTemporaryFile(&zip_file));
   VirtualFileSystem file_accessor;
-  const zip::ZipParams params{
-      .src_dir = base::FilePath(FILE_PATH_LITERAL("/test")),
-      .dest_file = zip_file,
-      .file_accessor = &file_accessor};
+  const zip::ZipParams params{.file_accessor = &file_accessor,
+                              .dest_file = zip_file};
   ASSERT_TRUE(zip::Zip(params));
 
   base::ScopedTempDir scoped_temp_dir;

ui/file_manager/file_manager/background/js/file_operation_manager_unittest.m.js

@@ -995,8 +995,8 @@ export function testZip(callback) {
         const lastEvent = events[events.length - 1];
         assertEquals('copy-progress', lastEvent.type);
         assertEquals('SUCCESS', lastEvent.reason);
-        assertEquals(10, lastEvent.status.totalBytes);
-        assertEquals(10, lastEvent.status.processedBytes);
+        assertEquals(1, lastEvent.status.totalBytes);
+        assertEquals(1, lastEvent.status.processedBytes);
 
         assertFalse(events.some(event => {
           return event.type === 'delete';

ui/file_manager/file_manager/background/js/file_operation_util.js

@@ -1168,28 +1168,8 @@ fileOperationUtil.ZipTask = class extends fileOperationUtil.Task {
    * @param {function()} callback Called when the initialize is completed.
    */
   initialize(callback) {
-    this.initialize_().finally(callback);
-  }
-
-  /**
-   * @private
-   */
-  async initialize_() {
-    this.totalBytes = 0;
-    const resolvedEntryMap = {};
-
-    for (const sourceEntry of assert(this.sourceEntries)) {
-      const resolvedEntries = await new Promise(
-          (resolve, reject) => fileOperationUtil.resolveRecursively_(
-              sourceEntry, resolve, reject));
-      for (const resolvedEntry of resolvedEntries) {
-        this.totalBytes += resolvedEntry.size;
-        resolvedEntryMap[resolvedEntry.toURL()] = resolvedEntry;
-      }
-    }
-
-    // For ZIP archiving, all the entries are processed at once.
-    this.processingEntries = [resolvedEntryMap];
+    this.totalBytes = this.sourceEntries.length;
+    callback();
   }
 
   /**
@@ -1243,16 +1223,10 @@ fileOperationUtil.ZipTask = class extends fileOperationUtil.Task {
     const destPath = await fileOperationUtil.deduplicatePath(
         this.targetDirEntry, destName + '.zip');
 
-    // The number of elements in processingEntries is 1. See also
-    // initialize().
-    const entries = [];
-    for (const url in this.processingEntries[0]) {
-      entries.push(this.processingEntries[0][url]);
-    }
-
     const success = await new Promise(
         resolve => chrome.fileManagerPrivate.zipSelection(
-            entries, this.zipBaseDirEntry, destPath, resolve));
+            assert(this.sourceEntries), this.zipBaseDirEntry, destPath,
+            resolve));
 
     if (!success) {
       // Cannot create ZIP archive.