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

components/favicon/core/favicon_service.cc

 // Copyright (c) 2012 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/favicon/core/favicon_service.h"
 
 #include <cmath>
 
 #include "base/hash.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 33 matching lines @@
     sizes_in_pixel.push_back(std::ceil(size_in_dip * scales[i]));
   }
   return sizes_in_pixel;
 }
 
 }  // namespace
 
 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() {
 }
 
 // static
 void FaviconService::FaviconResultsCallbackRunner(
     const favicon_base::FaviconResultsCallback& callback,
     const std::vector<favicon_base::FaviconRawBitmapResult>* results) {
   callback.Run(*results);
@@ skipping 84 matching lines @@
       page_url, icon_types, desired_sizes_in_pixel,
       base::Bind(&FaviconService::RunFaviconRawBitmapCallbackWithBitmapResults,
                  base::Unretained(this), callback, desired_size_in_pixel),
       tracker);
 }
 
 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 =
       base::Bind(&FaviconService::RunFaviconRawBitmapCallbackWithBitmapResults,
                  base::Unretained(this), callback, 0);
   if (favicon_client_ && favicon_client_->IsNativeApplicationURL(page_url)) {
     std::vector<int> desired_sizes_in_pixel;
     desired_sizes_in_pixel.push_back(0);
     return favicon_client_->GetFaviconForNativeApplicationURL(
         page_url, desired_sizes_in_pixel, favicon_results_callback, tracker);
   }
   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,
     int desired_size_in_dip,
     const favicon_base::FaviconResultsCallback& callback,
     base::CancelableTaskTracker* tracker) {
   return GetFaviconForPageURLImpl(
       page_url,
       icon_types,
       GetPixelSizesForFaviconScales(desired_size_in_dip),
@@ skipping 177 matching lines @@
                                          &resized_bitmap_data)) {
     callback.Run(favicon_base::FaviconRawBitmapResult());
     return;
   }
 
   bitmap_result.bitmap_data = base::RefCountedBytes::TakeVector(
       &resized_bitmap_data);
   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