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..dd7069131c6a54142f5fa30cea2a3144d82ecca7 100644
--- a/components/favicon/core/favicon_handler_unittest.cc
+++ b/components/favicon/core/favicon_handler_unittest.cc
@@ -11,7 +11,13 @@
 #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 "components/favicon/core/favicon_client.h"
 #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 +28,820 @@
 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::AtMost;
+using testing::InSequence;
+using testing::Invoke;
+using testing::Return;
+using testing::SaveArg;
+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) {
+  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) {
+    bool expired) {
   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 {
+class MockDelegate : public FaviconHandler::Delegate {
  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_|.
-  }
-
-  // Make downloads for any of |icon_urls| fail.
-  void FailDownloadForIconURLs(const std::set<GURL>& icon_urls) {
-    should_fail_download_icon_urls_ = icon_urls;
-  }
-
-  // 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 AddDownload(int download_id,
-                   const GURL& image_url,
-                   const std::vector<int>& image_sizes,
+  MOCK_METHOD3(DownloadImage,
+               int(const GURL& url,
                    int max_image_size,
-                   FaviconHandler::Delegate::ImageDownloadCallback callback) {
-    download_.reset(new Download(download_id, image_url, image_sizes,
-                                 max_image_size, callback));
-  }
-
-  void InvokeCallback();
-
-  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; }
-
- 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;
-  };
-
-  std::unique_ptr<Download> download_;
-  bool callback_invoked_;
-
-  // The icon URLs for which the download should fail.
-  std::set<GURL> should_fail_download_icon_urls_;
-
-  // 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_;
-
-  DISALLOW_COPY_AND_ASSIGN(DownloadHandler);
+                   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));
 };
 
-// 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 {
+// Fake that implements the calls to FaviconHalder::Delegate's DownloadImage(),
+// delegated to this class through MockDelegate.
+class FakeImageDownloader {
  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) {
-  }
+  struct Response {
+    int http_status_code = 404;
+    BitmapVector bitmaps;
+    SizeVector original_bitmap_sizes;
+  };
 
-  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) {
+  FakeImageDownloader() : num_downloads_(0) {}
+
+  // 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) {
+    const Response& response = responses_[url];
+    int download_id = ++num_downloads_;
+    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;
   }
 
-  ~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_;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(HistoryRequestHandler);
-};
-
-class TestDelegate : public FaviconHandler::Delegate {
- 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_;
+  void Add(const GURL& icon_url, const IntVector& sizes) {
+    AddWithOriginalSizes(icon_url, sizes, sizes);
   }
 
-  bool IsOffTheRecord() override { return false; }
+  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;
+  }
 
-  bool IsBookmarked(const GURL& url) override { return false; }
+  void AddError(const GURL& icon_url, int http_status_code) {
+    Response response;
+    response.http_status_code = http_status_code;
+    responses_[icon_url] = response;
+  }
 
-  void OnFaviconUpdated(
-      const GURL& page_url,
-      FaviconDriverObserver::NotificationIconType notification_icon_type,
-      const GURL& icon_url,
-      bool icon_url_changed,
-      const gfx::Image& image) override {
-    ++num_notifications_;
-    icon_url_ = icon_url;
-    image_ = image;
+  // Disables automatic callback for |url|. This is useful for emulating a
+  // download taking a long time. The callback will for DownloadImage() will be
+  // stored in |manual_callback_|.
+  void SetRunCallbackManuallyForUrl(const GURL& url) {
+    manual_callback_url_ = url;
   }
 
-  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; }
+  // 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();
+    return true;
+  }
 
  private:
-  GURL icon_url_;
-  gfx::Image image_;
-  size_t num_notifications_;
+  int num_downloads_;
 
-  // The unique id of a download request. It will be returned to a
-  // FaviconHandler.
-  int download_id_;
+  // URL to disable automatic callbacks for.
+  GURL manual_callback_url_;
 
-  DownloadHandler download_handler_;
+  // Callback for DownloadImage() request for |manual_callback_url_|.
+  base::Closure manual_callback_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestDelegate);
+  // Registered images.
+  std::map<GURL, Response> responses_;
 };
 
