Improve FaviconHandler tests by testing public API

components/favicon/core/favicon_handler_unittest.cc

Index: components/favicon/core/favicon_handler_unittest.cc
diff --git a/components/favicon/core/favicon_handler_unittest.cc b/components/favicon/core/favicon_handler_unittest.cc
index f4370bc599b41392c98a425a37e796e5a3514820..7514a5e2ecdd7b1cf8d2a1e117afb282d042b684 100644
--- a/components/favicon/core/favicon_handler_unittest.cc
+++ b/components/favicon/core/favicon_handler_unittest.cc
@@ -11,7 +11,14 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/strings/stringprintf.h"
+#include "components/favicon/core/favicon_client.h"

[pkotwicz, 2017/03/17 05:43:11]

I don't think that you need this import?

[mastiz, 2017/03/17 12:17:31]

Removed.

 #include "components/favicon/core/favicon_driver.h"
+#include "components/favicon/core/test/mock_favicon_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/layout.h"
@@ -22,1201 +29,799 @@
 namespace favicon {
 namespace {
 
-// Fill the given bmp with valid png data.
-void FillDataToBitmap(int w, int h, SkBitmap* bmp) {
-  bmp->allocN32Pixels(w, h);
+using favicon_base::FAVICON;
+using favicon_base::FaviconRawBitmapResult;
+using favicon_base::TOUCH_ICON;
+using favicon_base::TOUCH_PRECOMPOSED_ICON;
+using testing::AnyNumber;
+using testing::Assign;
+using testing::AtLeast;
+using testing::AtMost;

[pkotwicz, 2017/03/17 05:43:12]

This doesn't seem to be used. :)

[mastiz, 2017/03/17 12:17:31]

Done.

+using testing::ElementsAre;
+using testing::InSequence;
+using testing::Invoke;
+using testing::IsEmpty;
+using testing::Return;
+using testing::_;
+
+using IntVector = std::vector<int>;
+using URLVector = std::vector<GURL>;
+using BitmapVector = std::vector<SkBitmap>;
+using SizeVector = std::vector<gfx::Size>;
+
+MATCHER_P2(ImageSizeIs, width, height, "") {
+  *result_listener << "where size is " << arg.Width() << "x" << arg.Height();
+  return arg.Size() == gfx::Size(width, height);
+}
+
+// Fill the given bmp with some test data.
+SkBitmap CreateBitmap(int w, int h) {
+  SkBitmap bmp;
+  bmp.allocN32Pixels(w, h);
 
   unsigned char* src_data =
-      reinterpret_cast<unsigned char*>(bmp->getAddr32(0, 0));
+      reinterpret_cast<unsigned char*>(bmp.getAddr32(0, 0));
   for (int i = 0; i < w * h; i++) {
     src_data[i * 4 + 0] = static_cast<unsigned char>(i % 255);
     src_data[i * 4 + 1] = static_cast<unsigned char>(i % 255);
     src_data[i * 4 + 2] = static_cast<unsigned char>(i % 255);
     src_data[i * 4 + 3] = static_cast<unsigned char>(i % 255);
   }
+  return bmp;
 }
 
 // Fill the given data buffer with valid png data.
 void FillBitmap(int w, int h, std::vector<unsigned char>* output) {
-  SkBitmap bitmap;
-  FillDataToBitmap(w, h, &bitmap);
+  SkBitmap bitmap = CreateBitmap(w, h);
   gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, false, output);
 }
 
-void SetFaviconRawBitmapResult(
+std::vector<gfx::Size> CreateSquareSizes(const IntVector& sizes) {

[pkotwicz, 2017/03/17 05:43:11]

There are two callers of this method. There is only one caller of
CreateBitmaps()

Can you:
- Rename CreateBitmaps() to CreateBitmapsWithEdgeSizes() which takes in a
std::vector<int>. That reduces the number of callers of CreateSquareSizes() to
1.
- Can you inline CreateSquareSizes() into AddWithOriginalSizes()

[mastiz, 2017/03/17 12:17:31]

Done. Since you seem to be concerned about the number of callers, I inlined
CreateBitmaps() too, which has one single caller. This leaves
CreateBitmapWithEdgeSize(), following your naming suggestion.

+  std::vector<gfx::Size> result;
+  for (int size : sizes) {
+    result.emplace_back(size, size);
+  }
+  return result;
+}
+
+std::vector<SkBitmap> CreateBitmaps(const std::vector<gfx::Size>& sizes) {
+  std::vector<SkBitmap> bitmaps;
+  for (const gfx::Size& size : sizes) {
+    bitmaps.push_back(CreateBitmap(size.width(), size.height()));
+  }
+  return bitmaps;
+}
+
+std::vector<FaviconRawBitmapResult> CreateRawBitmapResult(
     const GURL& icon_url,
-    favicon_base::IconType icon_type,
-    bool expired,
-    std::vector<favicon_base::FaviconRawBitmapResult>* favicon_bitmap_results) {
+    favicon_base::IconType icon_type = FAVICON,
+    bool expired = false) {
   scoped_refptr<base::RefCountedBytes> data(new base::RefCountedBytes());
   FillBitmap(gfx::kFaviconSize, gfx::kFaviconSize, &data->data());
-  favicon_base::FaviconRawBitmapResult bitmap_result;
+  FaviconRawBitmapResult bitmap_result;
   bitmap_result.expired = expired;
   bitmap_result.bitmap_data = data;
   // Use a pixel size other than (0,0) as (0,0) has a special meaning.
   bitmap_result.pixel_size = gfx::Size(gfx::kFaviconSize, gfx::kFaviconSize);
   bitmap_result.icon_type = icon_type;
   bitmap_result.icon_url = icon_url;
-
-  favicon_bitmap_results->push_back(bitmap_result);
-}
-
-void SetFaviconRawBitmapResult(
-    const GURL& icon_url,
-    std::vector<favicon_base::FaviconRawBitmapResult>* favicon_bitmap_results) {
-  SetFaviconRawBitmapResult(icon_url,
-                            favicon_base::FAVICON,
-                            false /* expired */,
-                            favicon_bitmap_results);
+  return {bitmap_result};
 }
 
-// This class is used to save the download request for verifying with test case.
-class DownloadHandler {
+// Fake that implements the calls to FaviconHalder::Delegate's DownloadImage(),
+// delegated to this class through MockDelegate.
+class FakeImageDownloader {
  public:
-  DownloadHandler() : callback_invoked_(false) {}
-  ~DownloadHandler() {}
-
-  void Reset() {
-    download_.reset();
-    callback_invoked_ = false;
-    // Does not affect |should_fail_download_icon_urls_| and
-    // |failed_download_icon_urls_|.
+  struct Response {
+    int http_status_code = 404;
+    BitmapVector bitmaps;
+    SizeVector original_bitmap_sizes;
+  };
+
+  // Implementation of FaviconHalder::Delegate's DownloadImage(). If a given
+  // URL is not known (i.e. not previously added via Add()), it produces 404s.
+  int DownloadImage(const GURL& url,
+                    int max_image_size,
+                    FaviconHandler::Delegate::ImageDownloadCallback callback) {
+    downloads_.push_back(url);
+
+    const Response& response = responses_[url];
+    int download_id = static_cast<int>(downloads_.size());

[pkotwicz, 2017/03/17 05:43:12]

The download id is affected by ClearDownloads() :(. That's a bug. We should be
able to not care about this parameter soon thanks to your follow up CL though

[mastiz, 2017/03/17 12:17:32]

Done.

+    base::Closure bound_callback =
+        base::Bind(callback, download_id, response.http_status_code, url,
+                   response.bitmaps, response.original_bitmap_sizes);
+    if (url == manual_callback_url_)
+      manual_callback_ = bound_callback;
+    else
+      base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, bound_callback);
+    return download_id;
   }
 
-  // Make downloads for any of |icon_urls| fail.
-  void FailDownloadForIconURLs(const std::set<GURL>& icon_urls) {
-    should_fail_download_icon_urls_ = icon_urls;
+  void Add(const GURL& icon_url, const IntVector& sizes) {
+    AddWithOriginalSizes(icon_url, sizes, sizes);
   }
 
-  // Returns whether a download for |icon_url| did fail.
-  bool DidFailDownloadForIconURL(const GURL& icon_url) const {
-    return failed_download_icon_urls_.count(icon_url);
+  void AddWithOriginalSizes(const GURL& icon_url,
+                            const IntVector& sizes,
+                            const IntVector& original_sizes) {
+    DCHECK_EQ(sizes.size(), original_sizes.size());
+    Response response;
+    response.http_status_code = 200;
+    response.original_bitmap_sizes = CreateSquareSizes(original_sizes);
+    response.bitmaps = CreateBitmaps(CreateSquareSizes(sizes));
+    responses_[icon_url] = response;
   }
 
-  void AddDownload(int download_id,
-                   const GURL& image_url,
-                   const std::vector<int>& image_sizes,
-                   int max_image_size,
-                   FaviconHandler::Delegate::ImageDownloadCallback callback) {
-    download_.reset(new Download(download_id, image_url, image_sizes,
-                                 max_image_size, callback));
+  void AddError(const GURL& icon_url, int http_status_code) {
+    Response response;
+    response.http_status_code = http_status_code;
+    responses_[icon_url] = response;
   }
 
-  void InvokeCallback();
+  // Disables automatic callback for |url|. This is useful for emulating a
+  // download taking a long time. The callback will for DownloadImage() will be

[pkotwicz, 2017/03/17 05:43:11]

Nit: "callback will for" -> "callback for"

[mastiz, 2017/03/17 12:17:32]

Done.

+  // stored in |manual_callback_|.
+  void SetRunCallbackManuallyForUrl(const GURL& url) {
+    manual_callback_url_ = url;
+  }
 
-  bool HasDownload() const { return download_.get(); }
-  const GURL& GetImageUrl() const { return download_->image_url; }
-  void SetImageSizes(const std::vector<int>& sizes) {
-    download_->image_sizes = sizes; }
+  // Returns whether an ongoing download exists for a url previously selected
+  // via SetRunCallbackManuallyForUrl().
+  bool HasPendingManualCallback() { return !manual_callback_.is_null(); }
+
+  // Triggers the response for a download previously selected for manual
+  // triggering via SetRunCallbackManuallyForUrl().
+  bool RunCallbackManually() {
+    if (!HasPendingManualCallback())
+      return false;
+    manual_callback_.Run();

[pkotwicz, 2017/03/17 05:43:12]

In order for HasPendingManualCallback() to return the correct value, this
function should reset |manual_callback_|.

I don't think that HasPendingManualCallback() is necessary (Crashing when trying
to run a Callback::is_null() callback is fine), but HasPendingManualCallback()
does not hurt either.

[mastiz, 2017/03/17 12:17:32]

Done. Crashing tests in failed expectations is not great so leaving
HasPendingManualCallback() there.

+    return true;
+  }
 
- private:
-  struct Download {
-    Download(int id,
-             GURL url,
-             const std::vector<int>& sizes,
-             int max_size,
-             FaviconHandler::Delegate::ImageDownloadCallback callback)
-        : download_id(id),
-          image_url(url),
-          image_sizes(sizes),
-          max_image_size(max_size),
-          callback(callback) {}
-    ~Download() {}
-    int download_id;
-    GURL image_url;
-    std::vector<int> image_sizes;
-    int max_image_size;
-    FaviconHandler::Delegate::ImageDownloadCallback callback;
-  };
+  // Returns pending and completed download URLs.
+  const URLVector& downloads() const { return downloads_; }
 
-  std::unique_ptr<Download> download_;
-  bool callback_invoked_;
+  void ClearDownloads() { downloads_.clear(); }
 
-  // The icon URLs for which the download should fail.
-  std::set<GURL> should_fail_download_icon_urls_;
+ private:
+  // URL to disable automatic callbacks for.
+  GURL manual_callback_url_;
+
+  // Pending and completed download URLs.
+  URLVector downloads_;
 
-  // The icon URLs for which the download did fail. This should be a subset of
-  // |should_fail_download_icon_urls_|.
-  std::set<GURL> failed_download_icon_urls_;
+  // Callback for DownloadImage() request for |manual_callback_url_|.

[pkotwicz, 2017/03/17 05:43:12]

Nit: Group |manual_callback_url_| and |manual_callback_| one after the other

[mastiz, 2017/03/17 12:17:32]

Done.

+  base::Closure manual_callback_;
 
-  DISALLOW_COPY_AND_ASSIGN(DownloadHandler);
+  // Registered images.

[pkotwicz, 2017/03/17 05:43:12]

How about: "Registered responses."

[mastiz, 2017/03/17 12:17:32]

Done.

+  std::map<GURL, Response> responses_;

[pkotwicz, 2017/03/17 05:43:11]

DISALLOW_COPY_AND_ASSIGN(FakeImageDownloader)

[mastiz, 2017/03/17 12:17:32]

Done, although I don't think this buys anything on test code.

 };
 
-// This class is used to save the history request for verifying with test case.
-// It also will be used to simulate the history response.
-class HistoryRequestHandler {
+class MockDelegate : public FaviconHandler::Delegate {
  public:
-  HistoryRequestHandler(const GURL& page_url,
-                        const GURL& icon_url,
-                        int icon_type,
-                        const favicon_base::FaviconResultsCallback& callback)
-      : page_url_(page_url),
-        icon_url_(icon_url),
-        icon_type_(icon_type),
-        callback_(callback) {
-  }
-
-  HistoryRequestHandler(const GURL& page_url,
-                        const GURL& icon_url,
-                        int icon_type,
-                        const std::vector<unsigned char>& bitmap_data,
-                        const gfx::Size& size)
-      : page_url_(page_url),
-        icon_url_(icon_url),
-        icon_type_(icon_type),
-        bitmap_data_(bitmap_data),
-        size_(size) {
+  MockDelegate() {
+    // Delegate image downloading to FakeImageDownloader.
+    ON_CALL(*this, DownloadImage(_, _, _))
+        .WillByDefault(
+            Invoke(&fake_downloader_, &FakeImageDownloader::DownloadImage));
   }
 
-  ~HistoryRequestHandler() {}
-  void InvokeCallback();
-
-  const GURL page_url_;
-  const GURL icon_url_;
-  const int icon_type_;
-  const std::vector<unsigned char> bitmap_data_;
-  const gfx::Size size_;
-  std::vector<favicon_base::FaviconRawBitmapResult> history_results_;
-  favicon_base::FaviconResultsCallback callback_;
+  MOCK_METHOD3(DownloadImage,
+               int(const GURL& url,
+                   int max_image_size,
+                   ImageDownloadCallback callback));
+  MOCK_METHOD0(IsOffTheRecord, bool());
+  MOCK_METHOD1(IsBookmarked, bool(const GURL& url));
+  MOCK_METHOD5(OnFaviconUpdated,
+               void(const GURL& page_url,
+                    FaviconDriverObserver::NotificationIconType type,
+                    const GURL& icon_url,
+                    bool icon_url_changed,
+                    const gfx::Image& image));
+
+  FakeImageDownloader& fake_downloader() { return fake_downloader_; }
+
+  // Convenience getter for test readability. Returns pending and completed
+  // download URLs.
+  const URLVector& downloads() const { return fake_downloader_.downloads(); }
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(HistoryRequestHandler);
+  FakeImageDownloader fake_downloader_;
 };
 
-class TestDelegate : public FaviconHandler::Delegate {
+// FakeFaviconService mimics a FaviconService backend that allows setting up
+// test data stored via Store(). If Store() has not been called for a
+// particular URL, the callback is called with empty database results.
+class FakeFaviconService {
  public:
-  TestDelegate() : num_notifications_(0), download_id_(0) {}
-
-  int DownloadImage(const GURL& image_url,
-                    int max_bitmap_size,
-                    ImageDownloadCallback callback) override {
-    // Do not do a download if downloading |image_url| failed previously. This
-    // emulates the behavior of FaviconDriver::DownloadImage()
-    if (download_handler_.DidFailDownloadForIconURL(image_url)) {
-      download_handler_.AddDownload(download_id_, image_url, std::vector<int>(),
-                                    0, callback);
-      return 0;
-    }
-
-    download_id_++;
-    std::vector<int> sizes;
-    sizes.push_back(0);
-    download_handler_.AddDownload(download_id_, image_url, sizes,
-                                  max_bitmap_size, callback);
-    return download_id_;
+  // Stores favicon with bitmap data in |results| at |page_url| and |icon_url|.
+  void Store(const GURL& page_url,
+             const GURL& icon_url,
+             const std::vector<favicon_base::FaviconRawBitmapResult>& result) {
+    results_[icon_url] = result;
+    results_[page_url] = result;
   }
 
-  bool IsOffTheRecord() override { return false; }
+  // Returns pending and completed database request URLs.
+  const URLVector& db_requests() const { return db_requests_; }
 
-  bool IsBookmarked(const GURL& url) override { return false; }
+  void ClearDbRequests() { db_requests_.clear(); }
 
-  void OnFaviconUpdated(
-      const GURL& page_url,
-      FaviconDriverObserver::NotificationIconType notification_icon_type,
+  base::CancelableTaskTracker::TaskId GetFavicon(
       const GURL& icon_url,
-      bool icon_url_changed,
-      const gfx::Image& image) override {
-    ++num_notifications_;
-    icon_url_ = icon_url;
-    image_ = image;
+      favicon_base::IconType icon_type,
+      int desired_size_in_dip,
+      const favicon_base::FaviconResultsCallback& callback,
+      base::CancelableTaskTracker* tracker) {
+    return GetFaviconForPageOrIconURL(icon_url, callback, tracker);
   }
 
-  DownloadHandler* download_handler() { return &download_handler_; }
-  const GURL& icon_url() const { return icon_url_; }
-  const gfx::Image& image() const { return image_; }
-  size_t num_notifications() const { return num_notifications_; }
-  void ResetNumNotifications() { num_notifications_ = 0; }
+  base::CancelableTaskTracker::TaskId GetFaviconForPageURL(
+      const GURL& page_url,
+      int icon_types,
+      int desired_size_in_dip,
+      const favicon_base::FaviconResultsCallback& callback,
+      base::CancelableTaskTracker* tracker) {
+    return GetFaviconForPageOrIconURL(page_url, callback, tracker);
+  }
 
- private:
-  GURL icon_url_;
-  gfx::Image image_;
-  size_t num_notifications_;
+  base::CancelableTaskTracker::TaskId UpdateFaviconMappingsAndFetch(
+      const GURL& page_url,
+      const std::vector<GURL>& icon_urls,
+      int icon_types,
+      int desired_size_in_dip,
+      const favicon_base::FaviconResultsCallback& callback,
+      base::CancelableTaskTracker* tracker) {
+    CHECK_EQ(1U, icon_urls.size()) << "Multi-icon lookup not implemented";
+    return GetFaviconForPageOrIconURL(icon_urls.front(), callback, tracker);
+  }
 
-  // The unique id of a download request. It will be returned to a
-  // FaviconHandler.
-  int download_id_;
+ private:
+  base::CancelableTaskTracker::TaskId GetFaviconForPageOrIconURL(
+      const GURL& page_or_icon_url,
+      const favicon_base::FaviconResultsCallback& callback,
+      base::CancelableTaskTracker* tracker) {
+    db_requests_.push_back(page_or_icon_url);
 
-  DownloadHandler download_handler_;
+    return tracker->PostTask(base::ThreadTaskRunnerHandle::Get().get(),
+                             FROM_HERE,
+                             base::Bind(callback, results_[page_or_icon_url]));
+  }
 
-  DISALLOW_COPY_AND_ASSIGN(TestDelegate);
+  std::map<GURL, std::vector<favicon_base::FaviconRawBitmapResult>> results_;
+  URLVector db_requests_;

[pkotwicz, 2017/03/17 05:43:12]

DISALLOW_COPY_AND_ASSIGN(FakeFaviconService)

[mastiz, 2017/03/17 12:17:31]

Done, same here.

 };
 
-}  // namespace
-
-// This class is used to catch the FaviconHandler's download and history
-// request, and also provide the methods to access the FaviconHandler
-// internals.
-class TestFaviconHandler : public FaviconHandler {
+// MockFaviconService subclass that delegates DB reads to FakeFaviconService.
+class MockFaviconServiceWithFake : public MockFaviconService {
  public:
-  static int GetMaximalIconSize(favicon_base::IconType icon_type) {
-    return FaviconHandler::GetMaximalIconSize(icon_type);
+  MockFaviconServiceWithFake() {
+    // Delegate the various methods that read from the DB.
+    ON_CALL(*this, GetFavicon(_, _, _, _, _))
+        .WillByDefault(Invoke(&fake_, &FakeFaviconService::GetFavicon));
+    ON_CALL(*this, GetFaviconForPageURL(_, _, _, _, _))
+        .WillByDefault(
+            Invoke(&fake_, &FakeFaviconService::GetFaviconForPageURL));
+    ON_CALL(*this, UpdateFaviconMappingsAndFetch(_, _, _, _, _, _))
+        .WillByDefault(
+            Invoke(&fake_, &FakeFaviconService::UpdateFaviconMappingsAndFetch));
   }
 
-  TestFaviconHandler(FaviconHandler::Delegate* delegate,
-                     FaviconDriverObserver::NotificationIconType handler_type)
-      : FaviconHandler(nullptr, delegate, handler_type) {}
+  FakeFaviconService* fake() { return &fake_; }
 
-  ~TestFaviconHandler() override {}
-
-  HistoryRequestHandler* history_handler() {
-    return history_handler_.get();
-  }
+ private:
+  FakeFaviconService fake_;
 
-  // This method will take the ownership of the given handler.
-  void set_history_handler(HistoryRequestHandler* handler) {
-    history_handler_.reset(handler);
-  }
+  DISALLOW_COPY_AND_ASSIGN(MockFaviconServiceWithFake);
+};
 
-  FaviconURL* current_candidate() {
-    return FaviconHandler::current_candidate();
-  }
+class FaviconHandlerTest : public testing::Test {
+ protected:
+  const std::vector<gfx::Size> kEmptySizes;
 
-  size_t current_candidate_index() const {
-    return current_candidate_index_;
-  }
+  // Some known icons for which download will succeed.
+  const GURL kPageURL = GURL("http://www.google.com");
+  const GURL kIconURL10x10 = GURL("http://www.google.com/favicon10x10");
+  const GURL kIconURL12x12 = GURL("http://www.google.com/favicon12x12");
+  const GURL kIconURL16x16 = GURL("http://www.google.com/favicon16x16");
+  const GURL kIconURL64x64 = GURL("http://www.google.com/favicon64x64");
 
-  const FaviconCandidate& best_favicon_candidate() {
-    return best_favicon_candidate_;
-  }
+  FaviconHandlerTest() {
+    // Register various known icon URLs.
+    delegate_.fake_downloader().Add(kIconURL10x10, IntVector{10});
+    delegate_.fake_downloader().Add(kIconURL12x12, IntVector{12});
+    delegate_.fake_downloader().Add(kIconURL16x16, IntVector{16});
+    delegate_.fake_downloader().Add(kIconURL64x64, IntVector{64});
 
- protected:
-  void UpdateFaviconMappingAndFetch(
-      const GURL& page_url,
-      const GURL& icon_url,
-      favicon_base::IconType icon_type,
-      const favicon_base::FaviconResultsCallback& callback,
-      base::CancelableTaskTracker* tracker) override {
-    history_handler_.reset(new HistoryRequestHandler(page_url, icon_url,
-                                                     icon_type, callback));
+    // The score computed by SelectFaviconFrames() is dependent on the supported
+    // scale factors of the platform. It is used for determining the goodness of
+    // a downloaded bitmap in FaviconHandler::OnDidDownloadFavicon().
+    // Force the values of the scale factors so that the tests produce the same
+    // results on all platforms.
+    scoped_set_supported_scale_factors_.reset(
+        new ui::test::ScopedSetSupportedScaleFactors({ui::SCALE_FACTOR_100P}));
   }
 
-  void GetFaviconFromFaviconService(
-      const GURL& icon_url,
-      favicon_base::IconType icon_type,
-      const favicon_base::FaviconResultsCallback& callback,
-      base::CancelableTaskTracker* tracker) override {
-    history_handler_.reset(new HistoryRequestHandler(GURL(), icon_url,
-                                                     icon_type, callback));
+  bool VerifyAndClearExpectations() {
+    base::RunLoop().RunUntilIdle();
+    favicon_service_.fake()->ClearDbRequests();
+    delegate_.fake_downloader().ClearDownloads();
+    return testing::Mock::VerifyAndClearExpectations(&favicon_service_) &&
+           testing::Mock::VerifyAndClearExpectations(&delegate_);
   }
 
-  void GetFaviconForURLFromFaviconService(
-      const GURL& page_url,
-      int icon_types,
-      const favicon_base::FaviconResultsCallback& callback,
-      base::CancelableTaskTracker* tracker) override {
-    history_handler_.reset(new HistoryRequestHandler(page_url, GURL(),
-                                                     icon_types, callback));
+  // Creates a new handler and feeds in the page URL and the candidates.
+  // Returns the handler in case tests want to exercise further steps.
+  std::unique_ptr<FaviconHandler> RunHandlerWithCandidates(
+      FaviconDriverObserver::NotificationIconType handler_type,
+      const std::vector<favicon::FaviconURL>& candidates) {
+    auto handler = base::MakeUnique<FaviconHandler>(&favicon_service_,
+                                                    &delegate_, handler_type);
+    handler->FetchFavicon(kPageURL);
+    // The first RunUntilIdle() causes the FaviconService lookups be faster than
+    // OnUpdateFaviconURL(), which is the most likely scenario.
+    base::RunLoop().RunUntilIdle();
+    handler->OnUpdateFaviconURL(kPageURL, candidates);
+    base::RunLoop().RunUntilIdle();
+    return handler;
   }
 
-  void SetHistoryFavicons(const GURL& page_url,
-                          const GURL& icon_url,
-                          favicon_base::IconType icon_type,
-                          const gfx::Image& image) override {
-    scoped_refptr<base::RefCountedMemory> bytes = image.As1xPNGBytes();
-    std::vector<unsigned char> bitmap_data(bytes->front(),
-                                           bytes->front() + bytes->size());
-    history_handler_.reset(new HistoryRequestHandler(
-        page_url, icon_url, icon_type, bitmap_data, image.Size()));
+  // Same as above, but for the simplest case where all types are FAVICON and
+  // no sizes are provided.

[pkotwicz, 2017/03/17 05:43:12]

You should document in the comment that a FaviconHandler of type
NON_TOUCH_16_DIP is created

[mastiz, 2017/03/17 12:17:32]

Done.

+  std::unique_ptr<FaviconHandler> RunHandlerWithSimpleFaviconCandidates(
+      const std::vector<GURL>& urls) {
+    std::vector<favicon::FaviconURL> candidates;
+    for (const GURL& url : urls) {
+      candidates.emplace_back(url, FAVICON, kEmptySizes);
+    }
+    return RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                                    candidates);
   }
 
-  bool ShouldSaveFavicon() override { return true; }
-
-  GURL page_url_;
+  base::MessageLoopForUI message_loop_;
+  std::unique_ptr<ui::test::ScopedSetSupportedScaleFactors>
+      scoped_set_supported_scale_factors_;
+  testing::NiceMock<MockFaviconServiceWithFake> favicon_service_;
+  testing::NiceMock<MockDelegate> delegate_;
+};
 
- private:
-  std::unique_ptr<HistoryRequestHandler> history_handler_;
+TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
+  const GURL kIconURL("http://www.google.com/favicon");
 
-  DISALLOW_COPY_AND_ASSIGN(TestFaviconHandler);
-};
+  favicon_service_.fake()->Store(kPageURL, kIconURL,
+                                 CreateRawBitmapResult(kIconURL));
 
-namespace {
+  // Shouldn't request to download icon.

[pkotwicz, 2017/03/17 05:43:12]

Remove this comment. (Or move it above line 400)

[mastiz, 2017/03/17 12:17:32]

Removed, since I'm not a big fan of comments that state the obvious.

+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kIconURL, /*icon_url_changed=*/true, _));
 
-void HistoryRequestHandler::InvokeCallback() {
-  if (!callback_.is_null()) {
-    callback_.Run(history_results_);
-  }
+  RunHandlerWithSimpleFaviconCandidates({kIconURL});
+  EXPECT_THAT(delegate_.downloads(), IsEmpty());
 }
 
-void DownloadHandler::InvokeCallback() {
-  if (callback_invoked_)
-    return;
-
-  std::vector<gfx::Size> original_bitmap_sizes;
-  std::vector<SkBitmap> bitmaps;
-  if (should_fail_download_icon_urls_.count(download_->image_url)) {
-    failed_download_icon_urls_.insert(download_->image_url);
-  } else {
-    for (std::vector<int>::const_iterator i = download_->image_sizes.begin();
-         i != download_->image_sizes.end(); ++i) {
-      int original_size = (*i > 0) ? *i : gfx::kFaviconSize;
-      int downloaded_size = original_size;
-      if (download_->max_image_size != 0 &&
-          downloaded_size > download_->max_image_size) {
-        downloaded_size = download_->max_image_size;
-      }
-      SkBitmap bitmap;
-      FillDataToBitmap(downloaded_size, downloaded_size, &bitmap);
-      bitmaps.push_back(bitmap);
-      original_bitmap_sizes.push_back(gfx::Size(original_size, original_size));
-    }
-  }
-  download_->callback.Run(download_->download_id,
-                          /*=status_code=*/200, download_->image_url, bitmaps,
-                          original_bitmap_sizes);
-  callback_invoked_ = true;
+// Test that the FaviconHandler process finishes when:
+// - There is no data in the database for neither the page URL nor the icon URL.

[pkotwicz, 2017/03/17 05:43:12]

Sorry my fault. This should be:
"There is data in the database for neither the page URL nor the icon URL."

[mastiz, 2017/03/17 12:17:31]

Done.

+// AND
+// - FaviconService::GetFaviconForPageURL() callback returns before
+//   FaviconHandler::OnUpdateFaviconURL() is called.
+TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesSlower) {
+  InSequence seq;
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL16x16},
+                                            FAVICON, _, _, _));

[pkotwicz, 2017/03/17 05:43:13]

I don't think that it is useful to test that UpdateFaviconMappingsAndFetch() is
called (as opposed to GetFavicon() in the context of this test.

You can test the database request via
EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre())

This enables you to remove the InSequence

[mastiz, 2017/03/17 12:17:33]

InSequence can be removed either way, the question is whether you want to verify
the order.

Which test do you think is responsible for testing the calls to
UpdateFaviconMappingsAndFetch? It looks to me that this one *and*
DownloadUnknownFaviconIfCandidatesFaster are a good fit.

[pkotwicz, 2017/03/17 15:22:31]

We currently don't have a test which tests that UpdateFaviconMappingsAndFetch()
is called. In my opinion, tests should test one thing only. I am fine with
adding a test which tests that UpdateFaviconMappingsAndFetch() is called in none
incognito mode

[mastiz, 2017/03/17 15:49:28]

Done, and added a TODO to introduce a new test.

+  EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, FAVICON,
+                                            ImageSizeIs(16, 16)));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kIconURL16x16, /*icon_url_changed=*/true, _));

[pkotwicz, 2017/03/17 05:43:13]

Only the icon URL parameters are intersting in the SetFavicons() and
OnFaviconUpdated() calls

[mastiz, 2017/03/17 12:17:32]

Same here: parameters for OnFaviconUpdated() must be verified somewhere, and
this particular test differs in codepath from GetFaviconFromHistory(), so I
don't think we should relax expectations, unless you have some other test in
mind.

[pkotwicz, 2017/03/17 15:22:31]

Yes, they should be verified somewhere. I suggest that they these expectation be
verified in new tests

In my CL I added FaviconHandler.ShouldCallSetFaviconsWithCorrectIconType() to
test this

As we have discussed previously, we can add new tests in a separate CL

[mastiz, 2017/03/17 15:49:28]

While I see this argument for UpdateFaviconMappingsAndFetch(), I think splitting
the verification of SetFavicons and OnFaviconUpdated, in this case, is overdoing
it. You could also split the verification of image downloads. WDYT?

[pkotwicz, 2017/03/20 15:01:16]

Fair enough. You can test all of the arguments for SetFavicons() and
OnFaviconUpdated() here.

+
+  auto handler = base::MakeUnique<FaviconHandler>(
+      &favicon_service_, &delegate_, FaviconDriverObserver::NON_TOUCH_16_DIP);

[pkotwicz, 2017/03/17 05:43:12]

Can you create FaviconHandler on the stack instead of wrapped in a
std::unique_ptr<> ?

[mastiz, 2017/03/17 12:17:32]

Done.

+  handler->FetchFavicon(kPageURL);
+  // Causes FaviconService lookups be faster than OnUpdateFaviconURL().
+  base::RunLoop().RunUntilIdle();
+  handler->OnUpdateFaviconURL(
+      kPageURL, {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
 }
 
-class FaviconHandlerTest : public testing::Test {
- protected:
-  FaviconHandlerTest() {
-  }
+// Test that the FaviconHandler process does not save anything to the database
+// for incognito tabs.
+TEST_F(FaviconHandlerTest, DownloadUnknownFaviconInIncognito) {
+  ON_CALL(delegate_, IsOffTheRecord()).WillByDefault(Return(true));
 
-  ~FaviconHandlerTest() override {}
+  // No writes expected.
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(_, _, _, _, _, _))
+      .Times(0);
+  EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
 
-  // Simulates requesting a favicon for |page_url| given:
-  // - We have not previously cached anything in history for |page_url| or for
-  //   any of |candidates|.
-  // - The page provides favicons at |candidate_icons|.
-  // - The favicons at |candidate_icons| have edge pixel sizes of
-  //   |candidate_icon_sizes|.
-  void DownloadTillDoneIgnoringHistory(
-      TestDelegate* delegate,
-      TestFaviconHandler* favicon_handler,
-      const GURL& page_url,
-      const std::vector<FaviconURL>& candidate_icons,
-      const int* candidate_icon_sizes) {
-    size_t old_num_notifications = delegate->num_notifications();
-
-    UpdateFaviconURL(delegate, favicon_handler, page_url, candidate_icons);
-    EXPECT_EQ(candidate_icons.size(), favicon_handler->image_urls().size());
-
-    DownloadHandler* download_handler = delegate->download_handler();
-    for (size_t i = 0; i < candidate_icons.size(); ++i) {
-      favicon_handler->history_handler()->history_results_.clear();
-      favicon_handler->history_handler()->InvokeCallback();
-      ASSERT_TRUE(download_handler->HasDownload());
-      EXPECT_EQ(download_handler->GetImageUrl(),
-                candidate_icons[i].icon_url);
-      std::vector<int> sizes;
-      sizes.push_back(candidate_icon_sizes[i]);
-      download_handler->SetImageSizes(sizes);
-      download_handler->InvokeCallback();
-
-      download_handler->Reset();
-
-      if (delegate->num_notifications() > old_num_notifications)
-        return;
-    }
-  }
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL16x16, _, _));
 
-  void UpdateFaviconURL(TestDelegate* delegate,
-                        TestFaviconHandler* favicon_handler,
-                        const GURL& page_url,
-                        const std::vector<FaviconURL>& candidate_icons) {
-    delegate->ResetNumNotifications();
+  RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));

[pkotwicz, 2017/03/17 05:43:13]

We should check that |kIconURL16x16| is requested from the database (though not
via UpdateFaviconMappingsAndFetch())

EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(...));

[mastiz, 2017/03/17 12:17:32]

Done.

+}
 
-    favicon_handler->FetchFavicon(page_url);
-    favicon_handler->history_handler()->InvokeCallback();
+// Test that the FaviconHandler process finishes when:
+// - There is no data in the database for neither the page URL nor the icon URL.
+// AND
+// - FaviconService::GetFaviconForPageURL() callback returns after
+//   FaviconHandler::OnUpdateFaviconURL() is called.
+TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesFaster) {

[pkotwicz, 2017/03/17 05:43:12]

Nit: Might as well make DownloadUnknownFaviconIfCandidatesSlower and
DownloadUnknownFaviconIfCandidatesFaster tests consecutive

[mastiz, 2017/03/17 12:17:32]

Done.

+  InSequence seq;
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL16x16},
+                                            FAVICON, _, _, _));

[pkotwicz, 2017/03/17 05:43:12]

You can check this by checking
EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageUrl,
kIconURL16x16));

