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 289 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::OnSnippetImageDecodedFromNetwork(

[Marc Treib, 2016/08/10 11:38:15]

nit: move this below FetchSnippetImageFromNetwork, so the order of methods
roughly matches the actual order of events?

[jkrcal, 2016/08/10 11:57:03]

Done.

+    const ImageFetchedCallback& callback,
+    const std::string& snippet_id,
+    const gfx::Image& image) {
+  callback.Run(MakeUniqueID(provided_category_, snippet_id), image);
+}
+
 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.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(

[Marc Treib, 2016/08/10 11:38:15]

I think we can directly call the method here, since async things have already
happened before we got here.
If you do keep the PostTask: I don't think Unretained is safe here.

[jkrcal, 2016/08/10 11:57:03]

Done.

Why is it unsafe here when it is safe anywhere else in the code? Is it safe 10
lines below, in particular?

[Marc Treib, 2016/08/10 12:04:59]

It should be safe below because it's the image_fetcher_ that would call the
callback - if we get destroyed, we'll also destroy image_fetcher_, so it won't
call the callback anymore.
Here, we're just posting an "unowned" task, which could get executed even if we
get destroyed in the meantime. (Which is unlikely, but still)

[jkrcal, 2016/08/10 16:22:27]

Thanks, makes sense!

+        FROM_HERE,
+        base::Bind(&NTPSnippetsService::OnSnippetImageDecodedFromNetwork,
+                   base::Unretained(this), callback, snippet_id, gfx::Image()));
     return;
   }
 
   const NTPSnippet& snippet = *it->get();
+
+  // The image fetcher calls OnImageDataFetched() with the raw data (this object
+  // 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));

[Marc Treib, 2016/08/10 11:38:15]

misaligned

[jkrcal, 2016/08/10 11:57:03]

Done.

 }
 
 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