Refactoring: Moves image fetching and caching into CachedImageFetcher

components/ntp_snippets/remote/remote_suggestions_provider.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/remote/remote_suggestions_provider.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 203 matching lines @@
 void CallWithEmptyResults(const FetchDoneCallback& callback,
                           const Status& status) {
   if (callback.is_null()) {
     return;
   }
   callback.Run(status, std::vector<ContentSuggestion>());
 }
 
 }  // namespace
 
+CachedImageFetcher::CachedImageFetcher(
+    std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher,
+    std::unique_ptr<image_fetcher::ImageDecoder> image_decoder,
+    PrefService* pref_service,
+    RemoteSuggestionsDatabase* database)
+    : image_fetcher_(std::move(image_fetcher)),
+      image_decoder_(std::move(image_decoder)),
+      database_(database),
+      thumbnail_requests_throttler_(
+          pref_service,
+          RequestThrottler::RequestType::CONTENT_SUGGESTION_THUMBNAIL) {
+  // |image_fetcher_| can be null in tests.
+  if (image_fetcher_) {
+    image_fetcher_->SetImageFetcherDelegate(this);
+    image_fetcher_->SetDataUseServiceName(
+        data_use_measurement::DataUseUserData::NTP_SNIPPETS);
+  }
+}
+
+CachedImageFetcher::~CachedImageFetcher() {}
+
+void CachedImageFetcher::FetchSuggestionImage(
+    const ContentSuggestion::ID& suggestion_id,
+    const GURL& url,
+    const ImageFetchedCallback& callback) {
+  database_->LoadImage(
+      suggestion_id.id_within_category(),
+      base::Bind(&CachedImageFetcher::OnSnippetImageFetchedFromDatabase,
+                 base::Unretained(this), callback, suggestion_id, url));
+}
+
+// This function gets only called for caching the image data received from the
+// network. The actual decoding is done in OnSnippetImageDecodedFromDatabase().
+void CachedImageFetcher::OnImageDataFetched(
+    const std::string& id_within_category,
+    const std::string& image_data) {
+  if (image_data.empty()) {
+    return;
+  }
+  database_->SaveImage(id_within_category, image_data);
+}
+
+void CachedImageFetcher::OnImageDecodingDone(
+    const ImageFetchedCallback& callback,
+    const std::string& id_within_category,
+    const gfx::Image& image) {
+  callback.Run(image);
+}
+
+void CachedImageFetcher::OnSnippetImageFetchedFromDatabase(
+    const ImageFetchedCallback& callback,
+    const ContentSuggestion::ID& suggestion_id,
+    const GURL& url,
+    std::string data) {  // SnippetImageCallback requires nonconst reference.
+  // |image_decoder_| is null in tests.
+  if (image_decoder_ && !data.empty()) {
+    image_decoder_->DecodeImage(
+        data, base::Bind(
+                  &CachedImageFetcher::OnSnippetImageDecodedFromDatabase,
+                  base::Unretained(this), callback, suggestion_id, url));
+    return;
+  }
+  // Fetching from the DB failed; start a network fetch.
+  FetchSnippetImageFromNetwork(suggestion_id, url, callback);
+}
+
+void CachedImageFetcher::OnSnippetImageDecodedFromDatabase(
+    const ImageFetchedCallback& callback,
+    const ContentSuggestion::ID& suggestion_id,
+    const GURL& url,
+    const gfx::Image& image) {
+  if (!image.IsEmpty()) {
+    callback.Run(image);
+    return;
+  }
+  // If decoding the image failed, delete the DB entry.
+  database_->DeleteImage(suggestion_id.id_within_category());
+  FetchSnippetImageFromNetwork(suggestion_id, url, callback);
+};
+
+void CachedImageFetcher::FetchSnippetImageFromNetwork(
+    const ContentSuggestion::ID& suggestion_id,
+    const GURL& url,
+    const ImageFetchedCallback& callback) {
+  if (url.is_empty() ||
+      !thumbnail_requests_throttler_.DemandQuotaForRequest(
+          /*interactive_request=*/true)) {
+    // Return an empty image. Directly, this is never synchronous with the
+    // original FetchSuggestionImage() call - an asynchronous database query has
+    // happened in the meantime.
+    callback.Run(gfx::Image());
+    return;
+  }
+
+  image_fetcher_->StartOrQueueNetworkRequest(
+      suggestion_id.id_within_category(), url,
+      base::Bind(&CachedImageFetcher::OnImageDecodingDone,
+                 base::Unretained(this), callback));
+}
+
 RemoteSuggestionsProvider::RemoteSuggestionsProvider(
     Observer* observer,
     CategoryFactory* category_factory,
     PrefService* pref_service,
     const std::string& application_language_code,
     const UserClassifier* user_classifier,
     NTPSnippetsScheduler* scheduler,
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
     std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher,
     std::unique_ptr<image_fetcher::ImageDecoder> image_decoder,
     std::unique_ptr<RemoteSuggestionsDatabase> database,
     std::unique_ptr<RemoteSuggestionsStatusService> status_service)
     : ContentSuggestionsProvider(observer, category_factory),
       state_(State::NOT_INITED),
       pref_service_(pref_service),
       articles_category_(
           category_factory->FromKnownCategory(KnownCategories::ARTICLES)),
       application_language_code_(application_language_code),
       user_classifier_(user_classifier),
       scheduler_(scheduler),
       snippets_fetcher_(std::move(snippets_fetcher)),
