[Content suggestions] Allow to specify URLs for favicons

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>
@@ skipping 121 matching lines @@
   }
   providers_by_category_[suggestion_id.category()]->FetchSuggestionImage(
       suggestion_id, callback);
 }
 
 void ContentSuggestionsService::FetchSuggestionFavicon(
     const ContentSuggestion::ID& suggestion_id,
     int minimum_size_in_pixel,
     int desired_size_in_pixel,
     const ImageFetchedCallback& callback) {
-  // TODO(jkrcal): Allow the provider to provide (or possibly override) the URL
-  // for looking up the 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_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(callback, gfx::Image()));
     return;
   }
 
-  const GURL& publisher_url = position->url().GetWithEmptyPath();
+  const GURL& publisher_url =

[Marc Treib, 2017/04/12 11:48:00]

s/publisher_url/url_with_favicon/ ?

[jkrcal, 2017/04/12 12:44:18]

Done. Renamed to domain_with_favicon to make the trimming of the URL even more
apparent.

+      position->url_with_favicon().is_valid()
+          ? position->url_with_favicon().GetWithEmptyPath()

[Marc Treib, 2017/04/12 11:48:00]

Hm, should the fallback logic live in ContentSuggestion directly?

[jkrcal, 2017/04/12 12:44:19]

I do not think so, because it is CSS who is using the fetching API of
LargeIconService so it is its responsibility to choose what URL trimming to use.

[Marc Treib, 2017/04/12 12:50:18]

I didn't mean the trimming, just the "url_with_favicon.is_valid() ?
url_with_favicon : url"

[jkrcal, 2017/04/12 12:58:17]

Ah, fair enough. Done.

+          : position->url().GetWithEmptyPath();
 
   // 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(
       publisher_url, minimum_size_in_pixel, desired_size_in_pixel,
       base::Bind(&ContentSuggestionsService::OnGetFaviconFromCacheFinished,
                  base::Unretained(this), publisher_url, minimum_size_in_pixel,
                  desired_size_in_pixel, callback,
                  /*continue_to_google_server=*/true),
       &favicons_task_tracker_);
 }
 
 void ContentSuggestionsService::OnGetFaviconFromCacheFinished(

[Marc Treib, 2017/04/12 11:48:00]

Orthogonal to this CL: We might want to extract the icon fetching logic into a
separate class. Looks like the caching, fallback logic etc. could be
encapsulated nicely.

[jkrcal, 2017/04/12 12:44:19]

Good point, added a CL.

     const GURL& publisher_url,
     int minimum_size_in_pixel,
     int desired_size_in_pixel,
     const ImageFetchedCallback& callback,
     bool continue_to_google_server,
     const favicon_base::LargeIconImageResult& result) {
   if (!result.image.IsEmpty()) {
     callback.Run(result.image);
     return;
   }
@@ skipping 439 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