[Paint Preview] Add file compression

This CL adds file compression ability to the FileManager and switches
from last-accessed to last-modified times.

As per discussions lifetime will be tied to other existing lifetime
management systems: history or tab lifetime. So the method for deletion
based on time is no longer required.

Bug: 1010042
Change-Id: Ic8cc956fcee2476f52043edd646e24d399a5815d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1862707Reviewed-by: Mike Klein <mtklein@chromium.org>
Reviewed-by: Ian Vollick <vollick@chromium.org>
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713207}

components/paint_preview/browser/BUILD.gn

@@ -16,6 +16,7 @@ if (!is_ios) {
     deps = [
       "//base",
       "//cc/base",
+      "//third_party/zlib/google:zip",
       "//ui/gfx/geometry",
       "//url",
     ]

components/paint_preview/browser/DEPS

 include_rules = [
   "+cc/base",
+  "+third_party/zlib/google",
 ]

components/paint_preview/browser/file_manager.cc

@@ -9,12 +9,13 @@
 #include "base/hash/hash.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
+#include "third_party/zlib/google/zip.h"
 
 namespace paint_preview {
 
 namespace {
 
-constexpr char kPaintPreviewDirname[] = "paint_preview";
+constexpr char kZipExt[] = ".zip";
 
 std::string HashToHex(const GURL& url) {
   uint32_t hash = base::PersistentHash(url.spec());
@@ -24,73 +25,145 @@ std::string HashToHex(const GURL& url) {
 }  // namespace
 
 FileManager::FileManager(const base::FilePath& root_directory)
-    : root_directory_(root_directory.AppendASCII(kPaintPreviewDirname)) {}
+    : root_directory_(root_directory) {}
 FileManager::~FileManager() = default;
 
 size_t FileManager::GetSizeOfArtifactsFor(const GURL& url) {
-  return base::ComputeDirectorySize(
-      root_directory_.AppendASCII(HashToHex(url)));
+  base::FilePath path;
+  StorageType storage_type = GetPathForUrl(url, &path);
+  switch (storage_type) {
+    case kDirectory: {
+      return base::ComputeDirectorySize(
+          root_directory_.AppendASCII(HashToHex(url)));
+    }
+    case kZip: {
+      int64_t file_size = 0;
+      if (!base::GetFileSize(path, &file_size) || file_size < 0)
+        return 0;
+      return file_size;
+    }
+    case kNone:  // fallthrough
+    default:
+      return 0;
+  }
 }
 
 bool FileManager::GetCreatedTime(const GURL& url, base::Time* created_time) {
+  base::FilePath path;
+  StorageType storage_type = GetPathForUrl(url, &path);
+  if (storage_type == FileManager::StorageType::kNone)
+    return false;
   base::File::Info info;
-  if (!base::GetFileInfo(root_directory_.AppendASCII(HashToHex(url)), &info))
+  if (!base::GetFileInfo(path, &info))
     return false;
   *created_time = info.creation_time;
   return true;
 }
 
-bool FileManager::GetLastAccessedTime(const GURL& url,
-                                      base::Time* last_accessed_time) {
-  return LastAccessedTimeInternal(root_directory_.AppendASCII(HashToHex(url)),
-                                  last_accessed_time);
+bool FileManager::GetLastModifiedTime(const GURL& url,
+                                      base::Time* last_modified_time) {
+  base::FilePath path;
+  StorageType storage_type = GetPathForUrl(url, &path);
+  if (storage_type == FileManager::StorageType::kNone)
+    return false;
+  base::File::Info info;
+  if (!base::GetFileInfo(path, &info))
+    return false;
+  *last_modified_time = info.last_modified;
+  return true;
 }
 
 bool FileManager::CreateOrGetDirectoryFor(const GURL& url,
                                           base::FilePath* directory) {
-  base::FilePath path = root_directory_.AppendASCII(HashToHex(url));
-  base::File::Error error = base::File::FILE_OK;
-  if (base::CreateDirectoryAndGetError(path, &error)) {
-    *directory = path;
-    return true;
+  base::FilePath path;
+  StorageType storage_type = GetPathForUrl(url, &path);
+  switch (storage_type) {
+    case kNone: {
+      base::FilePath new_path = root_directory_.AppendASCII(HashToHex(url));
+      base::File::Error error = base::File::FILE_OK;
+      if (base::CreateDirectoryAndGetError(new_path, &error)) {
+        *directory = new_path;
+        return true;
+      }
+      DVLOG(1) << "ERROR: failed to create directory: " << path
+               << " with error code " << error;
+      return false;
+    }
+    case kDirectory: {
+      *directory = path;
+      return true;
+    }
+    case kZip: {
+      base::FilePath dst_path = root_directory_.AppendASCII(HashToHex(url));
+      base::File::Error error = base::File::FILE_OK;
+      if (!base::CreateDirectoryAndGetError(dst_path, &error)) {
+        DVLOG(1) << "ERROR: failed to create directory: " << path
+                 << " with error code " << error;
+        return false;
+      }
+      if (!zip::Unzip(path, dst_path)) {
+        DVLOG(1) << "ERROR: failed to unzip: " << path << " to " << dst_path;
+        return false;
+      }
+      base::DeleteFile(path, true);
+      *directory = dst_path;
+      return true;
+    }
+    default:
+      return false;
+  }
+}
+
+bool FileManager::CompressDirectoryFor(const GURL& url) {
+  base::FilePath path;
+  StorageType storage_type = GetPathForUrl(url, &path);
+  switch (storage_type) {
+    case kDirectory: {
+      // If there are no files in the directory, zip will succeed, but unzip
+      // will not. Thus don't compress since there is no point.
+      if (!base::ComputeDirectorySize(path))
+        return false;
+      base::FilePath dst_path = path.AddExtensionASCII(kZipExt);
+      if (!zip::Zip(path, dst_path, /* hidden files */ true))
+        return false;
+      base::DeleteFile(path, true);
+      return true;
+    }
+    case kZip:
+      return true;
+    case kNone:  // fallthrough
+    default:
+      return false;
   }
-  DLOG(ERROR) << "Error failed to create directory: " << path
-              << " with error code " << error;
-  return false;
 }
 
 void FileManager::DeleteArtifactsFor(const std::vector<GURL>& urls) {
-  for (const auto& url : urls)
-    base::DeleteFileRecursively(root_directory_.AppendASCII(HashToHex(url)));
+  for (const auto& url : urls) {
+    base::FilePath path;
+    StorageType storage_type = GetPathForUrl(url, &path);
+    if (storage_type == FileManager::StorageType::kNone)
+      continue;
+    base::DeleteFile(path, true);
+  }
 }
 
 void FileManager::DeleteAll() {
   base::DeleteFileRecursively(root_directory_);
 }
 
-void FileManager::DeleteAllOlderThan(base::Time deletion_time) {
-  std::vector<base::FilePath> dirs_to_delete;
-  base::FileEnumerator enumerator(root_directory_, false,
-                                  base::FileEnumerator::DIRECTORIES);
-  base::Time last_accessed_time;
-  for (base::FilePath dir = enumerator.Next(); !dir.empty();
-       dir = enumerator.Next()) {
-    if (!LastAccessedTimeInternal(dir, &last_accessed_time))
-      continue;
-    if (last_accessed_time < deletion_time)
-      dirs_to_delete.push_back(dir);
+FileManager::StorageType FileManager::GetPathForUrl(const GURL& url,
+                                                    base::FilePath* path) {
+  base::FilePath directory_path = root_directory_.AppendASCII(HashToHex(url));
+  if (base::PathExists(directory_path)) {
+    *path = directory_path;
+    return kDirectory;
   }
-  for (const auto& dir : dirs_to_delete)
-    base::DeleteFileRecursively(dir);
-}
-
-bool FileManager::LastAccessedTimeInternal(const base::FilePath& path,
-                                           base::Time* last_accessed_time) {
-  base::File::Info info;
-  if (!base::GetFileInfo(path, &info))
-    return false;
-  *last_accessed_time = info.last_accessed;
-  return true;
+  base::FilePath zip_path = directory_path.AddExtensionASCII(kZipExt);
+  if (base::PathExists(zip_path)) {
+    *path = zip_path;
+    return kZip;
+  }
+  return kNone;
 }
 
 }  // namespace paint_preview

components/paint_preview/browser/file_manager.h

@@ -16,33 +16,42 @@ namespace paint_preview {
 // user profile).
 class FileManager {
  public:
-  // Create a file manager for |root_directory|/paint_previews
+  // Create a file manager for |root_directory|. Top level items in
+  // |root_directoy| should be exclusively managed by this class. Items within
+  // the subdirectories it creates can be freely modified.
   explicit FileManager(const base::FilePath& root_directory);
   ~FileManager();
 
   // Get statistics about the time of creation and size of artifacts.
   size_t GetSizeOfArtifactsFor(const GURL& url);
   bool GetCreatedTime(const GURL& url, base::Time* created_time);
-  bool GetLastAccessedTime(const GURL& url, base::Time* last_accessed_time);
+  bool GetLastModifiedTime(const GURL& url, base::Time* last_modified_time);
 
-  // Creates or gets a subdirectory under |root_directory|/paint_previews/
-  // for |url| and assigns it to |directory|. Returns true and modifies
-  // |directory| on success.
+  // Creates or gets a subdirectory under |root_directory|/ for |url| and
+  // assigns it to |directory|. Returns true on success. If the directory was
+  // compressed then it is uncompressed automatically.
   bool CreateOrGetDirectoryFor(const GURL& url, base::FilePath* directory);
 
+  // Compresses the directory associated with |url|. Returns true on success or
+  // if the directory was already compressed.
+  // NOTE: an empty directory or a directory containing only empty
+  // files/directories will not compress.
+  bool CompressDirectoryFor(const GURL& url);
+
   // Deletes artifacts associated with |urls|.
   void DeleteArtifactsFor(const std::vector<GURL>& urls);
 
   // Deletes all stored paint previews stored in the |profile_directory_|.
   void DeleteAll();
 
-  // Deletes all captures with access times older than |deletion_time|. Slow and
-  // blocking as it relies on base::FileEnumerator.
-  void DeleteAllOlderThan(base::Time deletion_time);
-
  private:
-  bool LastAccessedTimeInternal(const base::FilePath& path,
-                                base::Time* last_accessed_time);
+  enum StorageType {
+    kNone = 0,
+    kDirectory = 1,
+    kZip = 2,
+  };
+
+  StorageType GetPathForUrl(const GURL& url, base::FilePath* path);
 
   base::FilePath root_directory_;
 

components/paint_preview/browser/file_manager_unittest.cc

@@ -18,17 +18,19 @@ TEST(FileManagerTest, TestStats) {
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   FileManager manager(temp_dir.GetPath());
   GURL url("https://www.chromium.org");
+  GURL missing_url("https://www.muimorhc.org");
   base::FilePath directory;
   EXPECT_TRUE(manager.CreateOrGetDirectoryFor(url, &directory));
   EXPECT_FALSE(directory.empty());
 
   base::Time created_time;
+  EXPECT_FALSE(manager.GetCreatedTime(missing_url, &created_time));
   EXPECT_TRUE(manager.GetCreatedTime(url, &created_time));
 
   base::TouchFile(directory, now - base::TimeDelta::FromSeconds(1),
                   now - base::TimeDelta::FromSeconds(1));
   base::Time accessed_time;
-  EXPECT_TRUE(manager.GetLastAccessedTime(url, &accessed_time));
+  EXPECT_TRUE(manager.GetLastModifiedTime(url, &accessed_time));
   base::FilePath proto_path = directory.AppendASCII("paint_preview.pb");
   base::File file(proto_path,
                   base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
@@ -39,7 +41,8 @@ TEST(FileManagerTest, TestStats) {
   base::TouchFile(directory, now + base::TimeDelta::FromSeconds(1),
                   now + base::TimeDelta::FromSeconds(1));
   base::Time later_accessed_time;
-  EXPECT_TRUE(manager.GetLastAccessedTime(url, &later_accessed_time));
+  EXPECT_FALSE(manager.GetLastModifiedTime(missing_url, &created_time));
+  EXPECT_TRUE(manager.GetLastModifiedTime(url, &later_accessed_time));
   EXPECT_GT(later_accessed_time, accessed_time);
 
   EXPECT_GE(manager.GetSizeOfArtifactsFor(url), kSize);
@@ -50,39 +53,113 @@ TEST(FileManagerTest, TestCreateOrGetDirectory) {
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   FileManager manager(temp_dir.GetPath());
   GURL url("https://www.chromium.org");
+
+  // Create a new directory.
   base::FilePath new_directory;
   EXPECT_TRUE(manager.CreateOrGetDirectoryFor(url, &new_directory));
   EXPECT_FALSE(new_directory.empty());
   EXPECT_TRUE(base::PathExists(new_directory));
-  base::FilePath old_directory;
-  EXPECT_TRUE(manager.CreateOrGetDirectoryFor(url, &old_directory));
-  EXPECT_FALSE(old_directory.empty());
-  EXPECT_EQ(old_directory, new_directory);
-  EXPECT_TRUE(base::PathExists(old_directory));
+
+  // Open an existing directory.
+  base::FilePath existing_directory;
+  EXPECT_TRUE(manager.CreateOrGetDirectoryFor(url, &existing_directory));
+  EXPECT_FALSE(existing_directory.empty());
+  EXPECT_EQ(existing_directory, new_directory);
+  EXPECT_TRUE(base::PathExists(existing_directory));
+
+  // A file needs to exist for compression to work.
+  base::FilePath test_file_path = existing_directory.AppendASCII("foo.txt");
+  {
+    base::File file(test_file_path,
+                    base::File::FLAG_CREATE | base::File::FLAG_WRITE);
+    const std::string data = "foobarbaz";
+    file.WriteAtCurrentPos(data.data(), data.size());
+  }
+  EXPECT_TRUE(base::PathExists(test_file_path));
+  base::FilePath test_file_path_empty =
+      existing_directory.AppendASCII("bar.txt");
+  {
+    base::File file(test_file_path_empty,
+                    base::File::FLAG_CREATE | base::File::FLAG_WRITE);
+  }
+  EXPECT_TRUE(base::PathExists(test_file_path_empty));
+
+  // Compress.
+  base::FilePath zip_path = existing_directory.AddExtensionASCII(".zip");
+  EXPECT_TRUE(manager.CompressDirectoryFor(url));
+  EXPECT_FALSE(base::PathExists(existing_directory));
+  EXPECT_FALSE(base::PathExists(test_file_path));
+  EXPECT_FALSE(base::PathExists(test_file_path_empty));
+  EXPECT_TRUE(base::PathExists(zip_path));
+  EXPECT_GT(manager.GetSizeOfArtifactsFor(url), 0U);
+  existing_directory.clear();
+
+  // Open a compressed file.
+  EXPECT_TRUE(manager.CreateOrGetDirectoryFor(url, &existing_directory));
+  EXPECT_EQ(existing_directory, new_directory);
+  EXPECT_TRUE(base::PathExists(existing_directory));
+  EXPECT_TRUE(base::PathExists(test_file_path));
+  EXPECT_TRUE(base::PathExists(test_file_path_empty));
+  EXPECT_FALSE(base::PathExists(zip_path));
+}
+
+TEST(FileManagerTest, TestCompressDirectory) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  FileManager manager(temp_dir.GetPath());
+  GURL url("https://www.chromium.org");
+
+  base::FilePath new_directory;
+  EXPECT_TRUE(manager.CreateOrGetDirectoryFor(url, &new_directory));
+  EXPECT_FALSE(new_directory.empty());
+  EXPECT_TRUE(base::PathExists(new_directory));
+
+  // Compression fails without valid contents.
+  base::FilePath zip_path = new_directory.AddExtensionASCII(".zip");
+  EXPECT_FALSE(manager.CompressDirectoryFor(url));
+  EXPECT_TRUE(base::PathExists(new_directory));
+  EXPECT_FALSE(base::PathExists(zip_path));
+
+  // Create a file with contents for compression to work.
+  {
+    base::File file(new_directory.AppendASCII("foo.txt"),
+                    base::File::FLAG_CREATE | base::File::FLAG_WRITE);
+    const std::string data = "foobarbaz";
+    file.WriteAtCurrentPos(data.data(), data.size());
+  }
+
+  EXPECT_TRUE(manager.CompressDirectoryFor(url));
+  EXPECT_FALSE(base::PathExists(new_directory));
+  EXPECT_TRUE(base::PathExists(zip_path));
 }
 
 TEST(FileManagerTest, TestDeleteArtifactsFor) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
   FileManager manager(temp_dir.GetPath());
+
   GURL cr_url("https://www.chromium.org");
   base::FilePath cr_directory;
   EXPECT_TRUE(manager.CreateOrGetDirectoryFor(cr_url, &cr_directory));
   EXPECT_FALSE(cr_directory.empty());
   EXPECT_TRUE(base::PathExists(cr_directory));
+
   GURL w3_url("https://www.w3.org");
   base::FilePath w3_directory;
   EXPECT_TRUE(manager.CreateOrGetDirectoryFor(w3_url, &w3_directory));
   EXPECT_FALSE(w3_directory.empty());
   EXPECT_TRUE(base::PathExists(w3_directory));
+
   manager.DeleteArtifactsFor(std::vector<GURL>({cr_url}));
   EXPECT_FALSE(base::PathExists(cr_directory));
   EXPECT_TRUE(base::PathExists(w3_directory));
+
   base::FilePath new_cr_directory;
   EXPECT_TRUE(manager.CreateOrGetDirectoryFor(cr_url, &new_cr_directory));
   EXPECT_FALSE(new_cr_directory.empty());
   EXPECT_TRUE(base::PathExists(new_cr_directory));
   EXPECT_EQ(cr_directory, new_cr_directory);
+
   manager.DeleteArtifactsFor(std::vector<GURL>({cr_url, w3_url}));
   EXPECT_FALSE(base::PathExists(new_cr_directory));
   EXPECT_FALSE(base::PathExists(w3_directory));
@@ -107,29 +184,4 @@ TEST(FileManagerTest, TestDeleteAll) {
   EXPECT_FALSE(base::PathExists(w3_directory));
 }
 
-TEST(FileManagerTest, TestDeleteAllOlderThan) {
-  base::Time now = base::Time::Now();
-  base::ScopedTempDir temp_dir;
-  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-  FileManager manager(temp_dir.GetPath());
-  GURL cr_url("https://www.chromium.org");
-  base::FilePath cr_directory;
-  EXPECT_TRUE(manager.CreateOrGetDirectoryFor(cr_url, &cr_directory));
-  EXPECT_FALSE(cr_directory.empty());
-  EXPECT_TRUE(base::PathExists(cr_directory));
-  base::TouchFile(cr_directory, now - base::TimeDelta::FromSeconds(1),
-                  now - base::TimeDelta::FromSeconds(1));
-
-  GURL w3_url("https://www.w3.org");
-  base::FilePath w3_directory;
-  EXPECT_TRUE(manager.CreateOrGetDirectoryFor(w3_url, &w3_directory));
-  EXPECT_FALSE(w3_directory.empty());
-  EXPECT_TRUE(base::PathExists(w3_directory));
-  base::TouchFile(w3_directory, now + base::TimeDelta::FromSeconds(1),
-                  now + base::TimeDelta::FromSeconds(1));
-  manager.DeleteAllOlderThan(now);
-  EXPECT_FALSE(base::PathExists(cr_directory));
-  EXPECT_TRUE(base::PathExists(w3_directory));
-}
-
 }  // namespace paint_preview