Getting rid of this EXPECT_CALL() enables removing the InSequence

[mastiz, 2017/03/17 12:17:32]

Clarification needed in earlier comment, in
DownloadUnknownFaviconIfCandidatesSlower, as per where the parameters for
UpdateFaviconMappingsAndFetch should be verified.

+  EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, FAVICON,
+                                            ImageSizeIs(16, 16)));

[pkotwicz, 2017/03/17 05:43:11]

We only care about the icon URL parameter. We can ignore all of the other
parameters.
Thus:
EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL16x16, _, _);

[mastiz, 2017/03/17 12:17:32]

Ditto.

+  EXPECT_CALL(delegate_,

[pkotwicz, 2017/03/17 05:43:11]

We should check the icon URL. I don't think that it is necessary to test the
value of the |icon_url_changed| parameter in this test

[mastiz, 2017/03/17 12:17:32]

Done.

+              OnFaviconUpdated(_, _, _, /*icon_url_changed=*/true, _));
+
+  auto handler = base::MakeUnique<FaviconHandler>(
+      &favicon_service_, &delegate_, FaviconDriverObserver::NON_TOUCH_16_DIP);

[pkotwicz, 2017/03/17 05:43:11]

Can FaviconHandler be on the stack as opposed to being wrapped in a
std::unique_ptr<> ?

