Always select best favicon bitmap

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 7514a5e2ecdd7b1cf8d2a1e117afb282d042b684..e478a44524165dc8c787d4e9d4a0ff4edaf0dc60 100644
--- a/components/favicon/core/favicon_handler_unittest.cc
+++ b/components/favicon/core/favicon_handler_unittest.cc
@@ -25,6 +25,7 @@
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/image/image.h"
+#include "ui/gfx/image/image_skia.h"
 
 namespace favicon {
 namespace {
@@ -33,6 +34,7 @@ using favicon_base::FAVICON;
 using favicon_base::FaviconRawBitmapResult;
 using favicon_base::TOUCH_ICON;
 using favicon_base::TOUCH_PRECOMPOSED_ICON;
+using testing::AllOf;
 using testing::AnyNumber;
 using testing::Assign;
 using testing::AtLeast;
@@ -54,26 +56,42 @@ MATCHER_P2(ImageSizeIs, width, 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);
+MATCHER_P(HasScaleFactor, scale, "") {
+  return arg.AsImageSkia().HasRepresentation(scale);
+}
 
-  unsigned char* src_data =
-      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);
+MATCHER_P2(ImageColorForScaleIs, scale, expected_color, "") {
+  gfx::ImageSkia image_skia = arg.AsImageSkia();
+  image_skia.EnsureRepsForSupportedScales();
+  const SkBitmap& bitmap = image_skia.GetRepresentation(scale).sk_bitmap();
+  if (bitmap.empty()) {
+    *result_listener << "expected color but no bitmap data available";
   }
-  return bmp;
+
+  SkAutoLockPixels lock(bitmap);
+  SkColor actual_color = bitmap.getColor(1, 1);
+  if (actual_color == expected_color) {
+    return true;
+  }
+
+  *result_listener << "expected color "
+                   << base::StringPrintf("%08X", expected_color)
+                   << " but actual color is "
+                   << base::StringPrintf("%08X", actual_color);
+  return false;
 }
 
-// Fill the given data buffer with valid png data.
-void FillBitmap(int w, int h, std::vector<unsigned char>* output) {
-  SkBitmap bitmap = CreateBitmap(w, h);
-  gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, false, output);
+MATCHER_P(HasIconURL, expected_url, "") {
+  *result_listener << "where icon URL is " << arg.icon_url;
+  return arg.icon_url == GURL(expected_url);
+}
+
+// Fill the given bmp with some test data.
+SkBitmap CreateBitmap(int w, int h, SkColor color) {
+  SkBitmap bmp;
+  bmp.allocN32Pixels(w, h);
+  bmp.eraseColor(color);
+  return bmp;
 }
 
 std::vector<gfx::Size> CreateSquareSizes(const IntVector& sizes) {
@@ -84,28 +102,42 @@ std::vector<gfx::Size> CreateSquareSizes(const IntVector& sizes) {
   return result;
 }
 
-std::vector<SkBitmap> CreateBitmaps(const std::vector<gfx::Size>& sizes) {
+std::vector<SkBitmap> CreateBitmaps(const std::vector<gfx::Size>& sizes,
+                                    const std::vector<SkColor>& colors) {
   std::vector<SkBitmap> bitmaps;
-  for (const gfx::Size& size : sizes) {
-    bitmaps.push_back(CreateBitmap(size.width(), size.height()));
+  for (size_t i = 0; i < sizes.size(); ++i) {
+    bitmaps.push_back(CreateBitmap(sizes[i].width(), sizes[i].height(),
+                                   colors[i]));
   }
   return bitmaps;
 }
 
-std::vector<FaviconRawBitmapResult> CreateRawBitmapResult(
-    const GURL& icon_url,
-    favicon_base::IconType icon_type = FAVICON,
-    bool expired = false) {
+FaviconRawBitmapResult CreateRawBitmapResult(const GURL& icon_url,
+                                             favicon_base::IconType icon_type,
+                                             bool expired,
+                                             const gfx::Size& size,
+                                             SkColor color) {
   scoped_refptr<base::RefCountedBytes> data(new base::RefCountedBytes());
-  FillBitmap(gfx::kFaviconSize, gfx::kFaviconSize, &data->data());
+  SkBitmap bitmap = CreateBitmap(size.width(), size.height(), color);
+  gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, false, &data->data());
   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.pixel_size = size;
   bitmap_result.icon_type = icon_type;
   bitmap_result.icon_url = icon_url;
-  return {bitmap_result};
+  return bitmap_result;
+}
+
+std::vector<FaviconRawBitmapResult> CreateRawBitmapResults(
+    const GURL& icon_url,
+    favicon_base::IconType icon_type = FAVICON,
+    bool expired = false) {
+  // Use a pixel size other than (0,0) as (0,0) has a special meaning.
+  return {
+      CreateRawBitmapResult(icon_url, icon_type, expired,
+                            gfx::Size(gfx::kFaviconSize, gfx::kFaviconSize),
+                            SK_ColorRED)};
 }
 
 // Fake that implements the calls to FaviconHalder::Delegate's DownloadImage(),
@@ -141,14 +173,29 @@ class FakeImageDownloader {
     AddWithOriginalSizes(icon_url, sizes, sizes);
   }
 
+  void AddWithColors(const GURL& icon_url, const IntVector& sizes,
+                     const std::vector<SkColor>& colors) {
+    AddWithColorsAndOriginalSizes(icon_url, sizes, colors, sizes);
+  }
+
   void AddWithOriginalSizes(const GURL& icon_url,
                             const IntVector& sizes,
                             const IntVector& original_sizes) {
+    std::vector<SkColor> colors;
+    colors.resize(sizes.size(), SK_ColorRED);
+    AddWithColorsAndOriginalSizes(icon_url, sizes, colors, original_sizes);
+  }
+
+  void AddWithColorsAndOriginalSizes(const GURL& icon_url,
+                                     const IntVector& sizes,
+                                     const std::vector<SkColor>& colors,
+                                     const IntVector& original_sizes) {
+    DCHECK_EQ(sizes.size(), colors.size());
     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));
+    response.bitmaps = CreateBitmaps(CreateSquareSizes(sizes), colors);
     responses_[icon_url] = response;
   }
 
@@ -250,7 +297,7 @@ class FakeFaviconService {
   base::CancelableTaskTracker::TaskId GetFavicon(
       const GURL& icon_url,
       favicon_base::IconType icon_type,
-      int desired_size_in_dip,
+      const std::vector<int>& desired_sizes_in_pixel,
       const favicon_base::FaviconResultsCallback& callback,
       base::CancelableTaskTracker* tracker) {
     return GetFaviconForPageOrIconURL(icon_url, callback, tracker);
@@ -259,7 +306,7 @@ class FakeFaviconService {
   base::CancelableTaskTracker::TaskId GetFaviconForPageURL(
       const GURL& page_url,
       int icon_types,
-      int desired_size_in_dip,
+      const std::vector<int>& desired_sizes_in_pixel,
       const favicon_base::FaviconResultsCallback& callback,
       base::CancelableTaskTracker* tracker) {
     return GetFaviconForPageOrIconURL(page_url, callback, tracker);
@@ -269,7 +316,7 @@ class FakeFaviconService {
       const GURL& page_url,
       const std::vector<GURL>& icon_urls,
       int icon_types,
-      int desired_size_in_dip,
+      const std::vector<int>& desired_sizes_in_pixel,
       const favicon_base::FaviconResultsCallback& callback,
       base::CancelableTaskTracker* tracker) {
     CHECK_EQ(1U, icon_urls.size()) << "Multi-icon lookup not implemented";
@@ -324,6 +371,7 @@ class FaviconHandlerTest : public testing::Test {
   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 kIconURL32x32 = GURL("http://www.google.com/favicon32x32");
   const GURL kIconURL64x64 = GURL("http://www.google.com/favicon64x64");
 
   FaviconHandlerTest() {
@@ -331,6 +379,7 @@ class FaviconHandlerTest : public testing::Test {
     delegate_.fake_downloader().Add(kIconURL10x10, IntVector{10});
     delegate_.fake_downloader().Add(kIconURL12x12, IntVector{12});
     delegate_.fake_downloader().Add(kIconURL16x16, IntVector{16});
+    delegate_.fake_downloader().Add(kIconURL32x32, IntVector{32});
     delegate_.fake_downloader().Add(kIconURL64x64, IntVector{64});
 
     // The score computed by SelectFaviconFrames() is dependent on the supported
@@ -389,7 +438,7 @@ TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
   const GURL kIconURL("http://www.google.com/favicon");
 
   favicon_service_.fake()->Store(kPageURL, kIconURL,
-                                 CreateRawBitmapResult(kIconURL));
+                                 CreateRawBitmapResults(kIconURL));
 
   // Shouldn't request to download icon.
   EXPECT_CALL(delegate_, OnFaviconUpdated(
@@ -476,7 +525,7 @@ TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesFaster) {
 TEST_F(FaviconHandlerTest, RedownloadExpiredPageUrlFavicon) {
   favicon_service_.fake()->Store(
       kPageURL, kIconURL16x16,
-      CreateRawBitmapResult(kIconURL16x16, FAVICON, /*expired=*/true));
+      CreateRawBitmapResults(kIconURL16x16, FAVICON, /*expired=*/true));
 
   // TODO(crbug.com/700811): It would be nice if we could check whether the two
   // OnFaviconUpdated() calls are called with different gfx::Images (as opposed
@@ -525,7 +574,7 @@ TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
   const GURL kNewIconURL = kIconURL16x16;
 
   favicon_service_.fake()->Store(kPageURL, kOldIconURL,
-                                 CreateRawBitmapResult(kOldIconURL));
+                                 CreateRawBitmapResults(kOldIconURL));
 
   InSequence seq;
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kOldIconURL, _, _));
@@ -569,9 +618,9 @@ TEST_F(FaviconHandlerTest, UpdateFavicon) {
   const GURL kNewIconURL("http://www.google.com/new_favicon");
 
   favicon_service_.fake()->Store(kPageURL, kIconURL,
-                                 CreateRawBitmapResult(kIconURL));
+                                 CreateRawBitmapResults(kIconURL));
   favicon_service_.fake()->Store(kSomePreviousPageURL, kNewIconURL,
-                                 CreateRawBitmapResult(kNewIconURL));
+                                 CreateRawBitmapResults(kNewIconURL));
 
   EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
 
@@ -591,8 +640,8 @@ TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
 
   favicon_service_.fake()->Store(
       kPageURL, kIconURL64x64,
-      CreateRawBitmapResult(kIconURL64x64, TOUCH_ICON,
-                            /*expired=*/true));
+      CreateRawBitmapResults(kIconURL64x64, TOUCH_ICON,
+                             /*expired=*/true));
 
   EXPECT_CALL(delegate_,
               OnFaviconUpdated(kPageURL, FaviconDriverObserver::TOUCH_LARGEST,
@@ -824,6 +873,57 @@ TEST_F(FaviconHandlerMultipleFaviconsTest, ChooseMinorUpsamplingOverHugeIcon) {
   EXPECT_EQ(17, DownloadTillDoneIgnoringHistory(IntVector{17, 256}));
 }
 
+// Test that the FaviconHandler process finishes when the database provides
+// a favicon with multiple bitmaps.
+TEST_F(FaviconHandlerMultipleFaviconsTest,
+       GetFaviconFromHistoryWithMultipleBitmaps) {
+  const GURL kIconURL32_16("http://www.google.com/a");
+  ui::test::ScopedSetSupportedScaleFactors({ui::SCALE_FACTOR_100P, ui::SCALE_FACTOR_200P});
+
+  favicon_service_.fake()->Store(
+      kPageURL, kIconURL32_16,
+      {CreateRawBitmapResult(kIconURL32_16, FAVICON, /*expired=*/false,
+                             gfx::Size(32, 32), SK_ColorGREEN),
+       CreateRawBitmapResult(kIconURL32_16, FAVICON, /*expired=*/false,
+                             gfx::Size(16, 16), SK_ColorBLUE)});
+
+  // Set 16x16 bitmap is needed for sync purposes.
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             _, _, _, _,
+                             AllOf(ImageSizeIs(16, 16),
+                                   ImageColorForScaleIs(1.0f, SK_ColorBLUE),
+                                   ImageColorForScaleIs(2.0f, SK_ColorGREEN))));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL32_16});
+}
+
+// Test that the FaviconHandler process finishes when:
+// - There is no data in the database for neither the page URL nor the icon URL.
+// - FaviconService::GetFaviconForPageURL() callback returns before
+//   FaviconHandler::OnUpdateFaviconURL() is called.
+// AND
+// - The page provides a favicon with multiple bitmaps.
+TEST_F(FaviconHandlerMultipleFaviconsTest,
+       DownloadUnknownFaviconWithMultipleBitmaps) {
+  const GURL kIconURL32_16("http://www.google.com/a");
+
+  delegate_.fake_downloader().AddWithColors(
+      kIconURL32_16, IntVector{32, 16}, {SK_ColorGREEN, SK_ColorBLUE});
+
+  // Set 16x16 bitmap is needed for sync purposes.
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _,
+                             AllOf(ImageSizeIs(16, 16),
+                                   ImageColorForScaleIs(1.0f, SK_ColorBLUE),
+                                   ImageColorForScaleIs(2.0f, SK_ColorGREEN))));
+  EXPECT_CALL(favicon_service_,
+              SetFavicons(kPageURL, kIconURL32_16, FAVICON,
+                             AllOf(ImageSizeIs(16, 16),
+                                   ImageColorForScaleIs(1.0f, SK_ColorBLUE),
+                                   ImageColorForScaleIs(2.0f, SK_ColorGREEN))));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL32_16});
+}
+
 // Test that the best favicon is selected when:
 // - The page provides several favicons.
 // - Downloading one of the page's icon URLs previously returned a 404.