-      image_fetcher_(std::move(image_fetcher)),
-      image_decoder_(std::move(image_decoder)),
       database_(std::move(database)),
+      image_fetcher_(std::move(image_fetcher),
+                     std::move(image_decoder),
+                     pref_service,
+                     database_.get()),
       status_service_(std::move(status_service)),
       fetch_when_ready_(false),
       nuke_when_initialized_(false),
-      thumbnail_requests_throttler_(
-          pref_service,
-          RequestThrottler::RequestType::CONTENT_SUGGESTION_THUMBNAIL),
       tick_clock_(base::MakeUnique<base::DefaultTickClock>()) {
   pref_service_->ClearPref(kDeprecatedSnippetHostsPref);
 
   RestoreCategoriesFromPrefs();
   // The articles category always exists. Add it if we didn't get it from prefs.
   // TODO(treib): Rethink this.
   category_contents_.insert(
       std::make_pair(articles_category_,
                      CategoryContent(BuildArticleCategoryInfo(base::nullopt))));
   // Tell the observer about all the categories.
@@ skipping 178 matching lines @@
     return;
   }
 
   auto content_it = category_contents_.find(suggestion_id.category());
   DCHECK(content_it != category_contents_.end());
   CategoryContent* content = &content_it->second;
   DismissSuggestionFromCategoryContent(content,
                                        suggestion_id.id_within_category());
 }
 
-void RemoteSuggestionsProvider::FetchSuggestionImage(
-    const ContentSuggestion::ID& suggestion_id,
-    const ImageFetchedCallback& callback) {
-  database_->LoadImage(
-      suggestion_id.id_within_category(),
-      base::Bind(&RemoteSuggestionsProvider::OnSnippetImageFetchedFromDatabase,
-                 base::Unretained(this), callback, suggestion_id));
-}
-
 void RemoteSuggestionsProvider::ClearHistory(
     base::Time begin,
     base::Time end,
     const base::Callback<bool(const GURL& url)>& filter) {
   // Both time range and the filter are ignored and all suggestions are removed,
   // because it is not known which history entries were used for the suggestions
   // personalization.
   if (!ready()) {
     nuke_when_initialized_ = true;
   } else {
@@ skipping 78 matching lines @@
   const CategoryContent& content =
       category_contents_.at(suggestion_id.category());
   const NTPSnippet* snippet =
       content.FindSnippet(suggestion_id.id_within_category());
   if (!snippet) {
     return GURL();
   }
   return snippet->salient_image_url();
 }
 
-// image_fetcher::ImageFetcherDelegate implementation.
-void RemoteSuggestionsProvider::OnImageDataFetched(
-    const std::string& id_within_category,
-    const std::string& image_data) {
-  if (image_data.empty()) {
-    return;
-  }
-
-  // Only save the image if the corresponding snippet still exists.
-  bool found = false;
-  for (const auto& entry : category_contents_) {
-    if (entry.second.FindSnippet(id_within_category)) {
-      found = true;
-      break;
-    }
-  }
-  if (!found) {
-    return;
-  }
-
-  // Only cache the data in the DB, the actual serving is done in the callback
-  // provided to |image_fetcher_| (OnSnippetImageDecodedFromNetwork()).
-  database_->SaveImage(id_within_category, image_data);
-}
-
 void RemoteSuggestionsProvider::OnDatabaseLoaded(
     NTPSnippet::PtrVector snippets) {
   if (state_ == State::ERROR_OCCURRED) {
     return;
   }
   DCHECK(state_ == State::NOT_INITED);
   DCHECK(base::ContainsKey(category_contents_, articles_category_));
 
   base::TimeDelta database_load_time =
       base::TimeTicks::Now() - database_load_start_;
@@ skipping 345 matching lines @@
     }
   }
 
   for (Category category : categories_to_erase) {
     category_contents_.erase(category);
   }
 
   StoreCategoriesToPrefs();
 }
 