[mastiz, 2017/03/17 12:17:32]

Done.

+  handler->FetchFavicon(kPageURL);
+  ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL));

[pkotwicz, 2017/03/17 05:43:12]

Nit: Can you please add a new line?

[mastiz, 2017/03/17 12:17:32]

Done.

+  // Feed in favicons without processing posted tasks (RunUntilIdle()).
+  handler->OnUpdateFaviconURL(
+      kPageURL, {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
+}
 
-    favicon_handler->OnUpdateFaviconURL(page_url, candidate_icons);
-  }
+// Test that the icon is redownloaded if the icon cached for the page URL
+// expired.
+TEST_F(FaviconHandlerTest, RedownloadExpiredPageUrlFavicon) {
+  favicon_service_.fake()->Store(
+      kPageURL, kIconURL16x16,
+      CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true));
+
+  // TODO(crbug.com/700811): It would be nice if we could check whether the two
+  // OnFaviconUpdated() calls are called with different gfx::Images (as opposed
+  // to calling OnFaviconUpdated() with the expired gfx::Image both times).
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL16x16, _, _))
+      .Times(AtLeast(1));

[pkotwicz, 2017/03/17 05:43:12]

Nit: Times(2)

[mastiz, 2017/03/17 12:17:32]

Done.

+  EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL16x16, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
+  // We know from the |kPageUrl| database request that |kIconURL16x16| has
+  // expired. A second request for |kIconURL16x16| should not have been made
+  // because it is redundant.
+  ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL));

[pkotwicz, 2017/03/17 05:43:13]

ASSERT_THAT() -> EXPECT_THAT()

[mastiz, 2017/03/17 12:17:31]

Done.

+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
+}
 
-  void SetUp() override {
-    // The score computed by SelectFaviconFrames() is dependent on the supported
-    // scale factors of the platform. It is used for determining the goodness of
-    // a downloaded bitmap in FaviconHandler::OnDidDownloadFavicon().
-    // Force the values of the scale factors so that the tests produce the same
-    // results on all platforms.
-    std::vector<ui::ScaleFactor> scale_factors;
-    scale_factors.push_back(ui::SCALE_FACTOR_100P);
-    scoped_set_supported_scale_factors_.reset(
-        new ui::test::ScopedSetSupportedScaleFactors(scale_factors));
-    testing::Test::SetUp();
-  }
+TEST_F(FaviconHandlerTest, Report404) {
+  const GURL k404IconURL("http://www.google.com/404.png");
 
-  std::unique_ptr<ui::test::ScopedSetSupportedScaleFactors>
-      scoped_set_supported_scale_factors_;
-};
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(k404IconURL));
 
-TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
-  const GURL page_url("http://www.google.com");
-  const GURL icon_url("http://www.google.com/favicon");
-
-  TestDelegate delegate;
-  TestFaviconHandler helper(&delegate, FaviconDriverObserver::NON_TOUCH_16_DIP);
-
-  helper.FetchFavicon(page_url);
-  HistoryRequestHandler* history_handler = helper.history_handler();
-  // Ensure the data given to history is correct.
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-  EXPECT_EQ(GURL(), history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::FAVICON, history_handler->icon_type_);
-
-  SetFaviconRawBitmapResult(icon_url, &history_handler->history_results_);
-
-  // Send history response.
-  history_handler->InvokeCallback();
-  // Verify FaviconHandler status
-  EXPECT_EQ(1u, delegate.num_notifications());
-  EXPECT_EQ(icon_url, delegate.icon_url());
-
-  // Simulates update favicon url.
-  std::vector<FaviconURL> urls;
-  urls.push_back(
-      FaviconURL(icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(page_url, urls);
-
-  // Verify FaviconHandler status
-  EXPECT_EQ(1u, helper.image_urls().size());
-  ASSERT_TRUE(helper.current_candidate());
-  ASSERT_EQ(icon_url, helper.current_candidate()->icon_url);
-  ASSERT_EQ(favicon_base::FAVICON, helper.current_candidate()->icon_type);
-
-  // Favicon shouldn't request to download icon.
-  EXPECT_FALSE(delegate.download_handler()->HasDownload());
+  RunHandlerWithSimpleFaviconCandidates({k404IconURL});
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(k404IconURL));
 }
 