-}  // 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 {
- public:
-  static int GetMaximalIconSize(favicon_base::IconType icon_type) {
-    return FaviconHandler::GetMaximalIconSize(icon_type);
-  }
-
-  TestFaviconHandler(FaviconHandler::Delegate* delegate,
-                     FaviconDriverObserver::NotificationIconType handler_type)
-      : FaviconHandler(nullptr, delegate, handler_type) {}
+class FaviconHandlerTest : public testing::Test {
+ protected:
+  const std::vector<gfx::Size> kEmptySizes;
+  const std::vector<SkBitmap> kEmptyBitmaps;
+  const std::vector<FaviconRawBitmapResult> kEmptyRawBitmapResult;
 
-  ~TestFaviconHandler() override {}
+  // 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");
 
-  HistoryRequestHandler* history_handler() {
-    return history_handler_.get();
+  FaviconHandlerTest() {
+    // Emulate the cache being initially empty, unless overriden in tests.
+    ON_CALL(favicon_service_, GetFaviconForPageURL(_, _, _, _, _))
+        .WillByDefault(PostReply<5>(kEmptyRawBitmapResult));
+    ON_CALL(favicon_service_, GetFavicon(_, _, _, _, _))
+        .WillByDefault(PostReply<5>(kEmptyRawBitmapResult));
+    ON_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(_, _, _, _, _, _))
+        .WillByDefault(PostReply<6>(kEmptyRawBitmapResult));
+    // Let's be "nice" about reads, to avoid boilerplate in tests.
+    EXPECT_CALL(favicon_service_, GetFaviconForPageURL(_, _, _, _, _))
+        .Times(AnyNumber());
+    EXPECT_CALL(favicon_service_, GetFavicon(_, _, _, _, _)).Times(AnyNumber());
+    EXPECT_CALL(favicon_service_, WasUnableToDownloadFavicon(_))
+        .Times(AnyNumber());
+    // Let's be "strict" about image downloads, unless explicitly listed in
+    // test.
+    EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0);

[pkotwicz, 2017/03/09 02:15:09]

Each test which wants to check that there has been zero downloads should do this
check explicitly

[mastiz, 2017/03/09 10:40:08]

Done, reverted.

+    // Delegate image downloading to FakeImageDownloader.
+    ON_CALL(delegate_, DownloadImage(_, _, _))
+        .WillByDefault(Invoke(&fake_image_downloader_,
+                              &FakeImageDownloader::DownloadImage));

[pkotwicz, 2017/03/09 02:15:08]

Can we move this ON_CALL to the MockDelegate constructor like
MockDownloadManagerService in download_manager_service_unittest.cc does? 

[mastiz, 2017/03/09 10:40:08]

Done, although I see no benefit.

+    // Register various known icon URLs.
+    fake_image_downloader_.Add(kIconURL10x10, IntVector{10});
+    fake_image_downloader_.Add(kIconURL12x12, IntVector{12});
+    fake_image_downloader_.Add(kIconURL16x16, IntVector{16});
+    fake_image_downloader_.Add(kIconURL64x64, IntVector{64});
+    // 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}));
   }
 
-  // This method will take the ownership of the given handler.
-  void set_history_handler(HistoryRequestHandler* handler) {
-    history_handler_.reset(handler);
+  // 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,
+      bool flush_events_before_candidates = true) {
+    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.
+    if (flush_events_before_candidates)
+      base::RunLoop().RunUntilIdle();
+    handler->OnUpdateFaviconURL(kPageURL, candidates);
+    base::RunLoop().RunUntilIdle();
+    return handler;
   }
 
-  FaviconURL* current_candidate() {
-    return FaviconHandler::current_candidate();
-  }
+  // 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|.
+  //
+  // Returns the chosen size among |candidate_icon_sizes| or -1 if none was
+  // chosen.
+  int DownloadTillDoneIgnoringHistory(
+      const std::vector<FaviconURL>& candidate_icons,
+      const IntVector& candidate_icon_sizes) {
+    DCHECK_EQ(candidate_icons.size(), candidate_icon_sizes.size());
+    // Set up 200 responses for all images, and the corresponding size.
+    for (size_t i = 0; i < candidate_icons.size(); ++i) {
+      const FaviconURL& candidate_icon = candidate_icons[i];
+      const GURL& icon_url = candidate_icon.icon_url;
+      const int icon_size = candidate_icon_sizes[i];
+      fake_image_downloader_.Add(icon_url, IntVector{icon_size});
+    }
 
-  size_t current_candidate_index() const {
-    return current_candidate_index_;
+    EXPECT_CALL(delegate_, DownloadImage(_, _, _))
+        .Times(AtMost(candidate_icon_sizes.size()));
+    EXPECT_CALL(favicon_service_,
+                UpdateFaviconMappingsAndFetch(_, _, _, _, _, _))
+        .Times(AtMost(candidate_icon_sizes.size()));
+    EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, _, FAVICON, _))
+        .Times(AtMost(1));
+
+    GURL chosen_icon_url;
+    ON_CALL(delegate_, OnFaviconUpdated(
+                           kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP, _,
+                           /*icon_url_changed=*/true, _))
+        .WillByDefault(SaveArg<2>(&chosen_icon_url));
+
+    RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             candidate_icons);
+    for (size_t i = 0; i < candidate_icons.size(); ++i) {
+      if (candidate_icons[i].icon_url == chosen_icon_url)
+        return candidate_icon_sizes[i];
+    }
+    return -1;
   }
 
-  const FaviconCandidate& best_favicon_candidate() {
-    return best_favicon_candidate_;
-  }
+  base::MessageLoopForUI message_loop_;
+  std::unique_ptr<ui::test::ScopedSetSupportedScaleFactors>
+      scoped_set_supported_scale_factors_;
+  testing::StrictMock<MockFaviconService> favicon_service_;
+  FakeImageDownloader fake_image_downloader_;
+  testing::NiceMock<MockDelegate> delegate_;
+};
 
- 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));
-  }
+// 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.
+TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
+  const GURL kIconURL("http://www.google.com/favicon");
 
-  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));
-  }
+  ON_CALL(favicon_service_, GetFaviconForPageURL(kPageURL, _, _, _, _))
+      .WillByDefault(PostReply<5>(
+          CreateRawBitmapResult(kIconURL, FAVICON, /*expired=*/false)));
 
-  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));
-  }
+  // Shouldn't request to download icon.
+  EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0);
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kIconURL, /*icon_url_changed=*/true, _));
 
-  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()));
-  }
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {FaviconURL(kIconURL, FAVICON, kEmptySizes)});
+}
 
