Support touch icon in FaviconHelper

chrome/browser/favicon_helper.h

Index: chrome/browser/favicon_helper.h
diff --git a/chrome/browser/favicon_helper.h b/chrome/browser/favicon_helper.h
index 0c10ff4db9d782cdc418025bc91c3fc789d1687d..ebda19534f95d544084fab9e4648d737d1f0961d 100644
--- a/chrome/browser/favicon_helper.h
+++ b/chrome/browser/favicon_helper.h
@@ -12,6 +12,7 @@
 #include "base/callback.h"
 #include "base/ref_counted.h"
 #include "chrome/browser/favicon_service.h"
+#include "chrome/common/icon_messages.h"
 #include "chrome/common/ref_counted_util.h"
 #include "content/browser/cancelable_request.h"
 #include "content/browser/tab_contents/tab_contents_observer.h"
@@ -67,7 +68,12 @@ class TabContents;
 
 class FaviconHelper : public TabContentsObserver {
  public:
-  explicit FaviconHelper(TabContents* tab_contents);
+  enum Types {

[sky, 2011/03/28 15:02:58]

Types -> Type

[michaelbai, 2011/03/29 00:00:50]

Done.

+    FAVICON,
+    TOUCH,
+  };
+
+  FaviconHelper(TabContents* tab_contents, Types icon_type);
   virtual ~FaviconHelper();
 
   // Initiates loading the favicon for the specified url.
@@ -81,28 +87,88 @@ class FaviconHelper : public TabContentsObserver {
   // downloaded image is not resized to the given image_size. If 0 is passed,
   // the first frame of the image is returned.
   typedef Callback3<int, bool, const SkBitmap&>::Type ImageDownloadCallback;
-  int DownloadImage(const GURL& image_url, int image_size,
+  int DownloadImage(const GURL& image_url,
+                    int image_size,
+                    history::IconType icon_type,
                     ImageDownloadCallback* callback);
 
   // Message Handler.  Must be public, because also called from
   // PrerenderContents.
-  void OnUpdateFaviconURL(int32 page_id, const GURL& icon_url);
+  void OnUpdateFaviconURL(int32 page_id,
+                          const std::vector<FaviconURL>& candidates);
+
+ protected:
+  // These virtual methods make FaviconHelper testable and are overridden by
+  // TestFaviconHelper
+  //
+  // Return the NavigationEntry for the active entry, or NULL if the active
+  // entries URL does not match that of the URL last passed to FetchFavicon.
+  virtual NavigationEntry* GetEntry();

[sky, 2011/03/28 15:02:58]

You need to move all the implementations to match the header.

[michaelbai, 2011/03/29 00:00:50]

Done.

+
+  // Asks the render to download favicon.

[sky, 2011/03/28 15:02:58]

Document return value.

[michaelbai, 2011/03/29 00:00:50]

Done.

+  virtual int DownloadFavicon(const GURL& image_url, int image_size);
+
+  // Ask the favicon from history
+  virtual void UpdateFaviconMappingAndFetch(
+      const GURL& page_url,
+      const GURL& icon_url,
+      history::IconType icon_type,
+      CancelableRequestConsumerBase* consumer,
+      FaviconService::FaviconDataCallback* callback);
+
+  virtual void GetFavicon(
+      const GURL& icon_url,
+      history::IconType icon_type,
+      CancelableRequestConsumerBase* consumer,
+      FaviconService::FaviconDataCallback* callback);
+
+  virtual void GetFaviconForURL(
+      const GURL& page_url,
+      int icon_types,
+      CancelableRequestConsumerBase* consumer,
+      FaviconService::FaviconDataCallback* callback);
+
+  virtual void SetHistoryFavicon(const GURL& page_url,
+                                 const GURL& icon_url,
+                                 const std::vector<unsigned char>& image_data,
+                                 history::IconType icon_type);
+
+  virtual FaviconService* GetFaviconService();
+
+  // Returns true if the favicon should be saved.
+  virtual bool ShouldSaveFavicon(const GURL& url);
 
  private:
+  friend class DownloadHandler; // For testing

[sky, 2011/03/28 15:02:58]

Instead of having so many friends, which is pain to maintain. Make one of your
classes (say TestFaviconHelper) a friend and have it expose everything you need.

[michaelbai, 2011/03/29 00:00:50]

Done.

+  friend class HistoryRequestHandler; // For testing
+  FRIEND_TEST_ALL_PREFIXES(FaviconHelperTest, DownloadFavicon);
+  FRIEND_TEST_ALL_PREFIXES(FaviconHelperTest, GetFaviconFromHistory);
+  FRIEND_TEST_ALL_PREFIXES(FaviconHelperTest, UpdateAndDownloadFavicon);
+  FRIEND_TEST_ALL_PREFIXES(FaviconHelperTest, UpdateFavicon);
+  FRIEND_TEST_ALL_PREFIXES(FaviconHelperTest, Download2ndFaviconURLCandidate);
+  FRIEND_TEST_ALL_PREFIXES(FaviconHelperTest, UpdateDuringDownloading);
+
   struct DownloadRequest {
-    DownloadRequest() {}
+    DownloadRequest();
+
     DownloadRequest(const GURL& url,
                     const GURL& image_url,
-                    ImageDownloadCallback* callback)
-        : url(url),
-          image_url(image_url),
-          callback(callback) { }
+                    ImageDownloadCallback* callback,
+                    history::IconType icon_type);
 
     GURL url;
     GURL image_url;
     ImageDownloadCallback* callback;
+    history::IconType icon_type;
   };
 
+  static bool do_url_and_icon_match(const FaviconURL& favicon_url,
+                                    const GURL& url,
+                                    history::IconType icon_type) {
+    return favicon_url.icon_url == url &&
+        favicon_url.icon_type == static_cast<IconType>(icon_type);
+  }
+
   // TabContentsObserver implementation.
   virtual bool OnMessageReceived(const IPC::Message& message);
 
@@ -111,14 +177,8 @@ class FaviconHelper : public TabContentsObserver {
                             bool errored,
                             const SkBitmap& image);
 
-  // Return the NavigationEntry for the active entry, or NULL if the active
-  // entries URL does not match that of the URL last passed to FetchFavicon.
-  NavigationEntry* GetEntry();
-
   Profile* profile();
 
-  FaviconService* GetFaviconService();
-
   // See description above class for details.
   void OnFaviconDataForInitialURL(FaviconService::Handle handle,
                                   history::FaviconData favicon);
@@ -126,7 +186,9 @@ class FaviconHelper : public TabContentsObserver {
   // If the favicon has expired, asks the renderer to download the favicon.
   // Otherwise asks history to update the mapping between page url and icon
   // url with a callback to OnFaviconData when done.
-  void DownloadFaviconOrAskHistory(NavigationEntry* entry);
+  void DownloadFaviconOrAskHistory(const GURL& page_url,
+                                   const GURL& icon_url,
+                                   history::IconType icon_type);
 
   // See description above class for details.
   void OnFaviconData(FaviconService::Handle handle,
@@ -134,14 +196,21 @@ class FaviconHelper : public TabContentsObserver {
 
   // Schedules a download for the specified entry. This adds the request to
   // download_requests_.
-  int ScheduleDownload(const GURL& url, const GURL& image_url, int image_size,
+  int ScheduleDownload(const GURL& url,
+                       const GURL& image_url,
+                       int image_size,
+                       history::IconType icon_type,
                        ImageDownloadCallback* callback);
 
   // Sets the image data for the favicon. This is invoked asynchronously after
   // we request the TabContents to download the favicon.
-  void SetFavicon(const GURL& url, const GURL& icon_url, const SkBitmap& image);
+  void SetFavicon(const GURL& url,
+                  const GURL& icon_url,
+                  const SkBitmap& image,
+                  history::IconType icon_type);
 
-  // Converts the image data to an SkBitmap and sets it on the NavigationEntry.
+  // Converts the FAVICON's image data to an SkBitmap and sets it on the
+  // NavigationEntry.
   // If the TabContents has a delegate, it is notified of the new favicon
   // (INVALIDATE_FAVICON).
   void UpdateFavicon(NavigationEntry* entry,
@@ -152,8 +221,13 @@ class FaviconHelper : public TabContentsObserver {
   // wide. Does nothing if the image is empty.
   SkBitmap ConvertToFaviconSize(const SkBitmap& image);
 
-  // Returns true if the favicon should be saved.
-  bool ShouldSaveFavicon(const GURL& url);
+  void FetchFaviconInternal();
+
+  // Return the current candidate if any.
+  FaviconURL* current_candidate() {
+    return (urls_.size() > current_url_index_) ?
+        &urls_[current_url_index_] : NULL;
+  }
 
   // Used for history requests.
   CancelableRequestConsumer cancelable_consumer_;
@@ -161,10 +235,6 @@ class FaviconHelper : public TabContentsObserver {
   // URL of the page we're requesting the favicon for.
   GURL url_;
 
-  // Whether we got the url for the page back from the renderer.
-  // See "Favicon Details" in tab_contents.cc for more details.
-  bool got_favicon_url_;
-
   // Whether we got the initial response for the favicon back from the renderer.
   // See "Favicon Details" in tab_contents.cc for more details.
   bool got_favicon_from_history_;
@@ -179,6 +249,21 @@ class FaviconHelper : public TabContentsObserver {
   typedef std::map<int, DownloadRequest> DownloadRequests;
   DownloadRequests download_requests_;
 
+  // The combination of the supported icon types.
+  const int icon_types_;
+
+  // The prioritized favicon candidates from the page back from the renderer.
+  std::vector<FaviconURL> urls_;
+
+  // The current candidate's index in urls_.
+  size_t current_url_index_;
+
+  // The FaviconData from history.
+  history::FaviconData history_icon_;
+
+  // The preferred icon size for supported icon types.
+  int preferred_icon_size_;

[sky, 2011/03/28 15:02:58]

Nuke this and calculate it from icon_types_ when you need it. Create a
preferred_icon_size() function if you want, but no need to create the field. In
fact you should really create cache the Type specified, and calculate
icon_types_ when you need it too.

[michaelbai, 2011/03/29 00:00:50]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(FaviconHelper);
 };