-TEST_F(FaviconHandlerTest, DownloadFavicon) {
-  const GURL page_url("http://www.google.com");
-  const GURL icon_url("http://www.google.com/favicon");
-
-  TestDelegate delegate;
-  TestFaviconHandler helper(&delegate, FaviconDriverObserver::NON_TOUCH_16_DIP);
-
-  helper.FetchFavicon(page_url);
-  HistoryRequestHandler* history_handler = helper.history_handler();
-  // Ensure the data given to history is correct.
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-  EXPECT_EQ(GURL(), history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::FAVICON, history_handler->icon_type_);
-
-  // Set icon data expired
-  SetFaviconRawBitmapResult(icon_url,
-                            favicon_base::FAVICON,
-                            true /* expired */,
-                            &history_handler->history_results_);
-  // Send history response.
-  history_handler->InvokeCallback();
-  // Verify FaviconHandler status
-  EXPECT_EQ(1u, delegate.num_notifications());
-  EXPECT_EQ(icon_url, delegate.icon_url());
-
-  // Simulates update favicon url.
-  std::vector<FaviconURL> urls;
-  urls.push_back(
-      FaviconURL(icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(page_url, urls);
-
-  // Verify FaviconHandler status
-  EXPECT_EQ(1u, helper.image_urls().size());
-  ASSERT_TRUE(helper.current_candidate());
-  ASSERT_EQ(icon_url, helper.current_candidate()->icon_url);
-  ASSERT_EQ(favicon_base::FAVICON, helper.current_candidate()->icon_type);
-
-  // Favicon should request to download icon now.
-  DownloadHandler* download_handler = delegate.download_handler();
-  EXPECT_TRUE(download_handler->HasDownload());
-
-  // Verify the download request.
-  EXPECT_EQ(icon_url, download_handler->GetImageUrl());
-
-  // Reset the history_handler to verify whether favicon is set.
-  helper.set_history_handler(nullptr);
-
-  // Smulates download done.
-  download_handler->InvokeCallback();
-
-  // New icon should be saved to history backend and navigation entry.
-  history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(icon_url, history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::FAVICON, history_handler->icon_type_);
-  EXPECT_LT(0U, history_handler->bitmap_data_.size());
-  EXPECT_EQ(page_url, history_handler->page_url_);
-
-  // Verify NavigationEntry.
-  EXPECT_EQ(2u, delegate.num_notifications());
-  EXPECT_EQ(icon_url, delegate.icon_url());
-  EXPECT_FALSE(delegate.image().IsEmpty());
-  EXPECT_EQ(gfx::kFaviconSize, delegate.image().Width());
+// Test that WasUnableToDownloadFavicon() is not called if a download returns
+// HTTP status 503.
+TEST_F(FaviconHandlerTest, NotReport503) {
+  const GURL k503IconURL("http://www.google.com/503.png");
+

[pkotwicz, 2017/03/17 05:43:11]

Nit: Remove new line

[mastiz, 2017/03/17 12:17:31]

All tests have a newline after URLs, I'm not following why you want to change
this one (there was one exception which I now made consistent).

[pkotwicz, 2017/03/17 15:22:31]

Ok, it is fine to stay as is

+  delegate_.fake_downloader().AddError(k503IconURL, 503);
+
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(_)).Times(0);
+
+  RunHandlerWithSimpleFaviconCandidates({k503IconURL});
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(k503IconURL));
 }
 
+// Test that FaviconHandler process finishes when:
+// - The icon URL used by the page has changed.
+// AND
+// - FaviconService::GetFaviconForPageURL() callback returns before
+//   FaviconHandler::OnUpdateFaviconURL() is called.

[pkotwicz, 2017/03/17 05:43:12]

I think that this comment better summarizes the test:

Test that FaviconHandler requests the new data when:
- There is valid data in the database for the page URL.
AND
- The icon URL used by the page has changed.
AND
- There is no data in database for the new icon URL.

[mastiz, 2017/03/17 12:17:32]

Done.

 TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
-  const GURL page_url("http://www.google.com");
-  const GURL icon_url("http://www.google.com/favicon");
-  const GURL new_icon_url("http://www.google.com/new_favicon");
-
-  TestDelegate delegate;
-  TestFaviconHandler helper(&delegate, FaviconDriverObserver::NON_TOUCH_16_DIP);
-
-  helper.FetchFavicon(page_url);
-  HistoryRequestHandler* history_handler = helper.history_handler();
-  // Ensure the data given to history is correct.
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-  EXPECT_EQ(GURL(), history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::FAVICON, history_handler->icon_type_);
-
-  // Set valid icon data.
-  SetFaviconRawBitmapResult(icon_url, &history_handler->history_results_);
-
-  // Send history response.
-  history_handler->InvokeCallback();
-  // Verify FaviconHandler status.
-  EXPECT_EQ(1u, delegate.num_notifications());
-  EXPECT_EQ(icon_url, delegate.icon_url());
-
-  // Reset the history_handler to verify whether new icon is requested from
-  // history.
-  helper.set_history_handler(nullptr);
-
-  // Simulates update with the different favicon url.
-  std::vector<FaviconURL> urls;
-  urls.push_back(FaviconURL(
-      new_icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(page_url, urls);
-
-  // Verify FaviconHandler status.
-  EXPECT_EQ(1u, helper.image_urls().size());
-  ASSERT_TRUE(helper.current_candidate());
-  ASSERT_EQ(new_icon_url, helper.current_candidate()->icon_url);
-  ASSERT_EQ(favicon_base::FAVICON, helper.current_candidate()->icon_type);
-
-  // Favicon should be requested from history.
-  history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(new_icon_url, history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::FAVICON, history_handler->icon_type_);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-
-  // Simulate not find icon.
-  history_handler->history_results_.clear();
-  history_handler->InvokeCallback();
-
-  // Favicon should request to download icon now.
-  DownloadHandler* download_handler = delegate.download_handler();
-  EXPECT_TRUE(download_handler->HasDownload());
-
-  // Verify the download request.
-  EXPECT_EQ(new_icon_url, download_handler->GetImageUrl());
-
-  // Reset the history_handler to verify whether favicon is set.
-  helper.set_history_handler(nullptr);
-
-  // Smulates download done.
-  download_handler->InvokeCallback();
-
-  // New icon should be saved to history backend and navigation entry.
-  history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(new_icon_url, history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::FAVICON, history_handler->icon_type_);
-  EXPECT_LT(0U, history_handler->bitmap_data_.size());
-  EXPECT_EQ(page_url, history_handler->page_url_);
-
-  // Verify NavigationEntry.
-  EXPECT_EQ(2u, delegate.num_notifications());
-  EXPECT_EQ(new_icon_url, delegate.icon_url());
-  EXPECT_FALSE(delegate.image().IsEmpty());
-  EXPECT_EQ(gfx::kFaviconSize, delegate.image().Width());
-}
+  const GURL kOldIconURL("http://www.google.com/old_favicon");
+  const GURL kNewIconURL = kIconURL16x16;
 
-TEST_F(FaviconHandlerTest, FaviconInHistoryInvalid) {
-  const GURL page_url("http://www.google.com");
-  const GURL icon_url("http://www.google.com/favicon");
+  favicon_service_.fake()->Store(kPageURL, kOldIconURL,
+                                 CreateRawBitmapResult(kOldIconURL));
 
-  TestDelegate delegate;
-  TestFaviconHandler helper(&delegate, FaviconDriverObserver::NON_TOUCH_16_DIP);
+  InSequence seq;
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kOldIconURL, _, _));
+  EXPECT_CALL(favicon_service_, SetFavicons(_, kNewIconURL, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kNewIconURL, _, _));
 
-  helper.FetchFavicon(page_url);
-  HistoryRequestHandler* history_handler = helper.history_handler();
-  // Ensure the data given to history is correct.
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-  EXPECT_EQ(GURL(), history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::FAVICON, history_handler->icon_type_);
+  RunHandlerWithSimpleFaviconCandidates({kNewIconURL});
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kNewIconURL));
+}
 
+// If there is data for the page URL in history which is invalid, test that:
+// - The invalid data is not sent to the UI.
+// - The icon is redownloaded.
+TEST_F(FaviconHandlerTest, FaviconInHistoryInvalid) {
   // Set non empty but invalid data.
-  favicon_base::FaviconRawBitmapResult bitmap_result;
+  FaviconRawBitmapResult bitmap_result;
   bitmap_result.expired = false;
   // Empty bitmap data is invalid.
   bitmap_result.bitmap_data = new base::RefCountedBytes();
   bitmap_result.pixel_size = gfx::Size(gfx::kFaviconSize, gfx::kFaviconSize);
-  bitmap_result.icon_type = favicon_base::FAVICON;
-  bitmap_result.icon_url = icon_url;
-  history_handler->history_results_.clear();
-  history_handler->history_results_.push_back(bitmap_result);
-
-  // Send history response.
-  history_handler->InvokeCallback();
-  // The NavigationEntry should not be set yet as the history data is invalid.
-  EXPECT_EQ(0u, delegate.num_notifications());
-  EXPECT_EQ(GURL(), delegate.icon_url());
-
-  // Reset the history_handler to verify whether new icon is requested from
-  // history.
-  helper.set_history_handler(nullptr);
-
-  // Simulates update with matching favicon URL.
-  std::vector<FaviconURL> urls;
-  urls.push_back(
-      FaviconURL(icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(page_url, urls);
-
-  // A download for the favicon should be requested, and we should not do
-  // another history request.
-  DownloadHandler* download_handler = delegate.download_handler();
-  EXPECT_TRUE(download_handler->HasDownload());
-  EXPECT_EQ(nullptr, helper.history_handler());
-
-  // Verify the download request.
-  EXPECT_EQ(icon_url, download_handler->GetImageUrl());
-
-  // Simulates download done.
-  download_handler->InvokeCallback();
-
-  // New icon should be saved to history backend and navigation entry.
-  history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(icon_url, history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::FAVICON, history_handler->icon_type_);
-  EXPECT_LT(0U, history_handler->bitmap_data_.size());
-  EXPECT_EQ(page_url, history_handler->page_url_);
-
-  // Verify NavigationEntry.
-  EXPECT_EQ(1u, delegate.num_notifications());
-  EXPECT_EQ(icon_url, delegate.icon_url());
-  EXPECT_FALSE(delegate.image().IsEmpty());
-  EXPECT_EQ(gfx::kFaviconSize, delegate.image().Width());
+  bitmap_result.icon_type = FAVICON;
+  bitmap_result.icon_url = kIconURL16x16;
+
+  favicon_service_.fake()->Store(kPageURL, kIconURL16x16, {bitmap_result});
+
+  // TODO(crbug.com/700811): It would be nice if we could check the image
+  // being published to rule out invalid data.
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL16x16, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
+
+  EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
 }
 
+// Test that no downloads are done if a user visits a page which changed its
+// favicon URL to a favicon URL which is already cached in the database.
 TEST_F(FaviconHandlerTest, UpdateFavicon) {
-  const GURL page_url("http://www.google.com");
-  const GURL icon_url("http://www.google.com/favicon");
-  const GURL new_icon_url("http://www.google.com/new_favicon");
-
-  TestDelegate delegate;
-  TestFaviconHandler helper(&delegate, FaviconDriverObserver::NON_TOUCH_16_DIP);
-
-  helper.FetchFavicon(page_url);
-  HistoryRequestHandler* history_handler = helper.history_handler();
-  // Ensure the data given to history is correct.
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-  EXPECT_EQ(GURL(), history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::FAVICON, history_handler->icon_type_);
-
-  SetFaviconRawBitmapResult(icon_url, &history_handler->history_results_);
-
-  // Send history response.
-  history_handler->InvokeCallback();
-  // Verify FaviconHandler status.
-  EXPECT_EQ(1u, delegate.num_notifications());
-  EXPECT_EQ(icon_url, delegate.icon_url());
-
-  // Reset the history_handler to verify whether new icon is requested from
-  // history.
-  helper.set_history_handler(nullptr);
-
-  // Simulates update with the different favicon url.
-  std::vector<FaviconURL> urls;
-  urls.push_back(FaviconURL(
-      new_icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(page_url, urls);
-
-  // Verify FaviconHandler status.
-  EXPECT_EQ(1u, helper.image_urls().size());
-  ASSERT_TRUE(helper.current_candidate());
-  ASSERT_EQ(new_icon_url, helper.current_candidate()->icon_url);
-  ASSERT_EQ(favicon_base::FAVICON, helper.current_candidate()->icon_type);
-
-  // Favicon should be requested from history.
-  history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(new_icon_url, history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::FAVICON, history_handler->icon_type_);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-
-  // Simulate find icon.
-  SetFaviconRawBitmapResult(new_icon_url, &history_handler->history_results_);
-  history_handler->InvokeCallback();
-
-  // Shouldn't request download favicon
-  EXPECT_FALSE(delegate.download_handler()->HasDownload());
-
-  // Verify the favicon status.
-  EXPECT_EQ(2u, delegate.num_notifications());
-  EXPECT_EQ(new_icon_url, delegate.icon_url());
-  EXPECT_FALSE(delegate.image().IsEmpty());
+  const GURL kSomePreviousPageURL("https://www.google.com/previous");
+  const GURL kIconURL("http://www.google.com/favicon");
+  const GURL kNewIconURL("http://www.google.com/new_favicon");
+
+  favicon_service_.fake()->Store(kPageURL, kIconURL,
+                                 CreateRawBitmapResult(kIconURL));
+  favicon_service_.fake()->Store(kSomePreviousPageURL, kNewIconURL,
+                                 CreateRawBitmapResult(kNewIconURL));
+
+  EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
+
+  InSequence seq;
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kNewIconURL, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kNewIconURL});
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kNewIconURL));
+  EXPECT_THAT(delegate_.downloads(), IsEmpty());
 }
 
 TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
