[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..ccf776188c6c4db2960bfadc72122f2d903236af 100644
--- a/components/favicon/core/favicon_service.cc
+++ b/components/favicon/core/favicon_service.cc
@@ -15,6 +15,8 @@
 #include "components/history/core/browser/history_service.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/color_analysis.h"
+#include "ui/gfx/color_utils.h"
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/image/image_skia.h"
 #include "url/gurl.h"
@@ -22,6 +24,8 @@
 namespace favicon {
 namespace {
 
+const double kMaxDominantColorLuminance = 0.67;
+
 // Helper to run callback with empty results if we cannot get the history
 // service.
 base::CancelableTaskTracker::TaskId RunWithEmptyResultAsync(
@@ -51,6 +55,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 +162,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 +176,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::getLargeIcon(
+    const GURL& pageURL,

[huangs, 2015/04/17 03:55:17]

NIT: pageURL --> page_url

[beaudoin, 2015/04/17 14:50:52]

Done.

+    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.
+  return GetLargestRawFaviconForPageURL(
+      pageURL,
+      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 +396,37 @@ 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) {
+

[huangs, 2015/04/17 03:55:17]

NIT: remove space

[beaudoin, 2015/04/17 14:50:52]

Done.

+  favicon_base::LargeIconResult result;
+
+  if (!bitmap_result.is_valid()) {

[huangs, 2015/04/17 03:55:17]

Semantically one would expect
  result.bitmap = bitmap_result;
to appear here. The fact that this assignment doesn't occur yet is an
optimization, so maybe add a comment blurb?

[beaudoin, 2015/04/17 14:50:52]

Done.

+    callback.Run(result);
+    return;
+  }
+
+  // If we found a bitmap, but it's smaller than the requested size, we
+  // compute the dominant color from the too-small bitmap. We adjust the
+  // luminance so it can be used as background for light text.
+  if (bitmap_result.pixel_size.width() < desired_size_in_pixel ||
+      bitmap_result.pixel_size.height() < desired_size_in_pixel) {
+    result.dominant_color =
+        color_utils::CalculateKMeanColorOfPNG(bitmap_result.bitmap_data);
+    color_utils::HSL color_hsl;

[huangs, 2015/04/17 03:55:17]

I think this logic should be moved to FallbackIconStyle (which already has
dependence on ui/gfx/color_utils.h). Perhaps as a helper routine to 

void SetColorFromSmallIcon(
    scoped_refptr<base::RefCountedMemory> bitmap_data
    FallbackIconStyle* style);

This works with comments in large_icon_source.cc to move more logic to
FallbackIconStyle.

[beaudoin, 2015/04/17 14:50:52]

Done.

+    color_utils::SkColorToHSL(result.dominant_color, &color_hsl);
+    color_hsl.l = std::min(color_hsl.l, kMaxDominantColorLuminance);
+    result.dominant_color =
+        color_utils::HSLToSkColor(color_hsl, SK_AlphaOPAQUE);
+    callback.Run(result);
+    return;
+  }
+
+  // The bitmap is the right size, use it.
+  result.bitmap = bitmap_result;
+  callback.Run(result);
+}
+
 }  // namespace favicon