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..e51f9ffee109e6ca267c63f26b536290f477153b 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 |

[sky, 2015/05/06 19:50:42]

Why do we need FAVICON here too? If you use FAVICON here won't that mean you'll
have two FaviconHandlers attempting to download FAVICON?

[Roger McFarlane (Chromium), 2015/05/06 21:22:51]

Yes and no.

Attempts to get the existing FAVICON and TOUCH handlers to do all the work were
unsuccessful (see https://codereview.chromium.org/1118073002/) because the
handler seems to only capture a single representation of the icon. I.e., if you
tell the regular favicon handler to capture a large icon, it forgoes the 16x16
version which is needed for tab rendering.

If you have any suggestions on how to fix that, I'd be glad to hear them and go
back to that approach.

But, it's not quite as bad as it may seem...

The issue you've brought up occurs when the small and large icon are in the same
file (e.g., an ICO file that stores both the small and the large images or a
single one-size fits all icon). In that case, the downside is that the file will
be downloaded twice (because the image fetcher bypasses the cache) but the
handler will (potentially) extract different representations from it.

In the more general case, the small and large icon are in different files,
possibly across different icon types. In this case, multiple downloads and
handlers are exactly what is needed.

In both cases, you want the bitmaps and the mappings to be updated indepedently
for the small and large icons, hence their treatment as different icon types by
the backend.

+          favicon_base::TOUCH_PRECOMPOSED_ICON;
     default:
       NOTREACHED();
   }
@@ -358,6 +360,10 @@ void FaviconHandler::OnUpdateFaviconURL(
 void FaviconHandler::ProcessCurrentUrl() {
   DCHECK(!image_urls_.empty());
 
+  // TODO(rogerm): To use fetch on demand for large icons: just add the Favicon
+  //     record and update the url mappings here, instead of downloading.
+  //     i.e., if (handler_type_ == LARGE) { Save(image_urls[0]); return; }
+
   // current_candidate() may return NULL if download_largest_icon_ is true and
   // all the sizes are larger than the max.
   if (PageChangedSinceFaviconWasRequested() || !current_candidate())
@@ -447,10 +453,12 @@ void FaviconHandler::OnDidDownloadFavicon(
   } else {
     // 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.
+      // If we're specifically handling large icons, save the icon using the
+      // virtual LARGE_ICON type, otherwise save the icon using its true type.
+      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 +548,12 @@ bool FaviconHandler::ShouldSaveFavicon(const GURL& url) {
 }
 
 int FaviconHandler::GetMaximalIconSize(favicon_base::IconType icon_type) {
+  if (handler_type_ == FaviconHandler::LARGE) {
+    // If we're only concerned with saving large icons, return a fixed large
+    // icon size, unless something is wrong with the icon type.
+    return icon_type == favicon_base::INVALID_ICON ? 0 : 192;
+  }
+
   switch (icon_type) {
     case favicon_base::FAVICON:
 #if defined(OS_ANDROID)
@@ -550,6 +564,12 @@ 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:
+      // This method is called to get the maximal size when we're about to
+      // request the download of a concrete icon type. LARGE_ICON is a virtual
+      // type, so we're not expecting a call to get a maximal size to downlaod.

[sky, 2015/05/06 19:50:42]

downlaod->download

[Roger McFarlane (Chromium), 2015/05/06 21:22:51]

Will do.

+      NOTREACHED();
+      return 192;
     case favicon_base::INVALID_ICON:
       return 0;
   }