-  const GURL page_url("http://www.google.com");
-  const GURL icon_url("http://www.google.com/favicon");
-  const GURL new_icon_url("http://www.google.com/new_favicon");
-
-  TestDelegate delegate;
-  TestFaviconHandler helper(&delegate, FaviconDriverObserver::TOUCH_LARGEST);
-  std::set<GURL> fail_downloads;
-  fail_downloads.insert(icon_url);
-  delegate.download_handler()->FailDownloadForIconURLs(fail_downloads);
-
-  helper.FetchFavicon(page_url);
-  HistoryRequestHandler* history_handler = helper.history_handler();
-  // Ensure the data given to history is correct.
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-  EXPECT_EQ(GURL(), history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::TOUCH_PRECOMPOSED_ICON | favicon_base::TOUCH_ICON,
-            history_handler->icon_type_);
-
-  // Icon not found.
-  history_handler->history_results_.clear();
-  // Send history response.
-  history_handler->InvokeCallback();
-  // Verify FaviconHandler status.
-  EXPECT_EQ(0u, delegate.num_notifications());
-  EXPECT_EQ(GURL(), delegate.icon_url());
-
-  // Reset the history_handler to verify whether new icon is requested from
-  // history.
-  helper.set_history_handler(nullptr);
-
-  // Simulates update with the different favicon url.
-  std::vector<FaviconURL> urls;
-  urls.push_back(FaviconURL(icon_url,
-                            favicon_base::TOUCH_PRECOMPOSED_ICON,
-                            std::vector<gfx::Size>()));
-  urls.push_back(FaviconURL(
-      new_icon_url, favicon_base::TOUCH_ICON, std::vector<gfx::Size>()));
-  urls.push_back(FaviconURL(
-      new_icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(page_url, urls);
-
-  // Verify FaviconHandler status.
-  EXPECT_EQ(2u, helper.image_urls().size());
-  EXPECT_EQ(0u, helper.current_candidate_index());
-  ASSERT_TRUE(helper.current_candidate());
-  ASSERT_EQ(icon_url, helper.current_candidate()->icon_url);
-  ASSERT_EQ(favicon_base::TOUCH_PRECOMPOSED_ICON,
-            helper.current_candidate()->icon_type);
-
-  // Favicon should be requested from history.
-  history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(icon_url, history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::TOUCH_PRECOMPOSED_ICON, history_handler->icon_type_);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-
-  // Simulate not find icon.
-  history_handler->history_results_.clear();
-  history_handler->InvokeCallback();
-
-  // Should request download favicon.
-  DownloadHandler* download_handler = delegate.download_handler();
-  EXPECT_TRUE(download_handler->HasDownload());
-
-  // Verify the download request.
-  EXPECT_EQ(icon_url, download_handler->GetImageUrl());
-
-  // Reset the history_handler to verify whether favicon is request from
-  // history.
-  helper.set_history_handler(nullptr);
-  download_handler->InvokeCallback();
-
-  // Left 1 url.
-  EXPECT_EQ(1u, helper.current_candidate_index());
-  ASSERT_TRUE(helper.current_candidate());
-  EXPECT_EQ(new_icon_url, helper.current_candidate()->icon_url);
-  EXPECT_EQ(favicon_base::TOUCH_ICON, helper.current_candidate()->icon_type);
-
-  // Favicon should be requested from history.
-  history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(new_icon_url, history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::TOUCH_ICON, history_handler->icon_type_);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-
-  // Reset download handler
-  download_handler->Reset();
-
-  // Simulates getting a expired icon from history.
-  SetFaviconRawBitmapResult(new_icon_url,
-                            favicon_base::TOUCH_ICON,
-                            true /* expired */,
-                            &history_handler->history_results_);
-  history_handler->InvokeCallback();
-
-  // Verify the download request.
-  EXPECT_TRUE(delegate.download_handler()->HasDownload());
-  EXPECT_EQ(new_icon_url, download_handler->GetImageUrl());
-
-  helper.set_history_handler(nullptr);
-
-  // Simulates icon being downloaded.
-  download_handler->InvokeCallback();
-
-  // New icon should be saved to history backend.
-  history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(new_icon_url, history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::TOUCH_ICON, history_handler->icon_type_);
-  EXPECT_LT(0U, history_handler->bitmap_data_.size());
-  EXPECT_EQ(page_url, history_handler->page_url_);
+  const GURL kIconURLReturning500("http://www.google.com/500.png");
+  delegate_.fake_downloader().AddError(kIconURLReturning500, 500);
+
+  favicon_service_.fake()->Store(
+      kPageURL, kIconURL64x64,
+      CreateRawBitmapResult(kIconURL64x64, TOUCH_ICON,
+                            /*expired=*/true));
+
+  EXPECT_CALL(delegate_,
+              OnFaviconUpdated(kPageURL, FaviconDriverObserver::TOUCH_LARGEST,
+                               kIconURL64x64, /*icon_url_changed=*/true, _));
+  EXPECT_CALL(delegate_,
+              OnFaviconUpdated(kPageURL, FaviconDriverObserver::TOUCH_LARGEST,
+                               kIconURL64x64, /*icon_url_changed=*/false, _));
+
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::TOUCH_LARGEST,
+      {
+          FaviconURL(kIconURLReturning500, TOUCH_PRECOMPOSED_ICON, kEmptySizes),
+          FaviconURL(kIconURL64x64, TOUCH_ICON, kEmptySizes),
+      });
+  // First download fails, second succeeds.
+  EXPECT_THAT(delegate_.downloads(),
+              ElementsAre(kIconURLReturning500, kIconURL64x64));
 }
 
+// TODO(mastiz): Make this test deal with FaviconURLs of type
+// favicon_base::FAVICON and add new ones like OnlyDownloadMatchingIconType and
+// CallSetFaviconsWithCorrectIconType.
 TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {

[pkotwicz, 2017/03/17 05:43:11]

For the commment, how about:

"Test that download data for icon URLs other than the current favicon
candidate URLs is ignored. This test tests the scenario where a download is in
flight when FaviconHandler::OnUpdateFaviconURL() is called."

[mastiz, 2017/03/17 12:17:31]

Done.

-  const GURL page_url("http://www.google.com");
-  const GURL icon_url("http://www.google.com/favicon");
-  const GURL new_icon_url("http://www.google.com/new_favicon");
-
-  TestDelegate delegate;
-  TestFaviconHandler helper(&delegate, FaviconDriverObserver::TOUCH_LARGEST);
-
-  helper.FetchFavicon(page_url);
-  HistoryRequestHandler* history_handler = helper.history_handler();
-  // Ensure the data given to history is correct.
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-  EXPECT_EQ(GURL(), history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::TOUCH_PRECOMPOSED_ICON | favicon_base::TOUCH_ICON,
-            history_handler->icon_type_);
-
-  // Icon not found.
-  history_handler->history_results_.clear();
-  // Send history response.
-  history_handler->InvokeCallback();
-  // Verify FaviconHandler status.
-  EXPECT_EQ(0u, delegate.num_notifications());
-  EXPECT_EQ(GURL(), delegate.icon_url());
-
-  // Reset the history_handler to verify whether new icon is requested from
-  // history.
-  helper.set_history_handler(nullptr);
-
-  // Simulates update with the different favicon url.
-  std::vector<FaviconURL> urls;
-  urls.push_back(FaviconURL(icon_url,
-                            favicon_base::TOUCH_PRECOMPOSED_ICON,
-                            std::vector<gfx::Size>()));
-  urls.push_back(FaviconURL(
-      new_icon_url, favicon_base::TOUCH_ICON, std::vector<gfx::Size>()));
-  urls.push_back(FaviconURL(
-      new_icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(page_url, urls);
-
-  // Verify FaviconHandler status.
-  EXPECT_EQ(2u, helper.image_urls().size());
-  ASSERT_EQ(0u, helper.current_candidate_index());
-  ASSERT_EQ(icon_url, helper.current_candidate()->icon_url);
-  ASSERT_EQ(favicon_base::TOUCH_PRECOMPOSED_ICON,
-            helper.current_candidate()->icon_type);
-
-  // Favicon should be requested from history.
-  history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(icon_url, history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::TOUCH_PRECOMPOSED_ICON, history_handler->icon_type_);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-
-  // Simulate not find icon.
-  history_handler->history_results_.clear();
-  history_handler->InvokeCallback();
-
-  // Should request download favicon.
-  DownloadHandler* download_handler = delegate.download_handler();
-  EXPECT_TRUE(download_handler->HasDownload());
-
-  // Verify the download request.
-  EXPECT_EQ(icon_url, download_handler->GetImageUrl());
-
-  // Reset the history_handler to verify whether favicon is request from
-  // history.
-  helper.set_history_handler(nullptr);
-  const GURL latest_icon_url("http://www.google.com/latest_favicon");
-  std::vector<FaviconURL> latest_urls;
-  latest_urls.push_back(FaviconURL(
-      latest_icon_url, favicon_base::TOUCH_ICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(page_url, latest_urls);
-
-  EXPECT_EQ(1u, helper.image_urls().size());
-  EXPECT_EQ(0u, helper.current_candidate_index());
-  EXPECT_EQ(latest_icon_url, helper.current_candidate()->icon_url);
-  EXPECT_EQ(favicon_base::TOUCH_ICON, helper.current_candidate()->icon_type);
-
-  // Whether new icon is requested from history
-  history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-  EXPECT_EQ(latest_icon_url, history_handler->icon_url_);
-  EXPECT_EQ(favicon_base::TOUCH_ICON, history_handler->icon_type_);
-  EXPECT_EQ(page_url, history_handler->page_url_);
-
-  // Reset the history_handler to verify whether favicon is request from
-  // history.
-  // Save the callback for late use.
-  favicon_base::FaviconResultsCallback callback = history_handler->callback_;
-  helper.set_history_handler(nullptr);
-
-  // Simulates download succeed.
-  download_handler->InvokeCallback();
-  // The downloaded icon should be thrown away as there is favicon update.
-  EXPECT_FALSE(helper.history_handler());
-
-  download_handler->Reset();
-
-  // Simulates getting the icon from history.
-  std::unique_ptr<HistoryRequestHandler> handler;
-  handler.reset(new HistoryRequestHandler(
-      page_url, latest_icon_url, favicon_base::TOUCH_ICON, callback));
-  SetFaviconRawBitmapResult(latest_icon_url,
-                            favicon_base::TOUCH_ICON,
-                            false /* expired */,
-                            &handler->history_results_);
-  handler->InvokeCallback();
-
-  // No download request.
-  EXPECT_FALSE(download_handler->HasDownload());
+  const GURL kIconURL("http://www.google.com/favicon");

[pkotwicz, 2017/03/17 05:43:12]

Nit: make these |kIconURL1| - |kIconURL3|

[mastiz, 2017/03/17 12:17:32]

Done.

+  const GURL kIconURL1 = kIconURL16x16;
+  const GURL kIconURL2 = kIconURL64x64;
+
+  // Defer the download completion such that RunUntilIdle() doesn't complete
+  // the download.
+  delegate_.fake_downloader().SetRunCallbackManuallyForUrl(kIconURL);

[pkotwicz, 2017/03/17 05:43:12]

Nit: Can you please add a new line?

[mastiz, 2017/03/17 12:17:32]

Done.

+  delegate_.fake_downloader().Add(kIconURL, IntVector{16});
+  delegate_.fake_downloader().Add(kIconURL2, IntVector{64});
+
+  std::unique_ptr<FaviconHandler> handler =
+      RunHandlerWithSimpleFaviconCandidates({kIconURL, kIconURL1});
+
+  ASSERT_TRUE(VerifyAndClearExpectations());
+  ASSERT_TRUE(delegate_.fake_downloader().HasPendingManualCallback());
+
+  // Favicon update should invalidate the ongoing download.
+  EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL2, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL2, _, _));
+
+  handler->OnUpdateFaviconURL(kPageURL,
+                              {FaviconURL(kIconURL2, FAVICON, kEmptySizes)});
+
+  // Finalizes download, which should be thrown away as the favicon URLs were
+  // updated.
+  EXPECT_TRUE(delegate_.fake_downloader().RunCallbackManually());
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kIconURL2));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL2));
 }
 
 // Test that sending an icon URL update identical to the previous icon URL
 // update is a no-op.
-TEST_F(FaviconHandlerTest, UpdateSameIconURLs) {
-  const GURL page_url("http://www.google.com");
-  const GURL icon_url1("http://www.google.com/favicon1");
-  const GURL icon_url2("http://www.google.com/favicon2");
-  std::vector<FaviconURL> favicon_urls;
-  favicon_urls.push_back(FaviconURL(GURL("http://www.google.com/favicon1"),
-                                    favicon_base::FAVICON,
-                                    std::vector<gfx::Size>()));
-  favicon_urls.push_back(FaviconURL(GURL("http://www.google.com/favicon2"),
-                                    favicon_base::FAVICON,
-                                    std::vector<gfx::Size>()));
-
-  TestDelegate delegate;
-  TestFaviconHandler helper(&delegate, FaviconDriverObserver::NON_TOUCH_16_DIP);
-
-  // Initiate a request for favicon data for |page_url|. History does not know
-  // about the page URL or the icon URLs.
-  helper.FetchFavicon(page_url);
-  helper.history_handler()->InvokeCallback();
-  helper.set_history_handler(nullptr);
-
-  // Got icon URLs.
-  helper.OnUpdateFaviconURL(page_url, favicon_urls);
-
-  // There should be an ongoing history request for |icon_url1|.
-  ASSERT_EQ(2u, helper.image_urls().size());
-  ASSERT_EQ(0u, helper.current_candidate_index());
-  HistoryRequestHandler* history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-
-  // Calling OnUpdateFaviconURL() with the same icon URLs should have no effect.
-  helper.OnUpdateFaviconURL(page_url, favicon_urls);
-  EXPECT_EQ(history_handler, helper.history_handler());
-
-  // Complete history request for |icon_url1| and do download.
-  helper.history_handler()->InvokeCallback();
-  helper.set_history_handler(nullptr);
-  delegate.download_handler()->SetImageSizes(std::vector<int>(1u, 10));
-  delegate.download_handler()->InvokeCallback();
-  delegate.download_handler()->Reset();
-
-  // There should now be an ongoing history request for |icon_url2|.
-  ASSERT_EQ(1u, helper.current_candidate_index());
-  history_handler = helper.history_handler();
-  ASSERT_TRUE(history_handler);
-
-  // Calling OnUpdateFaviconURL() with the same icon URLs should have no effect.
-  helper.OnUpdateFaviconURL(page_url, favicon_urls);
-  EXPECT_EQ(history_handler, helper.history_handler());
+TEST_F(FaviconHandlerTest, UpdateSameIconURLsWhileProcessingShouldBeNoop) {
+  const GURL kSlowLoadingIconURL("http://www.google.com/slow_favicon");
+
+  const std::vector<FaviconURL> favicon_urls = {
+      FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+      FaviconURL(kSlowLoadingIconURL, FAVICON, kEmptySizes),
+  };
+
+  // Defer the download completion such that RunUntilIdle() doesn't complete
+  // the download.
+  delegate_.fake_downloader().SetRunCallbackManuallyForUrl(kSlowLoadingIconURL);
+  delegate_.fake_downloader().Add(kSlowLoadingIconURL, IntVector{16});
+
+  std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls);
+
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kIconURL64x64, kSlowLoadingIconURL));
+  ASSERT_TRUE(VerifyAndClearExpectations());
+  ASSERT_TRUE(delegate_.fake_downloader().HasPendingManualCallback());
+
+  // Calling OnUpdateFaviconURL() with the same icon URLs should have no effect,
+  // despite the ongoing download.
+  handler->OnUpdateFaviconURL(kPageURL, favicon_urls);
+  base::RunLoop().RunUntilIdle();
+
+  // Complete the download.
+  EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _));
+  EXPECT_TRUE(delegate_.fake_downloader().RunCallbackManually());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_THAT(delegate_.downloads(), IsEmpty());
+}
+
+// Test that calling OnUpdateFaviconUrl() with the same icon URLs as before is a
+// no-op. This is important because OnUpdateFaviconUrl() is called when the page
+// finishes loading. This can occur several times for pages with iframes.
+TEST_F(FaviconHandlerTest, UpdateSameIconURLsAfterFinishedShouldBeNoop) {
+  const std::vector<FaviconURL> favicon_urls = {
+      FaviconURL(kIconURL10x10, FAVICON, kEmptySizes),
+      FaviconURL(kIconURL16x16, FAVICON, kEmptySizes),
+  };
+
+  std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls);
+
+  ASSERT_TRUE(VerifyAndClearExpectations());
+
+  // Calling OnUpdateFaviconURL() with identical data should be a no-op.
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0);
+  EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
+
+  handler->OnUpdateFaviconURL(kPageURL, favicon_urls);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_THAT(favicon_service_.fake()->db_requests(), IsEmpty());
+  EXPECT_THAT(delegate_.downloads(), IsEmpty());
 }
 
 // Fixes crbug.com/544560
