Extract Favicon Download logic from FaviconTabHelper

chrome/browser/favicon/favicon_download_helper.h

+// 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/callback.h"
+#include "chrome/common/favicon_url.h"

[Jói, 2012/10/16 21:56:52]

Is favicon_url.h free of other dependencies?

Once you have this change nailed down, I would suggest adding a
specific_include_rules section to chrome/browser/favicon/DEPS that nails down
the dependencies for the individual files reused by WebView, i.e. has a blanket
"-chrome" and "-chrome/browser" (the latter is needed because a DEPS file
further up will have "+chrome/browser") and then just allows the files we know
aren't going to transitively include the rest of the world.

That, or I guess this code could move to a new chrome/browser/component
subdirectory with strict DEPS rules.

[Cait (Slow), 2012/10/17 16:00:42]

Agree. Looking into breaking the remaining DEPS for favicon_util
(c/b/history/history_types and c/b/history/select_favicon_frames) and then I'll
put strict DEPS rules on it and favicon_download_helper.

+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
+
+namespace gfx {
+class Image;
+}
+
+class GURL;
+class FaviconDownloadDelegate;
+class SkBitmap;
+
+// FaviconDownloadHelper handles requests to download favicons, and listens for
+// the IPC messages from the renderer.
+//
+class FaviconDownloadHelper : public content::WebContentsObserver,

[Jói, 2012/10/16 21:56:52]

indent like this

class FaviconDownloadHelper
    : public content::WebContentsObserver,
      public content::WebContentsUserData<FaviconDownloadHelper> {

or like this if it fits

class A : public Foo,
          public Blat {

[Cait (Slow), 2012/10/17 16:00:42]

Done.

+      public content::WebContentsUserData<FaviconDownloadHelper> {
+ public:
+  virtual ~FaviconDownloadHelper();
+
+  // Download the favicon at |url|.
+  int DownloadFavicon(const GURL& url, int image_size);

[Jói, 2012/10/16 21:56:52]

Need to document the return value is the 'id' you will get via the delegate
method.

[Cait (Slow), 2012/10/17 16:00:42]

Done.

+
+  // Set our external delegate.
+  void SetExternalDelegate(FaviconDownloadDelegate* delegate) {
+    external_delegate_ = delegate;
+  }
+
+  // content::WebContentsObserver overrides.
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;

[Jói, 2012/10/16 21:56:52]

I think this can be protected; likewise OnDidDownloadFavicon.

[Cait (Slow), 2012/10/17 16:00:42]

Done.

+
+  // Message handler for IconHostMsg_DidDownloadFavicon. Called when the icon
+  // 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);
+
+  // Delegate to pass Favicon bitmaps back to.  Weak.
+  FaviconDownloadDelegate* external_delegate_;

[Jói, 2012/10/16 21:56:52]

Should be private. Can name it just delegate_.

[Cait (Slow), 2012/10/17 16:00:42]

Done.

+
+ private:
+  explicit FaviconDownloadHelper(content::WebContents* web_contents);

[Jói, 2012/10/16 21:56:52]

You could follow the example of
AutofillManager::CreateForWebContentsAndDelegate, instead of having a separate
SetExternalDelegate method.  I think this would be preferable since it seems
without a delegate this class is kind of pointless?

[Cait (Slow), 2012/10/17 16:00:42]

Done.

+  friend class content::WebContentsUserData<FaviconDownloadHelper>;
+  DISALLOW_COPY_AND_ASSIGN(FaviconDownloadHelper);
+};
+
+#endif  // CHROME_BROWSER_FAVICON_FAVICON_DOWNLOAD_HELPER_H_