Use high resolution icons where possible for streamlined hosted app icons.

chrome/browser/favicon/favicon_downloader.h

+// Copyright 2013 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.
+
+#ifndef CHROME_BROWSER_FAVICON_FAVICON_DOWNLOADER_H_
+#define CHROME_BROWSER_FAVICON_FAVICON_DOWNLOADER_H_
+
+#include <map>
+#include <set>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "content/public/browser/web_contents_observer.h"
+
+class SkBitmap;
+
+namespace content {
+struct FaviconURL;
+}
+
+namespace gfx {
+class Size;
+}
+
+// Class to help download all favicons for a tab.

[pkotwicz, 2013/12/03 19:06:49]

You are sure that you want to get the favicons for all the favicon URLs mapped
to the page as opposed of trying to figure out the best URL?
http://www.redditstatic.com/favicon.ico
http://www.redditstatic.com/icon.png
http://www.redditstatic.com/icon-touch.png
look pretty different
On Android, I think that we are trying to avoid using touch icons.

[calamity, 2013/12/04 05:31:48]

In this case, the large icon is used as the app icon on the NTP which is much
better than using any of the others. OTOH, the favicon is the best icon for the
taskbar badge because it's designed at the right size and doesn't lose detail
from downscaling.
Yeah, I think we are trying to push for the "sizes" attribute but very few
websites support that right now.

I think we can do something smarter if that information exists (like only
downloading the favicon urls we're actually interested in), but we would want to
do our best for sites that don't have this info by downloading all the favicons
and combining them to create an icon that can handle as many sizes as possible.

+class FaviconDownloader : public content::WebContentsObserver {
+ public:
+  typedef std::map<GURL, std::vector<SkBitmap> > FaviconMap;
+  typedef base::Callback<void(
+          bool,  /* success */
+          /* A map of icon urls to the bitmaps provided by that url. */
+          const FaviconMap&)>
+      FaviconDownloaderCallback;
+
+  FaviconDownloader(content::WebContents* web_contents,

[pkotwicz, 2013/12/03 19:06:49]

Can you add a comment as to why |extra_favicon_urls| is useful here too?

[calamity, 2013/12/04 05:31:48]

Done.

+                    std::vector<GURL> extra_favicon_urls,
+                    FaviconDownloaderCallback callback);
+  virtual ~FaviconDownloader();
+
+  void Start();
+
+ private:
+  friend class TestFaviconDownloader;
+
+  // Initiates a download of the image at |url| and returns the download id.
+  // This is overridden in testing.
+  virtual int DownloadImage(const GURL& url);
+
+  // Queries FaviconTabHelper for the page's current favicon URLs.
+  // This is overridden in testing.
+  virtual void PopulateFaviconURLsFromWebContents();
+
+  // Fetches icons for |favicon_url_candidates_| and |extra_favicon_urls_|.
+  // |callback| is run when all downloads complete.
+  void FetchIcons();
+
+  // Icon download callback.
+  void DidDownloadFavicon(int id,
+                          int http_status_code,
+                          const GURL& image_url,
+                          const std::vector<SkBitmap>& bitmaps,
+                          const std::vector<gfx::Size>& original_bitmap_sizes);
+
+  // content::WebContentsObserver overrides:
+  virtual void DidNavigateMainFrame(
+      const content::LoadCommittedDetails& details,
+      const content::FrameNavigateParams& params) OVERRIDE;
+  virtual void DidUpdateFaviconURL(
+      int32 page_id,
+      const std::vector<content::FaviconURL>& candidates) OVERRIDE;
+
+  // FaviconURLs for the current web contents.
+  // This is populated when PopulateFaviconURLsFromWebContents() is called if

[pkotwicz, 2013/12/03 19:06:49]

Can this be a bool instead? |got_favicon_urls_| or suchlike. You would need to
add a parameter to FetchIcons() to pass in the URLs to download.
PopulateFaviconURLsFromWebContents() should be renamed to
GetFaviconURLsFromWebContents() and have a return value.

[calamity, 2013/12/04 05:31:48]

Done.

+  // available. Otherwise it is populated when
+  // WebContentsObserver::DidUpdateFaviconURL() is called.
+  const std::vector<content::FaviconURL>* favicon_url_candidates_;
+
+  // URLs that aren't given by WebContentsObserver::DidUpdateFaviconURL() that
+  // should be used for this favicon. This is necessary in order to get touch
+  // icons on non-android environments.
+  std::vector<GURL> extra_favicon_urls_;
+
+  // The current page's icons. Populated by FetchIcons().

[pkotwicz, 2013/12/03 19:06:49]

How about: "The icons which were downloaded."

[calamity, 2013/12/04 05:31:48]

Done.

+  FaviconMap favicon_map_;
+
+  // Request ids of in-progress requests.
+  std::set<int> in_progress_requests_;
+
+  // Callback to run on favicon download completion.
+  FaviconDownloaderCallback callback_;
+
+  base::WeakPtrFactory<FaviconDownloader> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(FaviconDownloader);
+};
+
+#endif  // CHROME_BROWSER_FAVICON_FAVICON_DOWNLOADER_H_