Add request throttler to thumbnail fetching for articles on mobile NTP

components/ntp_snippets/ntp_snippets_service.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/ntp_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 194 matching lines @@
       suggestions_service_(suggestions_service),
       application_language_code_(application_language_code),
       scheduler_(scheduler),
       snippets_fetcher_(std::move(snippets_fetcher)),
       image_fetcher_(std::move(image_fetcher)),
       image_decoder_(std::move(image_decoder)),
       database_(std::move(database)),
       snippets_status_service_(std::move(status_service)),
       fetch_after_load_(false),
       provided_category_(
-          category_factory->FromKnownCategory(KnownCategories::ARTICLES)) {
+          category_factory->FromKnownCategory(KnownCategories::ARTICLES)),
+      thumbnail_requests_throttler_(
+          pref_service,
+          RequestThrottler::RequestType::CONTENT_SUGGESTION_THUMBNAIL) {
   if (database_->IsErrorState()) {
     EnterState(State::ERROR_OCCURRED, CategoryStatus::LOADING_ERROR);
     return;
   }
 
   database_->SetErrorCallback(base::Bind(&NTPSnippetsService::OnDatabaseError,
                                          base::Unretained(this)));
 
   // We transition to other states while finalizing the initialization, when the
   // database is done loading.
   database_->LoadSnippets(base::Bind(&NTPSnippetsService::OnDatabaseLoaded,
                                      base::Unretained(this)));
 }
 
 NTPSnippetsService::~NTPSnippetsService() {
 }
 
 // static
 void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterListPref(prefs::kSnippetHosts);
 
   NTPSnippetsStatusService::RegisterProfilePrefs(registry);
 }
 
-void NTPSnippetsService::FetchSnippets(bool force_request) {
+void NTPSnippetsService::FetchSnippets(bool interactive_request) {
   if (ready())
-    FetchSnippetsFromHosts(GetSuggestionsHosts(), force_request);
+    FetchSnippetsFromHosts(GetSuggestionsHosts(), interactive_request);
   else
     fetch_after_load_ = true;
 }
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts,
-    bool force_request) {
+    bool interactive_request) {
   if (!ready())
     return;
 
   if (snippets_.empty())
     UpdateCategoryStatus(CategoryStatus::AVAILABLE_LOADING);
 
-  snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
-                                            kMaxSnippetCount, force_request);
+  snippets_fetcher_->FetchSnippetsFromHosts(
+      hosts, application_language_code_, kMaxSnippetCount, interactive_request);
 }
 
 void NTPSnippetsService::RescheduleFetching() {
   // The scheduler only exists on Android so far, it's null on other platforms.
   if (!scheduler_)
     return;
 
   if (ready()) {
     base::Time now = base::Time::Now();
     scheduler_->Schedule(
@@ skipping 42 matching lines @@
   snippets_.erase(it);
 }
 
 void NTPSnippetsService::FetchSuggestionImage(
     const std::string& suggestion_id,
     const ImageFetchedCallback& callback) {
   std::string snippet_id = GetWithinCategoryIDFromUniqueID(suggestion_id);
   database_->LoadImage(
       snippet_id,
       base::Bind(&NTPSnippetsService::OnSnippetImageFetchedFromDatabase,
-                 base::Unretained(this), snippet_id, callback));
+                 base::Unretained(this), callback, snippet_id));
 }
 
 void NTPSnippetsService::ClearCachedSuggestionsForDebugging(Category category) {
   DCHECK_EQ(category, provided_category_);
   if (!initialized())
     return;
 
   if (snippets_.empty())
     return;
 
@@ skipping 290 matching lines @@
     if (snippet->expiry_date() < next_expiry)
       next_expiry = snippet->expiry_date();
   }
   DCHECK_GT(next_expiry, expiry);
   expiry_timer_.Start(FROM_HERE, next_expiry - expiry,
                       base::Bind(&NTPSnippetsService::ClearExpiredSnippets,
                                  base::Unretained(this)));
 }
 
 void NTPSnippetsService::OnSnippetImageFetchedFromDatabase(
+    const ImageFetchedCallback& callback,
     const std::string& snippet_id,
-    const ImageFetchedCallback& callback,
     std::string data) {
   // |image_decoder_| is null in tests.
   if (image_decoder_ && !data.empty()) {
     image_decoder_->DecodeImage(
         std::move(data),
-        base::Bind(&NTPSnippetsService::OnSnippetImageDecoded,
-                   base::Unretained(this), snippet_id, callback));
+        base::Bind(&NTPSnippetsService::OnSnippetImageDecodedFromDatabase,
+                   base::Unretained(this), callback, snippet_id));
     return;
   }
 
   // Fetching from the DB failed; start a network fetch.
   FetchSnippetImageFromNetwork(snippet_id, callback);
 }
 