-  bool ShouldSaveFavicon() override { return true; }
+// Test that OnFaviconUpdated() is called to update the UI when the initial DB
+// request completes if there is matching data in the database. This is nice
+// because we only get the icon URLs associated with the page once the page
+// loading stops which can take a while.
+TEST_F(FaviconHandlerTest, GetFaviconFromHistoryBeforeCandidates) {
+  const GURL kIconURL("http://www.google.com/favicon");
+
+  ON_CALL(favicon_service_, GetFaviconForPageURL(kPageURL, _, _, _, _))
+      .WillByDefault(PostReply<5>(
+          CreateRawBitmapResult(kIconURL, FAVICON, /*expired=*/false)));
+
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kIconURL, /*icon_url_changed=*/true, _));
+
+  FaviconHandler handler(&favicon_service_, &delegate_,
+                         FaviconDriverObserver::NON_TOUCH_16_DIP);
+  handler.FetchFavicon(kPageURL);
+  base::RunLoop().RunUntilIdle();
+}
 
-  GURL page_url_;
+TEST_F(FaviconHandlerTest, DownloadUnknownFavicon) {
+  InSequence seq;
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL16x16},
+                                            FAVICON, _, _, _));
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL16x16, _, _));
+  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, _));
+
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
+}
 
- private:
-  std::unique_ptr<HistoryRequestHandler> history_handler_;
+TEST_F(FaviconHandlerTest, DownloadUnknownFaviconInIncognito) {
+  ON_CALL(delegate_, IsOffTheRecord()).WillByDefault(Return(true));
 
-  DISALLOW_COPY_AND_ASSIGN(TestFaviconHandler);
-};
+  // No writes expected.
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(_, _, _, _, _, _))
+      .Times(0);
+  EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
 
-namespace {
+  InSequence seq;
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL16x16, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kIconURL16x16, /*icon_url_changed=*/true, _));
 
-void HistoryRequestHandler::InvokeCallback() {
-  if (!callback_.is_null()) {
-    callback_.Run(history_results_);
-  }
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
 }
 
-void DownloadHandler::InvokeCallback() {
-  if (callback_invoked_)
-    return;
+// Test that FaviconHandler process finishes when:
+// - The icon URL used by the page has changed.
+// AND
+// - FaviconService::GetFaviconForPageURL() callback returns after
+//   FaviconHandler::OnUpdateFaviconURL() is called.
+TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesFaster) {
+  InSequence seq;
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL16x16},
+                                            FAVICON, _, _, _));
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL16x16, _, _));
+  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, _));
+
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)},
+                           /*flush_events_before_candidates=*/false);
+}
 
-  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 icon is redownloaded if the icon cached for the page URL has
+// expired.
+TEST_F(FaviconHandlerTest, RedownloadExpiredFavicon) {
+  ON_CALL(favicon_service_, GetFaviconForPageURL(kPageURL, _, _, _, _))
+      .WillByDefault(PostReply<5>(
+          CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true)));

[pkotwicz, 2017/03/09 02:15:08]

TestFaviconService::Store(kPageURL, kIconURL16x16,
CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true))

would be a lot nicer than this

[mastiz, 2017/03/09 10:40:08]

I personally disagree this is better because the backend is complex enough to
make the Store() quite magic. On order to do progress with this CL, I hope we
can defer the possibility of adopting a fake FaviconService to the future,
because then I'd even argue a real backend should be adopted.

+

[pkotwicz, 2017/03/09 02:15:08]

We want to check that UpdateFaviconMappingsAndFetch() was not called for
|kIconURL16x16| 

[mastiz, 2017/03/09 10:40:08]

Done.

+  InSequence seq;
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kIconURL16x16, /*icon_url_changed=*/true, _));

[pkotwicz, 2017/03/09 02:15:08]

We don't care about the values of any of the parameters except for the icon URL

[mastiz, 2017/03/09 10:40:07]

Done, except for icon_url_changed which I think we do care about.

[pkotwicz, 2017/03/13 05:16:31]

I'd rather have separate tests which test the |icon_url_changed| parameter. It
is confusing to have one test test multiple things. For the purpose of this test
I think that we only care about the "icon URL"

[mastiz, 2017/03/13 12:46:02]

Done.

+  EXPECT_CALL(delegate_, DownloadImage(kIconURL16x16, _, _));
+  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=*/false, _));

[pkotwicz, 2017/03/09 02:15:08]

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).

You could do this by having the data in the database and the data which is
downloaded have a different color. Checking for the size is unfortunately not
helpful because FaviconHandlers of type FaviconDriverObserver::NON_TOUCH_16_DIP
resize the gfx::Image to 16x16 prior to calling Delegate::OnFaviconUpdated()

[mastiz, 2017/03/09 10:40:08]

Added TODO, since I believe the former tests didn't verify this either. I'll
address this in a follow-up patch.

[pkotwicz, 2017/03/13 05:16:31]

Please file a bug to do this :)

[mastiz, 2017/03/13 12:46:03]

Done.

+
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});

[pkotwicz, 2017/03/09 02:15:08]

Can you add RunHandlerWithGURLCandidates() which takes a std::vector<GURL>
and constructs FaviconURLs with favicon_base::FAVICON and empty sizes

[mastiz, 2017/03/09 10:40:07]

Done.

 }
 
-class FaviconHandlerTest : public testing::Test {
- protected:
-  FaviconHandlerTest() {
-  }
+// Test that an icon is not redownloaded if a previous attempt returned a 404.
+TEST_F(FaviconHandlerTest, NotRedownloadForPrevious404) {
+  ON_CALL(favicon_service_, WasUnableToDownloadFavicon(kIconURL16x16))
+      .WillByDefault(Return(true));
 
-  ~FaviconHandlerTest() override {}
+  EXPECT_CALL(delegate_, DownloadImage(_, _, _)).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();
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL16x16},
+                                            _, _, _, _));
 
-    UpdateFaviconURL(delegate, favicon_handler, page_url, candidate_icons);
-    EXPECT_EQ(candidate_icons.size(), favicon_handler->image_urls().size());
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
+}
 
