Add request throttler to thumbnail fetching for articles on mobile NTP

components/ntp_snippets/ntp_snippets_service.cc

Index: components/ntp_snippets/ntp_snippets_service.cc
diff --git a/components/ntp_snippets/ntp_snippets_service.cc b/components/ntp_snippets/ntp_snippets_service.cc
index 295acfbcd20c73bdc72e2300baca2449e4add84a..bde744fdf2863e96f07fed184b8fd3e77af28dd5 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -212,7 +212,10 @@ NTPSnippetsService::NTPSnippetsService(
       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;
@@ -237,24 +240,24 @@ void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   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() {
@@ -317,7 +320,7 @@ void NTPSnippetsService::FetchSuggestionImage(
   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) {
@@ -627,15 +630,15 @@ void NTPSnippetsService::ClearExpiredSnippets() {
 }
 
 void NTPSnippetsService::OnSnippetImageFetchedFromDatabase(
-    const std::string& snippet_id,
     const ImageFetchedCallback& callback,
+    const std::string& snippet_id,
     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;
   }
 
@@ -643,9 +646,9 @@ void NTPSnippetsService::OnSnippetImageFetchedFromDatabase(
   FetchSnippetImageFromNetwork(snippet_id, callback);
 }
 
-void NTPSnippetsService::OnSnippetImageDecoded(
-    const std::string& snippet_id,
+void NTPSnippetsService::OnSnippetImageDecodedFromDatabase(
     const ImageFetchedCallback& callback,
+    const std::string& snippet_id,
     const gfx::Image& image) {
   if (!image.IsEmpty()) {
     callback.Run(MakeUniqueID(provided_category_, snippet_id), image);
@@ -658,6 +661,13 @@ void NTPSnippetsService::OnSnippetImageDecoded(
   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) {
@@ -666,14 +676,27 @@ void NTPSnippetsService::FetchSnippetImageFromNetwork(
                    [&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) {