-void NTPSnippetsService::OnSnippetImageDecoded(
+void NTPSnippetsService::OnSnippetImageDecodedFromDatabase(
+    const ImageFetchedCallback& callback,
     const std::string& snippet_id,
-    const ImageFetchedCallback& callback,
     const gfx::Image& image) {
   if (!image.IsEmpty()) {
     callback.Run(MakeUniqueID(provided_category_, snippet_id), image);
     return;
   }
 
   // If decoding the image failed, delete the DB entry.
   database_->DeleteImage(snippet_id);
 
   FetchSnippetImageFromNetwork(snippet_id, callback);
 }
 
 void NTPSnippetsService::FetchSnippetImageFromNetwork(
     const std::string& snippet_id,
     const ImageFetchedCallback& callback) {
   auto it =
       std::find_if(snippets_.begin(), snippets_.end(),
                    [&snippet_id](const std::unique_ptr<NTPSnippet>& snippet) {
                      return snippet->id() == snippet_id;
                    });
-  if (it == snippets_.end()) {
-    callback.Run(MakeUniqueID(provided_category_, snippet_id), gfx::Image());
+
+  if (it == snippets_.end() ||
+      !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.

[tschumann, 2016/08/16 14:56:55]

i guess we want to record such events somewhere, hm?

[jkrcal, 2016/12/06 08:30:25]

This event is recorded in the request throttler. Do we need more?

+    OnSnippetImageDecodedFromNetwork(callback, snippet_id, gfx::Image());
     return;
   }
 
   const NTPSnippet& snippet = *it->get();
+
+  // The image fetcher calls OnImageDataFetched() with the raw data (this object

[tschumann, 2016/08/16 14:56:55]

probably outside of this CL but I find comments like these a bit confusing.
We probably should rename OnImageDataFetched() to CacheImageData().
This would document that this is actually independent from the individual
fetch-flow.
For the code here, it really doesn't matter that image caching is taking place
in-between; hence I'd be in favor of just dropping the comment.

That said, this class grows really, really large. The fact that
NTPSnippetService also implements ImageFetcherDelegate adds unnecssary
complexity (and after all the Service is conceptually not an
ImagerFetcherDeletage ;-)).
Instead, the cleaner solution would  be to define a CachedImageFetcher class
that handles the caching aspects and looks like an image fetcher to the
NTPSnippetService.

As said, not in this CL, but it's definitely a smell.

[jkrcal, 2016/12/06 08:30:25]

Ok, let's leave it up to a follow-up CL.

+  // is an ImageFetcherDelegate) and then also
+  // OnSnippetImageDecodedFromNetwork() after the raw data gets decoded.
   image_fetcher_->StartOrQueueNetworkRequest(
-      snippet.id(), snippet.salient_image_url(), callback);
+      snippet.id(), snippet.salient_image_url(),
+      base::Bind(&NTPSnippetsService::OnSnippetImageDecodedFromNetwork,
+                 base::Unretained(this), callback));
+}
+
+void NTPSnippetsService::OnSnippetImageDecodedFromNetwork(
+    const ImageFetchedCallback& callback,
+    const std::string& snippet_id,
+    const gfx::Image& image) {
+  callback.Run(MakeUniqueID(provided_category_, snippet_id), image);
 }
 
 void NTPSnippetsService::EnterStateEnabled(bool fetch_snippets) {
   if (fetch_snippets)
     FetchSnippets(/*force_request=*/false);
 
   // FetchSnippets should set the status to |AVAILABLE_LOADING| if relevant,
   // otherwise we transition to |AVAILABLE| here.
   if (category_status_ != CategoryStatus::AVAILABLE_LOADING)
     UpdateCategoryStatus(CategoryStatus::AVAILABLE);
@@ skipping 126 matching lines @@
 void NTPSnippetsService::UpdateCategoryStatus(CategoryStatus status) {
   if (status == category_status_)
     return;
 
   category_status_ = status;
   observer()->OnCategoryStatusChanged(this, provided_category_,
                                       category_status_);
 }
 
 }  // namespace ntp_snippets