-    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;
-    }
-  }
+TEST_F(FaviconHandlerTest, Report404) {
+  const GURL k404IconURL("http://www.google.com/404.png");
 
-  void UpdateFaviconURL(TestDelegate* delegate,
-                        TestFaviconHandler* favicon_handler,
-                        const GURL& page_url,
-                        const std::vector<FaviconURL>& candidate_icons) {
-    delegate->ResetNumNotifications();
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(_, _, _, _, _, _));
+  EXPECT_CALL(delegate_, DownloadImage(k404IconURL, _, _));
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(k404IconURL));
 
-    favicon_handler->FetchFavicon(page_url);
-    favicon_handler->history_handler()->InvokeCallback();
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {FaviconURL(k404IconURL, FAVICON, kEmptySizes)});
+}
 
-    favicon_handler->OnUpdateFaviconURL(page_url, candidate_icons);
-  }
+// 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");
 
-  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();
-  }
+  fake_image_downloader_.AddError(k503IconURL, 503);
 
-  std::unique_ptr<ui::test::ScopedSetSupportedScaleFactors>
-      scoped_set_supported_scale_factors_;
-};
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(k503IconURL)).Times(0);
 
-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());
-}
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(_, _, _, _, _, _));
+  EXPECT_CALL(delegate_, DownloadImage(k503IconURL, _, _));
 
-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());
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {FaviconURL(k503IconURL, FAVICON, kEmptySizes)});
 }
 
 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");
+  ON_CALL(favicon_service_, GetFaviconForPageURL(kPageURL, _, _, _, _))
+      .WillByDefault(PostReply<5>(CreateRawBitmapResult(kOldIconURL, FAVICON,
+                                                        /*expired=*/false)));
+
+  InSequence seq;
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kOldIconURL, /*icon_url_changed=*/true, _));
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL16x16},
+                                            FAVICON, _, _, _));
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL16x16, _, _));
+  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, _));
+
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
 }
 
+// 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) {
-  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 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;
+
+  ON_CALL(favicon_service_, GetFaviconForPageURL(kPageURL, _, _, _, _))
+      .WillByDefault(
+          PostReply<5>(std::vector<FaviconRawBitmapResult>{bitmap_result}));
+
+  InSequence seq;
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL16x16, _, _));
+  EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kIconURL16x16, /*icon_url_changed=*/true, _));
+
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
 }
 
+// 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 kIconURL("http://www.google.com/favicon");
+  const GURL kNewIconURL("http://www.google.com/new_favicon");
+
+  ON_CALL(favicon_service_, GetFaviconForPageURL(kPageURL, FAVICON, _, _, _))
+      .WillByDefault(PostReply<5>(
+          CreateRawBitmapResult(kIconURL, FAVICON, /*expired=*/false)));
+
+  // Shouldn't request to download icon.
+  EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0);
+
+  InSequence seq;
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kIconURL, /*icon_url_changed=*/true, _));
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kNewIconURL},
+                                            FAVICON, _, _, _))

[pkotwicz, 2017/03/09 02:15:08]

We don't need to check whether UpdateFaviconMappingsAndFetch() is called or not.
If a download was not done, FaviconHandler must have gotten the image data from
the database

[mastiz, 2017/03/09 10:40:08]

Needed unless we adopt NiceMock for FaviconService.

+      .WillOnce(PostReply<6>(
+          CreateRawBitmapResult(kNewIconURL, FAVICON, /*expired=*/false)));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kNewIconURL, /*icon_url_changed=*/true, _));

[pkotwicz, 2017/03/09 02:15:08]

For the sake of completeness we should check that FaviconService::SetFavicons()
is not called

[mastiz, 2017/03/09 10:40:08]

Done.

+
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {FaviconURL(kNewIconURL, FAVICON, kEmptySizes)});
 }
 
 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");
+  fake_image_downloader_.AddError(kIconURLReturning500, 500);
+
+  // First download will fail, second will succeed.
+  EXPECT_CALL(delegate_, DownloadImage(kIconURLReturning500, _, _));
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL64x64, _, _));
+
+  InSequence seq;
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    kPageURL, URLVector{kIconURLReturning500},
+                                    TOUCH_PRECOMPOSED_ICON, _, _, _));
+  // Simulates getting an expired icon from history.
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL64x64},
+                                            TOUCH_ICON, _, _, _))
+      .WillOnce(PostReply<6>(
+          CreateRawBitmapResult(kIconURL64x64, TOUCH_ICON, /*expired=*/true)));
+  EXPECT_CALL(delegate_,
+              OnFaviconUpdated(kPageURL, FaviconDriverObserver::TOUCH_LARGEST,
+                               kIconURL64x64, /*icon_url_changed=*/true, _));
+  // New icon should be saved to favicon service.
+  EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL64x64, TOUCH_ICON,
+                                            ImageSizeIs(64, 64)));
+  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),
+      });
 }
 
