[Icons NTP] Refactor large_icon_source to extract the logic shared between desktop and Android to f…

components/favicon/core/favicon_service.cc

Index: components/favicon/core/favicon_service.cc
diff --git a/components/favicon/core/favicon_service.cc b/components/favicon/core/favicon_service.cc
index 31c704128c517311916dedff7fa3eb3abcde2213..c3cf69196ad2778aacb2966274055c30965ea17d 100644
--- a/components/favicon/core/favicon_service.cc
+++ b/components/favicon/core/favicon_service.cc
@@ -51,6 +51,9 @@ std::vector<int> GetPixelSizesForFaviconScales(int size_in_dip) {
 FaviconService::FaviconService(FaviconClient* favicon_client,
                                history::HistoryService* history_service)
     : history_service_(history_service), favicon_client_(favicon_client) {
+  large_icon_types_.push_back(favicon_base::IconType::FAVICON);
+  large_icon_types_.push_back(favicon_base::IconType::TOUCH_ICON);
+  large_icon_types_.push_back(favicon_base::IconType::TOUCH_PRECOMPOSED_ICON);
 }
 
 FaviconService::~FaviconService() {
@@ -155,7 +158,7 @@ base::CancelableTaskTracker::TaskId
 FaviconService::GetLargestRawFaviconForPageURL(
     const GURL& page_url,
     const std::vector<int>& icon_types,
-    int minimum_size_in_pixels,
+    int minimum_size_in_pixel,
     const favicon_base::FaviconRawBitmapCallback& callback,
     base::CancelableTaskTracker* tracker) {
   favicon_base::FaviconResultsCallback favicon_results_callback =
@@ -169,11 +172,29 @@ FaviconService::GetLargestRawFaviconForPageURL(
   }
   if (history_service_) {
     return history_service_->GetLargestFaviconForURL(page_url, icon_types,
-        minimum_size_in_pixels, callback, tracker);
+        minimum_size_in_pixel, callback, tracker);
   }
   return RunWithEmptyResultAsync(favicon_results_callback, tracker);
 }
 
+base::CancelableTaskTracker::TaskId FaviconService::GetLargeIconOrFallbackStyle(
+    const GURL& page_url,
+    int desired_size_in_pixel,
+    const favicon_base::LargeIconCallback& callback,
+    base::CancelableTaskTracker* tracker) {
+  // TODO(beaudoin): For now this is just a wrapper around
+  //   GetLargestRawFaviconForPageURL. Add the logic required to select the best
+  //   possible large icon. Also add logic to fetch-on-demand when the URL of
+  //   a large icon is known but its bitmap is not available.

[pkotwicz, 2015/04/17 18:39:14]

I recommend creating a new service LargeIconService and adding this method there

Currently FaviconService is a thin wrapper on top of HistoryService. I kind of
want to keep it that way

- At least initially I would like the logic for fetching on demand not to live
in the FaviconService (Maybe we will end up merging LargeIconService and
FaviconService)
- I am undecided whether the logic to create a fallback icon with the dominant
color of a smaller icon if a large icon is not available should live in
FaviconService
- The logic to return a fallback icon using the first letter of the domain and
the default color when a favicon is not found should be in FaviconService

The current behavior of the function would place it most logically in
FallbackIconService but given that LargeIconService will likely exist, it is
probably worth creating the service and putting it there

[beaudoin, 2015/04/17 20:29:05]

Ok, I'll get that out in another service. Another reason to not put it in
FallbackIconService is that Android shouldn't need to instantiate a
FallbackIconService given it wants to render its fallback elsewhere.

+  return GetLargestRawFaviconForPageURL(
+      page_url,
+      large_icon_types_,
+      desired_size_in_pixel,
+      base::Bind(&FaviconService::RunLargeIconCallback,
+                 base::Unretained(this), callback, desired_size_in_pixel),
+      tracker);
+}
+
 base::CancelableTaskTracker::TaskId FaviconService::GetFaviconForPageURL(
     const GURL& page_url,
     int icon_types,
@@ -371,4 +392,31 @@ void FaviconService::RunFaviconRawBitmapCallbackWithBitmapResults(
   callback.Run(bitmap_result);
 }
 
+void FaviconService::RunLargeIconCallback(
+    const favicon_base::LargeIconCallback& callback,
+    int desired_size_in_pixel,
+    const favicon_base::FaviconRawBitmapResult& bitmap_result) {
+  // If there are no bitmap, we return a result with an empty |bitmap| and a
+  // default |fallback_icon_style|.
+  favicon_base::LargeIconResult result;
+  if (!bitmap_result.is_valid()) {
+    callback.Run(result);
+    return;
+  }
+
+  // If there is a bitmap but it's smaller than the requested size, we compute
+  // its dominant color and use it as background for |fallback_icon_style|.
+  if (bitmap_result.pixel_size.width() < desired_size_in_pixel ||
+      bitmap_result.pixel_size.height() < desired_size_in_pixel) {
+    favicon_base::SetDominantColorAsBackground(bitmap_result.bitmap_data,
+                                               &result.fallback_icon_style);
+    callback.Run(result);
+    return;
+  }
+
+  // The bitmap is the right size, use it.
+  result.bitmap = bitmap_result;

[huangs, 2015/04/17 15:30:47]

Can move this into "else" of previous "if" to use common callback.Run(result).

[huangs, 2015/04/17 20:44:04]

Ping on this comment (feel to reject, but just want to be sure that you saw this
:)

[beaudoin, 2015/04/20 19:15:05]

Done.

+  callback.Run(result);
+}
+
 }  // namespace favicon