Revert 128003 - Prioritize smaller favicons over larger ones for tabs, etc. (Take 2)

This CL broke Chrome Frame's RefreshContentsUATest.

Original CL: http://codereview.chromium.org/9696057

BUG=110143,119135
TEST=Test favicon behavior thoroughly. On Ash with the latest GTalk, avatar icons should show in the launcher and status icons should show in the title bar.

Review URL: http://codereview.chromium.org/9766008

TBR=stevenjb@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9814020

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128038 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 129 insertions, 288 deletions

diff --git a/chrome/browser/favicon/favicon_handler.cc b/chrome/browser/favicon/favicon_handler.cc
index 397e890..52e429c 100644
--- a/chrome/browser/favicon/favicon_handler.cc
+++ b/chrome/browser/favicon/favicon_handler.cc
@@ -51,8 +51,6 @@ bool DoUrlAndIconMatch(const FaviconURL& favicon_url,
 
 }  // namespace
 
-////////////////////////////////////////////////////////////////////////////////
-
 FaviconHandler::DownloadRequest::DownloadRequest()
     : icon_type(history::INVALID_ICON) {
 }
@@ -71,31 +69,6 @@ FaviconHandler::DownloadRequest::DownloadRequest(
       icon_type(icon_type) {
 }
 
-////////////////////////////////////////////////////////////////////////////////
-
-FaviconHandler::FaviconCandidate::FaviconCandidate()
-    : bitmap_size(0),
-      icon_type(history::INVALID_ICON) {
-}
-
-FaviconHandler::FaviconCandidate::~FaviconCandidate() {
-}
-
-FaviconHandler::FaviconCandidate::FaviconCandidate(
-    const GURL& url,
-    const GURL& image_url,
-    const gfx::Image& image,
-    int bitmap_size,
-    history::IconType icon_type)
-    : url(url),
-      image_url(image_url),
-      image(image),
-      bitmap_size(bitmap_size),
-      icon_type(icon_type) {
-}
-
-////////////////////////////////////////////////////////////////////////////////
-
 FaviconHandler::FaviconHandler(Profile* profile,
                                FaviconHandlerDelegate* delegate,
                                Type icon_type)
@@ -103,6 +76,7 @@ FaviconHandler::FaviconHandler(Profile* profile,
       favicon_expired_(false),
       icon_types_(icon_type == FAVICON ? history::FAVICON :
           history::TOUCH_ICON | history::TOUCH_PRECOMPOSED_ICON),
+      current_url_index_(0),
       profile_(profile),
       delegate_(delegate) {
   DCHECK(profile_);
@@ -125,6 +99,8 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   url_ = url;
 
   favicon_expired_ = got_favicon_from_history_ = false;
+  current_url_index_ = 0;
+  urls_.clear();
 
   // Request the favicon from the history service. In parallel to this the
   // renderer is going to notify us (well TabContents) when the favicon url is
@@ -148,47 +124,12 @@ FaviconService* FaviconHandler::GetFaviconService() {
   return profile_->GetFaviconService(Profile::EXPLICIT_ACCESS);
 }
 
-bool FaviconHandler::UpdateFaviconCandidate(const GURL& url,
-                                            const GURL& image_url,
-                                            const gfx::Image& image,
-                                            history::IconType icon_type) {
-  bool update_candidate = false;
-  bool exact_match = false;
-  SkBitmap bitmap = *(image.ToSkBitmap());
-  int bitmap_size = std::max(bitmap.width(), bitmap.height());
-  if (preferred_icon_size() == 0) {
-    update_candidate = true;
-    exact_match = true;
-  } else if (favicon_candidate_.icon_type == history::INVALID_ICON) {
-    // No current candidate, use this.
-    update_candidate = true;
-  } else {
-    if (bitmap_size == preferred_icon_size()) {
-      // Exact match, use this.
-      update_candidate = true;
-      exact_match = true;
-    } else {
-      // Compare against current candidate.
-      int cur_size = favicon_candidate_.bitmap_size;
-      if ((bitmap_size >= preferred_icon_size() && bitmap_size < cur_size) ||
-          (cur_size < preferred_icon_size() && bitmap_size > cur_size)) {
-        update_candidate = true;
-      }
-    }
-  }
-  if (update_candidate) {
-    favicon_candidate_ = FaviconCandidate(
-        url, image_url, image, bitmap_size, icon_type);
-  }
-  return exact_match;
-}
-
 void FaviconHandler::SetFavicon(
     const GURL& url,
     const GURL& image_url,
     const gfx::Image& image,
     history::IconType icon_type) {
-  SkBitmap bitmap = *(image.ToSkBitmap());
+  const SkBitmap& bitmap = image;
   const gfx::Image& sized_image = (preferred_icon_size() == 0 ||
       (preferred_icon_size() == bitmap.width() &&
        preferred_icon_size() == bitmap.height())) ?
@@ -202,10 +143,8 @@ void FaviconHandler::SetFavicon(
 
   if (url == url_ && icon_type == history::FAVICON) {
     NavigationEntry* entry = GetEntry();
-    if (entry) {
-      entry->GetFavicon().url = image_url;
+    if (entry)
       UpdateFavicon(entry, &sized_image);
-    }
   }
 }
 
@@ -231,33 +170,32 @@ void FaviconHandler::UpdateFavicon(NavigationEntry* entry,
 void FaviconHandler::OnUpdateFaviconURL(
     int32 page_id,
     const std::vector<FaviconURL>& candidates) {
+  NavigationEntry* entry = GetEntry();
+  if (!entry)
+    return;
 
-  image_urls_.clear();
-  favicon_candidate_ = FaviconCandidate();
+  bool got_favicon_url_update = false;
   for (std::vector<FaviconURL>::const_iterator i = candidates.begin();
        i != candidates.end(); ++i) {
-    if (!i->icon_url.is_empty() && (i->icon_type & icon_types_))
-      image_urls_.push_back(*i);
+    if (!i->icon_url.is_empty() && (i->icon_type & icon_types_)) {
+      if (!got_favicon_url_update) {
+        got_favicon_url_update = true;
+        urls_.clear();
+        current_url_index_ = 0;
+      }
+      urls_.push_back(*i);
+    }
   }
 
   // TODO(davemoore) Should clear on empty url. Currently we ignore it.
   // This appears to be what FF does as well.
-  if (image_urls_.empty())
+  // No URL was added.
+  if (!got_favicon_url_update)
     return;
 
   if (!GetFaviconService())
     return;
 
-  ProcessCurrentUrl();
-}
-
-void FaviconHandler::ProcessCurrentUrl() {
-  DCHECK(!image_urls_.empty());
-
-  NavigationEntry* entry = GetEntry();
-  if (!entry)
-    return;
-
   // For FAVICON.
   if (current_candidate()->icon_type == FaviconURL::FAVICON) {
     if (!favicon_expired_ && entry->GetFavicon().valid &&
@@ -297,23 +235,13 @@ void FaviconHandler::OnDidDownloadFavicon(int id,
                                i->second.icon_type)) {
     // The downloaded icon is still valid when there is no FaviconURL update
     // during the downloading.
-    bool request_next_icon = true;
     if (!errored) {
-      request_next_icon = !UpdateFaviconCandidate(
-          i->second.url, image_url, image, i->second.icon_type);
-    }
-    if (request_next_icon && GetEntry() && image_urls_.size() > 1) {
-      // Remove the first member of image_urls_ and process the remaining.
-      image_urls_.pop_front();
-      ProcessCurrentUrl();
-    } else if (favicon_candidate_.icon_type != history::INVALID_ICON) {
-      // No more icons to request, set the favicon from the candidate.
-      SetFavicon(favicon_candidate_.url,
-                 favicon_candidate_.image_url,
-                 favicon_candidate_.image,
-                 favicon_candidate_.icon_type);
-      // Reset candidate.
-      favicon_candidate_ = FaviconCandidate();
+      SetFavicon(i->second.url, image_url, image, i->second.icon_type);
+    } else if (GetEntry() && ++current_url_index_ < urls_.size()) {
+      // Copies all candidate except first one and notifies the FaviconHandler,
+      // so the next candidate can be processed.
+      std::vector<FaviconURL> new_candidates(urls_.begin() + 1, urls_.end());
+      OnUpdateFaviconURL(0, new_candidates);
     }
   }
   download_requests_.erase(i);
diff --git a/chrome/browser/favicon/favicon_handler.h b/chrome/browser/favicon/favicon_handler.h
index 6f93fb6..a59e944 100644
--- a/chrome/browser/favicon/favicon_handler.h
+++ b/chrome/browser/favicon/favicon_handler.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -18,7 +18,6 @@
 #include "chrome/common/ref_counted_util.h"
 #include "googleurl/src/gurl.h"
 #include "ui/gfx/favicon_size.h"
-#include "ui/gfx/image/image.h"
 
 class FaviconHandlerDelegate;
 class Profile;
@@ -30,6 +29,10 @@ namespace content {
 class NavigationEntry;
 }
 
+namespace gfx {
+class Image;
+}
+
 // FaviconHandler works with FaviconTabHelper to fetch the specific type of
 // favicon.
 //
@@ -69,11 +72,9 @@ class NavigationEntry;
 // db knew about the favicon), or requests the renderer to download the
 // favicon.
 //
-// When the renderer downloads favicons, it considers the entire list of
-// favicon candidates and chooses the one that best matches the preferred size
-// (or the first one if there is no preferred size). Once the matching favicon
-// has been determined, SetFavicon is called which updates the favicon of the
-// NavigationEntry and notifies the database to save the favicon.
+// When the renderer downloads the favicon SetFavicon is invoked,
+// at which point we update the favicon of the NavigationEntry and notify
+// the database to save the favicon.
 
 class FaviconHandler {
  public:
@@ -103,14 +104,10 @@ class FaviconHandler {
                     const FaviconTabHelper::ImageDownloadCallback& callback);
 
   // Message Handler.  Must be public, because also called from
-  // PrerenderContents. Collects the |image_urls| list.
+  // PrerenderContents.
   void OnUpdateFaviconURL(int32 page_id,
                           const std::vector<FaviconURL>& candidates);
 
-  // Processes the current image_irls_ entry, requesting the image from the
-  // history / download service.
-  void ProcessCurrentUrl();
-
   void OnDidDownloadFavicon(int id,
                             const GURL& image_url,
                             bool errored,
@@ -175,23 +172,6 @@ class FaviconHandler {
     history::IconType icon_type;
   };
 
-  struct FaviconCandidate {
-    FaviconCandidate();
-    ~FaviconCandidate();
-
-    FaviconCandidate(const GURL& url,
-                     const GURL& image_url,
-                     const gfx::Image& image,
-                     int bitmap_size,
-                     history::IconType icon_type);
-
-    GURL url;
-    GURL image_url;
-    gfx::Image image;
-    int bitmap_size;
-    history::IconType icon_type;
-  };
-
   // See description above class for details.
   void OnFaviconDataForInitialURL(FaviconService::Handle handle,
                                   history::FaviconData favicon);
@@ -215,13 +195,8 @@ class FaviconHandler {
                        history::IconType icon_type,
                        const FaviconTabHelper::ImageDownloadCallback& callback);
 
-  // Updates |favicon_candidate_| and returns true if it is an exact match.
-  bool UpdateFaviconCandidate(const GURL& url,
-                              const GURL& image_url,
-                              const gfx::Image& image,
-                              history::IconType icon_type);
-
-  // Sets the image data for the favicon.
+  // 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 gfx::Image& image,
@@ -236,15 +211,16 @@ class FaviconHandler {
   void UpdateFavicon(content::NavigationEntry* entry, const gfx::Image* image);
 
   // If the image is not already at its preferred size, scales the image such
-  // that either the width and/or height == gfx::kFaviconSize. Does nothing if
-  // the image is empty.
+  // that either the width and/or height is 16 pixels wide. Does nothing if the
+  // image is empty.
   gfx::Image ResizeFaviconIfNeeded(const gfx::Image& image);
 
   void FetchFaviconInternal();
 
   // Return the current candidate if any.
   FaviconURL* current_candidate() {
-    return (image_urls_.size() > 0) ? &image_urls_[0] : NULL;
+    return (urls_.size() > current_url_index_) ?
+        &urls_[current_url_index_] : NULL;
   }
 
   // Returns the preferred_icon_size according icon_types_, 0 means no
@@ -277,7 +253,10 @@ class FaviconHandler {
   const int icon_types_;
 
   // The prioritized favicon candidates from the page back from the renderer.
-  std::deque<FaviconURL> image_urls_;
+  std::vector<FaviconURL> urls_;
+
+  // The current candidate's index in urls_.
+  size_t current_url_index_;
 
   // The FaviconData from history.
   history::FaviconData history_icon_;
@@ -288,9 +267,6 @@ class FaviconHandler {
   // This handler's delegate.
   FaviconHandlerDelegate* delegate_;  // weak
 
-  // Current favicon candidate.
-  FaviconCandidate favicon_candidate_;
-
   DISALLOW_COPY_AND_ASSIGN(FaviconHandler);
 };
 
diff --git a/chrome/browser/favicon/favicon_handler_unittest.cc b/chrome/browser/favicon/favicon_handler_unittest.cc
index 745b77a..16952d1 100644
--- a/chrome/browser/favicon/favicon_handler_unittest.cc
+++ b/chrome/browser/favicon/favicon_handler_unittest.cc
@@ -1,8 +1,7 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
-#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/favicon/favicon_handler.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
@@ -46,47 +45,40 @@ void FillBitmap(int w, int h, std::vector<unsigned char>* output) {
 // It also will be used to invoke the onDidDownload callback.
 class DownloadHandler {
  public:
-  explicit DownloadHandler(TestFaviconHandler* favicon_helper)
-      : favicon_helper_(favicon_helper),
-        failed_(false) {
+  DownloadHandler(int download_id,
+                  const GURL& image_url,
+                  int image_size,
+                  TestFaviconHandler* favicon_helper)
+    : image_url_(image_url),
+      image_size_(image_size),
+      failed_(false),
+      download_id_(download_id),
+      favicon_helper_(favicon_helper) {
+    FillDataToBitmap(16, 16, &bitmap_);
   }
 
   virtual ~DownloadHandler() {
   }
 
-  void Reset() {
-    download_.reset(NULL);
-    failed_ = false;
-  }
-
-  void AddDownload(int download_id, const GURL& image_url, int image_size) {
-    download_.reset(new Download(download_id, image_url, image_size, false));
-  }
+  static void UpdateFaviconURL(FaviconHandler* helper,
+                               const std::vector<FaviconURL> urls);
 
   void InvokeCallback();
 
-  void set_failed(bool failed) { failed_ = failed; }
+  void UpdateFaviconURL(const std::vector<FaviconURL> urls);
 
-  bool HasDownload() const { return download_.get() != NULL; }
-  const GURL& GetImageUrl() const { return download_->image_url; }
-  int GetImageSize() const { return download_->image_size; }
-  void SetImageSize(int size) { download_->image_size = size; }
+  const GURL image_url_;
+  const int image_size_;
 
- private:
-  struct Download {
-    Download(int id, GURL url, int size, bool failed)
-        : download_id(id),
-          image_url(url),
-          image_size(size) {}
-    ~Download() {}
-    int download_id;
-    GURL image_url;
-    int image_size;
-  };
+  // Simulates download failed or not.
+  bool failed_;
 
+ private:
+  // Identified the specific download, will also be passed in
+  // OnDidDownloadFavicon callback.
+  int download_id_;
   TestFaviconHandler* favicon_helper_;
-  scoped_ptr<Download> download_;
-  bool failed_;
+  SkBitmap bitmap_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadHandler);
 };
@@ -177,7 +169,6 @@ class TestFaviconHandler : public FaviconHandler {
         entry_(NavigationEntry::Create()),
         download_id_(0) {
     entry_->SetURL(page_url);
-    download_handler_.reset(new DownloadHandler(this));
   }
 
   virtual ~TestFaviconHandler() {
@@ -196,12 +187,17 @@ class TestFaviconHandler : public FaviconHandler {
     return download_handler_.get();
   }
 
+  // This method will take the ownership of the given download_handler.
+  void set_download_handler(DownloadHandler* download_handler) {
+    download_handler_.reset(download_handler);
+  }
+
   virtual NavigationEntry* GetEntry() {
     return entry_.get();
   }
 
-  const std::deque<FaviconURL>& urls() {
-    return image_urls_;
+  const std::vector<FaviconURL>& urls() {
+    return urls_;
   }
 
   void FetchFavicon(const GURL& url) {
@@ -213,6 +209,13 @@ class TestFaviconHandler : public FaviconHandler {
     return FaviconHandler::current_candidate();
   }
 
+  void OnDidDownloadFavicon(int id,
+                            const GURL& image_url,
+                            bool errored,
+                            gfx::Image& image) {
+    FaviconHandler::OnDidDownloadFavicon(id, image_url, errored, image);
+  }
+
  protected:
   virtual void UpdateFaviconMappingAndFetch(
       const GURL& page_url,
@@ -244,7 +247,8 @@ class TestFaviconHandler : public FaviconHandler {
 
   virtual int DownloadFavicon(const GURL& image_url, int image_size) OVERRIDE {
     download_id_++;
-    download_handler_->AddDownload(download_id_, image_url, image_size);
+    download_handler_.reset(new DownloadHandler(download_id_, image_url,
+                                                image_size, this));
     return download_id_;
   }
 
@@ -286,19 +290,23 @@ class TestFaviconHandler : public FaviconHandler {
 
 namespace {
 
-void HistoryRequestHandler::InvokeCallback() {
-  if (!callback_.is_null())
-    callback_.Run(0, favicon_data_);
+void DownloadHandler::UpdateFaviconURL(FaviconHandler* helper,
+                                       const std::vector<FaviconURL> urls) {
+  helper->OnUpdateFaviconURL(0, urls);
+}
+
+void DownloadHandler::UpdateFaviconURL(const std::vector<FaviconURL> urls) {
+  UpdateFaviconURL(favicon_helper_, urls);
 }
 
 void DownloadHandler::InvokeCallback() {
-  SkBitmap bitmap;
-  int bitmap_size = (download_->image_size > 0) ?
-      download_->image_size : gfx::kFaviconSize;
-  FillDataToBitmap(bitmap_size, bitmap_size, &bitmap);
-  gfx::Image image(bitmap);
-  favicon_helper_->OnDidDownloadFavicon(
-      download_->download_id, download_->image_url, failed_, image);
+  gfx::Image image(new SkBitmap(bitmap_));
+  favicon_helper_->OnDidDownloadFavicon(download_id_, image_url_, failed_,
+                                        image);
+}
+
+void HistoryRequestHandler::InvokeCallback() {
+  callback_.Run(0, favicon_data_);
 }
 
 class FaviconHandlerTest : public ChromeRenderViewHostTestHarness {
@@ -340,7 +348,7 @@ TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
   // Simulates update favicon url.
   std::vector<FaviconURL> urls;
   urls.push_back(FaviconURL(icon_url, FaviconURL::FAVICON));
-  helper.OnUpdateFaviconURL(0, urls);
+  DownloadHandler::UpdateFaviconURL(&helper, urls);
 
   // Verify FaviconHandler status
   EXPECT_EQ(1U, helper.urls().size());
@@ -349,7 +357,8 @@ TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
   ASSERT_EQ(FaviconURL::FAVICON, helper.current_candidate()->icon_type);
 
   // Favicon shouldn't request to download icon.
-  EXPECT_FALSE(helper.download_handler()->HasDownload());
+  DownloadHandler* download_handler = helper.download_handler();
+  ASSERT_FALSE(download_handler);
 }
 
 TEST_F(FaviconHandlerTest, DownloadFavicon) {
@@ -384,7 +393,7 @@ TEST_F(FaviconHandlerTest, DownloadFavicon) {
   // Simulates update favicon url.
   std::vector<FaviconURL> urls;
   urls.push_back(FaviconURL(icon_url, FaviconURL::FAVICON));
-  helper.OnUpdateFaviconURL(0, urls);
+  DownloadHandler::UpdateFaviconURL(&helper, urls);
 
   // Verify FaviconHandler status
   EXPECT_EQ(1U, helper.urls().size());
@@ -394,11 +403,10 @@ TEST_F(FaviconHandlerTest, DownloadFavicon) {
 
   // Favicon should request to download icon now.
   DownloadHandler* download_handler = helper.download_handler();
-  EXPECT_TRUE(helper.download_handler()->HasDownload());
-
+  ASSERT_TRUE(download_handler);
   // Verify the download request.
-  EXPECT_EQ(icon_url, download_handler->GetImageUrl());
-  EXPECT_EQ(gfx::kFaviconSize, download_handler->GetImageSize());
+  EXPECT_EQ(icon_url, download_handler->image_url_);
+  EXPECT_EQ(gfx::kFaviconSize, download_handler->image_size_);
 
   // Reset the history_handler to verify whether favicon is set.
   helper.set_history_handler(NULL);
@@ -461,7 +469,7 @@ TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
   // Simulates update with the different favicon url.
   std::vector<FaviconURL> urls;
   urls.push_back(FaviconURL(new_icon_url, FaviconURL::FAVICON));
-  helper.OnUpdateFaviconURL(0, urls);
+  DownloadHandler::UpdateFaviconURL(&helper, urls);
 
   // Verify FaviconHandler status.
   EXPECT_EQ(1U, helper.urls().size());
@@ -484,11 +492,10 @@ TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
 
   // Favicon should request to download icon now.
   DownloadHandler* download_handler = helper.download_handler();
-  EXPECT_TRUE(helper.download_handler()->HasDownload());
-
+  ASSERT_TRUE(download_handler);
   // Verify the download request.
-  EXPECT_EQ(new_icon_url, download_handler->GetImageUrl());
-  EXPECT_EQ(gfx::kFaviconSize, download_handler->GetImageSize());
+  EXPECT_EQ(new_icon_url, download_handler->image_url_);
+  EXPECT_EQ(gfx::kFaviconSize, download_handler->image_size_);
 
   // Reset the history_handler to verify whether favicon is set.
   helper.set_history_handler(NULL);
@@ -551,7 +558,7 @@ TEST_F(FaviconHandlerTest, UpdateFavicon) {
   // Simulates update with the different favicon url.
   std::vector<FaviconURL> urls;
   urls.push_back(FaviconURL(new_icon_url, FaviconURL::FAVICON));
-  helper.OnUpdateFaviconURL(0, urls);
+  DownloadHandler::UpdateFaviconURL(&helper, urls);
 
   // Verify FaviconHandler status.
   EXPECT_EQ(1U, helper.urls().size());
@@ -577,7 +584,7 @@ TEST_F(FaviconHandlerTest, UpdateFavicon) {
   history_handler->InvokeCallback();
 
   // Shouldn't request download favicon
-  EXPECT_FALSE(helper.download_handler()->HasDownload());
+  EXPECT_FALSE(helper.download_handler());
 
   // Verify the favicon status.
   EXPECT_EQ(new_icon_url, helper.GetEntry()->GetFavicon().url);
@@ -622,7 +629,8 @@ TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
   urls.push_back(FaviconURL(icon_url, FaviconURL::TOUCH_PRECOMPOSED_ICON));
   urls.push_back(FaviconURL(new_icon_url, FaviconURL::TOUCH_ICON));
   urls.push_back(FaviconURL(new_icon_url, FaviconURL::FAVICON));
-  helper.OnUpdateFaviconURL(0, urls);
+
+  DownloadHandler::UpdateFaviconURL(&helper, urls);
 
   // Verify FaviconHandler status.
   EXPECT_EQ(2U, helper.urls().size());
@@ -644,17 +652,16 @@ TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
 
   // Should request download favicon.
   DownloadHandler* download_handler = helper.download_handler();
-  EXPECT_TRUE(helper.download_handler()->HasDownload());
-
+  EXPECT_TRUE(download_handler);
   // Verify the download request.
-  EXPECT_EQ(icon_url, download_handler->GetImageUrl());
-  EXPECT_EQ(0, download_handler->GetImageSize());
+  EXPECT_EQ(icon_url, download_handler->image_url_);
+  EXPECT_EQ(0, download_handler->image_size_);
 
   // Reset the history_handler to verify whether favicon is request from
   // history.
   helper.set_history_handler(NULL);
   // Smulates download failed.
-  download_handler->set_failed(true);
+  download_handler->failed_ = true;
   download_handler->InvokeCallback();
 
   // Left 1 url.
@@ -671,7 +678,7 @@ TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
   EXPECT_EQ(page_url, history_handler->page_url_);
 
   // Reset download handler
-  download_handler->Reset();
+  helper.set_download_handler(NULL);
 
   // Smulates getting a expired icon from history.
   history_handler->favicon_data_.known_icon = true;
@@ -684,9 +691,10 @@ TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
   history_handler->InvokeCallback();
 
   // Verify the download request.
-  EXPECT_TRUE(helper.download_handler()->HasDownload());
-  EXPECT_EQ(new_icon_url, download_handler->GetImageUrl());
-  EXPECT_EQ(0, download_handler->GetImageSize());
+  download_handler = helper.download_handler();
+  EXPECT_TRUE(download_handler);
+  EXPECT_EQ(new_icon_url, download_handler->image_url_);
+  EXPECT_EQ(0, download_handler->image_size_);
 
   helper.set_history_handler(NULL);
 
@@ -739,7 +747,8 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
   urls.push_back(FaviconURL(icon_url, FaviconURL::TOUCH_PRECOMPOSED_ICON));
   urls.push_back(FaviconURL(new_icon_url, FaviconURL::TOUCH_ICON));
   urls.push_back(FaviconURL(new_icon_url, FaviconURL::FAVICON));
-  helper.OnUpdateFaviconURL(0, urls);
+
+  DownloadHandler::UpdateFaviconURL(&helper, urls);
 
   // Verify FaviconHandler status.
   EXPECT_EQ(2U, helper.urls().size());
@@ -761,11 +770,10 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
 
   // Should request download favicon.
   DownloadHandler* download_handler = helper.download_handler();
-  EXPECT_TRUE(helper.download_handler()->HasDownload());
-
+  EXPECT_TRUE(download_handler);
   // Verify the download request.
-  EXPECT_EQ(icon_url, download_handler->GetImageUrl());
-  EXPECT_EQ(0, download_handler->GetImageSize());
+  EXPECT_EQ(icon_url, download_handler->image_url_);
+  EXPECT_EQ(0, download_handler->image_size_);
 
   // Reset the history_handler to verify whether favicon is request from
   // history.
@@ -773,8 +781,7 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
   const GURL latest_icon_url("http://www.google.com/latest_favicon");
   std::vector<FaviconURL> latest_urls;
   latest_urls.push_back(FaviconURL(latest_icon_url, FaviconURL::TOUCH_ICON));
-  helper.OnUpdateFaviconURL(0, latest_urls);
-
+  DownloadHandler::UpdateFaviconURL(&helper, latest_urls);
   EXPECT_EQ(1U, helper.urls().size());
   EXPECT_EQ(latest_icon_url, helper.current_candidate()->icon_url);
   EXPECT_EQ(FaviconURL::TOUCH_ICON, helper.current_candidate()->icon_type);
@@ -797,7 +804,7 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
   // The downloaded icon should be thrown away as there is favicon update.
   EXPECT_FALSE(helper.history_handler());
 
-  download_handler->Reset();
+  helper.set_download_handler(NULL);
 
   // Simulates getting the icon from history.
   scoped_ptr<HistoryRequestHandler> handler;
@@ -814,77 +821,7 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
   handler->InvokeCallback();
 
   // No download request.
-  EXPECT_FALSE(download_handler->HasDownload());
-}
-
-TEST_F(FaviconHandlerTest, MultipleFavicon) {
-  const GURL page_url("http://www.google.com");
-  const GURL icon_url("http://www.google.com/favicon");
-  const GURL icon_url_small("http://www.google.com/favicon_small");
-  const GURL icon_url_large("http://www.google.com/favicon_large");
-  const GURL icon_url_preferred("http://www.google.com/favicon_preferred");
-
-  TestFaviconHandlerDelegate delegate(contents());
-  Profile* profile = Profile::FromBrowserContext(
-      contents()->GetBrowserContext());
-  TestFaviconHandler helper(page_url, profile,
-                            &delegate, FaviconHandler::FAVICON);
-
-  helper.FetchFavicon(page_url);
-  HistoryRequestHandler* history_handler = helper.history_handler();
-
-  // Set valid icon data.
-  history_handler->favicon_data_.known_icon = true;
-  history_handler->favicon_data_.icon_type = history::FAVICON;
-  history_handler->favicon_data_.expired = false;
-  history_handler->favicon_data_.icon_url = icon_url;
-  scoped_refptr<RefCountedBytes> data = new RefCountedBytes();
-  FillBitmap(gfx::kFaviconSize, gfx::kFaviconSize, &data->data());
-  history_handler->favicon_data_.image_data = data;
-
-  // Send history response.
-  history_handler->InvokeCallback();
-
-  // Simulates update with the different favicon url.
-  std::vector<FaviconURL> urls;
-  // Note: the code will stop making requests when an icon matching the
-  // preferred size is found, so icon_url_preferred must be last.
-  urls.push_back(FaviconURL(icon_url_small, FaviconURL::FAVICON));
-  urls.push_back(FaviconURL(icon_url_large, FaviconURL::FAVICON));
-  urls.push_back(FaviconURL(icon_url_preferred, FaviconURL::FAVICON));
-  helper.OnUpdateFaviconURL(0, urls);
-
-  DownloadHandler* download_handler = helper.download_handler();
-
-  // Download the first icon (set not in history).
-  helper.history_handler()->favicon_data_.known_icon = false;
-  helper.history_handler()->InvokeCallback();
-  ASSERT_TRUE(download_handler->HasDownload());
-  EXPECT_EQ(icon_url_small, download_handler->GetImageUrl());
-  download_handler->SetImageSize(gfx::kFaviconSize / 2);
-  download_handler->InvokeCallback();
-
-  // Download the second icon (set not in history).
-  helper.history_handler()->favicon_data_.known_icon = false;
-  helper.history_handler()->InvokeCallback();
-  ASSERT_TRUE(download_handler->HasDownload());
-  EXPECT_EQ(icon_url_large, download_handler->GetImageUrl());
-  download_handler->SetImageSize(gfx::kFaviconSize * 2);
-  download_handler->InvokeCallback();
-
-  // Download the third icon (set not in history).
-  helper.history_handler()->favicon_data_.known_icon = false;
-  helper.history_handler()->InvokeCallback();
-  ASSERT_TRUE(download_handler->HasDownload());
-  EXPECT_EQ(icon_url_preferred, download_handler->GetImageUrl());
-  download_handler->SetImageSize(gfx::kFaviconSize);
-  download_handler->InvokeCallback();
-
-  // Verify correct icon size chosen.
-  EXPECT_EQ(icon_url_preferred, helper.GetEntry()->GetFavicon().url);
-  EXPECT_TRUE(helper.GetEntry()->GetFavicon().valid);
-  EXPECT_FALSE(helper.GetEntry()->GetFavicon().bitmap.empty());
-  EXPECT_EQ(gfx::kFaviconSize, helper.GetEntry()->GetFavicon().bitmap.width());
+  EXPECT_FALSE(helper.download_handler());
 }
 
 }  // namespace.