+// 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.
 TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {

[pkotwicz, 2017/03/09 02:15:09]

We want to:
- set initial state
- test the initial state
- run the test
- test the final state

In order for the testing of the final state to check things which did not occur
as part of setting the initial state, you have to call
VerifyAndClearExpectations()

You can probably put
VerifyAndClearExpectations(favicon_service_) and 
VerifyAndClearExpectations(delegate_) into a function called
VerifyAndClearExpectCalls()

[mastiz, 2017/03/09 10:40:08]

Done.

FYI, I've used VerifyAndClearExpectations in the past
(https://cs.chromium.org/chromium/src/components/ntp_tiles/most_visited_sites_...)
and you should know that it was always perceived as smell, although gmock
documents this is a technique:
https://g3doc.corp.google.com/third_party/gmock/g3doc/guide.md#UsingCheckPoints

In this case, I believe what you have in mind is to make "achieve downloading
state" as part of the test fixture. In the ideal world, the text fixture should
be set without exercising the class under test, which is not possible here.

The ugliness here is about the EXPECT_CALL(favicon_service_,
...).Times(AnyNumber()) that the code does on some trivial functions, without
actually adopting a NiceMock for FaviconService. I suspect you might instead
want to indeed adopt NiceMock for FaviconService, please let me know.

-  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");
+  const GURL kNewIconURL("http://www.google.com/new_favicon");
+  const GURL kLatestIconURL("http://www.google.com/latest_favicon");
+
+  // Defer the download completion such that RunUntilIdle() doesn't complete
+  // the download.
+  fake_image_downloader_.SetRunCallbackManuallyForUrl(kIconURL);
+  fake_image_downloader_.Add(kIconURL, IntVector{16});
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL, _, _));
+
+  // Make sure kIconURL isn't saved, although StrictMock does it implicitly.
+  EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL, _, _)).Times(0);

[pkotwicz, 2017/03/09 02:15:08]

We don't want SetFavicons() to be called at all for any URL. You can replace
kIconURL with '_'

[mastiz, 2017/03/09 10:40:07]

Done.

+
+  InSequence seq;
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL},
+                                            TOUCH_PRECOMPOSED_ICON, _, _, _));
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kLatestIconURL},
+                                            TOUCH_ICON, _, _, _))
+      .WillOnce(PostReply<6>(CreateRawBitmapResult(kLatestIconURL, TOUCH_ICON,
+                                                   /*expired=*/false)));

[pkotwicz, 2017/03/09 02:15:08]

We don't care about the UpdateFaviconMappingsAndFetch() calls in this test

[mastiz, 2017/03/09 10:40:07]

Needed unless we adopt NiceMock for FaviconService.

+  EXPECT_CALL(delegate_,
+              OnFaviconUpdated(kPageURL, FaviconDriverObserver::TOUCH_LARGEST,
+                               kLatestIconURL, /*icon_url_changed=*/true, _));

[pkotwicz, 2017/03/09 02:15:09]

kLatestIconUrl is the only parameter that we care about in the
OnFaviconUpdated() call

[mastiz, 2017/03/09 10:40:08]

Done. Note that my tests we covering the same expectations as before.

+
+  std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(

[pkotwicz, 2017/03/09 02:15:08]

1) Some of the old tests were testing too many things at once.
UpdateDuringDownloading() was one such test. Make this test deal with
FaviconURLs of type favicon_base::FAVICON

In https://codereview.chromium.org/2738663002 I added
FaviconHandlerTest.OnlyDownloadMatchingIconType
FaviconHandlerTest.ShouldCallSetFaviconsWithCorrectIconType and to test the
behavior of touch FaviconURLs

[mastiz, 2017/03/09 10:40:08]

Adopted FAVICON and added TODO for the rest. I propose we add new tests in
follow-up CLs, this was originally a refactoring of existing tests and is
already quite huge.

[pkotwicz, 2017/03/13 05:16:31]

Adding new tests in a follow up CL sounds good to me

+      FaviconDriverObserver::TOUCH_LARGEST,
+      {
+          FaviconURL(kIconURL, TOUCH_PRECOMPOSED_ICON, kEmptySizes),
+          FaviconURL(kNewIconURL, TOUCH_ICON, kEmptySizes),
+      });

[pkotwicz, 2017/03/09 02:15:08]

For the "verify the final state" part of this test you should check
EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kLatestIconURL, _, _));
EXPECT_CALL(favicon_service_, SetFavicons(_, kLatestIconURL, _, _))
EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(_,
URLVector{kLatestIconURL}, _, _, _, _));
EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0);

[mastiz, 2017/03/09 10:40:08]

Done, I added similar expectations but not exactly the ones above, since
kLatestIconURL is actually downloaded.

+
+  // Favicon update (should invalidate the ongoing download).
+  EXPECT_TRUE(fake_image_downloader_.HasPendingManualCallback());
+  handler->OnUpdateFaviconURL(
+      kPageURL, {FaviconURL(kLatestIconURL, TOUCH_ICON, kEmptySizes)});
+  // Finalizes download, which should be thrown away as there is favicon update.
+  EXPECT_TRUE(fake_image_downloader_.RunCallbackManually());
+  base::RunLoop().RunUntilIdle();
 }
 
-// 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 that calling OnUpdateFaviconUrl() with the same icon URLs as the
+// previous call to OnUpdateFaviconUrl() while the FaviconHandler process is in
+// progresss does not restart the process from the beginning.
+TEST_F(FaviconHandlerTest, UpdateSameIconURLsWhileProcessing) {
+  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.
+  fake_image_downloader_.SetRunCallbackManuallyForUrl(kSlowLoadingIconURL);
+  fake_image_downloader_.Add(kSlowLoadingIconURL, IntVector{16});
+
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL64x64, _, _));
+  EXPECT_CALL(delegate_, DownloadImage(kSlowLoadingIconURL, _, _));
+
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL64x64},
+                                            _, _, _, _));
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(
+                  kPageURL, URLVector{kSlowLoadingIconURL}, _, _, _, _));
+
+  std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls);
+
+  // Calling OnUpdateFaviconURL() with the same icon URLs should have no effect,
+  // despite the ongoing download.
+  EXPECT_TRUE(fake_image_downloader_.HasPendingManualCallback());
+  handler->OnUpdateFaviconURL(kPageURL, favicon_urls);
+  base::RunLoop().RunUntilIdle();
+
+  // Complete the download.
+  InSequence seq;
+  EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _));
+  EXPECT_TRUE(fake_image_downloader_.RunCallbackManually());
+  base::RunLoop().RunUntilIdle();
+}
+
+// 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 for pages with iframes.
+TEST_F(FaviconHandlerTest, UpdateSameIconURLsAfterFinished) {
+  const std::vector<FaviconURL> favicon_urls = {
+      FaviconURL(kIconURL10x10, FAVICON, kEmptySizes),
+      FaviconURL(kIconURL16x16, FAVICON, kEmptySizes),
+  };
+
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL10x10, _, _));
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL16x16, _, _));
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    _, URLVector{kIconURL10x10}, _, _, _, _));
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    _, URLVector{kIconURL16x16}, _, _, _, _));
+  EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, _, _));
+
+  std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls);
+

