Image Fetcher: Add CacheStrategy to CachedImageMetadataProto.

CacheStrategy enum is added to the CachedImageMetadataProto to control
the life cycle of cached image files.

Also we plumb the expiration_interval to metadata leveldb. When this
optional has value, the CachedImageMetadataProto will use
CacheStrategy::HOLD_UNTIL_EXPIRED, otherwise use the default
BEST_EFFORT.

Test coverage is added in *ImageMetadataStoreLevelDBTest.Save().

TBR=jianli@chromium.org

Bug: 1067049,1058534
Change-Id: I97080fcc85e3851868ee4c33e2c4b9fca2507744
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2141132
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: Brandon Wylie <wylieb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757660}

chrome/browser/offline_pages/android/prefetch_test_bridge.cc

@@ -63,7 +63,8 @@ JNI_EXPORT void JNI_PrefetchTestBridge_InsertIntoCachedImageFetcher(
   std::string image_data;
   base::android::JavaByteArrayToString(env, j_image_data, &image_data);
 
-  cache->SaveImage(url, image_data, false /* needs_transcoding */);
+  cache->SaveImage(url, image_data, false /* needs_transcoding */,
+                   base::nullopt /* expiration_interval */);
 }
 
 JNI_EXPORT void JNI_PrefetchTestBridge_AddCandidatePrefetchURL(

components/image_fetcher/core/cache/image_cache.cc

@@ -73,15 +73,16 @@ ImageCache::~ImageCache() = default;
 
 void ImageCache::SaveImage(std::string url,
                            std::string image_data,
-                           bool needs_transcoding) {
+                           bool needs_transcoding,
+                           ExpirationInterval expiration_interval) {
   // If the image data is larger than the cache's max size, bail out.
   if (image_data.length() > kCacheMaxSize) {
     return;
   }
 
-  base::OnceClosure request =
-      base::BindOnce(&ImageCache::SaveImageImpl, weak_ptr_factory_.GetWeakPtr(),
-                     url, std::move(image_data), needs_transcoding);
+  base::OnceClosure request = base::BindOnce(
+      &ImageCache::SaveImageImpl, weak_ptr_factory_.GetWeakPtr(), url,
+      std::move(image_data), needs_transcoding, std::move(expiration_interval));
   QueueOrStartRequest(std::move(request));
 }
 
@@ -153,7 +154,8 @@ void ImageCache::OnDependencyInitialized() {
 
 void ImageCache::SaveImageImpl(const std::string& url,
                                std::string image_data,
-                               bool needs_transcoding) {
+                               bool needs_transcoding,
+                               ExpirationInterval expiration_interval) {
   std::string key = ImageCache::HashUrlToKey(url);
 
   // If the cache is full, evict some stuff.
@@ -161,7 +163,8 @@ void ImageCache::SaveImageImpl(const std::string& url,
 
   size_t length = image_data.length();
   data_store_->SaveImage(key, std::move(image_data), needs_transcoding);
-  metadata_store_->SaveImageMetadata(key, length, needs_transcoding);
+  metadata_store_->SaveImageMetadata(key, length, needs_transcoding,
+                                     std::move(expiration_interval));
 }
 
 void ImageCache::LoadImageImpl(bool read_only,

components/image_fetcher/core/cache/image_cache.h

@@ -46,7 +46,8 @@ class ImageCache : public base::RefCounted<ImageCache> {
   // initialized yet, the call is queued.
   void SaveImage(std::string url,
                  std::string image_data,
-                 bool needs_transcoding);
+                 bool needs_transcoding,
+                 ExpirationInterval expiration_interval);
 
   // Loads the image data for the |url| and passes it to |callback|. If there's
   // no image in the cache, then an empty string is returned. If |read_only|
@@ -73,7 +74,8 @@ class ImageCache : public base::RefCounted<ImageCache> {
   // Saves the |image_data| for |url|.
   void SaveImageImpl(const std::string& url,
                      std::string image_data,
-                     bool needs_transcoding);
+                     bool needs_transcoding,
+                     ExpirationInterval expiration_interval);
   // Loads the data for |url|, calls the user back before updating metadata.
   void LoadImageImpl(bool read_only,
                      const std::string& url,

components/image_fetcher/core/cache/image_cache_unittest.cc

@@ -76,7 +76,8 @@ class CachedImageFetcherImageCacheTest : public testing::Test {
     CreateImageCache();
     InitializeImageCache();
 
-    image_cache()->SaveImage(kImageUrl, kImageData, needs_transcoding);
+    image_cache()->SaveImage(kImageUrl, kImageData, needs_transcoding,
+                             base::nullopt /* expiration_interval */);
     RunUntilIdle();
 
     ASSERT_TRUE(IsMetadataPresent(kImageUrlHashed));
@@ -140,7 +141,8 @@ class CachedImageFetcherImageCacheTest : public testing::Test {
   }
 
   void InjectMetadata(std::string key, int data_size, bool needs_transcoding) {
-    metadata_store_->SaveImageMetadata(key, data_size, needs_transcoding);
+    metadata_store_->SaveImageMetadata(key, data_size, needs_transcoding,
+                                       base::nullopt /* expiration_interval */);
   }
 
   void InjectData(std::string key, std::string data, bool needs_transcoding) {
@@ -187,7 +189,8 @@ TEST_F(CachedImageFetcherImageCacheTest, SanityTest) {
   InitializeImageCache();
 
   image_cache()->SaveImage(kImageUrl, kImageData,
-                           /* needs_transcoding */ false);
+                           /* needs_transcoding */ false,
+                           /* expiration_interval */ base::nullopt);
   RunUntilIdle();
 
   EXPECT_CALL(*this, DataCallback(false, kImageData));
@@ -215,7 +218,8 @@ TEST_F(CachedImageFetcherImageCacheTest, SaveCallsInitialization) {
 
   ASSERT_FALSE(IsCacheInitialized());
   image_cache()->SaveImage(kImageUrl, kImageData,
-                           /* needs_transcoding */ false);
+                           /* needs_transcoding */ false,
+                           /* expiration_interval */ base::nullopt);
   db()->InitStatusCallback(leveldb_proto::Enums::InitStatus::kOK);
   RunUntilIdle();
 
@@ -227,7 +231,8 @@ TEST_F(CachedImageFetcherImageCacheTest, Save) {
   InitializeImageCache();
 
   image_cache()->SaveImage(kImageUrl, kImageData,
-                           /* needs_transcoding */ false);
+                           /* needs_transcoding */ false,
+                           /* expiration_interval */ base::nullopt);
 
   EXPECT_CALL(*this, DataCallback(false, kImageData));
   image_cache()->LoadImage(

components/image_fetcher/core/cache/image_metadata_store.h

@@ -36,7 +36,8 @@ class ImageMetadataStore {
   // the existing metadata.
   virtual void SaveImageMetadata(const std::string& key,
                                  const size_t data_size,
-                                 bool needs_transcoding) = 0;
+                                 bool needs_transcoding,
+                                 ExpirationInterval expiration_interval) = 0;
 
   // Deletes the image metadata for the |key|.
   virtual void DeleteImageMetadata(const std::string& key) = 0;

components/image_fetcher/core/cache/image_metadata_store_leveldb.cc

@@ -105,9 +105,11 @@ void ImageMetadataStoreLevelDB::LoadImageMetadata(
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
-void ImageMetadataStoreLevelDB::SaveImageMetadata(const std::string& key,
-                                                  const size_t data_size,
-                                                  bool needs_transcoding) {
+void ImageMetadataStoreLevelDB::SaveImageMetadata(
+    const std::string& key,
+    const size_t data_size,
+    bool needs_transcoding,
+    ExpirationInterval expiration_interval) {
   // If the database is not initialized yet, ignore the request.
   if (!IsInitialized()) {
     return;
@@ -121,6 +123,11 @@ void ImageMetadataStoreLevelDB::SaveImageMetadata(const std::string& key,
   metadata_proto.set_creation_time(current_time);
   metadata_proto.set_last_used_time(current_time);
   metadata_proto.set_needs_transcoding(needs_transcoding);
+  if (expiration_interval.has_value()) {
+    metadata_proto.set_cache_strategy(CacheStrategy::HOLD_UNTIL_EXPIRED);
+    metadata_proto.set_expiration_interval(
+        expiration_interval->InMicroseconds());
+  }
 
   auto entries_to_save = std::make_unique<MetadataKeyEntryVector>();
   entries_to_save->emplace_back(key, metadata_proto);

components/image_fetcher/core/cache/image_metadata_store_leveldb.h

@@ -53,7 +53,8 @@ class ImageMetadataStoreLevelDB : public ImageMetadataStore {
                          ImageMetadataCallback callback) override;
   void SaveImageMetadata(const std::string& key,
                          const size_t data_size,
-                         bool needs_transcoding) override;
+                         bool needs_transcoding,
+                         ExpirationInterval expiration_interval) override;
   void DeleteImageMetadata(const std::string& key) override;
   void UpdateImageMetadata(const std::string& key) override;
   void GetAllKeys(KeysCallback callback) override;

components/image_fetcher/core/cache/image_metadata_store_leveldb_unittest.cc

@@ -64,8 +64,10 @@ class CachedImageFetcherImageMetadataStoreLevelDBTest : public testing::Test {
   void PrepareDatabase(bool initialize) {
     CreateDatabase();
     InitializeDatabase();
-    metadata_store()->SaveImageMetadata(kImageKey, kImageDataLength,
-                                        /* needs_transcoding */ false);
+    metadata_store()->SaveImageMetadata(
+        kImageKey, kImageDataLength,
+        /* needs_transcoding */ false,
+        /* expiration_interval */ base::nullopt);
     ASSERT_TRUE(IsDataPresent(kImageKey));
 
     if (!initialize) {
@@ -101,11 +103,13 @@ class CachedImageFetcherImageMetadataStoreLevelDBTest : public testing::Test {
     return db_store_.find(key) != db_store_.end();
   }
 
-  void AssertDataPresent(const std::string& key,
-                         int64_t data_size,
-                         base::Time creation_time,
-                         base::Time last_used_time,
-                         bool needs_transcoding) {
+  void AssertDataPresent(
+      const std::string& key,
+      int64_t data_size,
+      base::Time creation_time,
+      base::Time last_used_time,
+      bool needs_transcoding,
+      ExpirationInterval expiration_interval = base::nullopt) {
     if (!IsDataPresent(key)) {
       ASSERT_TRUE(false);
     }
@@ -117,6 +121,14 @@ class CachedImageFetcherImageMetadataStoreLevelDBTest : public testing::Test {
     ASSERT_EQ(entry.last_used_time(),
               last_used_time.since_origin().InMicroseconds());
     ASSERT_EQ(entry.needs_transcoding(), needs_transcoding);
+    if (expiration_interval) {
+      ASSERT_EQ(entry.expiration_interval(),
+                expiration_interval->InMicroseconds());
+      ASSERT_EQ(entry.cache_strategy(), CacheStrategy::HOLD_UNTIL_EXPIRED);
+    } else {
+      ASSERT_EQ(entry.cache_strategy(), CacheStrategy::BEST_EFFORT);
+      ASSERT_FALSE(entry.has_expiration_interval());
+    }
   }
 
   void RunUntilIdle() { task_environment_.RunUntilIdle(); }
@@ -154,7 +166,8 @@ TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, SaveBeforeInit) {
   EXPECT_FALSE(metadata_store()->IsInitialized());
   // Start an image load before the database is initialized.
   metadata_store()->SaveImageMetadata(kImageKey, kImageDataLength,
-                                      /* needs_transcoding */ false);
+                                      /* needs_transcoding */ false,
+                                      /* expiration_interval */ base::nullopt);
 
   InitializeDatabase();
   EXPECT_TRUE(metadata_store()->IsInitialized());
@@ -167,13 +180,17 @@ TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, Save) {
   InitializeDatabase();
 
   metadata_store()->SaveImageMetadata(kImageKey, kImageDataLength,
-                                      /* needs_transcoding */ false);
+                                      /* needs_transcoding */ false,
+                                      /* expiration_interval */ base::nullopt);
   AssertDataPresent(kImageKey, kImageDataLength, clock()->Now(), clock()->Now(),
                     /* needs_transcoding */ false);
+
+  ExpirationInterval expiration_interval = base::TimeDelta::FromHours(1);
   metadata_store()->SaveImageMetadata(kImageKey, kImageDataLength,
-                                      /* needs_transcoding */ true);
+                                      /* needs_transcoding */ true,
+                                      expiration_interval);
   AssertDataPresent(kImageKey, kImageDataLength, clock()->Now(), clock()->Now(),
-                    /* needs_transcoding */ true);
+                    /* needs_transcoding */ true, expiration_interval);
 }
 
 TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, DeleteBeforeInit) {
@@ -189,7 +206,8 @@ TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, Delete) {
   CreateDatabase();
   InitializeDatabase();
   metadata_store()->SaveImageMetadata(kImageKey, kImageDataLength,
-                                      /* needs_transcoding */ false);
+                                      /* needs_transcoding */ false,
+                                      /* expiration_interval */ base::nullopt);
   ASSERT_TRUE(IsDataPresent(kImageKey));
 
   // Delete the data.
@@ -204,7 +222,8 @@ TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, DeleteDifferentKey) {
   CreateDatabase();
   InitializeDatabase();
   metadata_store()->SaveImageMetadata(kImageKey, kImageDataLength,
-                                      /* needs_transcoding */ false);
+                                      /* needs_transcoding */ false,
+                                      /* expiration_interval */ base::nullopt);
   ASSERT_TRUE(IsDataPresent(kImageKey));
 
   // Delete the data.
@@ -280,7 +299,8 @@ TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, GetAllKeysBeforeInit) {
 TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, GetAllKeys) {
   PrepareDatabase(true);
   metadata_store()->SaveImageMetadata(kOtherImageKey, kImageDataLength,
-                                      /* needs_transcoding */ false);
+                                      /* needs_transcoding */ false,
+                                      /* expiration_interval */ base::nullopt);
 
   // A GC call before the db is initialized should be ignore.
   EXPECT_CALL(
@@ -295,7 +315,8 @@ TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, GetAllKeys) {
 TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, GetAllKeysLoadFailed) {
   PrepareDatabase(true);
   metadata_store()->SaveImageMetadata(kOtherImageKey, kImageDataLength,
-                                      /* needs_transcoding */ false);
+                                      /* needs_transcoding */ false,
+                                      /* expiration_interval */ base::nullopt);
 
   // A GC call before the db is initialized should be ignore.
   EXPECT_CALL(*this, OnKeysReturned(std::vector<std::string>({})));
@@ -366,7 +387,8 @@ TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest,
   // Insert an item one our later.
   clock()->SetNow(clock()->Now() + base::TimeDelta::FromHours(1));
   metadata_store()->SaveImageMetadata(kOtherImageKey, kImageDataLength,
-                                      /* needs_transcoding */ false);
+                                      /* needs_transcoding */ false,
+                                      /* expiration_interval*/ base::nullopt);
   clock()->SetNow(clock()->Now() - base::TimeDelta::FromHours(1));
   ASSERT_TRUE(IsDataPresent(kOtherImageKey));
 
@@ -438,7 +460,8 @@ TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest,
 TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, LoadImageMetadata) {
   PrepareDatabase(true);
   metadata_store()->SaveImageMetadata(kOtherImageKey, kImageDataLength,
-                                      /* needs_transcoding */ true);
+                                      /* needs_transcoding */ true,
+                                      /* expiration_interval*/ base::nullopt);
 
   EXPECT_CALL(*this, OnImageMetadataLoaded(_));
   metadata_store()->LoadImageMetadata(

components/image_fetcher/core/cache/image_store_types.h

@@ -38,6 +38,9 @@ using ImageMetadataCallback =
 // Returns a vector of keys.
 using KeysCallback = base::OnceCallback<void(std::vector<std::string>)>;
 
+// The expiration interval for CacheStrategy::HOLD_UNTIL_EXPIRED.
+using ExpirationInterval = base::Optional<base::TimeDelta>;
+
 }  // namespace image_fetcher
 
 #endif  // COMPONENTS_IMAGE_FETCHER_CORE_CACHE_IMAGE_STORE_TYPES_H_

components/image_fetcher/core/cache/proto/cached_image_metadata.proto

@@ -8,6 +8,18 @@ option optimize_for = LITE_RUNTIME;
 
 package image_fetcher;
 
+// Controls how cached image fetcher manages disk cache files.
+enum CacheStrategy {
+  // If reached storage space limit, the cache file may be deleted based on LRU
+  // eviction. The cache files also have a default expiration time after
+  // |creation_time|.
+  BEST_EFFORT = 0;
+
+  // The cache file will be deleted after an |expiration_interval| after
+  // |last_used_time|.
+  HOLD_UNTIL_EXPIRED = 1;
+}
+
 message CachedImageMetadataProto {
   // The key for the source image.
   optional string key = 1;
@@ -26,4 +38,11 @@ message CachedImageMetadataProto {
 
   // True if the image data needs transcoding before it can be trusted.
   optional bool needs_transcoding = 5;
+
+  // The cache strategy used to control the cache file lifecycle.
+  optional CacheStrategy cache_strategy = 6 [default = BEST_EFFORT];
+
+  // The expiration interval since |last_used_time| to delete the cache file for
+  // CacheStrategy::HOLD_UNTIL_EXPIRED. Measured in microseconds.
+  optional int64 expiration_interval = 7;
 }

components/image_fetcher/core/cached_image_fetcher.cc

@@ -326,7 +326,8 @@ void CachedImageFetcher::StoreData(bool cache_result_needs_transcoding,
     bool needs_transcoding = !is_image_data_transcoded &&
                              request.params.allow_needs_transcoding_file();
     image_cache_->SaveImage(std::move(url), std::move(image_data),
-                            /* needs_transcoding */ needs_transcoding);
+                            /* needs_transcoding */ needs_transcoding,
+                            request.params.expiration_interval());
   }
 }
 

components/image_fetcher/core/cached_image_fetcher_unittest.cc

@@ -96,7 +96,8 @@ class CachedImageFetcherTest : public testing::Test {
 
     // Use an initial request to start the cache up.
     image_cache_->SaveImage(ImageUrl().spec(), kImageData,
-                            /* needs_transcoding */ false);
+                            /* needs_transcoding */ false,
+                            /* expiration_interval */ base::nullopt);
     RunUntilIdle();
     db_->InitStatusCallback(leveldb_proto::Enums::InitStatus::kOK);
     image_cache_->DeleteImage(ImageUrl().spec());
@@ -169,7 +170,8 @@ MATCHER(NonEmptyString, "") {
 TEST_F(CachedImageFetcherTest, FetchImageFromCache) {
   // Save the image in the database.
   image_cache()->SaveImage(ImageUrl().spec(), kImageData,
-                           /* needs_transcoding */ false);
+                           /* needs_transcoding */ false,
+                           /* expiration_interval */ base::nullopt);
   RunUntilIdle();
 
   base::MockCallback<ImageDataFetcherCallback> data_callback;
@@ -193,7 +195,8 @@ TEST_F(CachedImageFetcherTest, FetchImageFromCache) {
 TEST_F(CachedImageFetcherTest, FetchImageFromCacheNeedsTranscoding) {
   // Save the image in the database.
   image_cache()->SaveImage(ImageUrl().spec(), kImageData,
-                           /* needs_transcoding */ true);
+                           /* needs_transcoding */ true,
+                           /* expiration_interval */ base::nullopt);
   RunUntilIdle();
 
   base::MockCallback<ImageDataFetcherCallback> data_callback;
@@ -219,7 +222,8 @@ TEST_F(CachedImageFetcherTest, FetchImageFromCacheReadOnly) {
   CreateCachedImageFetcher(/* read_only */ true);
   // Save the image in the database.
   image_cache()->SaveImage(ImageUrl().spec(), kImageData,
-                           /* needs_transcoding */ false);
+                           /* needs_transcoding */ false,
+                           /* expiration_interval */ base::nullopt);
   test_url_loader_factory()->AddResponse(ImageUrl().spec(), kImageData);
   RunUntilIdle();
   {
@@ -371,7 +375,8 @@ TEST_F(CachedImageFetcherTest, FetchImageWithoutTranscodingDoesNotDecode) {
 TEST_F(CachedImageFetcherTest, FetchImageWithSkipDiskCache) {
   // Save the image in the database.
   image_cache()->SaveImage(ImageUrl().spec(), kImageDataOther,
-                           /* needs_transcoding */ false);
+                           /* needs_transcoding */ false,
+                           /* expiration_interval */ base::nullopt);
   RunUntilIdle();
   test_url_loader_factory()->AddResponse(ImageUrl().spec(), kImageData);
 

components/image_fetcher/core/reduced_mode_image_fetcher_unittest.cc

@@ -90,7 +90,8 @@ class ReducedModeImageFetcherTest : public testing::Test {
 
     // Use an initial request to start the cache up.
     image_cache_->SaveImage(ImageUrl().spec(), kImageData,
-                            /* needs_transcoding */ false);
+                            /* needs_transcoding */ false,
+                            /* expiration_interval */ base::nullopt);
     RunUntilIdle();
     db_->InitStatusCallback(leveldb_proto::Enums::InitStatus::kOK);
     image_cache_->DeleteImage(ImageUrl().spec());
@@ -165,14 +166,16 @@ class ReducedModeImageFetcherTest : public testing::Test {
 TEST_F(ReducedModeImageFetcherTest, FetchNeedsTranscodingImageFromCache) {
   // Save the image that needs transcoding in the database.
   image_cache()->SaveImage(ImageUrl().spec(), kImageData,
-                           /* needs_transcoding */ true);
+                           /* needs_transcoding */ true,
+                           /* expiration_interval */ base::nullopt);
   VerifyCacheHit();
 }
 
 TEST_F(ReducedModeImageFetcherTest, FetchImageFromCache) {
   // Save the image that doesn't need transcoding in the database.
   image_cache()->SaveImage(ImageUrl().spec(), kImageData,
-                           /* needs_transcoding */ false);
+                           /* needs_transcoding */ false,
+                           /* expiration_interval */ base::nullopt);
   VerifyCacheHit();
 }