[Remote suggestions] Get favicon URLs from archive

components/ntp_snippets/content_suggestions_service.cc

Index: components/ntp_snippets/content_suggestions_service.cc
diff --git a/components/ntp_snippets/content_suggestions_service.cc b/components/ntp_snippets/content_suggestions_service.cc
index 9a5f3fbbb36806f9f3701b724628905eeb88c0d2..6d5fdac6fc6fa210ba78a715ff93899d6bc51816 100644
--- a/components/ntp_snippets/content_suggestions_service.cc
+++ b/components/ntp_snippets/content_suggestions_service.cc
@@ -21,6 +21,7 @@
 #include "components/favicon_base/fallback_icon_style.h"
 #include "components/favicon_base/favicon_types.h"
 #include "components/ntp_snippets/pref_names.h"
+#include "components/ntp_snippets/remote/remote_suggestions_provider.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "ui/gfx/image/image.h"
@@ -160,6 +161,7 @@ void ContentSuggestionsService::FetchSuggestionFavicon(
     int minimum_size_in_pixel,
     int desired_size_in_pixel,
     const ImageFetchedCallback& callback) {
+  GURL domain_with_favicon;
   std::vector<ContentSuggestion>* suggestions =
       &suggestions_by_category_[suggestion_id.category()];
   auto position =
@@ -167,15 +169,24 @@ void ContentSuggestionsService::FetchSuggestionFavicon(
                    [&suggestion_id](const ContentSuggestion& suggestion) {
                      return suggestion_id == suggestion.id();
                    });
-  if (position == suggestions->end() || !large_icon_service_) {
+  if (position != suggestions->end()) {

[tschumann, 2017/04/21 08:27:55]

nit: this is a bit tricky to follow (especially with my other comment):
I'd be in favor of moving this into a helper method (you can write it next to
this method to have easier navigation when reading the code).
like

GURL domain_with_favicon = GetFaviconDomain(suggestion_id);

You can then write the selection using return statements which is easier to
follow:
if (position != suggestions->end()) {
  return position->url_with_favicon();
}

if (...) {
  ...;
}
return GURL();

[jkrcal, 2017/04/21 10:57:01]

Done.

+    domain_with_favicon = position->url_with_favicon();
+  } else if (remote_suggestions_provider_ &&
+             suggestion_id.category().IsKnownCategory(
+                 KnownCategories::ARTICLES)) {

[tschumann, 2017/04/21 08:27:54]

unknown remote suggestions should also be tackled here.
We should user providers_by_category and compare against
static_cast<ContentSuggestionsProvider*>(remote_suggestions_provider). Not sure
if that's nicer though. On the other hand, it guarantees we're asking the right
provider (and the provider implementation acutally checks the category...).

[jkrcal, 2017/04/21 10:57:00]

Due to privacy, we call the new code path from java only for ARTICLES in M59. So
this is overgeneralizing. I hope we find a nicer way around for M60 (for which I
hope we unblock privacy).

[tschumann, 2017/04/21 11:23:58]

I would prefer we only have a single place where we decide for which articles to
fetch those.
That decision should be done on a higher level and down here we should just
properly handle Favicon  requests.

[jkrcal, 2017/04/21 13:30:54]

Okay, done.

+    // TODO(jkrcal): Fix how Fetch more works or find other ways to remove this
+    // hack. crbug.com/714031
+    domain_with_favicon =
+        remote_suggestions_provider_->GetUrlWithFavicon(suggestion_id);
+  }
+
+  if (!domain_with_favicon.is_valid() || !large_icon_service_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(callback, gfx::Image()));
     RecordFaviconFetchResult(FaviconFetchResult::FAILURE);
     return;
   }
 
-  const GURL& domain_with_favicon = position->url_with_favicon();
-
   // TODO(jkrcal): Create a general wrapper function in LargeIconService that
   // does handle the get-from-cache-and-fallback-to-google-server functionality
   // in one shot (for all clients that do not need to react in between).