[IC] Performance improvements and bug fixes

A summary:
- A bunch of work was being done on the UI thread. Now it's being
done off-thread.
- Network requests were dispatched all at once, slowing everything
down. Now they're dispatched one at a time so the first thumbnail
is loaded in quickly.
- One weak ptr was being passed across threads, resulting in a crash.
This is fixed now.

Bug: 864634
Change-Id: I8dc3797a66c0ccc048b9d9613333df7450ec50c1
Reviewed-on: https://chromium-review.googlesource.com/c/1306634
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604474}

components/image_fetcher/core/cache/image_cache.cc

@@ -11,6 +11,7 @@
 #include "base/sha1.h"
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
+#include "base/task_runner_util.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
@@ -37,16 +38,17 @@ constexpr int kCacheResizeWhenFull = 48 * 1024 * 1024;  // 48mb.
 constexpr int kCacheItemsTimeToLiveDays = 7;
 constexpr int kImageCacheEvictionIntervalHours = 24;
 
-std::string HashUrlToKey(const std::string& input) {
-  return base32::Base32Encode(base::SHA1HashString(input));
-}
-
 void OnStartupEvictionQueued() {}
 
 }  // namespace
 
 namespace image_fetcher {
 
+// static
+std::string ImageCache::HashUrlToKey(const std::string& input) {
+  return base32::Base32Encode(base::SHA1HashString(input));
+}
+
 // static
 void ImageCache::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterTimePref(kPrefLastStartupEviction, base::Time());
@@ -102,8 +104,7 @@ void ImageCache::QueueOrStartRequest(base::OnceClosure request) {
     return;
   }
 
-  // Post task for fairness with tasks that may be queued.
-  task_runner_->PostTask(FROM_HERE, std::move(request));
+  std::move(request).Run();
 }
 
 void ImageCache::MaybeStartInitialization() {
@@ -129,7 +130,7 @@ void ImageCache::OnDependencyInitialized() {
 
   // Everything is initialized, take care of the queued requests.
   for (base::OnceClosure& request : queued_requests_) {
-    task_runner_->PostTask(FROM_HERE, std::move(request));
+    std::move(request).Run();
   }
   queued_requests_.clear();
 
@@ -147,7 +148,7 @@ void ImageCache::OnDependencyInitialized() {
 }
 
 void ImageCache::SaveImageImpl(const std::string& url, std::string image_data) {
-  std::string key = HashUrlToKey(url);
+  std::string key = ImageCache::HashUrlToKey(url);
 
   // If the cache is full, evict some stuff.
   RunEvictionWhenFull();
@@ -160,7 +161,7 @@ void ImageCache::SaveImageImpl(const std::string& url, std::string image_data) {
 void ImageCache::LoadImageImpl(bool read_only,
                                const std::string& url,
                                ImageDataCallback callback) {
-  std::string key = HashUrlToKey(url);
+  std::string key = ImageCache::HashUrlToKey(url);
 
   data_store_->LoadImage(key, std::move(callback));
   if (!read_only) {
@@ -169,7 +170,7 @@ void ImageCache::LoadImageImpl(bool read_only,
 }
 
 void ImageCache::DeleteImageImpl(const std::string& url) {
-  std::string key = HashUrlToKey(url);
+  std::string key = ImageCache::HashUrlToKey(url);
 
   data_store_->DeleteImage(key);
   metadata_store_->DeleteImageMetadata(key);

components/image_fetcher/core/cache/image_cache.h

@@ -31,6 +31,7 @@ class ImageMetadataStore;
 // ImageMetadataStore.
 class ImageCache : public base::RefCounted<ImageCache> {
  public:
+  static std::string HashUrlToKey(const std::string& input);
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
   ImageCache(std::unique_ptr<ImageDataStore> data_storage,

components/image_fetcher/core/cache/image_cache_unittest.cc

@@ -165,6 +165,11 @@ class ImageCacheTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(ImageCacheTest);
 };
 
+TEST_F(ImageCacheTest, HashUrlToKeyTest) {
+  ASSERT_EQ(ImageCache::HashUrlToKey("foo"), ImageCache::HashUrlToKey("foo"));
+  ASSERT_NE(ImageCache::HashUrlToKey("foo"), ImageCache::HashUrlToKey("bar"));
+}
+
 TEST_F(ImageCacheTest, SanityTest) {
   CreateImageCache();
   InitializeImageCache();

components/image_fetcher/core/cached_image_fetcher.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/timer/elapsed_timer.h"
 #include "components/image_fetcher/core/cache/cached_image_fetcher_metrics_reporter.h"
 #include "components/image_fetcher/core/cache/image_cache.h"
@@ -41,17 +42,14 @@ void ImageCallbackIfPresent(ImageFetcherCallback image_callback,
   std::move(image_callback).Run(id, image, metadata);
 }
 
-bool EncodeSkBitmapToPNG(const SkBitmap& bitmap,
-                         std::vector<unsigned char>* dest) {
-  if (!bitmap.readyToDraw() || bitmap.isNull()) {
-    return false;
-  }
-
-  return gfx::PNGCodec::Encode(
+std::string EncodeSkBitmapToPNG(SkBitmap bitmap) {
+  std::vector<unsigned char> encoded_data;
+  bool result = gfx::PNGCodec::Encode(
       static_cast<const unsigned char*>(bitmap.getPixels()),
       gfx::PNGCodec::FORMAT_RGBA, gfx::Size(bitmap.width(), bitmap.height()),
       static_cast<int>(bitmap.rowBytes()), /* discard_transparency */ false,
-      std::vector<gfx::PNGCodec::Comment>(), dest);
+      std::vector<gfx::PNGCodec::Comment>(), &encoded_data);
+  return result ? std::string(encoded_data.begin(), encoded_data.end()) : "";
 }
 
 }  // namespace
@@ -76,7 +74,6 @@ void CachedImageFetcher::SetDataUseServiceName(
 }
 
 void CachedImageFetcher::SetDesiredImageFrameSize(const gfx::Size& size) {
-  image_fetcher_->SetDesiredImageFrameSize(size);
   desired_image_frame_size_ = size;
 }
 
@@ -99,9 +96,10 @@ void CachedImageFetcher::FetchImageAndData(
   image_cache_->LoadImage(
       read_only_, image_url.spec(),
       base::BindOnce(&CachedImageFetcher::OnImageFetchedFromCache,
-                     weak_ptr_factory_.GetWeakPtr(), base::Time::Now(), id,
-                     image_url, std::move(data_callback),
-                     std::move(image_callback), traffic_annotation));
+                     weak_ptr_factory_.GetWeakPtr(), base::Time::Now(),
+                     desired_image_frame_size_, id, image_url,
+                     std::move(data_callback), std::move(image_callback),
+                     traffic_annotation));
 
   CachedImageFetcherMetricsReporter::ReportEvent(
       CachedImageFetcherEvent::kImageRequest);
@@ -109,6 +107,7 @@ void CachedImageFetcher::FetchImageAndData(
 
 void CachedImageFetcher::OnImageFetchedFromCache(
     base::Time start_time,
+    const gfx::Size& size,
     const std::string& id,
     const GURL& image_url,
     ImageDataFetcherCallback data_callback,
@@ -117,9 +116,9 @@ void CachedImageFetcher::OnImageFetchedFromCache(
     std::string image_data) {
   if (image_data.empty()) {
     // Fetching from the DB failed, start a network fetch.
-    FetchImageFromNetwork(/* cache_hit */ false, start_time, id, image_url,
-                          std::move(data_callback), std::move(image_callback),
-                          traffic_annotation);
+    EnqueueFetchImageFromNetwork(/* cache_hit */ false, start_time, size, id,
+                                 image_url, std::move(data_callback),
+                                 std::move(image_callback), traffic_annotation);
 
     CachedImageFetcherMetricsReporter::ReportEvent(
         CachedImageFetcherEvent::kCacheMiss);
@@ -128,10 +127,11 @@ void CachedImageFetcher::OnImageFetchedFromCache(
                           RequestMetadata());
     // TODO(wylieb): On Android, do this in-process.
     GetImageDecoder()->DecodeImage(
-        image_data, desired_image_frame_size_,
+        image_data, size,
         base::BindRepeating(&CachedImageFetcher::OnImageDecodedFromCache,
-                            weak_ptr_factory_.GetWeakPtr(), start_time, id,
-                            image_url, base::Passed(std::move(data_callback)),
+                            weak_ptr_factory_.GetWeakPtr(), start_time, size,
+                            id, image_url,
+                            base::Passed(std::move(data_callback)),
                             base::Passed(std::move(image_callback)),
                             traffic_annotation, image_data));
     CachedImageFetcherMetricsReporter::ReportEvent(
@@ -141,6 +141,7 @@ void CachedImageFetcher::OnImageFetchedFromCache(
 
 void CachedImageFetcher::OnImageDecodedFromCache(
     base::Time start_time,
+    const gfx::Size& size,
     const std::string& id,
     const GURL& image_url,
     ImageDataFetcherCallback data_callback,
@@ -150,9 +151,9 @@ void CachedImageFetcher::OnImageDecodedFromCache(
     const gfx::Image& image) {
   if (image.IsEmpty()) {
     // Upon failure, fetch from the network.
-    FetchImageFromNetwork(/* cache_hit */ true, start_time, id, image_url,
-                          std::move(data_callback), std::move(image_callback),
-                          traffic_annotation);
+    EnqueueFetchImageFromNetwork(/* cache_hit */ true, start_time, size, id,
+                                 image_url, std::move(data_callback),
+                                 std::move(image_callback), traffic_annotation);
 
     CachedImageFetcherMetricsReporter::ReportEvent(
         CachedImageFetcherEvent::kCacheDecodingError);
@@ -163,9 +164,27 @@ void CachedImageFetcher::OnImageDecodedFromCache(
   }
 }
 
+void CachedImageFetcher::EnqueueFetchImageFromNetwork(
+    bool cache_hit,
+    base::Time start_time,
+    const gfx::Size& size,
+    const std::string& id,
+    const GURL& image_url,
+    ImageDataFetcherCallback data_callback,
+    ImageFetcherCallback image_callback,
+    const net::NetworkTrafficAnnotationTag& traffic_annotation) {
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&CachedImageFetcher::FetchImageFromNetwork,
+                     weak_ptr_factory_.GetWeakPtr(), cache_hit, start_time,
+                     size, id, image_url, std::move(data_callback),
+                     std::move(image_callback), traffic_annotation));
+}
+
 void CachedImageFetcher::FetchImageFromNetwork(
     bool cache_hit,
     base::Time start_time,
+    const gfx::Size& size,
     const std::string& id,
     const GURL& image_url,
     ImageDataFetcherCallback data_callback,
@@ -173,6 +192,7 @@ void CachedImageFetcher::FetchImageFromNetwork(
     const net::NetworkTrafficAnnotationTag& traffic_annotation) {
   // Fetch image data and the image itself. The image data will be stored in
   // the image cache, and the image will be returned to the caller.
+  image_fetcher_->SetDesiredImageFrameSize(size);
   image_fetcher_->FetchImageAndData(
       id, image_url,
       base::BindOnce(&CachedImageFetcher::DecodeDataForCaching,
@@ -221,17 +241,30 @@ void CachedImageFetcher::DecodeDataForCaching(
   DataCallbackIfPresent(std::move(data_callback), image_data, request_metadata);
   GetImageDecoder()->DecodeImage(
       image_data, /* Decoding for cache shouldn't specify size */ gfx::Size(),
-      base::BindRepeating(&CachedImageFetcher::EncodeDataAndCache,
+      base::BindRepeating(&CachedImageFetcher::StartEncodingDataAndCache,
                           weak_ptr_factory_.GetWeakPtr(), image_url));
 }
 
-void CachedImageFetcher::EncodeDataAndCache(const GURL& image_url,
+void CachedImageFetcher::StartEncodingDataAndCache(const GURL& image_url,
                                                    const gfx::Image& image) {
-  std::vector<unsigned char> encoded_data;
-  // If the image is empty, or there's a problem encoding the image, don't save
-  // it.
-  if (image.IsEmpty() ||
-      !EncodeSkBitmapToPNG(*image.ToSkBitmap(), &encoded_data)) {
+  SkBitmap bitmap = image.IsEmpty() ? SkBitmap() : *image.ToSkBitmap();
+  // If the bitmap is null or otherwise not ready, skip encoding.
+  if (!bitmap.readyToDraw() || bitmap.isNull()) {
+    StoreEncodedData(image_url, "");
+    return;
+  }
+
+  // Post a task to another thread to encode the image data downloaded.
+  base::PostTaskAndReplyWithResult(
+      FROM_HERE, base::BindOnce(&EncodeSkBitmapToPNG, std::move(bitmap)),
+      base::BindOnce(&CachedImageFetcher::StoreEncodedData,
+                     weak_ptr_factory_.GetWeakPtr(), image_url));
+}
+
+void CachedImageFetcher::StoreEncodedData(const GURL& image_url,
+                                          std::string image_data) {
+  // If the image is empty, delete the image.
+  if (image_data.empty()) {
     CachedImageFetcherMetricsReporter::ReportEvent(
         CachedImageFetcherEvent::kTranscodingError);
     image_cache_->DeleteImage(image_url.spec());
@@ -239,8 +272,7 @@ void CachedImageFetcher::EncodeDataAndCache(const GURL& image_url,
   }
 
   if (!read_only_) {
-    image_cache_->SaveImage(image_url.spec(), std::string(encoded_data.begin(),
-                                                          encoded_data.end()));
+    image_cache_->SaveImage(image_url.spec(), std::move(image_data));
   }
 }
 

components/image_fetcher/core/cached_image_fetcher.h

@@ -11,6 +11,7 @@
 
 #include "base/containers/flat_map.h"
 #include "base/memory/weak_ptr.h"
+#include "base/task/post_task.h"
 #include "base/timer/timer.h"
 #include "components/image_fetcher/core/cache/cached_image_fetcher_metrics_reporter.h"
 #include "components/image_fetcher/core/image_decoder.h"
@@ -59,6 +60,7 @@ class CachedImageFetcher : public ImageFetcher {
   // Cache
   void OnImageFetchedFromCache(
       base::Time start_time,
+      const gfx::Size& size,
       const std::string& id,
       const GURL& image_url,
       ImageDataFetcherCallback image_data_callback,
@@ -67,6 +69,7 @@ class CachedImageFetcher : public ImageFetcher {
       std::string image_data);
   void OnImageDecodedFromCache(
       base::Time start_time,
+      const gfx::Size& size,
       const std::string& id,
       const GURL& image_url,
       ImageDataFetcherCallback image_data_callback,
@@ -76,9 +79,19 @@ class CachedImageFetcher : public ImageFetcher {
       const gfx::Image& image);
 
   // Network
+  void EnqueueFetchImageFromNetwork(
+      bool cache_hit,
+      base::Time start_time,
+      const gfx::Size& size,
+      const std::string& id,
+      const GURL& image_url,
+      ImageDataFetcherCallback image_data_callback,
+      ImageFetcherCallback image_callback,
+      const net::NetworkTrafficAnnotationTag& traffic_annotation);
   void FetchImageFromNetwork(
       bool cache_hit,
       base::Time start_time,
+      const gfx::Size& size,
       const std::string& id,
       const GURL& image_url,
       ImageDataFetcherCallback image_data_callback,
@@ -95,7 +108,9 @@ class CachedImageFetcher : public ImageFetcher {
                             const GURL& image_url,
                             const std::string& image_data,
                             const RequestMetadata& request_metadata);
-  void EncodeDataAndCache(const GURL& image_url, const gfx::Image& image);
+  void StartEncodingDataAndCache(const GURL& image_url,
+                                 const gfx::Image& image);
+  void StoreEncodedData(const GURL& image_url, std::string image_data);
 
   // Whether the ImageChache is allowed to be modified in any way from requests
   // made by this CachedImageFetcher. This includes updating last used times,
@@ -106,7 +121,7 @@ class CachedImageFetcher : public ImageFetcher {
 
   scoped_refptr<ImageCache> image_cache_;
 
-  // When true, read the cache as write-only. Used for when users are incognito.
+  // When true, operations won't affect the longeivity of valid cache items.
   bool read_only_;
 
   gfx::Size desired_image_frame_size_;