@@ -878,7 +978,7 @@ TEST_F(FaviconHandlerTest, FaviconInvalidURL) {
   EXPECT_THAT(delegate_.downloads(), IsEmpty());
 }
 
-TEST_F(FaviconHandlerTest, TestSortFavicon) {
+TEST_F(FaviconHandlerTest, TestSortFavicon16DIP) {
   const std::vector<favicon::FaviconURL> kSourceIconURLs{
       FaviconURL(GURL("http://www.google.com/a"), FAVICON,
                  {gfx::Size(1, 1), gfx::Size(17, 17)}),
@@ -887,37 +987,103 @@ TEST_F(FaviconHandlerTest, TestSortFavicon) {
       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)};
+      FaviconURL(GURL("http://www.google.com/e"), FAVICON, kEmptySizes),
+      FaviconURL(GURL("http://www.google.com/f"), FAVICON,
+                 {gfx::Size(10, 10), gfx::Size(400, 400)})};
+
+  std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_16_DIP, kSourceIconURLs);
+
+  EXPECT_THAT(handler->image_urls(),
+              ElementsAre(HasIconURL("http://www.google.com/c"),
+                          HasIconURL("http://www.google.com/a"),
+                          HasIconURL("http://www.google.com/f"),
+                          HasIconURL("http://www.google.com/b"),
+                          // The rest of bitmaps come in order, there is no
+                          // sizes attribute.
+                          HasIconURL("http://www.google.com/d"),
+                          HasIconURL("http://www.google.com/e")));
+}
+
+TEST_F(FaviconHandlerTest, TestSortFaviconLargest) {
+  const std::vector<favicon::FaviconURL> kSourceIconURLs{
+      FaviconURL(GURL("http://www.google.com/a"), FAVICON,
+                 {gfx::Size(1, 1), gfx::Size(17, 17)}),
+      FaviconURL(GURL("http://www.google.com/b"), FAVICON,
+                 {gfx::Size(1024, 1024), gfx::Size(512, 512)}),
+      FaviconURL(GURL("http://www.google.com/c"), FAVICON,
+                 {gfx::Size(16, 16), gfx::Size(14, 14)}),
+      FaviconURL(GURL("http://www.google.com/d"), FAVICON, kEmptySizes),
+      FaviconURL(GURL("http://www.google.com/e"), FAVICON, kEmptySizes),
+      FaviconURL(GURL("http://www.google.com/f"), FAVICON,
+                 {gfx::Size(10, 10), gfx::Size(400, 400)})};
 
   std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_LARGEST, kSourceIconURLs);
 
