Extract Favicon Download logic from FaviconTabHelper

chrome/browser/favicon/favicon_download_helper.h

Index: chrome/browser/favicon/favicon_download_helper.h
diff --git a/chrome/browser/favicon/favicon_download_helper.h b/chrome/browser/favicon/favicon_download_helper.h
new file mode 100644
index 0000000000000000000000000000000000000000..7445aee5437843bc24acbbad1d5e6efb87123035
--- /dev/null
+++ b/chrome/browser/favicon/favicon_download_helper.h
@@ -0,0 +1,75 @@
+// Copyright (c) 2012 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_DOWNLOAD_HELPER_H_
+#define CHROME_BROWSER_FAVICON_FAVICON_DOWNLOAD_HELPER_H_
+
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "chrome/common/favicon_url.h"
+#include "content/public/browser/web_contents_observer.h"
+
+namespace content {
+class WebContents;
+}
+
+namespace gfx {
+class Image;
+}
+
+class GURL;
+class FaviconDownloadHelperDelegate;
+class SkBitmap;
+
+// FaviconDownloadHelper handles requests to download favicons, and listens for
+// the IPC messages from the renderer.
+class FaviconDownloadHelper
+    : public content::WebContentsObserver {

[joth, 2012/10/30 19:19:54]

nit: on one line now

[Cait (Slow), 2012/10/30 20:46:33]

Done.

+ public:
+  FaviconDownloadHelper(content::WebContents* web_contents,
+                        FaviconDownloadHelperDelegate* delegate);
+
+  virtual ~FaviconDownloadHelper();
+
+  // Download the favicon at |url|. Returns the unique id of the download
+  // request. The id will be passed to
+  // FaviconDownloadHelperDelegate::OnDidDownloadFavicon once the favicon has

[sky, 2012/10/30 19:59:38]

Use () after function names, eg
FaviconDownloadHelperDelegate::OnDidDownloadFavicon()

[Cait (Slow), 2012/10/30 20:46:33]

Done.

+  // been retrieved.
+  // Note that |image_size| is a hint for images with multiple sizes. The
+  // downloaded image is not resized to the given image_size. If 0 is passed,
+  // the first frame of the image is returned.
+  int DownloadFavicon(const GURL& url, int image_size);
+
+ protected:
+

[joth, 2012/10/30 19:19:54]

nit: no need for extra \n after protected:

[Cait (Slow), 2012/10/30 20:46:33]

Done.

+  // content::WebContentsObserver overrides.
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+
+  // Message handler for IconHostMsg_DidDownloadFavicon. Called when the icon

[sky, 2012/10/30 19:59:38]

Is there a reason to have these non-virtual methods in the protected section?
Can they be in the private section?

[Cait (Slow), 2012/10/30 20:46:33]

Done.

+  // at |image_url| has been downloaded.
+  // |bitmaps| is a list of all the frames of the icon at |image_url|.
+  void OnDidDownloadFavicon(int id,
+                            const GURL& image_url,
+                            bool errored,
+                            int requested_size,
+                            const std::vector<SkBitmap>& bitmaps);
+
+  // Message Handler.
+  void OnUpdateFaviconURL(int32 page_id,
+                          const std::vector<FaviconURL>& candidates);
+
+ private:
+  // Delegate to pass Favicon bitmaps back to.  Weak.
+  FaviconDownloadHelperDelegate* delegate_;
+
+  // Download ids.
+  typedef std::vector<int> DownloadIdList;

[sky, 2012/10/30 19:59:38]

typedefs should be before functions and members.
Also, is there a reason to go with a vector here rather than a set?

[Cait (Slow), 2012/10/30 20:46:33]

Done.

+  DownloadIdList download_ids_;
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(FaviconDownloadHelper);
+};
+
+#endif  // CHROME_BROWSER_FAVICON_FAVICON_DOWNLOAD_HELPER_H_