[pkotwicz, 2017/03/09 02:15:08]

You should call VerifyExpectCalls() and have new expectations for the "test
final state" part of the test.

In particular
EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(_, _, _, _, _,
_)).Times(0); 
EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0);
EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0);
EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);

[mastiz, 2017/03/09 10:40:08]

Done.

+  // Calling OnUpdateFaviconURL() with identical data should be a no-op.
+  handler->OnUpdateFaviconURL(kPageURL, favicon_urls);
+  base::RunLoop().RunUntilIdle();
 }
 
 // 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);
+  const GURL kIconURL1(
+      "http://wwww.page_which_animates_favicon.com/frame1.png");
+  const GURL kIconURL2(
+      "http://wwww.page_which_animates_favicon.com/frame2.png");
 
   // 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
+  // FaviconHandler should download |kIconURL1| and |kIconURL2|. |kIconURL1| is
+  // the better match.
+  fake_image_downloader_.Add(kIconURL1, IntVector{15});
+  fake_image_downloader_.Add(kIconURL2, IntVector{10});
+
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL1, _, _));
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL2, _, _)).Times(2);

[pkotwicz, 2017/03/09 02:15:08]

- We only care about the downloads which occur in the "setup" part of the test.
The reason that we care about the downloads in the "setup" part is that we want
to ensure that FaviconHandler tried both candidates

- For the setup of this test it is critical that the download of |kIconURL1|
occurs before the download of |kIconURL2|.

The GMock syntax would be I guess
{
  InSequence seq;
  EXPECT_CALL(delegate_, DownloadImage(kIconURL1, _, _));
  EXPECT_CALL(delegate_, DownloadImage(kIconURL2, _, _));
}

When I am going through the CL I am comparing the assertions against those in my
version of FaviconHandlerTest.ShouldCallOnFaviconAvailableAfterIconURLChange

[mastiz, 2017/03/09 10:40:08]

Done.

+
+  // FaviconHandler should ask for the mappings and fetch expired icons.
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    _, URLVector{kIconURL1}, FAVICON, _, _, _))
+      .WillOnce(PostReply<6>(
+          CreateRawBitmapResult(kIconURL1, FAVICON, /*expired=*/true)));

[pkotwicz, 2017/03/09 02:15:08]

It is unimportant for there to be data in the database for this test. The test
works just fine without any data in the database

(It also results in a less insane number of calls to
Delegate::OnUpdateFaviconURL(). The insane number of calls to
Delegate::OnUpdateFaviconURL() is a bug.)

[mastiz, 2017/03/09 10:40:07]

Done. Please file a bug that describes the repeated notifications and assign it
to me.

+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    _, URLVector{kIconURL2}, _, _, _, _))
+      .WillOnce(PostReply<6>(
+          CreateRawBitmapResult(kIconURL2, FAVICON, /*expired=*/true)));
+
+  // Two FaviconDriver::OnFaviconUpdated() notifications should be sent for
+  // |kIconURL1|, one before and one after the download.
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL1, _, _)).Times(2);
   // 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);
+  // |kIconURL2|.
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL2, _, _));
+  EXPECT_CALL(favicon_service_,
+              SetFavicons(_, kIconURL1, _, ImageSizeIs(16, 16)));
+
+  std::unique_ptr<FaviconHandler> handler =
+      RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                               {
+                                   FaviconURL(kIconURL1, FAVICON, kEmptySizes),
+                                   FaviconURL(kIconURL2, FAVICON, kEmptySizes),
+                               });

[pkotwicz, 2017/03/09 02:15:08]

We want to:
- set initial state
- test the initial state
- run the test
- test the final state

In order for the testing of the final state to check things which did not occur
as part of setting the initial state, you have to call
VerifyAndClearExpectations()

[mastiz, 2017/03/09 10:40:08]

Done.

 
   // 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>())));
+  InSequence seq;
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    _, URLVector{kIconURL2}, _, _, _, _))
+      .WillOnce(PostReply<6>(
+          CreateRawBitmapResult(kIconURL2, FAVICON, /*expired=*/true)));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL2,
+                                          /*icon_url_changed=*/true, _));
+  EXPECT_CALL(favicon_service_,
+              SetFavicons(_, kIconURL2, _, ImageSizeIs(16, 16)));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL2, _, _));
+
+  handler->OnUpdateFaviconURL(kPageURL,
+                              {FaviconURL(kIconURL2, FAVICON, kEmptySizes)});
+  base::RunLoop().RunUntilIdle();
+}
 
