Save large icons to a new LARGE_ICON type

components/favicon/core/favicon_handler.cc

Index: components/favicon/core/favicon_handler.cc
diff --git a/components/favicon/core/favicon_handler.cc b/components/favicon/core/favicon_handler.cc
index 3befa3491828e4e127d74984f61048540b62d2a7..f8d1776f03ad55b6d16d622c55ad110ddb450d03 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -225,9 +225,11 @@ int FaviconHandler::GetIconTypesFromHandlerType(
   switch (handler_type) {
     case FAVICON:
       return favicon_base::FAVICON;
-    case TOUCH:  // Falls through.
-    case LARGE:
+    case TOUCH:
       return favicon_base::TOUCH_ICON | favicon_base::TOUCH_PRECOMPOSED_ICON;
+    case LARGE:
+      return favicon_base::FAVICON | favicon_base::TOUCH_ICON |
+          favicon_base::TOUCH_PRECOMPOSED_ICON;
     default:
       NOTREACHED();
   }
@@ -349,6 +351,10 @@ void FaviconHandler::OnUpdateFaviconURL(
   if (download_largest_icon_)
     SortAndPruneImageUrls();
 
+  // TODO(rogerm): To use fetch on demand for large icons: just add the Favicon

[huangs, 2015/05/04 05:17:30]

NIT: Perhaps the comment should be in ProcessCurrentUrl(), and replace
"processing" with "downloading"?

[Roger McFarlane (Chromium), 2015/05/04 18:48:15]

Done.

+  //     record and update the url mappings here, instead of processing the url.
+  //     i.e., if (handler_type_ == LARGE) { Save(image_urls[0]); return; }
+
   // TODO(davemoore) Should clear on empty url. Currently we ignore it.
   // This appears to be what FF does as well.
   if (!image_urls_.empty())
@@ -448,9 +454,10 @@ void FaviconHandler::OnDidDownloadFavicon(
     // We have either found the ideal candidate or run out of candidates.
     if (best_favicon_candidate_.icon_type != favicon_base::INVALID_ICON) {
       // No more icons to request, set the favicon from the candidate.
+      favicon_base::IconType saved_type = handler_type_ == LARGE ?
+          favicon_base::LARGE_ICON : best_favicon_candidate_.icon_type;
       SetFavicon(best_favicon_candidate_.url, best_favicon_candidate_.image_url,
-                 best_favicon_candidate_.image,
-                 best_favicon_candidate_.icon_type);
+                 best_favicon_candidate_.image, saved_type);
     }
     // Clear download related state.
     image_urls_.clear();
@@ -540,6 +547,11 @@ bool FaviconHandler::ShouldSaveFavicon(const GURL& url) {
 }
 
 int FaviconHandler::GetMaximalIconSize(favicon_base::IconType icon_type) {
+  if (handler_type_ == FaviconHandler::LARGE &&
+      icon_type != favicon_base::INVALID_ICON) {
+    return 192;

[huangs, 2015/05/04 05:17:30]

I'd prefer
    return icon_type == favicon_base::INVALID_ICON ? 0 : 192;
to keep flow with LARGE separate with the rest.

Also,
#if defined(OS_ANDROID)
    NOT_REACHED();
#endif

[Roger McFarlane (Chromium), 2015/05/04 18:48:15]

Done.
Why?

+  }
+
   switch (icon_type) {
     case favicon_base::FAVICON:
 #if defined(OS_ANDROID)
@@ -550,6 +562,9 @@ int FaviconHandler::GetMaximalIconSize(favicon_base::IconType icon_type) {
     case favicon_base::TOUCH_ICON:
     case favicon_base::TOUCH_PRECOMPOSED_ICON:
       return kTouchIconSize;
+    case favicon_base::LARGE_ICON:
+      NOTREACHED();  // This is a virtual icon type.
+      return 192;
     case favicon_base::INVALID_ICON:
       return 0;
   }