+// Tests that Delegate::OnFaviconUpdated() is called if:
+// - The best icon on the initial page is not the last icon.
+// - All of the initial page's icons are downloaded.
+// AND
+// - JavaScript modifies the page's <link rel="icon"> tags to contain only the
+//   last icon.
 TEST_F(FaviconHandlerTest,
        OnFaviconAvailableNotificationSentAfterIconURLChange) {
-  const GURL kPageURL("http://www.page_which_animates_favicon.com");
-  const GURL kIconURL1("http://wwww.page_which_animates_favicon.com/frame1.png");
-  const GURL kIconURL2("http://wwww.page_which_animates_favicon.com/frame2.png");
-
-  TestDelegate delegate;
-  TestFaviconHandler helper(&delegate, FaviconDriverObserver::NON_TOUCH_16_DIP);
-
-  // Initial state:
-  // - The database does not know about |kPageURL|.
-  // - The page uses |kIconURL1| and |kIconURL2|.
-  // - The database knows about both |kIconURL1| and |kIconURl2|. Both icons
-  //   are expired in the database.
-  helper.FetchFavicon(kPageURL);
-  ASSERT_TRUE(helper.history_handler());
-  helper.history_handler()->InvokeCallback();
-  {
-    std::vector<FaviconURL> icon_urls;
-    icon_urls.push_back(
-        FaviconURL(kIconURL1, favicon_base::FAVICON, std::vector<gfx::Size>()));
-    icon_urls.push_back(
-        FaviconURL(kIconURL2, favicon_base::FAVICON, std::vector<gfx::Size>()));
-    helper.OnUpdateFaviconURL(kPageURL, icon_urls);
-  }
-
-  // FaviconHandler should request from history and download |kIconURL1| and
-  // |kIconURL2|. |kIconURL1| is the better match. A
-  // FaviconDriver::OnFaviconUpdated() notification should be sent for
-  // |kIconURL1|.
-  ASSERT_EQ(0u, delegate.num_notifications());
-  ASSERT_TRUE(helper.history_handler());
-  SetFaviconRawBitmapResult(kIconURL1,
-                            favicon_base::FAVICON,
-                            true /* expired */,
-                            &helper.history_handler()->history_results_);
-  helper.history_handler()->InvokeCallback();
-  helper.set_history_handler(nullptr);
-  ASSERT_TRUE(delegate.download_handler()->HasDownload());
-  delegate.download_handler()->SetImageSizes(std::vector<int>(1u, 15));
-  delegate.download_handler()->InvokeCallback();
-  delegate.download_handler()->Reset();
-
-  ASSERT_TRUE(helper.history_handler());
-  helper.history_handler()->InvokeCallback();
-  SetFaviconRawBitmapResult(kIconURL2,
-                            favicon_base::FAVICON,
-                            true /* expired */,
-                            &helper.history_handler()->history_results_);
-  helper.history_handler()->InvokeCallback();
-  helper.set_history_handler(nullptr);
-  ASSERT_TRUE(delegate.download_handler()->HasDownload());
-  delegate.download_handler()->SetImageSizes(std::vector<int>(1u, 10));
-  delegate.download_handler()->InvokeCallback();
-  delegate.download_handler()->Reset();
-
-  ASSERT_LT(0u, delegate.num_notifications());
-  ASSERT_EQ(kIconURL1, delegate.icon_url());
-
-  // Clear the history handler because SetHistoryFavicons() sets it.
-  helper.set_history_handler(nullptr);
+  const GURL kIconURL1(
+      "http://wwww.page_which_animates_favicon.com/frame1.png");
+  const GURL kIconURL2(
+      "http://wwww.page_which_animates_favicon.com/frame2.png");
+
+  // |kIconURL1| is the better match.
+  delegate_.fake_downloader().Add(kIconURL1, IntVector{15});
+  delegate_.fake_downloader().Add(kIconURL2, IntVector{10});
+
+  // Two FaviconDriver::OnFaviconUpdated() notifications should be sent for
+  // |kIconURL1|, one before and one after the download.
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL1, _, _));
+
+  std::unique_ptr<FaviconHandler> handler =
+      RunHandlerWithSimpleFaviconCandidates({kIconURL1, kIconURL2});
+
+  // Both |kIconURL1| and |kIconURL2| should have been requested from the
+  // database and downloaded. |kIconURL2| should have been fetched from the
+  // database and downloaded last.
+  ASSERT_THAT(delegate_.downloads(), ElementsAre(kIconURL1, kIconURL2));
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kIconURL1, kIconURL2));
+  ASSERT_TRUE(VerifyAndClearExpectations());
 
   // Simulate the page changing it's icon URL to just |kIconURL2| via
   // Javascript.
-  helper.OnUpdateFaviconURL(
-      kPageURL,
-      std::vector<FaviconURL>(1u, FaviconURL(kIconURL2, favicon_base::FAVICON,
-                                             std::vector<gfx::Size>())));
-
-  // FaviconHandler should request from history and download |kIconURL2|. A
-  // FaviconDriver::OnFaviconUpdated() notification should be sent for
-  // |kIconURL2|.
-  delegate.ResetNumNotifications();
-  ASSERT_TRUE(helper.history_handler());
-  SetFaviconRawBitmapResult(kIconURL2,
-                            favicon_base::FAVICON,
-                            true /* expired */,
-                            &helper.history_handler()->history_results_);
-  helper.history_handler()->InvokeCallback();
-  helper.set_history_handler(nullptr);
-  ASSERT_TRUE(delegate.download_handler()->HasDownload());
-  delegate.download_handler()->InvokeCallback();
-  delegate.download_handler()->Reset();
-  ASSERT_LT(0u, delegate.num_notifications());
-  EXPECT_EQ(kIconURL2, delegate.icon_url());
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL2, _, _));
+  handler->OnUpdateFaviconURL(kPageURL,
+                              {FaviconURL(kIconURL2, FAVICON, kEmptySizes)});
+  base::RunLoop().RunUntilIdle();
 }
 
 // Test the favicon which is selected when the web page provides several
 // favicons and none of the favicons are cached in history.
 // The goal of this test is to be more of an integration test than
 // SelectFaviconFramesTest.*.