-  // 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());
+// The goal of these tests is to be more of an integration test than
+// SelectFaviconFramesTest.*. They test which favicon is selected when the web
+// page provides several favicons and none are cached in history.
+class FaviconHandlerMultipleFaviconsTest : public FaviconHandlerTest {
+ protected:
+  const std::vector<FaviconURL> kSourceIconURLs = {
+      FaviconURL(GURL("http://www.google.com/a"), FAVICON, kEmptySizes),
+      FaviconURL(GURL("http://www.google.com/b"), FAVICON, kEmptySizes),
+      FaviconURL(GURL("http://www.google.com/c"), FAVICON, kEmptySizes),
+      FaviconURL(GURL("http://www.google.com/d"), FAVICON, kEmptySizes),
+      FaviconURL(GURL("http://www.google.com/e"), FAVICON, kEmptySizes)};
+
+  FaviconHandlerMultipleFaviconsTest() {
+    scoped_set_supported_scale_factors_.reset(
+        new ui::test::ScopedSetSupportedScaleFactors(
+            {ui::SCALE_FACTOR_100P, ui::SCALE_FACTOR_200P}));
+  }
+
+  // Same as from the base class but with fixed FaviconURLs.
+  int DownloadTillDoneIgnoringHistory(const IntVector& candidate_icon_sizes) {
+    return FaviconHandlerTest::DownloadTillDoneIgnoringHistory(
+        std::vector<FaviconURL>(
+            kSourceIconURLs.begin(),
+            kSourceIconURLs.begin() + candidate_icon_sizes.size()),
+        candidate_icon_sizes);
+  }
+};
+
+// Test that if there are several single resolution favicons to choose from
+// that the largest exact match is chosen.
+TEST_F(FaviconHandlerMultipleFaviconsTest, ChooseLargestExactMatch) {
+  EXPECT_EQ(32, DownloadTillDoneIgnoringHistory(
+                    std::vector<int>{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(std::vector<int>{16, 24, 48, 256}));
 }
 
-// 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());
+// Test that favicons which need to be upsampled a little or downsampled
+// a little are preferred over huge favicons.
+TEST_F(FaviconHandlerMultipleFaviconsTest,
+       ChooseMinorResamplingOverHugeFavicons) {
+  EXPECT_EQ(48, DownloadTillDoneIgnoringHistory(std::vector<int>{256, 48}));
+}
+
+TEST_F(FaviconHandlerMultipleFaviconsTest,
+       ChooseMinorResamplingOverHugeFavicons2) {
+  EXPECT_EQ(17, DownloadTillDoneIgnoringHistory(std::vector<int>{17, 256}));
 }
 
 // Test that the best favicon is selected when:
@@ -1224,51 +849,33 @@ 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(kIconURL64x64))
+      .WillByDefault(Return(true));
+
+  EXPECT_EQ(16, DownloadTillDoneIgnoringHistory(
+                    {
+                        FaviconURL(kIconURL16x16, FAVICON, kEmptySizes),
+                        FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+                    },
+                    IntVector{16, 64}));
+}
 
-  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());
+// 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.
+TEST_F(FaviconHandlerTest, MultipleFaviconsLast404) {
+  ON_CALL(favicon_service_, WasUnableToDownloadFavicon(kIconURL64x64))
+      .WillByDefault(Return(true));
+
+  EXPECT_EQ(10, DownloadTillDoneIgnoringHistory(
+                    {
+                        // Sizes intentionally wrong to reproduce a corner case.
+                        FaviconURL(kIconURL10x10, FAVICON, {gfx::Size(16, 16)}),
+                        FaviconURL(kIconURL64x64, FAVICON, {gfx::Size(15, 15)}),
+                    },
+                    IntVector{10, 64}));
 }
 
 // Test that no favicon is selected when:
@@ -1276,100 +883,53 @@ 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);
+
+  EXPECT_EQ(-1, DownloadTillDoneIgnoringHistory(
+                    {
+                        FaviconURL(kIconURL16x16, FAVICON, kEmptySizes),
+                        FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+                    },
+                    IntVector{16, 64}));
 }
 
 // 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>());
+  // Shouldn't request to download icon.
+  EXPECT_CALL(delegate_, DownloadImage(_, _, _)).Times(0);
+  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());
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_16_DIP,
+      {FaviconURL(kInvalidFormatURL, FAVICON, kEmptySizes)});
 }
 
 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/b"), FAVICON,
