[Remote suggestions] Get favicon URLs from archive

components/ntp_snippets/content_suggestions_service.cc

 // Copyright 2016 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/content_suggestions_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/default_clock.h"
 #include "base/values.h"
 #include "components/favicon/core/large_icon_service.h"
 #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"
 
 namespace ntp_snippets {
 
 namespace {
 
 // Enumeration listing all possible outcomes for fetch attempts of favicons for
 // content suggestions. Used for UMA histograms, so do not change existing
@@ skipping 119 matching lines @@
   providers_by_category_[suggestion_id.category()]->FetchSuggestionImage(
       suggestion_id, callback);
 }
 
 // TODO(jkrcal): Split the favicon fetching into a separate class.
 void ContentSuggestionsService::FetchSuggestionFavicon(
     const ContentSuggestion::ID& suggestion_id,
     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 =
       std::find_if(suggestions->begin(), suggestions->end(),
                    [&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).
   large_icon_service_->GetLargeIconImageOrFallbackStyle(
       domain_with_favicon, minimum_size_in_pixel, desired_size_in_pixel,
       base::Bind(&ContentSuggestionsService::OnGetFaviconFromCacheFinished,
                  base::Unretained(this), domain_with_favicon,
                  minimum_size_in_pixel, desired_size_in_pixel, callback,
                  /*continue_to_google_server=*/true),
       &favicons_task_tracker_);
@@ skipping 460 matching lines @@
 void ContentSuggestionsService::StoreDismissedCategoriesToPrefs() {
   base::ListValue list;
   for (const auto& category_provider_pair : dismissed_providers_by_category_) {
     list.AppendInteger(category_provider_pair.first.id());
   }
 
   pref_service_->Set(prefs::kDismissedCategories, list);
 }
 
 }  // namespace ntp_snippets