-TEST_F(FaviconHandlerTest, MultipleFavicons) {
-  const GURL kPageURL("http://www.google.com");
-  const FaviconURL kSourceIconURLs[] = {
-      FaviconURL(GURL("http://www.google.com/a"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>()),
-      FaviconURL(GURL("http://www.google.com/b"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>()),
-      FaviconURL(GURL("http://www.google.com/c"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>()),
-      FaviconURL(GURL("http://www.google.com/d"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>()),
-      FaviconURL(GURL("http://www.google.com/e"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>())};
-
-  // Set the supported scale factors to 1x and 2x. This affects the behavior of
-  // SelectFaviconFrames().
-  std::vector<ui::ScaleFactor> scale_factors;
-  scale_factors.push_back(ui::SCALE_FACTOR_100P);
-  scale_factors.push_back(ui::SCALE_FACTOR_200P);
-  ui::test::ScopedSetSupportedScaleFactors scoped_supported(scale_factors);
-
-  // 1) Test that if there are several single resolution favicons to choose from
-  // that the largest exact match is chosen.
-  TestDelegate delegate1;
-  TestFaviconHandler handler1(&delegate1,
-                              FaviconDriverObserver::NON_TOUCH_16_DIP);
-
-  const int kSizes1[] = { 16, 24, 32, 48, 256 };
-  std::vector<FaviconURL> urls1(kSourceIconURLs,
-                                kSourceIconURLs + arraysize(kSizes1));
-  DownloadTillDoneIgnoringHistory(&delegate1, &handler1, kPageURL, urls1,
-                                  kSizes1);
-
-  EXPECT_EQ(nullptr, handler1.current_candidate());
-  EXPECT_EQ(1u, delegate1.num_notifications());
-  EXPECT_FALSE(delegate1.image().IsEmpty());
-  EXPECT_EQ(gfx::kFaviconSize, delegate1.image().Width());
-
-  size_t expected_index = 2u;
-  EXPECT_EQ(32, kSizes1[expected_index]);
-  EXPECT_EQ(kSourceIconURLs[expected_index].icon_url, delegate1.icon_url());
-
-  // 2) Test that if there are several single resolution favicons to choose
-  // from, the exact match is preferred even if it results in upsampling.
-  TestDelegate delegate2;
-  TestFaviconHandler handler2(&delegate2,
-                              FaviconDriverObserver::NON_TOUCH_16_DIP);
-
-  const int kSizes2[] = { 16, 24, 48, 256 };
-  std::vector<FaviconURL> urls2(kSourceIconURLs,
-                                kSourceIconURLs + arraysize(kSizes2));
-  DownloadTillDoneIgnoringHistory(&delegate2, &handler2, kPageURL, urls2,
-                                  kSizes2);
-  EXPECT_EQ(1u, delegate2.num_notifications());
-  expected_index = 0u;
-  EXPECT_EQ(16, kSizes2[expected_index]);
-  EXPECT_EQ(kSourceIconURLs[expected_index].icon_url, delegate2.icon_url());
-
-  // 3) Test that favicons which need to be upsampled a little or downsampled
-  // a little are preferred over huge favicons.
-  TestDelegate delegate3;
-  TestFaviconHandler handler3(&delegate3,
-                              FaviconDriverObserver::NON_TOUCH_16_DIP);
-
-  const int kSizes3[] = { 256, 48 };
-  std::vector<FaviconURL> urls3(kSourceIconURLs,
-                                kSourceIconURLs + arraysize(kSizes3));
-  DownloadTillDoneIgnoringHistory(&delegate3, &handler3, kPageURL, urls3,
-                                  kSizes3);
-  EXPECT_EQ(1u, delegate3.num_notifications());
-  expected_index = 1u;
-  EXPECT_EQ(48, kSizes3[expected_index]);
-  EXPECT_EQ(kSourceIconURLs[expected_index].icon_url, delegate3.icon_url());
-
-  TestDelegate delegate4;
-  TestFaviconHandler handler4(&delegate4,
-                              FaviconDriverObserver::NON_TOUCH_16_DIP);
-
-  const int kSizes4[] = { 17, 256 };
-  std::vector<FaviconURL> urls4(kSourceIconURLs,
-                                kSourceIconURLs + arraysize(kSizes4));
-  DownloadTillDoneIgnoringHistory(&delegate4, &handler4, kPageURL, urls4,
-                                  kSizes4);
-  EXPECT_EQ(1u, delegate4.num_notifications());
-  expected_index = 0u;
-  EXPECT_EQ(17, kSizes4[expected_index]);
-  EXPECT_EQ(kSourceIconURLs[expected_index].icon_url, delegate4.icon_url());
+class FaviconHandlerMultipleFaviconsTest : public FaviconHandlerTest {
+ protected:
+  FaviconHandlerMultipleFaviconsTest() {
+    // Set the supported scale factors to 1x and 2x. This affects the behavior
+    // of SelectFaviconFrames().
+    scoped_set_supported_scale_factors_.reset();  // Need to delete first.
+    scoped_set_supported_scale_factors_.reset(
+        new ui::test::ScopedSetSupportedScaleFactors(
+            {ui::SCALE_FACTOR_100P, ui::SCALE_FACTOR_200P}));
+  }
+
+  // Simulates requesting a favicon for |page_url| given:
+  // - We have not previously cached anything in history for |page_url| or for
+  //   any of candidates.
+  // - The page provides favicons with edge pixel sizes of
+  //   |candidate_icon_sizes|.
+  // - Candidates are assumed of type FAVICON and the URLs are generated
+  //   internally for testing purposes.
+  //
+  // Returns the chosen size among |candidate_icon_sizes| or -1 if none was
+  // chosen.
+  int DownloadTillDoneIgnoringHistory(const IntVector& candidate_icon_sizes) {
+    std::vector<FaviconURL> candidate_icons;
+    int chosen_icon_size = -1;
+
+    for (int icon_size : candidate_icon_sizes) {
+      const GURL icon_url(base::StringPrintf(
+          "https://www.google.com/generated/%dx%d", icon_size, icon_size));
+      // Set up 200 responses for all images, and the corresponding size.
+      delegate_.fake_downloader().Add(icon_url, IntVector{icon_size});
+      // Create test candidates of type FAVICON and a fake URL.
+      candidate_icons.emplace_back(icon_url, FAVICON, kEmptySizes);
+
+      ON_CALL(delegate_, OnFaviconUpdated(_, _, icon_url, _, _))
+          .WillByDefault(Assign(&chosen_icon_size, icon_size));
+    }
+
+    RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             candidate_icons);
+    return chosen_icon_size;
+  }
+};
+
+// Tests that running FaviconHandler
+// - Pn an OS which supports the 1x and 2x scale factor

[pkotwicz, 2017/03/17 05:43:13]

Pn -> On

[mastiz, 2017/03/17 12:17:31]

Done.

+// - On a page with <link rel="icon"> tags with no "sizes" information selects

[pkotwicz, 2017/03/17 05:43:12]

Nit: new line before "selects" ("selects" is not part of the bullet point)

[mastiz, 2017/03/17 12:17:32]

Done.

+//   the largest exact match. Note that a 32x32 PNG image is not a "true
+//   exact match" on an OS which supports an 1x and 2x. A "true exact match" is
+//   a .ico file with 16x16 and 32x32 bitmaps.
+TEST_F(FaviconHandlerMultipleFaviconsTest, ChooseLargestExactMatch) {
+  EXPECT_EQ(32,
+            DownloadTillDoneIgnoringHistory(IntVector{16, 24, 32, 48, 256}));
+}
+
+// Test that if there are several single resolution favicons to choose
+// from, the exact match is preferred even if it results in upsampling.
+TEST_F(FaviconHandlerMultipleFaviconsTest, ChooseExactMatchDespiteUpsampling) {
+  EXPECT_EQ(16, DownloadTillDoneIgnoringHistory(IntVector{16, 24, 48, 256}));
+}
+
+// Test that favicons which need to be upsampled a little or downsampled
+// a little are preferred over huge favicons.
+TEST_F(FaviconHandlerMultipleFaviconsTest,
+       ChooseMinorDownsamplingOverHugeIcon) {
+  EXPECT_EQ(48, DownloadTillDoneIgnoringHistory(IntVector{256, 48}));
+}
+
+TEST_F(FaviconHandlerMultipleFaviconsTest, ChooseMinorUpsamplingOverHugeIcon) {
+  EXPECT_EQ(17, DownloadTillDoneIgnoringHistory(IntVector{17, 256}));
 }
 

[pkotwicz, 2017/03/17 05:43:12]

Nit: Move the other 404 related tests here Report404 and NotReport503

[mastiz, 2017/03/17 12:17:32]

Done.

 // Test that the best favicon is selected when:
@@ -1224,51 +829,26 @@ TEST_F(FaviconHandlerTest, MultipleFavicons) {
 // - Downloading one of the page's icon URLs previously returned a 404.
 // - None of the favicons are cached in the Favicons database.
 TEST_F(FaviconHandlerTest, MultipleFavicons404) {
-  const GURL kPageURL("http://www.google.com");
-  const GURL k404IconURL("http://www.google.com/404.png");
-  const FaviconURL k404FaviconURL(
-      k404IconURL, favicon_base::FAVICON, std::vector<gfx::Size>());
-  const FaviconURL kFaviconURLs[] = {
-      FaviconURL(GURL("http://www.google.com/a"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>()),
-      k404FaviconURL,
-      FaviconURL(GURL("http://www.google.com/c"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>()),
-  };
+  ON_CALL(favicon_service_, WasUnableToDownloadFavicon(kIconURL16x16))
+      .WillByDefault(Return(true));
 
-  TestDelegate delegate;
-  TestFaviconHandler handler(&delegate,
-                             FaviconDriverObserver::NON_TOUCH_16_DIP);
-  DownloadHandler* download_handler = delegate.download_handler();
-
-  std::set<GURL> k404URLs;
-  k404URLs.insert(k404IconURL);
-  download_handler->FailDownloadForIconURLs(k404URLs);
-
-  // Make the initial download for |k404IconURL| fail.
-  const int kSizes1[] = { 0 };
-  std::vector<FaviconURL> urls1(1u, k404FaviconURL);
-  DownloadTillDoneIgnoringHistory(&delegate, &handler, kPageURL, urls1,
-                                  kSizes1);
-  EXPECT_TRUE(download_handler->DidFailDownloadForIconURL(k404IconURL));
-
-  // Do a fetch now that the initial download for |k404IconURL| has failed. The
-  // behavior is different because OnDidDownloadFavicon() is invoked
-  // synchronously from DownloadFavicon().
-  const int kSizes2[] = { 10, 0, 16 };
-  std::vector<FaviconURL> urls2(kFaviconURLs,
-                                kFaviconURLs + arraysize(kFaviconURLs));
-  DownloadTillDoneIgnoringHistory(&delegate, &handler, kPageURL, urls2,
-                                  kSizes2);
-
-  EXPECT_EQ(nullptr, handler.current_candidate());
-  EXPECT_EQ(1u, delegate.num_notifications());
-  EXPECT_FALSE(delegate.image().IsEmpty());
-  int expected_index = 2u;
-  EXPECT_EQ(16, kSizes2[expected_index]);
-  EXPECT_EQ(kFaviconURLs[expected_index].icon_url, delegate.icon_url());
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL64x64, _, _));
+  RunHandlerWithSimpleFaviconCandidates({kIconURL16x16, kIconURL64x64});
+  ASSERT_THAT(delegate_.downloads(), ElementsAre(kIconURL64x64));
+}
+
+// Test that the best favicon is selected when:
+// - The page provides several favicons.
+// - Downloading the last page icon URL previously returned a 404.
+// - None of the favicons are cached in the Favicons database.
+// - Size hints are misleading and cause the 404 icon to be processed last.

[pkotwicz, 2017/03/17 05:43:13]

Can you please change the last point. The test tests the case of there being no
sizes information at all. Perhaps:

- All of the icons are downloaded because none of the icons have the ideal size.
- The 404 icon is last.

[mastiz, 2017/03/17 12:17:31]

Done.

+TEST_F(FaviconHandlerTest, MultipleFaviconsLast404) {
+  ON_CALL(favicon_service_, WasUnableToDownloadFavicon(kIconURL16x16))
+      .WillByDefault(Return(true));
+
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL64x64, _, _));
+  RunHandlerWithSimpleFaviconCandidates({kIconURL64x64, kIconURL16x16});

[pkotwicz, 2017/03/17 05:43:12]

Can you use a differently sized URL than |kIconURL16x16| in this test and
MultipleFavicons404

Using 16x16 icon as "random data" for FaviconHandlers created for
NON_TOUCH_16_DIP and FaviconURLs with no sizes information is confusing because:
- 16x16 icons are a perfect match and cause downloading to stop
- non 16x16 icons are not perfect data and don't stop downloading

[mastiz, 2017/03/17 12:17:32]

Done, also for MultipleFaviconsAll404.

The truth is it doesn't matter because the icon doesn't get downloaded and tests
specifically verify that's the case.

[pkotwicz, 2017/03/17 15:22:31]

I know that it doesn't matter. However, using 16x16 icons does make the tests
more confusing in my opinion

+  ASSERT_THAT(delegate_.downloads(), ElementsAre(kIconURL64x64));
 }
 
 // Test that no favicon is selected when:
@@ -1276,100 +856,41 @@ TEST_F(FaviconHandlerTest, MultipleFavicons404) {
 // - Downloading the page's icons has previously returned a 404.
 // - None of the favicons are cached in the Favicons database.
 TEST_F(FaviconHandlerTest, MultipleFaviconsAll404) {
-  const GURL kPageURL("http://www.google.com");
-  const GURL k404IconURL1("http://www.google.com/4041.png");
-  const GURL k404IconURL2("http://www.google.com/4042.png");
-  const FaviconURL kFaviconURLs[] = {
-      FaviconURL(k404IconURL1,
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>()),
-      FaviconURL(k404IconURL2,
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>()),
-  };
-
-  TestDelegate delegate;
-  TestFaviconHandler handler(&delegate,
-                             FaviconDriverObserver::NON_TOUCH_16_DIP);
-  DownloadHandler* download_handler = delegate.download_handler();
-
-  std::set<GURL> k404URLs;
-  k404URLs.insert(k404IconURL1);
-  k404URLs.insert(k404IconURL2);
-  download_handler->FailDownloadForIconURLs(k404URLs);
-
-  // Make the initial downloads for |kFaviconURLs| fail.
-  for (const FaviconURL& favicon_url : kFaviconURLs) {
-    const int kSizes[] = { 0 };
-    std::vector<FaviconURL> urls(1u, favicon_url);
-    DownloadTillDoneIgnoringHistory(&delegate, &handler, kPageURL, urls,
-                                    kSizes);
-  }
-  EXPECT_TRUE(download_handler->DidFailDownloadForIconURL(k404IconURL1));
-  EXPECT_TRUE(download_handler->DidFailDownloadForIconURL(k404IconURL2));
-
-  // Do a fetch now that the initial downloads for |kFaviconURLs| have failed.
-  // The behavior is different because OnDidDownloadFavicon() is invoked
-  // synchronously from DownloadFavicon().
-  const int kSizes[] = { 0, 0 };
-  std::vector<FaviconURL> urls(kFaviconURLs,
-                               kFaviconURLs + arraysize(kFaviconURLs));
-  DownloadTillDoneIgnoringHistory(&delegate, &handler, kPageURL, urls, kSizes);
-
-  EXPECT_EQ(nullptr, handler.current_candidate());
-  EXPECT_EQ(0u, delegate.num_notifications());
-  EXPECT_TRUE(delegate.image().IsEmpty());
+  ON_CALL(favicon_service_, WasUnableToDownloadFavicon(kIconURL16x16))
+      .WillByDefault(Return(true));
+  ON_CALL(favicon_service_, WasUnableToDownloadFavicon(kIconURL64x64))
+      .WillByDefault(Return(true));
+
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0);
+  RunHandlerWithSimpleFaviconCandidates({kIconURL16x16, kIconURL64x64});
+  ASSERT_THAT(delegate_.downloads(), IsEmpty());
 }
 
 // Test that no favicon is selected when the page's only icon uses an invalid
 // URL syntax.
 TEST_F(FaviconHandlerTest, FaviconInvalidURL) {
-  const GURL kPageURL("http://www.google.com");
   const GURL kInvalidFormatURL("invalid");
   ASSERT_TRUE(kInvalidFormatURL.is_empty());
 
-  FaviconURL favicon_url(kInvalidFormatURL, favicon_base::FAVICON,
-                         std::vector<gfx::Size>());
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0);
 
-  TestDelegate delegate;
-  TestFaviconHandler handler(&delegate,
-                             FaviconDriverObserver::NON_TOUCH_16_DIP);
-  UpdateFaviconURL(&delegate, &handler, kPageURL,
-                   std::vector<FaviconURL>(1u, favicon_url));
-  EXPECT_EQ(0u, handler.image_urls().size());
+  RunHandlerWithSimpleFaviconCandidates({kInvalidFormatURL});
+  EXPECT_THAT(delegate_.downloads(), IsEmpty());
 }
 
 TEST_F(FaviconHandlerTest, TestSortFavicon) {
-  const GURL kPageURL("http://www.google.com");
-  std::vector<gfx::Size> icon1;
-  icon1.push_back(gfx::Size(1024, 1024));
-  icon1.push_back(gfx::Size(512, 512));
-
-  std::vector<gfx::Size> icon2;
-  icon2.push_back(gfx::Size(15, 15));
-  icon2.push_back(gfx::Size(16, 16));
-
-  std::vector<gfx::Size> icon3;
-  icon3.push_back(gfx::Size(16, 16));
-  icon3.push_back(gfx::Size(14, 14));
-
-  const FaviconURL kSourceIconURLs[] = {
-      FaviconURL(GURL("http://www.google.com/a"), favicon_base::FAVICON, icon1),
-      FaviconURL(GURL("http://www.google.com/b"), favicon_base::FAVICON, icon2),
-      FaviconURL(GURL("http://www.google.com/c"), favicon_base::FAVICON, icon3),
-      FaviconURL(GURL("http://www.google.com/d"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>()),
-      FaviconURL(GURL("http://www.google.com/e"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>())};
-
-  TestDelegate delegate1;
-  TestFaviconHandler handler1(&delegate1,
-                              FaviconDriverObserver::NON_TOUCH_LARGEST);
-  std::vector<FaviconURL> urls1(kSourceIconURLs,
-                                kSourceIconURLs + arraysize(kSourceIconURLs));
-  UpdateFaviconURL(&delegate1, &handler1, kPageURL, urls1);
+  const std::vector<favicon::FaviconURL> kSourceIconURLs{
+      FaviconURL(GURL("http://www.google.com/a"), FAVICON,
+                 {gfx::Size(1, 1), gfx::Size(17, 17)}),
+      FaviconURL(GURL("http://www.google.com/b"), FAVICON,
+                 {gfx::Size(1024, 1024), gfx::Size(512, 512)}),
+      FaviconURL(GURL("http://www.google.com/c"), FAVICON,
+                 {gfx::Size(16, 16), gfx::Size(14, 14)}),
+      FaviconURL(GURL("http://www.google.com/d"), FAVICON, kEmptySizes),
+      FaviconURL(GURL("http://www.google.com/e"), FAVICON, kEmptySizes)};
+
+  std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST, kSourceIconURLs);
 
   struct ExpectedResult {
     // The favicon's index in kSourceIconURLs.
@@ -1377,19 +898,19 @@ TEST_F(FaviconHandlerTest, TestSortFavicon) {
     // Width of largest bitmap.
     int width;
   } results[] = {
-    // First is icon1, though its size larger than maximal.
-    {0, 1024},
-    // Second is icon2
-    // The 16x16 is largest.
-    {1, 16},
-    // Third is icon3 though it has same size as icon2.
-    // The 16x16 is largest.
-    {2, 16},
-    // The rest of bitmaps come in order, there is no sizes attribute.
-    {3, -1},
-    {4, -1},
+      // First is icon2, though its size larger than maximal.
+      {1, 1024},
+      // Second is icon1
+      // The 17x17 is largest.
+      {0, 17},
+      // Third is icon3 though it has same size as icon1.
+      // The 16x16 is largest.

[pkotwicz, 2017/03/17 05:43:11]

This comment is no longer accurate.

[mastiz, 2017/03/17 12:17:32]

Done.

+      {2, 16},
+      // The rest of bitmaps come in order, there is no sizes attribute.

[pkotwicz, 2017/03/17 05:43:12]

Nit: Add quotes around "sizes"

[mastiz, 2017/03/17 12:17:32]

Done.

+      {3, -1},
+      {4, -1},
   };
-  const std::vector<FaviconURL>& icons = handler1.image_urls();
+  const std::vector<FaviconURL>& icons = handler->image_urls();
   ASSERT_EQ(5u, icons.size());
   for (size_t i = 0; i < icons.size(); ++i) {
     EXPECT_EQ(kSourceIconURLs[results[i].favicon_index].icon_url,
@@ -1400,278 +921,97 @@ TEST_F(FaviconHandlerTest, TestSortFavicon) {
 }
 
 TEST_F(FaviconHandlerTest, TestDownloadLargestFavicon) {
-  const GURL kPageURL("http://www.google.com");
-  std::vector<gfx::Size> icon1;
-  icon1.push_back(gfx::Size(1024, 1024));
-  icon1.push_back(gfx::Size(512, 512));
-
-  std::vector<gfx::Size> icon2;
-  icon2.push_back(gfx::Size(15, 15));
-  icon2.push_back(gfx::Size(14, 14));
-
-  std::vector<gfx::Size> icon3;
-  icon3.push_back(gfx::Size(16, 16));
-  icon3.push_back(gfx::Size(512, 512));
-
-  const FaviconURL kSourceIconURLs[] = {
-      FaviconURL(
-          GURL("http://www.google.com/a"), favicon_base::FAVICON, icon1),
-      FaviconURL(
-          GURL("http://www.google.com/b"), favicon_base::FAVICON, icon2),
-      FaviconURL(
-          GURL("http://www.google.com/c"), favicon_base::FAVICON, icon3),
-      FaviconURL(GURL("http://www.google.com/d"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>()),
-      FaviconURL(GURL("http://www.google.com/e"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>())};
-
-  TestDelegate delegate1;
-  TestFaviconHandler handler1(&delegate1,
-                              FaviconDriverObserver::NON_TOUCH_LARGEST);
-
-  std::set<GURL> fail_icon_urls;
-  for (size_t i = 0; i < arraysize(kSourceIconURLs); ++i) {
-    fail_icon_urls.insert(kSourceIconURLs[i].icon_url);
-  }
-  delegate1.download_handler()->FailDownloadForIconURLs(fail_icon_urls);
-
-  std::vector<FaviconURL> urls1(kSourceIconURLs,
-                                kSourceIconURLs + arraysize(kSourceIconURLs));
-  UpdateFaviconURL(&delegate1, &handler1, kPageURL, urls1);
-
-  // Simulate the download failed, to check whether the icons were requested
-  // to download according their size.
-  struct ExpectedResult {
-    // The favicon's index in kSourceIconURLs.
-    size_t favicon_index;
-    // Width of largest bitmap.
-    int width;
-  } results[] = {
-    {0, 1024},
-    {2, 512},
-    {1, 15},
-    // The rest of bitmaps come in order.
-    {3, -1},
-    {4, -1},
-  };
+  // Names represent the bitmap sizes per icon.
+  const GURL kIconURL1024_512("http://www.google.com/a");
+  const GURL kIconURL15_14("http://www.google.com/b");
+  const GURL kIconURL16_512("http://www.google.com/c");
+  const GURL kIconURLWithoutSize1("http://www.google.com/d");
+  const GURL kIconURLWithoutSize2("http://www.google.com/e");
+
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {FaviconURL(kIconURL1024_512, FAVICON,
+                  {gfx::Size(1024, 1024), gfx::Size(512, 512)}),
+       FaviconURL(kIconURL15_14, FAVICON,
+                  {gfx::Size(15, 15), gfx::Size(14, 14)}),
+       FaviconURL(kIconURL16_512, FAVICON,
+                  {gfx::Size(16, 16), gfx::Size(512, 512)}),
+       FaviconURL(kIconURLWithoutSize1, FAVICON, kEmptySizes),
+       FaviconURL(kIconURLWithoutSize2, FAVICON, kEmptySizes)});
+
+  // Icon URLs are not registered and hence 404s will be produced, which
+  // allows checking whether the icons were requested according to their
+  // size, which allows checking the order that the favicons were downloaded.

[pkotwicz, 2017/03/17 05:43:13]

Nit: Remove "which allows checking whether the icons were requested according to
their size,"

[mastiz, 2017/03/17 12:17:31]

Done.

+  // The favicons should have been requested in decreasing order of their sizes.
+  // Favicons without any <link sizes=""> attribute should have been downloaded
+  // last.
+  EXPECT_THAT(delegate_.downloads(),
+              ElementsAre(kIconURL1024_512, kIconURL16_512, kIconURL15_14,
+                          kIconURLWithoutSize1, kIconURLWithoutSize2));
+}
 
-  for (int i = 0; i < 5; ++i) {
-    EXPECT_EQ(kSourceIconURLs[results[i].favicon_index].icon_url,
-              handler1.current_candidate()->icon_url);
-    if (results[i].width != -1) {
-      EXPECT_EQ(results[i].width, handler1.current_candidate()->
-                icon_sizes[0].width());
-    }
+TEST_F(FaviconHandlerTest, TestSelectLargestFavicon) {
+  const GURL kIconURL1("http://www.google.com/b");
+  const GURL kIconURL2("http://www.google.com/c");
 
-    // Simulate no favicon from history.
-    handler1.history_handler()->history_results_.clear();
-    handler1.history_handler()->InvokeCallback();
+  delegate_.fake_downloader().Add(kIconURL1, IntVector{15});
+  delegate_.fake_downloader().Add(kIconURL2, IntVector{14, 16});
 
-    // Verify download request
-    ASSERT_TRUE(delegate1.download_handler()->HasDownload());
-    EXPECT_EQ(kSourceIconURLs[results[i].favicon_index].icon_url,
-              delegate1.download_handler()->GetImageUrl());
+  // Verify NotifyFaviconAvailable().
+  EXPECT_CALL(delegate_,
+              OnFaviconUpdated(_, FaviconDriverObserver::NON_TOUCH_LARGEST,
+                               kIconURL2, _, _));
 
-    delegate1.download_handler()->InvokeCallback();
-    delegate1.download_handler()->Reset();
-  }
-}
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {FaviconURL(kIconURL1, FAVICON, {gfx::Size(15, 15)}),
+       FaviconURL(kIconURL2, FAVICON, {gfx::Size(14, 14), gfx::Size(16, 16)})});
 
-TEST_F(FaviconHandlerTest, TestSelectLargestFavicon) {
-  const GURL kPageURL("http://www.google.com");
-
-  std::vector<gfx::Size> one_icon;
-  one_icon.push_back(gfx::Size(15, 15));
-
-  std::vector<gfx::Size> two_icons;
-  two_icons.push_back(gfx::Size(14, 14));
-  two_icons.push_back(gfx::Size(16, 16));
-
-  const FaviconURL kSourceIconURLs[] = {
-      FaviconURL(
-          GURL("http://www.google.com/b"), favicon_base::FAVICON, one_icon),
-      FaviconURL(
-          GURL("http://www.google.com/c"), favicon_base::FAVICON, two_icons)};
-
-  TestDelegate delegate1;
-  TestFaviconHandler handler1(&delegate1,
-                              FaviconDriverObserver::NON_TOUCH_LARGEST);
-  std::vector<FaviconURL> urls1(kSourceIconURLs,
-                                kSourceIconURLs + arraysize(kSourceIconURLs));
-  UpdateFaviconURL(&delegate1, &handler1, kPageURL, urls1);
-
-  ASSERT_EQ(2u, handler1.image_urls().size());
-
-  // Index of largest favicon in kSourceIconURLs.
-  size_t i = 1;
-  // The largest bitmap's index in Favicon .
-  int b = 1;
-
-  // Verify the icon_bitmaps_ was initialized correctly.
-  EXPECT_EQ(kSourceIconURLs[i].icon_url,
-            handler1.current_candidate()->icon_url);
-  EXPECT_EQ(kSourceIconURLs[i].icon_sizes[b],
-            handler1.current_candidate()->icon_sizes[0]);
-
-  // Simulate no favicon from history.
-  handler1.history_handler()->history_results_.clear();
-  handler1.history_handler()->InvokeCallback();
-
-  // Verify download request
-  ASSERT_TRUE(delegate1.download_handler()->HasDownload());
-  EXPECT_EQ(kSourceIconURLs[i].icon_url,
-            delegate1.download_handler()->GetImageUrl());
-
-  // Give the correct download result.
-  std::vector<int> sizes;
-  for (std::vector<gfx::Size>::const_iterator j =
-           kSourceIconURLs[i].icon_sizes.begin();
-       j != kSourceIconURLs[i].icon_sizes.end(); ++j)
-    sizes.push_back(j->width());
-
-  delegate1.download_handler()->SetImageSizes(sizes);
-  delegate1.download_handler()->InvokeCallback();
-
-  // Verify the largest bitmap has been saved into history.
-  EXPECT_EQ(kSourceIconURLs[i].icon_url, handler1.history_handler()->icon_url_);
-  EXPECT_EQ(kSourceIconURLs[i].icon_sizes[b],
-            handler1.history_handler()->size_);
-  // Verify NotifyFaviconAvailable().
-  EXPECT_EQ(1u, delegate1.num_notifications());
-  EXPECT_EQ(kSourceIconURLs[i].icon_url, delegate1.icon_url());
-  EXPECT_EQ(kSourceIconURLs[i].icon_sizes[b], delegate1.image().Size());
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL2));
 }
 
 TEST_F(FaviconHandlerTest, TestFaviconWasScaledAfterDownload) {
-  const GURL kPageURL("http://www.google.com");
-  const int kMaximalSize =
-    TestFaviconHandler::GetMaximalIconSize(favicon_base::FAVICON);
-
-  std::vector<gfx::Size> icon1;
-  icon1.push_back(gfx::Size(kMaximalSize + 1, kMaximalSize + 1));
-
-  std::vector<gfx::Size> icon2;
-  icon2.push_back(gfx::Size(kMaximalSize + 2, kMaximalSize + 2));
-
-  const FaviconURL kSourceIconURLs[] = {
-      FaviconURL(
-          GURL("http://www.google.com/b"), favicon_base::FAVICON, icon1),
-      FaviconURL(
-          GURL("http://www.google.com/c"), favicon_base::FAVICON, icon2)};
-
-  TestDelegate delegate1;
-  TestFaviconHandler handler1(&delegate1,
-                              FaviconDriverObserver::NON_TOUCH_LARGEST);
-  std::vector<FaviconURL> urls1(kSourceIconURLs,
-                                kSourceIconURLs + arraysize(kSourceIconURLs));
-  UpdateFaviconURL(&delegate1, &handler1, kPageURL, urls1);
-
-  ASSERT_EQ(2u, handler1.image_urls().size());
-
-  // Index of largest favicon in kSourceIconURLs.
-  size_t i = 1;
-  // The largest bitmap's index in Favicon .
-  int b = 0;
-
-  // Verify the icon_bitmaps_ was initialized correctly.
-  EXPECT_EQ(kSourceIconURLs[i].icon_url,
-            handler1.current_candidate()->icon_url);
-  EXPECT_EQ(kSourceIconURLs[i].icon_sizes[b],
-            handler1.current_candidate()->icon_sizes[0]);
-
-  // Simulate no favicon from history.
-  handler1.history_handler()->history_results_.clear();
-  handler1.history_handler()->InvokeCallback();
-
-  // Verify download request
-  ASSERT_TRUE(delegate1.download_handler()->HasDownload());
-  EXPECT_EQ(kSourceIconURLs[i].icon_url,
-            delegate1.download_handler()->GetImageUrl());
-
-  // Give the scaled download bitmap.
-  std::vector<int> sizes;
-  sizes.push_back(kMaximalSize);
-
-  delegate1.download_handler()->SetImageSizes(sizes);
-  delegate1.download_handler()->InvokeCallback();
-
-  // Verify the largest bitmap has been saved into history though it was
-  // scaled down to maximal size and smaller than icon1 now.
-  EXPECT_EQ(kSourceIconURLs[i].icon_url, handler1.history_handler()->icon_url_);
-  EXPECT_EQ(gfx::Size(kMaximalSize, kMaximalSize),
-            handler1.history_handler()->size_);
+  const int kMaximalSize = FaviconHandler::GetMaximalIconSize(FAVICON);
+
+  const GURL kIconURL1("http://www.google.com/b");
+  const GURL kIconURL2("http://www.google.com/c");
+
+  const int kOriginalSize1 = kMaximalSize + 1;
+  const int kOriginalSize2 = kMaximalSize + 2;
+
+  delegate_.fake_downloader().AddWithOriginalSizes(
+      kIconURL1, IntVector{kMaximalSize}, IntVector{kOriginalSize1});
+  delegate_.fake_downloader().AddWithOriginalSizes(
+      kIconURL2, IntVector{kMaximalSize}, IntVector{kOriginalSize2});
+
+  // Verify the largest bitmap was selected although it was scaled down to
+  // maximal size and smaller than |kIconURL1| now.
+  EXPECT_CALL(delegate_,
+              OnFaviconUpdated(_, _, kIconURL2, _,
+                               ImageSizeIs(kMaximalSize, kMaximalSize)));
+
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {FaviconURL(kIconURL1, FAVICON,
+                  SizeVector{gfx::Size(kOriginalSize1, kOriginalSize1)}),
+       FaviconURL(kIconURL2, FAVICON,
+                  SizeVector{gfx::Size(kOriginalSize2, kOriginalSize2)})});
+
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL2));
 }
 
+// Test that if several icons are downloaded because the icons are smaller than
+// expected that OnFaviconUpdated() is called with the largest downloaded
+// bitmap.
 TEST_F(FaviconHandlerTest, TestKeepDownloadedLargestFavicon) {
-  const GURL kPageURL("http://www.google.com");
-
-  std::vector<gfx::Size> icon1;
-  icon1.push_back(gfx::Size(16, 16));
-  const int actual_size1 = 10;
-
-  std::vector<gfx::Size> icon2;
-  icon2.push_back(gfx::Size(15, 15));
-  const int actual_size2 = 12;
-
-  const FaviconURL kSourceIconURLs[] = {
-      FaviconURL(GURL("http://www.google.com/b"), favicon_base::FAVICON, icon1),
-      FaviconURL(GURL("http://www.google.com/c"), favicon_base::FAVICON, icon2),
-      FaviconURL(GURL("http://www.google.com/d"),
-                 favicon_base::FAVICON,
-                 std::vector<gfx::Size>())};
-
-  TestDelegate delegate1;
-  TestFaviconHandler handler1(&delegate1,
-                              FaviconDriverObserver::NON_TOUCH_LARGEST);
-  std::vector<FaviconURL> urls1(kSourceIconURLs,
-                                kSourceIconURLs + arraysize(kSourceIconURLs));
-  UpdateFaviconURL(&delegate1, &handler1, kPageURL, urls1);
-  ASSERT_EQ(3u, handler1.image_urls().size());
-
-  // Simulate no favicon from history.
-  handler1.history_handler()->history_results_.clear();
-  handler1.history_handler()->InvokeCallback();
-
-  // Verify the first icon was request to download
-  ASSERT_TRUE(delegate1.download_handler()->HasDownload());
-  EXPECT_EQ(kSourceIconURLs[0].icon_url,
-            delegate1.download_handler()->GetImageUrl());
-
-  // Give the incorrect size.
-  std::vector<int> sizes;
-  sizes.push_back(actual_size1);
-  delegate1.download_handler()->SetImageSizes(sizes);
-  delegate1.download_handler()->InvokeCallback();
-  delegate1.download_handler()->Reset();
-
-  // Simulate no favicon from history.
-  handler1.history_handler()->history_results_.clear();
-  handler1.history_handler()->InvokeCallback();
-
-  // Verify the 2nd icon was request to download
-  ASSERT_TRUE(delegate1.download_handler()->HasDownload());
-  EXPECT_EQ(kSourceIconURLs[1].icon_url,
-            delegate1.download_handler()->GetImageUrl());
-
-  // Very the best candidate is icon1
-  EXPECT_EQ(kSourceIconURLs[0].icon_url,
-            handler1.best_favicon_candidate().image_url);
-  EXPECT_EQ(gfx::Size(actual_size1, actual_size1),
-            handler1.best_favicon_candidate().image.Size());
-
-  // Give the incorrect size.
-  sizes.clear();
-  sizes.push_back(actual_size2);
-  delegate1.download_handler()->SetImageSizes(sizes);
-  delegate1.download_handler()->InvokeCallback();
-  delegate1.download_handler()->Reset();
-
-  // Verify icon2 has been saved into history.
-  EXPECT_EQ(kSourceIconURLs[1].icon_url, handler1.history_handler()->icon_url_);
-  EXPECT_EQ(gfx::Size(actual_size2, actual_size2),
-            handler1.history_handler()->size_);
+  EXPECT_CALL(delegate_,
+              OnFaviconUpdated(_, _, kIconURL12x12, _, ImageSizeIs(12, 12)));
+
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {FaviconURL(kIconURL10x10, FAVICON, SizeVector{gfx::Size(16, 16)}),
+       FaviconURL(kIconURL12x12, FAVICON, SizeVector{gfx::Size(15, 15)}),
+       FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
 }
 
 }  // namespace