+                 {gfx::Size(15, 15), gfx::Size(16, 16)}),
+      FaviconURL(GURL("http://www.google.com/a"), 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)};
+
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(_, _, _, _, _, _));
+  EXPECT_CALL(delegate_, DownloadImage(_, _, _)).WillOnce(Return(1));
+
+  std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST, kSourceIconURLs);
 
   struct ExpectedResult {
     // The favicon's index in kSourceIconURLs.
@@ -1377,19 +937,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 16x16 is largest.
+      {0, 16},
+      // Third is icon3 though it has same size as icon1.
+      // The 16x16 is largest.
+      {2, 16},
+      // The rest of bitmaps come in order, there is no sizes attribute.
+      {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 +960,113 @@ 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},
-  };
-
-  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());
+  {

[pkotwicz, 2017/03/09 02:15:08]

I think that it is easy to have bugs with this test. In particular, the order
that the bitmap sizes are downloaded is non obvious (the order that the icon
URLs are downloaded is obvious).

I think that this would make the order that the bitmap sizes are downloaded
clearer:

const FaviconURL kSourceIconURLs[] = {
    ...
};
struct ExpectedResult {
    ...
} results[] = {
    ...
};

{
  InSequence seq;
  for (const ExpectedResult& result : results) {
    FaviconURL expected_favicon = kSourceIconURLs[result.favicon_index];
    if (result.edge_size > 0) {
      EXPECT_THAT(favicon.icon_sizes,
testing::Contains(gfx::Size(result.edge_size, result.edge_size)));
    }
    EXPECT_CALL(delegate_, DownloadImage(expected_favicon.icon_url, _, _));
  }
}

Alternatively, you could have different variables for each FaviconURL and name
the FaviconURLs based on the sizes.
For instance:
FaviconURL favicon_1024_and_512 =
    FaviconURL(GURL("http://www.google.com/a"),
               FAVICON,
               {gfx::Size(1024, 1024), gfx::Size(512, 512)});

[mastiz, 2017/03/09 10:40:07]

Done, went ahead with the second.

+    InSequence seq;
+    for (const GURL& icon_url : std::vector<GURL>{
+             GURL("http://www.google.com/a"), GURL("http://www.google.com/c"),
+             GURL("http://www.google.com/b"),
+             // The rest of bitmaps come in order, there is no sizes attribute.
+             GURL("http://www.google.com/d"),
+             GURL("http://www.google.com/e")}) {
+      // Icon URLs are not registered and hence 404s will be produced, which
+      // allows checking whether the icons were requested according to their
+      // size.
+      EXPECT_CALL(delegate_, DownloadImage(icon_url, _, _));
     }
-
-    // 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[results[i].favicon_index].icon_url,
-              delegate1.download_handler()->GetImageUrl());
-
-    delegate1.download_handler()->InvokeCallback();
-    delegate1.download_handler()->Reset();
   }
+
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(_, _, _, _, _, _))
+      .Times(5);

[pkotwicz, 2017/03/09 02:15:08]

We don't care about calls to UpdateFaviconMappingsAndFetch() here

[mastiz, 2017/03/09 10:40:08]

Needed unless we adopt a NiceMock for FaviconService.

+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(_)).Times(5);
+
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {FaviconURL(GURL("http://www.google.com/a"), FAVICON,
+                  {gfx::Size(1024, 1024), gfx::Size(512, 512)}),
+       FaviconURL(GURL("http://www.google.com/b"), FAVICON,
+                  {gfx::Size(15, 15), gfx::Size(14, 14)}),
+       FaviconURL(GURL("http://www.google.com/c"), FAVICON,
+                  {gfx::Size(16, 16), gfx::Size(512, 512)}),
+       FaviconURL(GURL("http://www.google.com/d"), FAVICON, kEmptySizes),
+       FaviconURL(GURL("http://www.google.com/e"), FAVICON, kEmptySizes)});
 }
 
 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();
+  const GURL kIconURL1("http://www.google.com/b");
+  const GURL kIconURL2("http://www.google.com/c");
+
+  fake_image_downloader_.Add(kIconURL2, IntVector{14, 16});
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL2, _, _));
 
   // 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_);
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL2}, _,
+                                            _, _, _));
+  EXPECT_CALL(favicon_service_,
+              SetFavicons(kPageURL, kIconURL2, FAVICON, ImageSizeIs(16, 16)));
   // 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_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_LARGEST,
+                             kIconURL2, /*icon_url_changed=*/true, _));
+
+  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, 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();
+  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;
+
+  fake_image_downloader_.AddWithOriginalSizes(
+      kIconURL2, IntVector{kMaximalSize}, IntVector{kOriginalSize2});
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL2, _, _));
 
   // 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_);
+  // scaled down to maximal size and smaller than |kIconURL1| now.
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL2}, _,
+                                            _, _, _));
+  EXPECT_CALL(favicon_service_,
+              SetFavicons(kPageURL, kIconURL2, FAVICON,
+                          ImageSizeIs(kMaximalSize, kMaximalSize)));
+
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {FaviconURL(kIconURL1, FAVICON,
+                  SizeVector{gfx::Size(kOriginalSize1, kOriginalSize1)}),
+       FaviconURL(kIconURL2, FAVICON,
+                  SizeVector{gfx::Size(kOriginalSize2, kOriginalSize2)})});
 }
 
 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_);
+  {
+    InSequence seq;
+    EXPECT_CALL(favicon_service_,
+                UpdateFaviconMappingsAndFetch(
+                    kPageURL, URLVector{kIconURL10x10}, _, _, _, _));
+    // Very the best candidate is icon 2.
+    EXPECT_CALL(favicon_service_,
+                UpdateFaviconMappingsAndFetch(
+                    kPageURL, URLVector{kIconURL12x12}, _, _, _, _));
+  }

[pkotwicz, 2017/03/09 02:15:08]

We don't care about the UpdateFaviconMappingsAndFetch() calls in the context of
this test

[mastiz, 2017/03/09 10:40:08]

Ditto.

+
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL10x10, _, _));
+  EXPECT_CALL(delegate_, DownloadImage(kIconURL12x12, _, _));
+  EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL12x12, FAVICON,
+                                            ImageSizeIs(12, 12)));

[pkotwicz, 2017/03/09 02:15:08]

In my CL I have made all of the tests check for the Delegate::OnFaviconUpdated()
call. I understand that the old test calls FaviconService::SetFavicons()

Checking for Delegate::OnFaviconUpdated() and FaviconService::SetFavicons() is
somewhat interchangeable. FaviconService::SetFavicons() is called a subset of
the times that Delegate::OnFaviconUpdated()  is called.

As usual, we only care about the selected icon URL. Thus
EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL12x12, _, _));

[mastiz, 2017/03/09 10:40:08]

Ditto: while FaviconService uses StrictMock, it's more convenient to verify
SetFavicons.

+
+  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