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..0bf0ff3406ea775be040bce13ab4cd3902808bfa 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();
   }
@@ -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())
@@ -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.

[beaudoin, 2015/05/05 00:54:00]

Augment the comment to indicate what save_type means?

[Roger McFarlane (Chromium), 2015/05/05 15:09:47]

Done.

+      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,10 @@ bool FaviconHandler::ShouldSaveFavicon(const GURL& url) {
 }
 
 int FaviconHandler::GetMaximalIconSize(favicon_base::IconType icon_type) {
+  if (handler_type_ == FaviconHandler::LARGE) {
+    return icon_type == favicon_base::INVALID_ICON ? 0 : 192;

[beaudoin, 2015/05/05 00:54:00]

Comment to explain this logic?

[Roger McFarlane (Chromium), 2015/05/05 15:09:47]

Done.

+  }
+
   switch (icon_type) {
     case favicon_base::FAVICON:
 #if defined(OS_ANDROID)
@@ -550,6 +561,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;

[beaudoin, 2015/05/05 00:54:00]

Strange to return such a specific value behind a NOTREACHED()...

[Roger McFarlane (Chromium), 2015/05/05 15:09:47]

We don't expect that GetMaximalIconSize will be called with LARGE_ICON because
this method is used to determine the size of a "real" icon to be downloaded;
hence the NOTREACHED.

But, the correct value to return, should this ever occur is 192.

So, I've set up the code to do the "right" thing, and added an assertion so that
we're alerted if this every unexpectedly called in this way.

[beaudoin, 2015/05/05 19:47:52]

Acknowledged, although a quick comment to explain this might save a WTF moment
to a future reader.

[Roger McFarlane (Chromium), 2015/05/06 16:06:16]

Done.

     case favicon_base::INVALID_ICON:
       return 0;
   }