-void RemoteSuggestionsProvider::OnSnippetImageFetchedFromDatabase(
-    const ImageFetchedCallback& callback,
-    const ContentSuggestion::ID& suggestion_id,
-    std::string data) {  // SnippetImageCallback requires nonconst reference.
-  // |image_decoder_| is null in tests.
-  if (image_decoder_ && !data.empty()) {
-    image_decoder_->DecodeImage(
-        data, base::Bind(
-                  &RemoteSuggestionsProvider::OnSnippetImageDecodedFromDatabase,
-                  base::Unretained(this), callback, suggestion_id));
-    return;
-  }
-
-  // Fetching from the DB failed; start a network fetch.
-  FetchSnippetImageFromNetwork(suggestion_id, callback);
-}
-
-void RemoteSuggestionsProvider::OnSnippetImageDecodedFromDatabase(
-    const ImageFetchedCallback& callback,
-    const ContentSuggestion::ID& suggestion_id,
-    const gfx::Image& image) {
-  if (!image.IsEmpty()) {
-    callback.Run(image);
-    return;
-  }
-
-  // If decoding the image failed, delete the DB entry.
-  database_->DeleteImage(suggestion_id.id_within_category());
-
-  FetchSnippetImageFromNetwork(suggestion_id, callback);
-}
-
-void RemoteSuggestionsProvider::FetchSnippetImageFromNetwork(
+void RemoteSuggestionsProvider::FetchSuggestionImage(
     const ContentSuggestion::ID& suggestion_id,
     const ImageFetchedCallback& callback) {
   if (!base::ContainsKey(category_contents_, suggestion_id.category())) {
-    OnSnippetImageDecodedFromNetwork(
-        callback, suggestion_id.id_within_category(), gfx::Image());
+    callback.Run(gfx::Image());
     return;
   }
-
   GURL image_url = FindSnippetImageUrl(suggestion_id);

[jkrcal, 2016/12/12 09:01:11]

What about empty |image_url| (because the snippet does not exist any more, etc)?
I see it treated neither here nor in CachedImageFetcher.

[tschumann, 2016/12/13 20:59:09]

interesting! The actual behavior is still the same (we check the URL after a
failed database lookup). However, chances for that lookup to succeed are very
small, given we don't have any trace of the image.
We can probably save the database access -- I added a test for this.

Marc, wdyt?

[Marc Treib, 2016/12/14 09:02:15]

Yup, this SGTM - I don't see what a DB lookup with an empty GURL would ever
accomplish :)

-
-  if (image_url.is_empty() ||
-      !thumbnail_requests_throttler_.DemandQuotaForRequest(
-          /*interactive_request=*/true)) {
-    // Return an empty image. Directly, this is never synchronous with the
-    // original FetchSuggestionImage() call - an asynchronous database query has
-    // happened in the meantime.
-    OnSnippetImageDecodedFromNetwork(
-        callback, suggestion_id.id_within_category(), gfx::Image());
-    return;
-  }
-
-  image_fetcher_->StartOrQueueNetworkRequest(
-      suggestion_id.id_within_category(), image_url,
-      base::Bind(&RemoteSuggestionsProvider::OnSnippetImageDecodedFromNetwork,
-                 base::Unretained(this), callback));
-}
-
-void RemoteSuggestionsProvider::OnSnippetImageDecodedFromNetwork(
-    const ImageFetchedCallback& callback,
-    const std::string& id_within_category,
-    const gfx::Image& image) {
-  callback.Run(image);
+  image_fetcher_.FetchSuggestionImage(suggestion_id, image_url, callback);
 }
 
 void RemoteSuggestionsProvider::EnterStateReady() {
   if (nuke_when_initialized_) {
     NukeAllSnippets();
     nuke_when_initialized_ = false;
   }
 
   auto article_category_it = category_contents_.find(articles_category_);
   DCHECK(article_category_it != category_contents_.end());
@@ skipping 28 matching lines @@
 }
 
 void RemoteSuggestionsProvider::FinishInitialization() {
   if (nuke_when_initialized_) {
     // We nuke here in addition to EnterStateReady, so that it happens even if
     // we enter the DISABLED state below.
     NukeAllSnippets();
     nuke_when_initialized_ = false;
   }
 
-  // |image_fetcher_| can be null in tests.
-  if (image_fetcher_) {
-    image_fetcher_->SetImageFetcherDelegate(this);
-    image_fetcher_->SetDataUseServiceName(
-        data_use_measurement::DataUseUserData::NTP_SNIPPETS);
-  }
-
   // Note: Initializing the status service will run the callback right away with
   // the current state.
   status_service_->Init(base::Bind(&RemoteSuggestionsProvider::OnStatusChanged,
                                    base::Unretained(this)));
 
   // Always notify here even if we got nothing from the database, because we
   // don't know how long the fetch will take or if it will even complete.
   for (const auto& item : category_contents_) {
     Category category = item.first;
     const CategoryContent& content = item.second;
@@ skipping 266 matching lines @@
 RemoteSuggestionsProvider::CategoryContent::CategoryContent(CategoryContent&&) =
     default;
 
 RemoteSuggestionsProvider::CategoryContent::~CategoryContent() = default;
 
 RemoteSuggestionsProvider::CategoryContent&
 RemoteSuggestionsProvider::CategoryContent::operator=(CategoryContent&&) =
     default;
 
 }  // namespace ntp_snippets