-  struct ExpectedResult {
-    // The favicon's index in kSourceIconURLs.
-    size_t favicon_index;
-    // Width of largest bitmap.
-    int width;
-  } results[] = {
-      // First is icon2, though its size larger than maximal.
-      {1, 1024},
-      // Second is icon1
-      // The 17x17 is largest.
-      {0, 17},
-      // Third is icon3 though it has same size as icon1.
-      // The 16x16 is largest.
-      {2, 16},
-      // The rest of bitmaps come in order, there is no sizes attribute.
-      {3, -1},
-      {4, -1},
-  };
-  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,
-              icons[i].icon_url);
-    if (results[i].width != -1)
-      EXPECT_EQ(results[i].width, icons[i].icon_sizes[0].width());
-  }
+  EXPECT_THAT(handler->image_urls(),
+              ElementsAre(HasIconURL("http://www.google.com/f"),
+                          HasIconURL("http://www.google.com/b"),
+                          HasIconURL("http://www.google.com/a"),
+                          HasIconURL("http://www.google.com/c"),
+                          // The rest of bitmaps come in order, there is no
+                          // sizes attribute.
+                          HasIconURL("http://www.google.com/d"),
+                          HasIconURL("http://www.google.com/e")));
+}
+
+TEST_F(FaviconHandlerTest, DownloadNoMoreAfterExactMatch) {
+  RunHandlerWithSimpleFaviconCandidates({kIconURL16x16, kIconURL64x64});
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
+}
+
+TEST_F(FaviconHandlerTest, DownloadExactMatchFirstIfSizesProvided) {
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_16_DIP,
+      {
+          FaviconURL(kIconURL64x64, FAVICON, SizeVector{gfx::Size(64, 64)}),
+          FaviconURL(kIconURL16x16, FAVICON, SizeVector{gfx::Size(16, 16)}),
+      });
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
+}
+
+TEST_F(FaviconHandlerTest, DownloadLargeButNotAboveLimit) {
+  const GURL kIconURL192x192 = GURL("http://www.google.com/favicon192x192");
+  const GURL kIconURL512x512 = GURL("http://www.google.com/favicon512x512");
+
+  delegate_.fake_downloader().Add(kIconURL192x192, IntVector{192});
+  delegate_.fake_downloader().Add(kIconURL512x512, IntVector{512});
+
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {
+          FaviconURL(kIconURL512x512, FAVICON, SizeVector{gfx::Size(512, 512)}),
+          FaviconURL(kIconURL192x192, FAVICON, SizeVector{gfx::Size(192, 192)}),
+      });
+
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL192x192));
+}
+
+// Test that the high-resolution favicon is selected when:
+// - The page provides several favicons.
+// - The low-resolution one is cached in the Favicons database (e.g. was written
+//   via sync).
+// AND
+// - The high-resolution candidate is the best match.
+TEST_F(FaviconHandlerTest, DownloadHighResolutionWhenOnlyLowResolutionCached) {
+  favicon_service_.fake()->Store(kPageURL, kIconURL16x16,
+                                 CreateRawBitmapResults(kIconURL16x16, FAVICON,
+                                                        /*expired=*/false));
+
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL32x32, _, _));
+
+  RunHandlerWithCandidates(
+      FaviconDriverObserver::NON_TOUCH_LARGEST,
+      {
+          FaviconURL(kIconURL16x16, FAVICON, SizeVector{gfx::Size(16, 16)}),
+          FaviconURL(kIconURL32x32, FAVICON, SizeVector{gfx::Size(32, 32)}),
+      });
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL32x32));
 }
 
 TEST_F(FaviconHandlerTest, TestDownloadLargestFavicon) {
@@ -971,7 +1137,11 @@ TEST_F(FaviconHandlerTest, TestSelectLargestFavicon) {
 }
 
 TEST_F(FaviconHandlerTest, TestFaviconWasScaledAfterDownload) {
-  const int kMaximalSize = FaviconHandler::GetMaximalIconSize(FAVICON);
+#if defined(OS_IOS)
+  const int kMaximalSize = 144;
+#else
+  const int kMaximalSize = 192;
+#endif
 
   const GURL kIconURL1("http://www.google.com/b");
   const GURL kIconURL2("http://www.google.com/c");