[zip] Optimize DirectoryContentEntry

Simple optimizations and code clean-up to avoid copying file paths. BUG=chromium:912236 TEST=autoninja -C out/release zlib_unittests && out/release/zlib_unittests TEST=autoninja -C out/release browser_tests && testing/xvfb.py out/release/browser_tests --gtest_filter="ZipFileCreatorTest* Change-Id: Id58e38d93219c4f18cbdc8c9e9ed6bfc90a4b64b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2901539 Reviewed-by: Noel Gordon <[email protected]> Commit-Queue: François Degros <[email protected]> Cr-Commit-Position: refs/heads/master@{#883817} NOKEYCHECK=True GitOrigin-RevId: de47c67919e498dbb6a64c6481a3052de608c08f
mjfroman · May 18, 2021 · 4c0b44d · 4c0b44d
1 parent b2a0124
commit 4c0b44d
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 20 deletions.
diff --git a/zlib/google/zip.cc b/zlib/google/zip.cc
@@ -50,19 +50,20 @@ std::unique_ptr<WriterDelegate> CreateFilePathWriterDelegate(
 
 class DirectFileAccessor : public FileAccessor {
  public:
-  explicit DirectFileAccessor(base::FilePath src_dir) : src_dir_(src_dir) {}
   ~DirectFileAccessor() override = default;
 
   std::vector<base::File> OpenFilesForReading(
       const std::vector<base::FilePath>& paths) override {
     std::vector<base::File> files;
+
     for (const auto& path : paths) {
       base::File file;
       if (base::PathExists(path) && !base::DirectoryExists(path)) {
         file = base::File(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
       }
       files.push_back(std::move(file));
     }
+
     return files;
   }
 
@@ -73,29 +74,26 @@ class DirectFileAccessor : public FileAccessor {
   std::vector<DirectoryContentEntry> ListDirectoryContent(
       const base::FilePath& dir) override {
     std::vector<DirectoryContentEntry> files;
+
     base::FileEnumerator file_enumerator(
         dir, false /* recursive */,
         base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES);
-    for (base::FilePath path = file_enumerator.Next(); !path.value().empty();
+    for (base::FilePath path = file_enumerator.Next(); !path.empty();
          path = file_enumerator.Next()) {
-      files.push_back(DirectoryContentEntry(path, base::DirectoryExists(path)));
+      const bool is_directory = base::DirectoryExists(path);
+      files.push_back({std::move(path), is_directory});
     }
+
     return files;
   }
 
   base::Time GetLastModifiedTime(const base::FilePath& path) override {
     base::File::Info file_info;
     if (!base::GetFileInfo(path, &file_info)) {
-      LOG(ERROR) << "Failed to retrieve file modification time for "
-                 << path.value();
+      LOG(ERROR) << "Cannot get modification time for '" << path << "'";
     }
     return file_info.last_modified;
   }
-
- private:
-  base::FilePath src_dir_;
-
-  DISALLOW_COPY_AND_ASSIGN(DirectFileAccessor);
 };
 
 }  // namespace
@@ -106,7 +104,7 @@ std::ostream& operator<<(std::ostream& out, const Progress& progress) {
 }
 
 bool Zip(const ZipParams& params) {
-  DirectFileAccessor default_accessor(params.src_dir);
+  DirectFileAccessor default_accessor;
   FileAccessor* const file_accessor = params.file_accessor ?: &default_accessor;
 
   Paths files_to_add = params.src_files;

diff --git a/zlib/google/zip.h b/zlib/google/zip.h
@@ -35,8 +35,6 @@ class FileAccessor {
   virtual ~FileAccessor() = default;
 
   struct DirectoryContentEntry {
-    DirectoryContentEntry(const base::FilePath& path, bool is_directory)
-        : path(path), is_directory(is_directory) {}
     base::FilePath path;
     bool is_directory = false;
   };

diff --git a/zlib/google/zip_unittest.cc b/zlib/google/zip_unittest.cc
@@ -79,13 +79,10 @@ class VirtualFileSystem : public zip::FileAccessor {
     DCHECK(success);
     files_[bar2_txt_path] = std::move(file);
 
-    file_tree_[test_dir] = std::vector<DirectoryContentEntry>{
-        DirectoryContentEntry(foo_txt_path, /*is_dir=*/false),
-        DirectoryContentEntry(bar_dir, /*is_dir=*/true)};
-    file_tree_[bar_dir] = std::vector<DirectoryContentEntry>{
-        DirectoryContentEntry(bar1_txt_path, /*is_dir=*/false),
-        DirectoryContentEntry(bar2_txt_path, /*is_dir=*/false)};
+    file_tree_[test_dir] = {{foo_txt_path, false}, {bar_dir, true}};
+    file_tree_[bar_dir] = {{bar1_txt_path, false}, {bar2_txt_path, false}};
   }
+
   ~VirtualFileSystem() override = default;
 
  private:
@@ -109,7 +106,7 @@ class VirtualFileSystem : public zip::FileAccessor {
     auto iter = file_tree_.find(dir);
     if (iter == file_tree_.end()) {
       NOTREACHED();
-      return std::vector<DirectoryContentEntry>();
+      return {};
     }
     return iter->second;
   }